Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2774423003:
Removing invalidateActionMode (Closed)
DescriptionRemoving invalidateActionMode
Moved invalidateActionMode directly into showActionMode since it is the only callsite.
I've also moved the showing of the action mode to the end of showActionMode. I
think that makes more sense because you'd only want to show the action mode
after it has been invalidated. Showing before the invalidation could cause the old
menu items to be shown.
I've also removed some Android I specific code.
Review-Url: https://codereview.chromium.org/2774423003
Cr-Commit-Position: refs/heads/master@{#460208}
Committed: https://chromium.googlesource.com/chromium/src/+/6264dc5bf2672ef8629a00a030a3713adbee6b87
Patch Set 1 #Patch Set 2 : fixing unhiding on showActionMode #
Total comments: 8
Patch Set 3 : only unhide for invalidate #Messages
Total messages: 27 (18 generated)
The CQ bit was checked by amaralp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by amaralp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Removing invalidateActionMode Currently invalidateActionMode is used to both reset action mode visibility and to invalidate the action mode. ========== to ========== Removing invalidateActionMode The caller of showActionMode shouldn't have to know or care if the action mode is valid. The end state of either case should be the same. I've also moved the showing of the action mode to the end of showActionMode. I think that makes more sense because you'd only want to show the action mode after it has been invalidated. Showing before the invalidation could cause the old menu items to be shown. I've also removed some Android I specific code. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Removing invalidateActionMode The caller of showActionMode shouldn't have to know or care if the action mode is valid. The end state of either case should be the same. I've also moved the showing of the action mode to the end of showActionMode. I think that makes more sense because you'd only want to show the action mode after it has been invalidated. Showing before the invalidation could cause the old menu items to be shown. I've also removed some Android I specific code. ========== to ========== Removing invalidateActionMode Moved invalidateActionMode directly into showActionMode since it is the only callsite. I've also moved the showing of the action mode to the end of showActionMode. I think that makes more sense because you'd only want to show the action mode after it has been invalidated. Showing before the invalidation could cause the old menu items to be shown. I've also removed some Android I specific code. ==========
amaralp@chromium.org changed reviewers: + boliu@chromium.org, jinsukkim@chromium.org, timav@chromium.org
https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:217: mUnselectAllOnDismiss = true; |mUnselectAllOnDismiss| gets turned on but didn't before. Is this okay?
In the CL description "show" has two meanings: we call ActionMode.show() in order to create the ActionMode object, and we also show when we stop hiding it. I got somewhat confused at first, although it made perfect sense to me after I figured out which show you were talking about. Maybe use the word "unhide" for the second case? https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:216: hideActionMode(false); Can the action mode be hidden (i.e. mHidden == true) when isActionModeValid() returns false? I've assumed not, but I could be mistaken. If the action mode, indeed, cannot be hidden when mActionMode == null and the call to hideActionMode(false) will always be a no-op in this case, maybe move it to |if (isActionModeValid() {...}| block?
https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:216: hideActionMode(false); On 2017/03/28 02:54:42, Tima Vaisburd wrote: > Can the action mode be hidden (i.e. mHidden == true) when isActionModeValid() > returns false? > I've assumed not, but I could be mistaken. > > If the action mode, indeed, cannot be hidden when mActionMode == null and the > call to hideActionMode(false) will always be a no-op in this case, maybe move it > to |if (isActionModeValid() {...}| block? So only SELECTION_HANDLES_SHOWN can fall into invalidate. So the question is can SELECTION_HANDLES_SHOWN happen when action mode is hidden? I can't prove it can't be hidden, so better be safe? https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:217: mUnselectAllOnDismiss = true; On 2017/03/28 02:19:59, Jinsuk Kim wrote: > |mUnselectAllOnDismiss| gets turned on but didn't before. Is this okay? Looks ok, it would have already been false in the invalidate case afaict. Every place where it's set to false also finishes the action mode.
https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:216: hideActionMode(false); On 2017/03/28 17:09:19, boliu wrote: > On 2017/03/28 02:54:42, Tima Vaisburd wrote: > > Can the action mode be hidden (i.e. mHidden == true) when isActionModeValid() > > returns false? > > I've assumed not, but I could be mistaken. > > > > If the action mode, indeed, cannot be hidden when mActionMode == null and the > > call to hideActionMode(false) will always be a no-op in this case, maybe move > it > > to |if (isActionModeValid() {...}| block? > > So only SELECTION_HANDLES_SHOWN can fall into invalidate. So the question is can > SELECTION_HANDLES_SHOWN happen when action mode is hidden? > > I can't prove it can't be hidden, so better be safe? Action mode can be hidden, but it should exist. I meant, I think that the hidden property of the ActionMode should reflect the state of the _existing_ ActionMode object. If that object does not exist (not yet created, or already finished) then mHidden must be false, or there is an inconsistency. I do not think we want to set it hidden in advance? With that hideActionMode(false) should always be a no-op for the else case, but this is not a big issue. The code as-is is fine with me too.
lgtm https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:216: hideActionMode(false); On 2017/03/28 17:39:52, Tima Vaisburd wrote: > On 2017/03/28 17:09:19, boliu wrote: > > On 2017/03/28 02:54:42, Tima Vaisburd wrote: > > > Can the action mode be hidden (i.e. mHidden == true) when > isActionModeValid() > > > returns false? > > > I've assumed not, but I could be mistaken. > > > > > > If the action mode, indeed, cannot be hidden when mActionMode == null and > the > > > call to hideActionMode(false) will always be a no-op in this case, maybe > move > > it > > > to |if (isActionModeValid() {...}| block? > > > > So only SELECTION_HANDLES_SHOWN can fall into invalidate. So the question is > can > > SELECTION_HANDLES_SHOWN happen when action mode is hidden? > > > > I can't prove it can't be hidden, so better be safe? > > Action mode can be hidden, but it should exist. > > I meant, I think that the hidden property of the ActionMode should reflect the > state of the _existing_ ActionMode object. If that object does not exist (not > yet created, or already finished) then mHidden must be false, or there is an > inconsistency. I do not think we want to set it hidden in advance? > > With that hideActionMode(false) should always be a no-op for the else case, but > this is not a big issue. The code as-is is fine with me too. Oh, ok. Yeah I got confused, old code in invalidate used to unhide, so yeah, this is fine.
lgtm
The CQ bit was checked by amaralp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:216: hideActionMode(false); On 2017/03/28 at 17:44:12, boliu wrote: > On 2017/03/28 17:39:52, Tima Vaisburd wrote: > > On 2017/03/28 17:09:19, boliu wrote: > > > On 2017/03/28 02:54:42, Tima Vaisburd wrote: > > > > Can the action mode be hidden (i.e. mHidden == true) when > > isActionModeValid() > > > > returns false? > > > > I've assumed not, but I could be mistaken. > > > > > > > > If the action mode, indeed, cannot be hidden when mActionMode == null and > > the > > > > call to hideActionMode(false) will always be a no-op in this case, maybe > > move > > > it > > > > to |if (isActionModeValid() {...}| block? > > > > > > So only SELECTION_HANDLES_SHOWN can fall into invalidate. So the question is > > can > > > SELECTION_HANDLES_SHOWN happen when action mode is hidden? > > > > > > I can't prove it can't be hidden, so better be safe? > > > > Action mode can be hidden, but it should exist. > > > > I meant, I think that the hidden property of the ActionMode should reflect the > > state of the _existing_ ActionMode object. If that object does not exist (not > > yet created, or already finished) then mHidden must be false, or there is an > > inconsistency. I do not think we want to set it hidden in advance? > > > > With that hideActionMode(false) should always be a no-op for the else case, but > > this is not a big issue. The code as-is is fine with me too. > > Oh, ok. Yeah I got confused, old code in invalidate used to unhide, so yeah, this is fine. The only times mHidden is set to true is when a scroll starts or when a selection handle drag begins. The only times it is set back to false is when the scroll ends or the drag ends. Right now I can't think of a scenario where a scroll/drag would start but not end. I've gone ahead and moved the unhiding back to being only done for the invalidation case. I think the bigger problem is that SelectionPopupController is holding state of the action mode and it is difficult to reason when this state is being reset. I'm thinking of making a wrapper class for the action mode that could keep track of mHidden, mUnselectAllOnDismiss, etc., but I'll wait until timav@'s patch lands before trying that. https://codereview.chromium.org/2774423003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:217: mUnselectAllOnDismiss = true; On 2017/03/28 at 17:09:19, boliu wrote: > On 2017/03/28 02:19:59, Jinsuk Kim wrote: > > |mUnselectAllOnDismiss| gets turned on but didn't before. Is this okay? > > Looks ok, it would have already been false in the invalidate case afaict. Every place where it's set to false also finishes the action mode. It would have been already false. Also I've changed the code so that it doesn't get set anymore.
The CQ bit was unchecked by amaralp@chromium.org
The CQ bit was checked by amaralp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2774423003/#ps40001 (title: "only unhide for invalidate")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490730649287210,
"parent_rev": "92cb8c04c342bb35a96ee77c055f7cec8ff3ee41", "commit_rev":
"6264dc5bf2672ef8629a00a030a3713adbee6b87"}
Message was sent while issue was closed.
Description was changed from ========== Removing invalidateActionMode Moved invalidateActionMode directly into showActionMode since it is the only callsite. I've also moved the showing of the action mode to the end of showActionMode. I think that makes more sense because you'd only want to show the action mode after it has been invalidated. Showing before the invalidation could cause the old menu items to be shown. I've also removed some Android I specific code. ========== to ========== Removing invalidateActionMode Moved invalidateActionMode directly into showActionMode since it is the only callsite. I've also moved the showing of the action mode to the end of showActionMode. I think that makes more sense because you'd only want to show the action mode after it has been invalidated. Showing before the invalidation could cause the old menu items to be shown. I've also removed some Android I specific code. Review-Url: https://codereview.chromium.org/2774423003 Cr-Commit-Position: refs/heads/master@{#460208} Committed: https://chromium.googlesource.com/chromium/src/+/6264dc5bf2672ef8629a00a030a3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6264dc5bf2672ef8629a00a030a3... |
