Removed SelectionPopupController.invalidateActionMode()

Removed SelectionPopupController.invalidateActionMode()

I believe invalidateActionMode() is not necessary. The only callsite
for it is in showActionMode() and it is only called if
isActionModeValid() is true. The callsites for showActionMode() are:

1) SelectionPopupController.restoreSelectionPopupsIfNecessary which
   only calls showActionMode() if isActionModeValid() is false.
   Since showActionMode() won't call invalidateActionMode() if
   isActionModeValid() is false this case can't result in a call.

2) ContentViewCore.showSelectActionMode() which in turn is only called
   in ContentViewCore.onRotationChanged().  In onRotationChanged(),
   showSelectActionMode() is called right after
   hidePopupsAndPreserveSelection() which makes isActionModeValid()
   false. Again this means that showActionMode() wont result result
   in invalidateActionMode() being called.

3) SelectionPopupController.onSelectionEvent() when the event is
   SELECTION_HANDLES_SHOWN. As far as I know SELECTION_HANDLES_SHOWN
   should result in a new action mode since it means we have a new
   selection. I can't think of a reason why we'd want to invalidate
   instead of destroying the old action mode creating a new one.

Review-Url: https://codereview.chromium.org/2767183002
Cr-Commit-Position: refs/heads/master@{#458997}
Committed: https://chromium.googlesource.com/chromium/src/+/31843a5be08800e6bdf2a1f7c07eef342ad1abc6

[amaralp, 2017-03-22 19:08:55]

The CQ bit was checked by amaralp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 19:09:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[amaralp, 2017-03-22 21:32:00]

Description was changed from

==========
Removed SelectionPopupController.invalidateActionMode()

I believe invalidateActionMode() is not necessary. The only callsite
for it is in showActionMode() and it is only called if
isActionModeValid() is true. The callsites for showActionMode() are:

1) SelectionPopupController.restoreSelectionPopupsIfNecessary which
   only calls showActionMode() if isActionModeValid() is false
   that call to showActionMode won't result in invalidateActionMode()

2) ContentViewCore.showSelectActionMode() which in turn is only called
   in ContentViewCore.onRotationChanged().  In onRotationChanged(),
   showSelectActionMode() is called right after
   hidePopupsAndPreserveSelection() which makes isActionModeValid()
   false. Again this means that showActionMode() wont result result
   in invalidateActionMode() being called.

3) SelectionPopupController.onSelectionEvent() when the event is
   SELECTION_HANDLES_SHOWN. As far as I know SELECTION_HANDLES_SHOWN
   should result in a new action mode since it means we have a new
   selection. I can't think of a reason why we'd want to invalidate
   instead of create a new one.
==========

to

==========
Removed SelectionPopupController.invalidateActionMode()

I believe invalidateActionMode() is not necessary. The only callsite
for it is in showActionMode() and it is only called if
isActionModeValid() is true. The callsites for showActionMode() are:

1) SelectionPopupController.restoreSelectionPopupsIfNecessary which
   only calls showActionMode() if isActionModeValid() is false
   that call to showActionMode won't result in invalidateActionMode()

2) ContentViewCore.showSelectActionMode() which in turn is only called
   in ContentViewCore.onRotationChanged().  In onRotationChanged(),
   showSelectActionMode() is called right after
   hidePopupsAndPreserveSelection() which makes isActionModeValid()
   false. Again this means that showActionMode() wont result result
   in invalidateActionMode() being called.

3) SelectionPopupController.onSelectionEvent() when the event is
   SELECTION_HANDLES_SHOWN. As far as I know SELECTION_HANDLES_SHOWN
   should result in a new action mode since it means we have a new
   selection. I can't think of a reason why we'd want to invalidate
   instead of destroying the old action mode creating a new one.
==========

[amaralp, 2017-03-22 21:33:16]

The CQ bit was checked by amaralp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 21:33:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[amaralp, 2017-03-22 21:34:07]

The CQ bit was checked by amaralp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 21:34:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[amaralp, 2017-03-22 22:24:09]

amaralp@chromium.org changed reviewers:
+ boliu@chromium.org, jinsukkim@chromium.org

[amaralp, 2017-03-22 22:27:19]

Description was changed from

==========
Removed SelectionPopupController.invalidateActionMode()

I believe invalidateActionMode() is not necessary. The only callsite
for it is in showActionMode() and it is only called if
isActionModeValid() is true. The callsites for showActionMode() are:

1) SelectionPopupController.restoreSelectionPopupsIfNecessary which
   only calls showActionMode() if isActionModeValid() is false
   that call to showActionMode won't result in invalidateActionMode()

2) ContentViewCore.showSelectActionMode() which in turn is only called
   in ContentViewCore.onRotationChanged().  In onRotationChanged(),
   showSelectActionMode() is called right after
   hidePopupsAndPreserveSelection() which makes isActionModeValid()
   false. Again this means that showActionMode() wont result result
   in invalidateActionMode() being called.

3) SelectionPopupController.onSelectionEvent() when the event is
   SELECTION_HANDLES_SHOWN. As far as I know SELECTION_HANDLES_SHOWN
   should result in a new action mode since it means we have a new
   selection. I can't think of a reason why we'd want to invalidate
   instead of destroying the old action mode creating a new one.
==========

to

==========
Removed SelectionPopupController.invalidateActionMode()

I believe invalidateActionMode() is not necessary. The only callsite
for it is in showActionMode() and it is only called if
isActionModeValid() is true. The callsites for showActionMode() are:

1) SelectionPopupController.restoreSelectionPopupsIfNecessary which
   only calls showActionMode() if isActionModeValid() is false.
   Since showActionMode() won't call invalidateActionMode() if
   isActionModeValid() is false this case can't result in a call.

2) ContentViewCore.showSelectActionMode() which in turn is only called
   in ContentViewCore.onRotationChanged().  In onRotationChanged(),
   showSelectActionMode() is called right after
   hidePopupsAndPreserveSelection() which makes isActionModeValid()
   false. Again this means that showActionMode() wont result result
   in invalidateActionMode() being called.

3) SelectionPopupController.onSelectionEvent() when the event is
   SELECTION_HANDLES_SHOWN. As far as I know SELECTION_HANDLES_SHOWN
   should result in a new action mode since it means we have a new
   selection. I can't think of a reason why we'd want to invalidate
   instead of destroying the old action mode creating a new one.
==========

[amaralp, 2017-03-22 22:27:27]

PTAL

[commit-bot: I haz the power, 2017-03-22 22:42:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 22:42:44]

Dry run: This issue passed the CQ dry run.

[Jinsuk Kim, 2017-03-22 23:52:29]

lgtm

[boliu, 2017-03-23 03:37:23]

lgtm

nice. I was wondering what's the difference between show and invalidate when
reviewing tima's CL. +tima fyi

[amaralp, 2017-03-23 03:39:17]

The CQ bit was checked by amaralp@chromium.org

[commit-bot: I haz the power, 2017-03-23 03:39:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 03:44:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490240357721600,
"parent_rev": "1b27dad7e468ddaaf74d9b483bf14015c9f848e3", "commit_rev":
"31843a5be08800e6bdf2a1f7c07eef342ad1abc6"}

[commit-bot: I haz the power, 2017-03-23 03:45:39]

Description was changed from

==========
Removed SelectionPopupController.invalidateActionMode()

I believe invalidateActionMode() is not necessary. The only callsite
for it is in showActionMode() and it is only called if
isActionModeValid() is true. The callsites for showActionMode() are:

1) SelectionPopupController.restoreSelectionPopupsIfNecessary which
   only calls showActionMode() if isActionModeValid() is false.
   Since showActionMode() won't call invalidateActionMode() if
   isActionModeValid() is false this case can't result in a call.

2) ContentViewCore.showSelectActionMode() which in turn is only called
   in ContentViewCore.onRotationChanged().  In onRotationChanged(),
   showSelectActionMode() is called right after
   hidePopupsAndPreserveSelection() which makes isActionModeValid()
   false. Again this means that showActionMode() wont result result
   in invalidateActionMode() being called.

3) SelectionPopupController.onSelectionEvent() when the event is
   SELECTION_HANDLES_SHOWN. As far as I know SELECTION_HANDLES_SHOWN
   should result in a new action mode since it means we have a new
   selection. I can't think of a reason why we'd want to invalidate
   instead of destroying the old action mode creating a new one.
==========

to

==========
Removed SelectionPopupController.invalidateActionMode()

I believe invalidateActionMode() is not necessary. The only callsite
for it is in showActionMode() and it is only called if
isActionModeValid() is true. The callsites for showActionMode() are:

1) SelectionPopupController.restoreSelectionPopupsIfNecessary which
   only calls showActionMode() if isActionModeValid() is false.
   Since showActionMode() won't call invalidateActionMode() if
   isActionModeValid() is false this case can't result in a call.

2) ContentViewCore.showSelectActionMode() which in turn is only called
   in ContentViewCore.onRotationChanged().  In onRotationChanged(),
   showSelectActionMode() is called right after
   hidePopupsAndPreserveSelection() which makes isActionModeValid()
   false. Again this means that showActionMode() wont result result
   in invalidateActionMode() being called.

3) SelectionPopupController.onSelectionEvent() when the event is
   SELECTION_HANDLES_SHOWN. As far as I know SELECTION_HANDLES_SHOWN
   should result in a new action mode since it means we have a new
   selection. I can't think of a reason why we'd want to invalidate
   instead of destroying the old action mode creating a new one.

Review-Url: https://codereview.chromium.org/2767183002
Cr-Commit-Position: refs/heads/master@{#458997}
Committed:
https://chromium.googlesource.com/chromium/src/+/31843a5be08800e6bdf2a1f7c07e...
==========

[commit-bot: I haz the power, 2017-03-23 03:45:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/31843a5be08800e6bdf2a1f7c07e...

[amaralp, 2017-03-25 02:32:18]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2776883002/ by amaralp@chromium.org.

The reason for reverting is: For non-floating menu we should invalidate instead
of destroying and creating..

[boliu, 2017-03-25 21:16:40]

On 2017/03/25 02:32:18, amaralp wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/2776883002/ by mailto:amaralp@chromium.org.
> 
> The reason for reverting is: For non-floating menu we should invalidate
instead
> of destroying and creating..

Can you write that as a comment somewhere in code?

And regardless, can some of these call sites be simplified? It's still the case
that only a single call site needs invalidate, right?

[amaralp, 2017-03-27 17:47:44]

On 2017/03/25 at 21:16:40, boliu wrote:
> On 2017/03/25 02:32:18, amaralp wrote:
> > A revert of this CL (patchset #2 id:20001) has been created in
> > https://codereview.chromium.org/2776883002/ by mailto:amaralp@chromium.org.
> > 
> > The reason for reverting is: For non-floating menu we should invalidate
instead
> > of destroying and creating..
> 
> Can you write that as a comment somewhere in code?
> 
> And regardless, can some of these call sites be simplified? It's still the
case that only a single call site needs invalidate, right?

I think things can still be simplified. I'll upload a new CL.

[Tima Vaisburd, 2017-03-27 18:23:13]

On 2017/03/27 17:47:44, amaralp wrote:
> On 2017/03/25 at 21:16:40, boliu wrote:
> > On 2017/03/25 02:32:18, amaralp wrote:
> > > A revert of this CL (patchset #2 id:20001) has been created in
> > > https://codereview.chromium.org/2776883002/ by
mailto:amaralp@chromium.org.
> > > 
> > > The reason for reverting is: For non-floating menu we should invalidate
> instead
> > > of destroying and creating..
> > 
> > Can you write that as a comment somewhere in code?
> > 
> > And regardless, can some of these call sites be simplified? It's still the
> case that only a single call site needs invalidate, right?
> 
> I think things can still be simplified. I'll upload a new CL.

I will be happy to synchronize http://crrev.com/2740103006 with this work.
I think selectAll() would need to either invalidate() or finish and restart as
well.