Blink handle selection handle visibility

Blink handle selection handle visibility

The goal of this CL is to move all of the selection handle visibility to Blink.

Previously the touch selection handle visibility was handled all in the browser
in TouchSelectionController. This was done by the renderer passing the selection
information to the browser through OnSwapCompositorFrame using
CompositorFrameMetadata. This would get passed down to TouchSelectionController
which would decide whether or not it wants to show handles on the new
selection. This meant that the browser side needed to handle touch and split
the responsibility for handles across processes.

The new approach has Blink deciding when to show the touch handles. It does
this by keeping track of a isHandleVisible boolean in the VisibleSelection
class. We also add a boolean handle_visibility_changed_ to LayerTreeHost. This
is necessary because we always want to force a draw whenever the handle
visibility changed to notify the browser through the SwapCompositorFrame IPC.
The handles are in some sense part of the frame that should cause damage, and
this worked mostly by luck before (handle updates happened to piggyback on
selection updates).

Finally the handles are propagated to the browser in the
CompositorFrameMetadata as before except that only selection with handles are
propagated. Handleless selections are viewed the same way as no selection in
the browser. In other words, the browser doesn't know or care about selections
that have no handles. This means we had to remove the SELECTION_ESTABLISHED and
SELECTION_DISSOLVED from the SelectionEventType enum since we don’t know what
is happening with all selections. Luckily this was only used in
ContextualSearch and some tests and those instances were taken care of.

BUG=633281
Review-Url: https://codereview.chromium.org/2201853002
Cr-Commit-Position: refs/heads/master@{#444119}
Committed: https://chromium.googlesource.com/chromium/src/+/1107a295dcee2f4ee3f6c24f836c6adfa7b3830d

[amaralp, 2016-08-01 18:10:42]

Description was changed from

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
==========

to

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[amaralp, 2016-08-01 19:08:32]

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

[commit-bot: I haz the power, 2016-08-01 19:09:15]

Dry run: 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, 2016-08-01 19:09:16]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[amaralp, 2016-08-01 19:09:35]

amaralp@chromium.org changed reviewers:
+ aelias@chromium.org, sievers@chromium.org

[commit-bot: I haz the power, 2016-08-01 19:11:02]

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

[commit-bot: I haz the power, 2016-08-01 19:11:03]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2016-08-01 21:56:24]

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

[commit-bot: I haz the power, 2016-08-01 21:56:50]

Dry run: 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, 2016-08-01 21:56:52]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-08-01 21:58:39]

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

[commit-bot: I haz the power, 2016-08-01 21:58:40]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2016-08-01 23:53:26]

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

[commit-bot: I haz the power, 2016-08-01 23:53:42]

Dry run: 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, 2016-08-01 23:53:44]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-08-01 23:55:01]

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

[commit-bot: I haz the power, 2016-08-01 23:55:02]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2016-08-02 17:56:02]

PTAL

[aelias_OOO_until_Jul13, 2016-08-02 18:30:33]

https://codereview.chromium.org/2201853002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/compositing/CompositedSelection.h
(right):

https://codereview.chromium.org/2201853002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedSelection.h:60: bool
isHandleVisible;
I don't think this is needed.  Just change FrameView.cpp to call
ChromeClient::clearCompositedSelection() instead of
ChromeClient::updateCompositedSelection() if Blink considers the selection
handles invisible.  This should allow you to avoid adding any code in either CC
or the browser.

[amaralp, 2016-08-08 20:15:30]

On 2016/08/02 at 18:30:33, aelias wrote:
>
https://codereview.chromium.org/2201853002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/compositing/CompositedSelection.h
(right):
> 
>
https://codereview.chromium.org/2201853002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/compositing/CompositedSelection.h:60:
bool isHandleVisible;
> I don't think this is needed.  Just change FrameView.cpp to call
ChromeClient::clearCompositedSelection() instead of
ChromeClient::updateCompositedSelection() if Blink considers the selection
handles invisible.  This should allow you to avoid adding any code in either CC
or the browser.

Done.

[aelias_OOO_until_Jul13, 2016-08-08 21:30:51]

Overall structure looks good.  Some general comments:
- Patch is failing to apply, please rebase and try a commit dry run (I recommend
doing this before you get to anything else -- it has a good chance of informing
you about some failing test you weren't aware of).
- Please write a lot more in your patch description: explain the design of your
change, why it's the right design, and what are the user-observable changes.
- Please link to other bugs (comma-separated multiple bugs is a common pattern)
if you know this will fix one of the other bugs filed against you.

https://codereview.chromium.org/2201853002/diff/160001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2201853002/diff/160001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1934: selection_changed_ = selection_ != selection;
This relies on RegisterSelection being called again on the next commit.  But,
there may be multiple draws between two commits, and only the first draw is
needed.  I think we should be mutating state at draw time.

The damage-tracking code clears viewport_damage_rect_ in
LayerTreeHostImpl::PrepareToDraw.  I suggest roughly imitating that pattern for
this.

https://codereview.chromium.org/2201853002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2201853002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.cpp:193:
newSelection.setIsHandleVisible(event.event().fromTouch());
The changes in this file are intentionally changing/introducing new behavior, so
could you write a few unit tests for the cases you care about?

https://codereview.chromium.org/2201853002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.cpp:605: if
(selection().contains(vPoint) && !newSelection.isHandleVisible()) {
FrameSelection::contains() performs another hit test, but you have a
HitTestResult in this method already.  Please just do the same comparison as the
last line of FrameSelection::contains() with the existing HitTestResult.

[amaralp, 2016-08-09 22:27:20]

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

[commit-bot: I haz the power, 2016-08-09 22:27:47]

Dry run: 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, 2016-08-09 22:27:49]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-08-09 22:30:52]

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

[commit-bot: I haz the power, 2016-08-09 22:30:53]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2016-08-11 01:00:58]

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

[commit-bot: I haz the power, 2016-08-11 01:02:22]

Dry run: 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, 2016-08-11 01:02:22]

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

[commit-bot: I haz the power, 2016-08-11 01:02:24]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[amaralp, 2016-08-11 01:03:40]

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

[commit-bot: I haz the power, 2016-08-11 01:04:24]

Dry run: 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, 2016-08-11 01:04:26]

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

[commit-bot: I haz the power, 2016-08-11 01:04:27]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[amaralp, 2016-08-11 01:05:06]

Description was changed from

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
==========

[amaralp, 2016-08-11 01:05:13]

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

[commit-bot: I haz the power, 2016-08-11 01:06:30]

Dry run: 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, 2016-08-11 01:30:04]

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

[commit-bot: I haz the power, 2016-08-11 01:30:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amaralp, 2016-08-12 00:58:57]

Description was changed from

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
==========

to

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[amaralp, 2016-08-12 00:59:56]

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

[commit-bot: I haz the power, 2016-08-12 01:00:45]

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

[amaralp, 2016-08-12 01:01:18]

Description was changed from

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
==========

[commit-bot: I haz the power, 2016-08-12 01:26:31]

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

[commit-bot: I haz the power, 2016-08-12 01:26:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[amaralp, 2016-08-12 19:28:58]

The CQ bit was checked by amaralp@chromium.org

[amaralp, 2016-08-12 19:29:05]

The CQ bit was unchecked by amaralp@chromium.org

[amaralp, 2016-08-12 20:52:19]

Description was changed from

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
==========

to

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[amaralp, 2016-08-12 20:52:40]

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

[commit-bot: I haz the power, 2016-08-12 20:53:13]

Dry run: 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, 2016-08-12 21:30:11]

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

[commit-bot: I haz the power, 2016-08-12 21:30:12]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[amaralp, 2016-08-19 21:15:52]

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

[commit-bot: I haz the power, 2016-08-19 21:16:28]

Dry run: 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, 2016-08-19 21:27:27]

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

[commit-bot: I haz the power, 2016-08-19 21:27:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[amaralp, 2016-08-19 23:18:58]

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

[commit-bot: I haz the power, 2016-08-19 23:19:36]

Dry run: 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, 2016-08-20 00:05:31]

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

[commit-bot: I haz the power, 2016-08-20 00:05:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amaralp, 2016-08-22 19:45:59]

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

[commit-bot: I haz the power, 2016-08-22 19:46:55]

Dry run: 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, 2016-08-22 21:00:11]

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

[commit-bot: I haz the power, 2016-08-22 21:00:12]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[amaralp, 2016-08-24 23:18:05]

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

[commit-bot: I haz the power, 2016-08-24 23:18:45]

Dry run: 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, 2016-08-25 00:12:13]

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

[commit-bot: I haz the power, 2016-08-25 00:12:15]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[amaralp, 2016-08-25 20:31:04]

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

[commit-bot: I haz the power, 2016-08-25 20:32:06]

Dry run: 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, 2016-08-25 21:36:03]

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

[commit-bot: I haz the power, 2016-08-25 21:36:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[amaralp, 2016-09-08 21:53:24]

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

[commit-bot: I haz the power, 2016-09-08 21:54:10]

Dry run: 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, 2016-09-08 22:50:15]

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

[commit-bot: I haz the power, 2016-09-08 22:50:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[amaralp, 2016-09-15 21:09:23]

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

[commit-bot: I haz the power, 2016-09-15 21:09:49]

Dry run: 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, 2016-09-15 21:50:34]

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

[commit-bot: I haz the power, 2016-09-15 21:50:35]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[amaralp, 2016-09-19 18:59:13]

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

[commit-bot: I haz the power, 2016-09-19 19:00:11]

Dry run: 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, 2016-09-19 20:10:26]

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

[commit-bot: I haz the power, 2016-09-19 20:10:27]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[amaralp, 2016-09-30 17:29:13]

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

[commit-bot: I haz the power, 2016-09-30 17:29:52]

Dry run: 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, 2016-09-30 17:33:17]

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

[commit-bot: I haz the power, 2016-09-30 17:33:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[amaralp, 2016-09-30 21:32:40]

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

[commit-bot: I haz the power, 2016-09-30 21:33:20]

Dry run: 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, 2016-09-30 22:44:23]

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

[commit-bot: I haz the power, 2016-09-30 22:44:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amaralp, 2016-10-03 18:46:55]

Description was changed from

==========
Blink handle selection handle visibility

Moved handle selection logic from browswer to blink.

BUG=633281
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Blink handle selection handle visibility

The goal of this CL is to move all of the selection handle visibility to blink. 

Previously the touch selection handle visibility was handled all in the browser
in TouchSelectionContoller. This was done by the renderer passing the selection
information to the browser through OnSwapCompositorFrame using
CompositorFrameMetaData. This would get passed down to TouchSelectionController
which would decide whether or not it wants to show handles on the new selection.

The new approach has blink deciding when to show the touch handles. It does this
by keeping track of a isHandleVisible boolean in the VisibleSelection class. If
the source of the selection warrants touch selection handles then this boolean
is set to true. We also add a boolean handle_visibility_changed_ to
LayerTreeHost. This is necessary because we always want to draw a frame whenever
the handle visibility changed to notify the browser through the
SwapCompositorFrame IPC. LayerTreeHost tries to be efficient and only draw when
it is necessary (i.e. there is a visible change) so we use the
handle_visibility_changed_ boolean to force it to draw. 

Finally the handles are propagated to the browser in the CompositorFrameMetaData
as before except that only selection with handles are propagated. Handleless
selections are viewed the same way as no selection in the browser. In other
words the browser know or care about selections that have no handles. This means
we had to remove the SELECTION_ESTABLISHED and SELECTION_DISSOLVED from the
SelectionEventType enum since we don’t know what is happening with all
selections. Luckily this was only used in ContextualSearch and some tests and
those instances were taken care of.


BUG=633281
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[amaralp, 2016-10-03 18:47:57]

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

[commit-bot: I haz the power, 2016-10-03 18:48:28]

Dry run: 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, 2016-10-03 19:49:33]

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

[commit-bot: I haz the power, 2016-10-03 19:49:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amaralp, 2016-10-03 21:13:55]

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

[commit-bot: I haz the power, 2016-10-03 21:14:51]

Dry run: 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, 2016-10-03 22:42:19]

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

[commit-bot: I haz the power, 2016-10-03 22:42:20]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[aelias_OOO_until_Jul13, 2016-10-04 04:14:24]

Description was changed from

==========
Blink handle selection handle visibility

The goal of this CL is to move all of the selection handle visibility to blink. 

Previously the touch selection handle visibility was handled all in the browser
in TouchSelectionContoller. This was done by the renderer passing the selection
information to the browser through OnSwapCompositorFrame using
CompositorFrameMetaData. This would get passed down to TouchSelectionController
which would decide whether or not it wants to show handles on the new selection.

The new approach has blink deciding when to show the touch handles. It does this
by keeping track of a isHandleVisible boolean in the VisibleSelection class. If
the source of the selection warrants touch selection handles then this boolean
is set to true. We also add a boolean handle_visibility_changed_ to
LayerTreeHost. This is necessary because we always want to draw a frame whenever
the handle visibility changed to notify the browser through the
SwapCompositorFrame IPC. LayerTreeHost tries to be efficient and only draw when
it is necessary (i.e. there is a visible change) so we use the
handle_visibility_changed_ boolean to force it to draw. 

Finally the handles are propagated to the browser in the CompositorFrameMetaData
as before except that only selection with handles are propagated. Handleless
selections are viewed the same way as no selection in the browser. In other
words the browser know or care about selections that have no handles. This means
we had to remove the SELECTION_ESTABLISHED and SELECTION_DISSOLVED from the
SelectionEventType enum since we don’t know what is happening with all
selections. Luckily this was only used in ContextualSearch and some tests and
those instances were taken care of.


BUG=633281
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Blink handle selection handle visibility

The goal of this CL is to move all of the selection handle visibility to Blink.

Previously the touch selection handle visibility was handled all in the browser
in TouchSelectionController. This was done by the renderer passing the selection
information to the browser through OnSwapCompositorFrame using
CompositorFrameMetadata. This would get passed down to TouchSelectionController
which would decide whether or not it wants to show handles on the new
selection. This meant that the browser side needed to handle touch and split
the responsibility for handles across processes.

The new approach has Blink deciding when to show the touch handles. It does
this by keeping track of a isHandleVisible boolean in the VisibleSelection
class. We also add a boolean handle_visibility_changed_ to LayerTreeHost. This
is necessary because we always want to force a draw whenever the handle
visibility changed to notify the browser through the SwapCompositorFrame IPC.
The handles are in some sense part of the frame that should cause damage, and
this worked mostly by luck before (handle updates happened to piggyback on
selection updates).

Finally the handles are propagated to the browser in the
CompositorFrameMetadata as before except that only selection with handles are
propagated. Handleless selections are viewed the same way as no selection in
the browser. In other words, the browser doesn't know or care about selections
that have no handles. This means we had to remove the SELECTION_ESTABLISHED and
SELECTION_DISSOLVED from the SelectionEventType enum since we don’t know what
is happening with all selections. Luckily this was only used in
ContextualSearch and some tests and those instances were taken care of.
==========

[aelias_OOO_until_Jul13, 2016-10-04 04:14:50]

Description was changed from

==========
Blink handle selection handle visibility

The goal of this CL is to move all of the selection handle visibility to Blink.

Previously the touch selection handle visibility was handled all in the browser
in TouchSelectionController. This was done by the renderer passing the selection
information to the browser through OnSwapCompositorFrame using
CompositorFrameMetadata. This would get passed down to TouchSelectionController
which would decide whether or not it wants to show handles on the new
selection. This meant that the browser side needed to handle touch and split
the responsibility for handles across processes.

The new approach has Blink deciding when to show the touch handles. It does
this by keeping track of a isHandleVisible boolean in the VisibleSelection
class. We also add a boolean handle_visibility_changed_ to LayerTreeHost. This
is necessary because we always want to force a draw whenever the handle
visibility changed to notify the browser through the SwapCompositorFrame IPC.
The handles are in some sense part of the frame that should cause damage, and
this worked mostly by luck before (handle updates happened to piggyback on
selection updates).

Finally the handles are propagated to the browser in the
CompositorFrameMetadata as before except that only selection with handles are
propagated. Handleless selections are viewed the same way as no selection in
the browser. In other words, the browser doesn't know or care about selections
that have no handles. This means we had to remove the SELECTION_ESTABLISHED and
SELECTION_DISSOLVED from the SelectionEventType enum since we don’t know what
is happening with all selections. Luckily this was only used in
ContextualSearch and some tests and those instances were taken care of.
==========

to

==========
Blink handle selection handle visibility

The goal of this CL is to move all of the selection handle visibility to Blink.

Previously the touch selection handle visibility was handled all in the browser
in TouchSelectionController. This was done by the renderer passing the selection
information to the browser through OnSwapCompositorFrame using
CompositorFrameMetadata. This would get passed down to TouchSelectionController
which would decide whether or not it wants to show handles on the new
selection. This meant that the browser side needed to handle touch and split
the responsibility for handles across processes.

The new approach has Blink deciding when to show the touch handles. It does
this by keeping track of a isHandleVisible boolean in the VisibleSelection
class. We also add a boolean handle_visibility_changed_ to LayerTreeHost. This
is necessary because we always want to force a draw whenever the handle
visibility changed to notify the browser through the SwapCompositorFrame IPC.
The handles are in some sense part of the frame that should cause damage, and
this worked mostly by luck before (handle updates happened to piggyback on
selection updates).

Finally the handles are propagated to the browser in the
CompositorFrameMetadata as before except that only selection with handles are
propagated. Handleless selections are viewed the same way as no selection in
the browser. In other words, the browser doesn't know or care about selections
that have no handles. This means we had to remove the SELECTION_ESTABLISHED and
SELECTION_DISSOLVED from the SelectionEventType enum since we don’t know what
is happening with all selections. Luckily this was only used in
ContextualSearch and some tests and those instances were taken care of.

BUG=633281
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[amaralp, 2016-10-04 17:53:32]

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

[commit-bot: I haz the power, 2016-10-04 17:53:52]

Dry run: 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, 2016-10-04 19:09:54]

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

[commit-bot: I haz the power, 2016-10-04 19:09:55]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[amaralp, 2016-10-04 19:13:48]

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

[commit-bot: I haz the power, 2016-10-04 19:14:17]

Dry run: 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, 2016-10-04 22:35:58]

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

[commit-bot: I haz the power, 2016-10-04 22:36:00]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-10-06 01:22:38]

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

[commit-bot: I haz the power, 2016-10-06 01:23:07]

Dry run: 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, 2016-10-06 02:53:47]

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

[commit-bot: I haz the power, 2016-10-06 02:53:49]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-10-06 03:10:07]

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

[commit-bot: I haz the power, 2016-10-06 03:10:27]

Dry run: 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, 2016-10-06 03:16:11]

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

[commit-bot: I haz the power, 2016-10-06 03:16:12]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-10-06 03:16:53]

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

[commit-bot: I haz the power, 2016-10-06 03:17:11]

Dry run: 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, 2016-10-06 05:04:49]

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

[commit-bot: I haz the power, 2016-10-06 05:04:50]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-10-06 19:14:43]

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

[amaralp, 2016-10-06 19:14:47]

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

[commit-bot: I haz the power, 2016-10-06 19:15:34]

Dry run: 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, 2016-10-06 22:13:50]

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

[commit-bot: I haz the power, 2016-10-06 22:13:52]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[amaralp, 2016-10-06 23:58:25]

Looks like the tests all pass now. PTAL

[aelias_OOO_until_Jul13, 2016-10-07 02:40:20]

lgtm modulo minor comments

https://codereview.chromium.org/2201853002/diff/1130001/content/browser/rende...
File content/browser/renderer_host/input/stylus_text_selector.h (left):

https://codereview.chromium.org/2201853002/diff/1130001/content/browser/rende...
content/browser/renderer_host/input/stylus_text_selector.h:34: virtual void
OnStylusSelectEnd() = 0;
Begin/End pairs are a common pattern and it's weird to only have the Begin, even
if right now the End is just logging.  I'd prefer this be kept.

https://codereview.chromium.org/2201853002/diff/1130001/content/browser/rende...
content/browser/renderer_host/input/stylus_text_selector.h:70: bool dragged_;
Please move this cleanup to separate patch to keep this one a closure of related
changes.

https://codereview.chromium.org/2201853002/diff/1130001/content/browser/rende...
File content/browser/renderer_host/render_widget_host_view_android.h (left):

https://codereview.chromium.org/2201853002/diff/1130001/content/browser/rende...
content/browser/renderer_host/render_widget_host_view_android.h:194: void
OnStylusSelectEnd() override;
I think it's fine to keep the base class method and just have an empty override
here.

https://codereview.chromium.org/2201853002/diff/1130001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2201853002/diff/1130001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:203:
newSelection.setIsHandleVisible(event.event().fromTouch());
Can you add a test that handles are *not* created for mouse events?

[aelias_OOO_until_Jul13, 2016-10-07 02:43:36]

aelias@chromium.org changed reviewers:
+ danakj@chromium.org, donnd@chromium.org, mohsen@chromium.org,
yosin@chromium.org
- sievers@chromium.org

[aelias_OOO_until_Jul13, 2016-10-07 02:43:38]

Adding OWNERS:

- danakj@ for cc/ (damage-related)
- donnd@ for contextual_search/
- mohsen@ for ui/touch_selection/
- yosin@ for Source/core (VisibleSelection)

[yosin_UTC9, 2016-10-07 05:17:59]

https://codereview.chromium.org/2201853002/diff/1130001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/VisibleSelection.h (right):

https://codereview.chromium.org/2201853002/diff/1130001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/VisibleSelection.h:69: bool
isHandleVisible);
Please move |isHandleVisible| to |FrameSelection|. |VisibleSelection| is a basic
data structure and read as |VisibleRange|.
Also as nature of |isHandleVisible|, it is only related to the selection what
user sees, so |FrameSelection| is the right place.

[Donn Denman, 2016-10-07 22:10:00]

donnd@chromium.org changed reviewers:
+ twellington@chromium.org

[Donn Denman, 2016-10-07 22:10:03]

This change concerns me for it's impact on Contextual Search (CS) for two
reasons:
1) CS relies on a selection without handles for its basic functionality of
handling tap gestures.  Presumably this all still works, otherwise a lot of
tests would fail.  Maybe we just need to update the description of this CL to
mention that ContextualSearchClient still gets messages from ContentViewCore
about selections without handles?
2) This seems likely to make CS instrumentation tests flakier. We added
selection established/dissolved to allow our tests to wait until a selection
change had been propagated down to the renderer process.  Without this, a
sequence of gestures in a test could get ahead of the renderer and cause flaky
failures.  Is there any mechanism to wait until the selection change has taken
effect in the renderer/Blink.

We're available to talk f2f about these concerns!

https://chromiumcodereview.appspot.com/2201853002/diff/1130001/chrome/android...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://chromiumcodereview.appspot.com/2201853002/diff/1130001/chrome/android...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:822:
waitForPanelToClose();
Please remove this line.  This isn't needed since tapBarToExpandAndClosePanel
already waits for the close.

https://chromiumcodereview.appspot.com/2201853002/diff/1130001/chrome/android...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2008:
waitForPanelToClose();
Also please remove this line.  This isn't needed since clickToTriggerPrefetch
already waits for the close.

[amaralp, 2016-10-11 20:37:23]

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

[commit-bot: I haz the power, 2016-10-11 20:38:01]

Dry run: 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, 2016-10-11 20:46:36]

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

[commit-bot: I haz the power, 2016-10-11 20:46:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amaralp, 2016-10-11 21:36:33]

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

[commit-bot: I haz the power, 2016-10-11 21:37:16]

Dry run: 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, 2016-10-11 21:51:01]

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

[commit-bot: I haz the power, 2016-10-11 21:51:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amaralp, 2016-10-11 22:25:59]

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

[commit-bot: I haz the power, 2016-10-11 22:26:27]

Dry run: 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, 2016-10-11 22:41:36]

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

[commit-bot: I haz the power, 2016-10-11 22:41:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amaralp, 2016-10-11 22:45:01]

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

[commit-bot: I haz the power, 2016-10-11 22:45:46]

Dry run: 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, 2016-10-11 23:11:47]

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

[commit-bot: I haz the power, 2016-10-11 23:11:49]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[amaralp, 2016-10-11 23:41:55]

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

[commit-bot: I haz the power, 2016-10-11 23:42:20]

Dry run: 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, 2016-10-12 01:19:19]

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

[commit-bot: I haz the power, 2016-10-12 01:19:21]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[amaralp, 2016-10-12 02:47:51]

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

[commit-bot: I haz the power, 2016-10-12 02:48:03]

Dry run: 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, 2016-10-12 04:25:40]

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

[commit-bot: I haz the power, 2016-10-12 04:25:42]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-10-13 02:47:11]

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

[commit-bot: I haz the power, 2016-10-13 02:47:26]

Dry run: 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, 2016-10-13 03:37:47]

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

[commit-bot: I haz the power, 2016-10-13 03:37:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[amaralp, 2016-10-13 18:07:58]

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

[commit-bot: I haz the power, 2016-10-13 18:08:19]

Dry run: 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, 2016-10-13 19:10:15]

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

[commit-bot: I haz the power, 2016-10-13 19:10:17]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[amaralp, 2016-10-14 00:36:27]

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

[commit-bot: I haz the power, 2016-10-14 00:36:48]

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

[amaralp, 2016-10-14 00:53:07]

PTAL, all comments have been addressed.

https://codereview.chromium.org/2201853002/diff/160001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2201853002/diff/160001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:1934: selection_changed_ = selection_ != selection;
On 2016/08/08 at 21:30:51, aelias wrote:
> This relies on RegisterSelection being called again on the next commit.  But,
there may be multiple draws between two commits, and only the first draw is
needed.  I think we should be mutating state at draw time.
> 
> The damage-tracking code clears viewport_damage_rect_ in
LayerTreeHostImpl::PrepareToDraw.  I suggest roughly imitating that pattern for
this.

Done.

https://codereview.chromium.org/2201853002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2201853002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionController.cpp:605: if
(selection().contains(vPoint) && !newSelection.isHandleVisible()) {
On 2016/08/08 at 21:30:51, aelias wrote:
> FrameSelection::contains() performs another hit test, but you have a
HitTestResult in this method already.  Please just do the same comparison as the
last line of FrameSelection::contains() with the existing HitTestResult.

Done.

https://codereview.chromium.org/2201853002/diff/1130001/chrome/android/javate...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/2201853002/diff/1130001/chrome/android/javate...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:822:
waitForPanelToClose();
On 2016/10/07 at 22:10:03, Donn Denman wrote:
> Please remove this line.  This isn't needed since tapBarToExpandAndClosePanel
already waits for the close.

Changed from waitForSelectionDissolved() to waitForSelectionEmpty().

https://codereview.chromium.org/2201853002/diff/1130001/chrome/android/javate...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2008:
waitForPanelToClose();
On 2016/10/07 at 22:10:03, Donn Denman wrote:
> Also please remove this line.  This isn't needed since clickToTriggerPrefetch
> already waits for the close.

Changed from waitForSelectionDissolved() to waitForSelectionEmpty().

https://codereview.chromium.org/2201853002/diff/1130001/content/browser/rende...
File content/browser/renderer_host/input/stylus_text_selector.h (left):

https://codereview.chromium.org/2201853002/diff/1130001/content/browser/rende...
content/browser/renderer_host/input/stylus_text_selector.h:34: virtual void
OnStylusSelectEnd() = 0;
On 2016/10/07 at 02:40:20, aelias wrote:
> Begin/End pairs are a common pattern and it's weird to only have the Begin,
even if right now the End is just logging.  I'd prefer this be kept.

Done.

https://codereview.chromium.org/2201853002/diff/1130001/content/browser/rende...
content/browser/renderer_host/input/stylus_text_selector.h:70: bool dragged_;
On 2016/10/07 at 02:40:20, aelias wrote:
> Please move this cleanup to separate patch to keep this one a closure of
related changes.

Done.

https://codereview.chromium.org/2201853002/diff/1130001/content/browser/rende...
File content/browser/renderer_host/render_widget_host_view_android.h (left):

https://codereview.chromium.org/2201853002/diff/1130001/content/browser/rende...
content/browser/renderer_host/render_widget_host_view_android.h:194: void
OnStylusSelectEnd() override;
On 2016/10/07 at 02:40:20, aelias wrote:
> I think it's fine to keep the base class method and just have an empty
override here.

Done.

https://codereview.chromium.org/2201853002/diff/1130001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2201853002/diff/1130001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:203:
newSelection.setIsHandleVisible(event.event().fromTouch());
On 2016/10/07 at 02:40:20, aelias wrote:
> Can you add a test that handles are *not* created for mouse events?

Added to EventHandlerTest.cpp

https://codereview.chromium.org/2201853002/diff/1130001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/VisibleSelection.h (right):

https://codereview.chromium.org/2201853002/diff/1130001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/VisibleSelection.h:69: bool
isHandleVisible);
On 2016/10/07 at 05:17:59, Yosi_UTC9 wrote:
> Please move |isHandleVisible| to |FrameSelection|. |VisibleSelection| is a
basic data structure and read as |VisibleRange|.
> Also as nature of |isHandleVisible|, it is only related to the selection what
user sees, so |FrameSelection| is the right place.

Moved.

[Donn Denman, 2016-10-14 01:05:43]

LGTM with just a small suggestion.

We'll want to watch the Contextual Search tests to see if they become more
flaky, but we can deal with that if it arises.  With this patch you preserved
the bottleneck to check the selection so we can flesh out a fix there, or
disable tests, in follow-up CLs.  Theresa, feel free to chime in if you don't
agree!

https://codereview.chromium.org/2201853002/diff/1290001/chrome/android/java/s...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java
(right):

https://codereview.chromium.org/2201853002/diff/1290001/chrome/android/java/s...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:513:
return "".equals(mSelectedText);
Since mSelectedText can be null, it's safer to use
TextUtils.isEmpty(mSelectedText);
With
import android.text.TextUtils;

[commit-bot: I haz the power, 2016-10-14 02:20:57]

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

[commit-bot: I haz the power, 2016-10-14 02:20:59]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-10-14 19:19:02]

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

[commit-bot: I haz the power, 2016-10-14 19:19:54]

Dry run: 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, 2016-10-14 21:13:38]

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

[commit-bot: I haz the power, 2016-10-14 21:13:40]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mohsen, 2016-10-15 03:49:36]

https://codereview.chromium.org/2201853002/diff/1330001/content/browser/rende...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/2201853002/diff/1330001/content/browser/rende...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:161:
ui::TouchSelectionController::INSERTION_ACTIVE) &&
I think this will break long-press on empty textfields. Long press generates a
context menu request from blink which is handled here and the touch selection
menu is shown instead of the regular context menu if touch selection is or
"going to be" active. The "going to be" case happens in cases that the context
menu request arrives before the selection update (which is very likely as the
selection update happens at the frame boundary, but the context menu request
happens immediately). You have removed the "going to be" check here. Maybe, the
proper solution is to not request the context menu in the first place now that
we know in blink side that a handle is going to be shown?

https://codereview.chromium.org/2201853002/diff/1330001/content/browser/rende...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc
(left):

https://codereview.chromium.org/2201853002/diff/1330001/content/browser/rende...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:154:
bool ClearTextfield() { return ExecuteScript(shell(), "clear_textfield()"); }
I think you can also remove clear_textfield() from
content/test/data/touch_selection.html.

https://codereview.chromium.org/2201853002/diff/1330001/ui/touch_selection/to...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/2201853002/diff/1330001/ui/touch_selection/to...
ui/touch_selection/touch_selection_controller.cc:59:
response_pending_input_event_(INPUT_EVENT_TYPE_NONE),
Is |response_pending_input_event_| variable still needed? We needed this mainly
to decide whether the handles should be shown on a selection change or not which
should not be the case anymore with this CL.

https://codereview.chromium.org/2201853002/diff/1330001/ui/touch_selection/to...
ui/touch_selection/touch_selection_controller.cc:81: || end.type() ==
gfx::SelectionBound::EMPTY) {
nit: putting || at the end of the previous line would be more consistent with
the rest of this file and, I believe, most of chrome code base.

[yosin_UTC9, 2016-10-17 07:27:46]

https://codereview.chromium.org/2201853002/diff/1330001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/2201853002/diff/1330001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.h:128: bool
isHandleVisible = false);
Could you pass |isHandleVisible| in |SetSelectionOptions| rather than adding
additional |bool| parameter?
If so, patch size becomes smaller.
Note: Blink doesn't prefer |bool| parameter, if we really needed it should be
|enum class|.

https://codereview.chromium.org/2201853002/diff/1330001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.h:262:
DoNotAdjustEndpoints
Good catch!
http://crrev.com/2424703002 will fix this. Committing now...

https://codereview.chromium.org/2201853002/diff/1330001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/SelectionController.h (right):

https://codereview.chromium.org/2201853002/diff/1330001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.h:102: bool
isHandleVisible = false);
Could you introduce |enum class| instead of using |bool| parameter?
Since |updateSelectionForMouseDownDispatchingSelectStart()|, please avoid to use
default parameter.
See |CursorAlignOnScroll| in "FrameSelection.h"

[amaralp, 2016-10-18 19:50:29]

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

[commit-bot: I haz the power, 2016-10-18 19:51:00]

Dry run: 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, 2016-10-18 21:35:04]

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

[commit-bot: I haz the power, 2016-10-18 21:35:06]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-10-19 02:51:21]

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

[commit-bot: I haz the power, 2016-10-19 02:53:08]

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

[amaralp, 2016-10-19 04:26:39]

https://codereview.chromium.org/2201853002/diff/1330001/content/browser/rende...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/2201853002/diff/1330001/content/browser/rende...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:161:
ui::TouchSelectionController::INSERTION_ACTIVE) &&
On 2016/10/15 at 03:49:36, mohsen wrote:
> I think this will break long-press on empty textfields. Long press generates a
context menu request from blink which is handled here and the touch selection
menu is shown instead of the regular context menu if touch selection is or
"going to be" active. The "going to be" case happens in cases that the context
menu request arrives before the selection update (which is very likely as the
selection update happens at the frame boundary, but the context menu request
happens immediately). You have removed the "going to be" check here. Maybe, the
proper solution is to not request the context menu in the first place now that
we know in blink side that a handle is going to be shown?

Yeah I think you are right.
I'm not sure I understand what you mean by not requesting the context menu.
Would moving the two DCHECK's (|param.is_editable| and
|params.selection_text.empty()|) into the if condition be a viable workaround?

https://codereview.chromium.org/2201853002/diff/1330001/content/browser/rende...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc
(left):

https://codereview.chromium.org/2201853002/diff/1330001/content/browser/rende...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:154:
bool ClearTextfield() { return ExecuteScript(shell(), "clear_textfield()"); }
On 2016/10/15 at 03:49:36, mohsen wrote:
> I think you can also remove clear_textfield() from
content/test/data/touch_selection.html.

Done.

https://codereview.chromium.org/2201853002/diff/1330001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/FrameSelection.h (right):

https://codereview.chromium.org/2201853002/diff/1330001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.h:128: bool
isHandleVisible = false);
On 2016/10/17 at 07:27:46, Yosi_UTC9 wrote:
> Could you pass |isHandleVisible| in |SetSelectionOptions| rather than adding
additional |bool| parameter?
> If so, patch size becomes smaller.
> Note: Blink doesn't prefer |bool| parameter, if we really needed it should be
|enum class|.

Done.

https://codereview.chromium.org/2201853002/diff/1330001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.h:262:
DoNotAdjustEndpoints
On 2016/10/17 at 07:27:46, Yosi_UTC9 wrote:
> Good catch!
> http://crrev.com/2424703002 will fix this. Committing now...

Thanks!

https://codereview.chromium.org/2201853002/diff/1330001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/SelectionController.h (right):

https://codereview.chromium.org/2201853002/diff/1330001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.h:102: bool
isHandleVisible = false);
On 2016/10/17 at 07:27:46, Yosi_UTC9 wrote:
> Could you introduce |enum class| instead of using |bool| parameter?
> Since |updateSelectionForMouseDownDispatchingSelectStart()|, please avoid to
use default parameter.
> See |CursorAlignOnScroll| in "FrameSelection.h"

Done.

https://codereview.chromium.org/2201853002/diff/1330001/ui/touch_selection/to...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/2201853002/diff/1330001/ui/touch_selection/to...
ui/touch_selection/touch_selection_controller.cc:59:
response_pending_input_event_(INPUT_EVENT_TYPE_NONE),
On 2016/10/15 at 03:49:36, mohsen wrote:
> Is |response_pending_input_event_| variable still needed? We needed this
mainly to decide whether the handles should be shown on a selection change or
not which should not be the case anymore with this CL.

I don't think it is needed. If it's OK with you I'd like to have a later CL
remove it since this CL is already getting big and I want to get it landed as
soon as possible.

https://codereview.chromium.org/2201853002/diff/1330001/ui/touch_selection/to...
ui/touch_selection/touch_selection_controller.cc:81: || end.type() ==
gfx::SelectionBound::EMPTY) {
On 2016/10/15 at 03:49:36, mohsen wrote:
> nit: putting || at the end of the previous line would be more consistent with
the rest of this file and, I believe, most of chrome code base.

done

[commit-bot: I haz the power, 2016-10-19 05:17:45]

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

[commit-bot: I haz the power, 2016-10-19 05:17:46]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_TIMED_OUT,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[yosin_UTC9, 2016-10-19 06:33:06]

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:104:
m_handleVisibility(HandleVisibility::NotVisible),
nit: Let's initialize in class declaration instead of here.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:162: bool
isHandleVisible = userTriggered == UserTriggered;
nit: This variable name makes me confusing, shouldHandleVisible is better.
nit: We don't need to use constant variable |isHandleVisible| here.
Note: I introduce |moveCaretSelection()| as replacement of |moveTo()| for moving
an implementation from |WebLocalFrameImpl|, http://crrev.com/2431153002

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:226: const
HandleVisibility& handleVisibility) {
Since |HandleVisibility| is |enum|, we don't need to use const-ref.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:289: HandleVisibility
handleVisibility = options & HandleVisible
nit: s/HandleVisibility/const HandleVisibility/

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:214:
VisibleSelection::selectionFromContentsOfNode(innerNode).isCaret();
nit: VisibleSelection::selectionFromContentsOfNode() will be gone soon.
Please write as:
createVisibleSelection(SelectionInFlatTree::Builder().selectAllChildren(*innerNode).build()).isCaret()

See http://crrev.com/2432173005

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:215: bool
notLeftClick = event.event().pointerProperties().button !=
nit: s/bool/const bool/

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:413:
HandleVisibility visibility = HandleVisibility::NotVisible;
Better to use |handleVisibility|, name |visibility| is ambiguous.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right):

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/input/EventHandlerTest.cpp:287: FrameSelection&
selection = document().frame()->selection();
nit: Could you add |selection()| getter to |EventHandlerTest| to avoid this
pattern in other test cases?

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5370: int
startEdgeTopInLayerX = expectedResult.Get(context, 1)
nit:s/int/const int/

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5374: int
startEdgeTopInLayerY = expectedResult.Get(context, 2)
nit:s/int/const int/

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5378: int
startEdgeBottomInLayerX = expectedResult.Get(context, 3)
nit:s/int/const int/

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5382: int
startEdgeBottomInLayerY = expectedResult.Get(context, 4)
nit:s/int/const int/

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5387: int
endEdgeTopInLayerX = expectedResult.Get(context, 6)
nit:s/int/const int/

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5391: int
endEdgeTopInLayerY = expectedResult.Get(context, 7)
nit:s/int/const int/

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5395: int
endEdgeBottomInLayerX = expectedResult.Get(context, 8)
nit:s/int/const int/

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5399: int
endEdgeBottomInLayerY = expectedResult.Get(context, 9)
nit:s/int/const int/

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5404: IntPoint hitPoint =
nit:s/IntPoint/const IntPoint/

[mohsen, 2016-10-19 16:08:34]

I tried your CL on Chrome OS and found one regression:
  1) select some text with touch so that touch handles are shown;
  2) move the mouse so that touch handles disappear;
  3) scroll the page (using mouse, keyboard, or touch).

After scroll ends touch handles appear again!

https://codereview.chromium.org/2201853002/diff/1330001/content/browser/rende...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/2201853002/diff/1330001/content/browser/rende...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:161:
ui::TouchSelectionController::INSERTION_ACTIVE) &&
On 2016/10/19 at 04:26:38, amaralp wrote:
> On 2016/10/15 at 03:49:36, mohsen wrote:
> > I think this will break long-press on empty textfields. Long press generates
a context menu request from blink which is handled here and the touch selection
menu is shown instead of the regular context menu if touch selection is or
"going to be" active. The "going to be" case happens in cases that the context
menu request arrives before the selection update (which is very likely as the
selection update happens at the frame boundary, but the context menu request
happens immediately). You have removed the "going to be" check here. Maybe, the
proper solution is to not request the context menu in the first place now that
we know in blink side that a handle is going to be shown?
> 
> Yeah I think you are right.
> I'm not sure I understand what you mean by not requesting the context menu.
> Would moving the two DCHECK's (|param.is_editable| and
|params.selection_text.empty()|) into the if condition be a viable workaround?

When user long-presses on a page, blink either selects some text or decides to
show a context menu, for which a request is sent to the browser (browser is
responsible to show the context menu). Here, we are intercepting that request
and sometimes show the touch selection menu instead of the context menu.
Instead, now that handle visibility is determined in blink, blink can probably
be smart enough to detect this case and avoid requesting the context menu in the
first place. I'm not sure I understand your suggestion to move DCHECK's into the
if condition (which if condition?) and how it would help?

https://codereview.chromium.org/2201853002/diff/1330001/ui/touch_selection/to...
File ui/touch_selection/touch_selection_controller.cc (right):

https://codereview.chromium.org/2201853002/diff/1330001/ui/touch_selection/to...
ui/touch_selection/touch_selection_controller.cc:59:
response_pending_input_event_(INPUT_EVENT_TYPE_NONE),
On 2016/10/19 at 04:26:38, amaralp wrote:
> On 2016/10/15 at 03:49:36, mohsen wrote:
> > Is |response_pending_input_event_| variable still needed? We needed this
mainly to decide whether the handles should be shown on a selection change or
not which should not be the case anymore with this CL.
> 
> I don't think it is needed. If it's OK with you I'd like to have a later CL
remove it since this CL is already getting big and I want to get it landed as
soon as possible.

Sure. Maybe just add a TODO.

[amaralp, 2016-10-20 19:22:00]

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

[commit-bot: I haz the power, 2016-10-20 19:22:23]

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

[danakj, 2016-10-20 22:40:26]

https://codereview.chromium.org/2201853002/diff/1390001/cc/trees/layer_tree_h...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2201853002/diff/1390001/cc/trees/layer_tree_h...
cc/trees/layer_tree_host_impl.cc:793: !frame->handle_visibility_changed) {
I think you could use a local variable here instead of adding this to FrameData.
And include a comment explaining that this means there is damage but why there's
no damage rects associated with it, what is the plumbing story/how does this
work.

https://codereview.chromium.org/2201853002/diff/1390001/cc/trees/layer_tree_i...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2201853002/diff/1390001/cc/trees/layer_tree_i...
cc/trees/layer_tree_impl.cc:1978:
layer_tree_host_impl_->HandleVisibilityChanged();
I think a nicer way to frame this is to have the bool on LayerTreeImpl.

Then LTHI can query the active LayerTreeImpl to see if handle visibility
changed, instead of this informing LTHI if its the active tree.

It would require a Reset function for LTHI to call when it reads it. Maybe
GetAndReset could be combined if u want.

If LayerTreeImpl has lots of damage-generating things we could generalize this
eventually to like "get damage for tree or something" but I don't see that yet.

[commit-bot: I haz the power, 2016-10-20 22:56:19]

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

[commit-bot: I haz the power, 2016-10-20 22:56:22]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-10-20 23:17:24]

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

[commit-bot: I haz the power, 2016-10-20 23:18:14]

Dry run: 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, 2016-10-21 00:39:16]

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

[commit-bot: I haz the power, 2016-10-21 00:39:18]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[amaralp, 2016-10-21 02:47:37]

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

[commit-bot: I haz the power, 2016-10-21 02:47:57]

Dry run: 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, 2016-10-21 03:07:43]

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

[commit-bot: I haz the power, 2016-10-21 03:07:45]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[amaralp, 2016-10-21 03:21:01]

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

[commit-bot: I haz the power, 2016-10-21 03:21:38]

Dry run: 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, 2016-10-21 03:37:13]

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

[commit-bot: I haz the power, 2016-10-21 03:37:19]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[amaralp, 2016-10-21 03:45:26]

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

[commit-bot: I haz the power, 2016-10-21 03:45:54]

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

[amaralp, 2016-10-21 03:47:20]

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:104:
m_handleVisibility(HandleVisibility::NotVisible),
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit: Let's initialize in class declaration instead of here.

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:162: bool
isHandleVisible = userTriggered == UserTriggered;
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit: This variable name makes me confusing, shouldHandleVisible is better.
> nit: We don't need to use constant variable |isHandleVisible| here.
> Note: I introduce |moveCaretSelection()| as replacement of |moveTo()| for
moving an implementation from |WebLocalFrameImpl|, http://crrev.com/2431153002

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:226: const
HandleVisibility& handleVisibility) {
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> Since |HandleVisibility| is |enum|, we don't need to use const-ref.

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:289: HandleVisibility
handleVisibility = options & HandleVisible
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit: s/HandleVisibility/const HandleVisibility/

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:214:
VisibleSelection::selectionFromContentsOfNode(innerNode).isCaret();
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit: VisibleSelection::selectionFromContentsOfNode() will be gone soon.
> Please write as:
>
createVisibleSelection(SelectionInFlatTree::Builder().selectAllChildren(*innerNode).build()).isCaret()
> 
> See http://crrev.com/2432173005

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:215: bool
notLeftClick = event.event().pointerProperties().button !=
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit: s/bool/const bool/

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:413:
HandleVisibility visibility = HandleVisibility::NotVisible;
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> Better to use |handleVisibility|, name |visibility| is ambiguous.

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right):

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/core/input/EventHandlerTest.cpp:287: FrameSelection&
selection = document().frame()->selection();
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit: Could you add |selection()| getter to |EventHandlerTest| to avoid this
pattern in other test cases?

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5370: int
startEdgeTopInLayerX = expectedResult.Get(context, 1)
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit:s/int/const int/

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5374: int
startEdgeTopInLayerY = expectedResult.Get(context, 2)
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit:s/int/const int/

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5378: int
startEdgeBottomInLayerX = expectedResult.Get(context, 3)
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit:s/int/const int/

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5382: int
startEdgeBottomInLayerY = expectedResult.Get(context, 4)
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit:s/int/const int/

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5387: int
endEdgeTopInLayerX = expectedResult.Get(context, 6)
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit:s/int/const int/

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5391: int
endEdgeTopInLayerY = expectedResult.Get(context, 7)
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit:s/int/const int/

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5395: int
endEdgeBottomInLayerX = expectedResult.Get(context, 8)
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit:s/int/const int/

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5399: int
endEdgeBottomInLayerY = expectedResult.Get(context, 9)
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit:s/int/const int/

Done.

https://codereview.chromium.org/2201853002/diff/1370001/third_party/WebKit/So...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:5404: IntPoint hitPoint =
On 2016/10/19 at 06:33:05, Yosi_UTC9 wrote:
> nit:s/IntPoint/const IntPoint/

Done.

https://codereview.chromium.org/2201853002/diff/1390001/cc/trees/layer_tree_i...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2201853002/diff/1390001/cc/trees/layer_tree_i...
cc/trees/layer_tree_impl.cc:1978:
layer_tree_host_impl_->HandleVisibilityChanged();
On 2016/10/20 at 22:40:26, danakj wrote:
> I think a nicer way to frame this is to have the bool on LayerTreeImpl.
> 
> Then LTHI can query the active LayerTreeImpl to see if handle visibility
changed, instead of this informing LTHI if its the active tree.
> 
> It would require a Reset function for LTHI to call when it reads it. Maybe
GetAndReset could be combined if u want.
> 
> If LayerTreeImpl has lots of damage-generating things we could generalize this
eventually to like "get damage for tree or something" but I don't see that yet.

Done.

https://codereview.chromium.org/2201853002/diff/1390001/cc/trees/layer_tree_i...
cc/trees/layer_tree_impl.cc:1978:
layer_tree_host_impl_->HandleVisibilityChanged();
On 2016/10/20 at 22:40:26, danakj wrote:
> I think a nicer way to frame this is to have the bool on LayerTreeImpl.
> 
> Then LTHI can query the active LayerTreeImpl to see if handle visibility
changed, instead of this informing LTHI if its the active tree.
> 
> It would require a Reset function for LTHI to call when it reads it. Maybe
GetAndReset could be combined if u want.
> 
> If LayerTreeImpl has lots of damage-generating things we could generalize this
eventually to like "get damage for tree or something" but I don't see that yet.

Done.

[commit-bot: I haz the power, 2016-10-21 04:28:23]

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

[commit-bot: I haz the power, 2016-10-21 04:28:25]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[danakj, 2016-10-21 20:53:33]

LG just a minor request

https://codereview.chromium.org/2201853002/diff/1450001/cc/trees/layer_tree_h...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2201853002/diff/1450001/cc/trees/layer_tree_h...
cc/trees/layer_tree_host_impl.cc:1109: frame,
active_tree_->GetAndResetHandleVisibilityChanged());
can you call this from inside CalculateRenderPasses, and move the big comment
onto this call (which will store it in a local var there to be used below it in
the if statement)?

[amaralp, 2016-10-21 21:27:44]

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

[commit-bot: I haz the power, 2016-10-21 21:28:50]

Dry run: 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, 2016-10-21 21:33:31]

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

[commit-bot: I haz the power, 2016-10-21 21:33:34]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[amaralp, 2016-10-21 22:12:35]

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

[commit-bot: I haz the power, 2016-10-21 22:13:09]

Dry run: 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, 2016-10-21 22:50:46]

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

[commit-bot: I haz the power, 2016-10-21 22:50:49]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[amaralp, 2016-10-21 22:53:01]

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

[commit-bot: I haz the power, 2016-10-21 22:53:44]

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

[amaralp, 2016-10-21 23:02:29]

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

[commit-bot: I haz the power, 2016-10-21 23:03:02]

Dry run: 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, 2016-10-22 00:36:16]

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

[commit-bot: I haz the power, 2016-10-22 00:36:19]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[amaralp, 2016-10-25 20:42:09]

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

[commit-bot: I haz the power, 2016-10-25 20:42:43]

Dry run: 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, 2016-10-25 21:31:28]

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

[commit-bot: I haz the power, 2016-10-25 21:31:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[amaralp, 2016-10-26 01:11:06]

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

[commit-bot: I haz the power, 2016-10-26 01:11:40]

Dry run: 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, 2016-10-26 02:11:41]

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

[commit-bot: I haz the power, 2016-10-26 02:11:43]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[amaralp, 2016-10-26 05:00:24]

amaralp@chromium.org changed reviewers:
+ bokan@chromium.org

[amaralp, 2016-10-26 05:03:40]

Added bokan@ for Source/core/[frame|input|layout] OWNERS

https://codereview.chromium.org/2201853002/diff/1450001/cc/trees/layer_tree_h...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2201853002/diff/1450001/cc/trees/layer_tree_h...
cc/trees/layer_tree_host_impl.cc:1109: frame,
active_tree_->GetAndResetHandleVisibilityChanged());
On 2016/10/21 at 20:53:33, danakj wrote:
> can you call this from inside CalculateRenderPasses, and move the big comment
onto this call (which will store it in a local var there to be used below it in
the if statement)?

Done

[bokan, 2016-10-26 16:17:23]

Source/core and Source/web code lgtm

https://codereview.chromium.org/2201853002/diff/1570001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2201853002/diff/1570001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:902: bool
FrameSelection::contains(const LayoutPoint& point) {
Please rename point to pointInContent

[amaralp, 2016-10-26 18:28:35]

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

[commit-bot: I haz the power, 2016-10-26 18:29:10]

Dry run: 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, 2016-10-26 20:14:52]

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

[commit-bot: I haz the power, 2016-10-26 20:14:55]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-11-05 01:10:39]

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

[commit-bot: I haz the power, 2016-11-05 01:11:00]

Dry run: 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, 2016-11-05 03:18:52]

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

[commit-bot: I haz the power, 2016-11-05 03:18:54]

Dry run: This issue passed the CQ dry run.

[amaralp, 2016-11-16 23:32:44]

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

[commit-bot: I haz the power, 2016-11-16 23:34:19]

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

[amaralp, 2016-11-17 01:09:27]

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

[commit-bot: I haz the power, 2016-11-17 01:11:08]

Dry run: 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, 2016-11-17 03:15:17]

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

[commit-bot: I haz the power, 2016-11-17 03:15:19]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)

[amaralp, 2016-11-22 22:08:19]

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

[commit-bot: I haz the power, 2016-11-22 22:08:54]

Dry run: 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, 2016-11-22 22:12:06]

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

[commit-bot: I haz the power, 2016-11-22 22:12:08]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[mohsen, 2016-11-22 23:23:54]

/ui/touch_selection/ LGTM with nits

https://codereview.chromium.org/2201853002/diff/1670001/content/browser/rende...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2201853002/diff/1670001/content/browser/rende...
content/browser/renderer_host/render_widget_host_view_aura.cc:2180: void
RenderWidgetHostViewAura::SelectionUpdated(
nit: This is apparently only called from one place. Can you inline it there?

https://codereview.chromium.org/2201853002/diff/1670001/ui/touch_selection/to...
File ui/touch_selection/touch_selection_controller.h (right):

https://codereview.chromium.org/2201853002/diff/1670001/ui/touch_selection/to...
ui/touch_selection/touch_selection_controller.h:91: void
WillHandleTapEvent(const gfx::PointF& location, int tap_count);
nit: Now that this function is not returning bool, remove the "Will" from the
name.

https://codereview.chromium.org/2201853002/diff/1670001/ui/touch_selection/to...
ui/touch_selection/touch_selection_controller.h:94: void
WillHandleLongPressEvent(base::TimeTicks event_time,
nit: Now that this function is not returning bool, remove the "Will" from the
name.
Edit

[amaralp, 2016-11-23 03:19:11]

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

[commit-bot: I haz the power, 2016-11-23 03:19:27]

Dry run: 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, 2016-11-23 03:20:35]

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

[commit-bot: I haz the power, 2016-11-23 03:20:38]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[yosin_UTC9, 2016-11-24 02:29:04]

https://codereview.chromium.org/2201853002/diff/1710001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2201853002/diff/1710001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:781: bool
FrameSelection::contains(const LayoutPoint& pointInContent) {
Could you move |FrameSelection::contains()| changes to another patch to explain
specific motivation of this change?

[amaralp, 2016-12-01 21:54:08]

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

[commit-bot: I haz the power, 2016-12-01 21:54:47]

Dry run: 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, 2016-12-01 21:56:49]

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

[commit-bot: I haz the power, 2016-12-01 21:56:51]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2016-12-01 23:58:03]

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

[commit-bot: I haz the power, 2016-12-01 23:58:57]

Dry run: 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, 2016-12-02 00:03:26]

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

[commit-bot: I haz the power, 2016-12-02 00:03:29]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2016-12-05 18:30:18]

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

[commit-bot: I haz the power, 2016-12-05 18:30:46]

Dry run: 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, 2016-12-05 18:34:04]

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

[commit-bot: I haz the power, 2016-12-05 18:34:06]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2016-12-13 01:23:55]

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

[commit-bot: I haz the power, 2016-12-13 01:24:56]

Dry run: 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, 2016-12-13 01:30:31]

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

[commit-bot: I haz the power, 2016-12-13 01:30:34]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2016-12-14 19:06:04]

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

[commit-bot: I haz the power, 2016-12-14 19:06:50]

Dry run: 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, 2016-12-14 19:11:00]

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

[commit-bot: I haz the power, 2016-12-14 19:11:02]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2016-12-14 20:50:49]

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

[commit-bot: I haz the power, 2016-12-14 20:51:27]

Dry run: 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, 2016-12-14 20:54:35]

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

[commit-bot: I haz the power, 2016-12-14 20:54:38]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2016-12-14 22:41:44]

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

[commit-bot: I haz the power, 2016-12-14 22:42:09]

Dry run: 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, 2016-12-14 22:44:36]

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

[commit-bot: I haz the power, 2016-12-14 22:44:40]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-03 20:25:53]

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

[commit-bot: I haz the power, 2017-01-03 20:26:21]

Dry run: 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-01-03 20:28:00]

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

[commit-bot: I haz the power, 2017-01-03 20:28:02]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-03 22:03:47]

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

[commit-bot: I haz the power, 2017-01-03 22:04:18]

Dry run: 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-01-03 22:06:37]

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

[commit-bot: I haz the power, 2017-01-03 22:06:40]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-03 22:46:06]

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

[commit-bot: I haz the power, 2017-01-03 22:46:32]

Dry run: 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-01-03 22:48:50]

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

[commit-bot: I haz the power, 2017-01-03 22:48:53]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-03 23:58:59]

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

[commit-bot: I haz the power, 2017-01-03 23:59:31]

Dry run: 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-01-04 00:02:55]

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

[commit-bot: I haz the power, 2017-01-04 00:02:57]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-04 19:03:26]

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

[commit-bot: I haz the power, 2017-01-04 19:04:12]

Dry run: 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-01-04 19:06:16]

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

[commit-bot: I haz the power, 2017-01-04 19:06:18]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-04 21:33:24]

On 2016/11/24 at 02:29:04, yosin wrote:
>
https://codereview.chromium.org/2201853002/diff/1710001/third_party/WebKit/So...
> File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/2201853002/diff/1710001/third_party/WebKit/So...
> third_party/WebKit/Source/core/editing/FrameSelection.cpp:781: bool
FrameSelection::contains(const LayoutPoint& pointInContent) {
> Could you move |FrameSelection::contains()| changes to another patch to
explain specific motivation of this change?

Done.

[amaralp, 2017-01-12 23:50:11]

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

[commit-bot: I haz the power, 2017-01-12 23:51:04]

Dry run: 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-01-12 23:57:37]

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

[commit-bot: I haz the power, 2017-01-12 23:57:39]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[yosin_UTC9, 2017-01-13 01:55:18]

core/editing lgtm w/ nits

https://codereview.chromium.org/2201853002/diff/1970001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2201853002/diff/1970001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:183: LayoutPoint
vPoint = view->rootFrameToContents(event.event().position());
nit: s/LayoutPoint/const LayoutPoint/
to avoid copy.

https://codereview.chromium.org/2201853002/diff/1970001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:679:
FrameSelection::SetSelectionOptions options =
nit: How about below?

const FrameSelection::SetSelectionOptions options = isHandleVisible
  ? FrameSelection::CloseTyping | FrameSelection::ClearTypingStyle |
FrameSelection::HandleVisible
  : FrameSelection::CloseTyping | FrameSelection::ClearTypingStyle;

const variable is easier to track value. ;-)

https://codereview.chromium.org/2201853002/diff/1970001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:768: bool
isHandleVisible = event.event().fromTouch() && newSelection.isRange();
nit: s/bool/const bool/

[amaralp, 2017-01-13 19:37:17]

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

[commit-bot: I haz the power, 2017-01-13 19:38:00]

Dry run: 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-01-13 19:41:57]

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

[commit-bot: I haz the power, 2017-01-13 19:41:59]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-13 20:02:33]

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

[commit-bot: I haz the power, 2017-01-13 20:03:34]

Dry run: 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-01-13 20:08:30]

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

[commit-bot: I haz the power, 2017-01-13 20:08:33]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-13 22:09:58]

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

[commit-bot: I haz the power, 2017-01-13 22:10:33]

Dry run: 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-01-13 22:13:47]

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

[commit-bot: I haz the power, 2017-01-13 22:13:50]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-13 23:52:52]

danakj@, did you forget to lgtm or have I not addressed your comments?

https://codereview.chromium.org/2201853002/diff/1570001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/2201853002/diff/1570001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/FrameSelection.cpp:902: bool
FrameSelection::contains(const LayoutPoint& point) {
On 2016/10/26 at 16:17:23, bokan wrote:
> Please rename point to pointInContent

Done.

https://codereview.chromium.org/2201853002/diff/1970001/third_party/WebKit/So...
File third_party/WebKit/Source/core/editing/SelectionController.cpp (right):

https://codereview.chromium.org/2201853002/diff/1970001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:183: LayoutPoint
vPoint = view->rootFrameToContents(event.event().position());
On 2017/01/13 at 01:55:18, Yosi_UTC9 wrote:
> nit: s/LayoutPoint/const LayoutPoint/
> to avoid copy.

Done.

https://codereview.chromium.org/2201853002/diff/1970001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:679:
FrameSelection::SetSelectionOptions options =
On 2017/01/13 at 01:55:18, Yosi_UTC9 wrote:
> nit: How about below?
> 
> const FrameSelection::SetSelectionOptions options = isHandleVisible
>   ? FrameSelection::CloseTyping | FrameSelection::ClearTypingStyle |
FrameSelection::HandleVisible
>   : FrameSelection::CloseTyping | FrameSelection::ClearTypingStyle;
> 
> const variable is easier to track value. ;-)

Done.

https://codereview.chromium.org/2201853002/diff/1970001/third_party/WebKit/So...
third_party/WebKit/Source/core/editing/SelectionController.cpp:768: bool
isHandleVisible = event.event().fromTouch() && newSelection.isRange();
On 2017/01/13 at 01:55:18, Yosi_UTC9 wrote:
> nit: s/bool/const bool/

Done.

[amaralp, 2017-01-14 00:02:12]

The CQ bit was checked by amaralp@chromium.org

[amaralp, 2017-01-14 00:02:13]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
donnd@chromium.org, bokan@chromium.org, mohsen@chromium.org, yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2201853002/#ps2050001
(title: "fixed use of WebGestureEvent")

[commit-bot: I haz the power, 2017-01-14 00:03:11]

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-01-14 00:06:07]

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

[commit-bot: I haz the power, 2017-01-14 00:06:12]

Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-14 01:57:25]

The CQ bit was checked by amaralp@chromium.org

[amaralp, 2017-01-14 01:57:26]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
donnd@chromium.org, bokan@chromium.org, mohsen@chromium.org, yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2201853002/#ps2070001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-14 01:57:45]

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-01-14 01:59:09]

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

[commit-bot: I haz the power, 2017-01-14 01:59:14]

Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[amaralp, 2017-01-14 03:44:33]

The CQ bit was checked by amaralp@chromium.org

[amaralp, 2017-01-14 03:44:34]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
donnd@chromium.org, bokan@chromium.org, mohsen@chromium.org, yosin@chromium.org
Link to the patchset: https://codereview.chromium.org/2201853002/#ps2090001
(title: "fixed incorrect rebase")

[commit-bot: I haz the power, 2017-01-14 03:45:01]

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-01-14 03:46:08]

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

[commit-bot: I haz the power, 2017-01-14 03:46:12]

Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[danakj, 2017-01-16 15:28:44]

On 2017/01/13 23:52:52, amaralp wrote:
> danakj@, did you forget to lgtm or have I not addressed your comments?

Oh sorry I forgot I was on this, and aelias has you covered. LGTM tho thanks!

[amaralp, 2017-01-17 18:15:30]

Description was changed from

==========
Blink handle selection handle visibility

The goal of this CL is to move all of the selection handle visibility to Blink.

Previously the touch selection handle visibility was handled all in the browser
in TouchSelectionController. This was done by the renderer passing the selection
information to the browser through OnSwapCompositorFrame using
CompositorFrameMetadata. This would get passed down to TouchSelectionController
which would decide whether or not it wants to show handles on the new
selection. This meant that the browser side needed to handle touch and split
the responsibility for handles across processes.

The new approach has Blink deciding when to show the touch handles. It does
this by keeping track of a isHandleVisible boolean in the VisibleSelection
class. We also add a boolean handle_visibility_changed_ to LayerTreeHost. This
is necessary because we always want to force a draw whenever the handle
visibility changed to notify the browser through the SwapCompositorFrame IPC.
The handles are in some sense part of the frame that should cause damage, and
this worked mostly by luck before (handle updates happened to piggyback on
selection updates).

Finally the handles are propagated to the browser in the
CompositorFrameMetadata as before except that only selection with handles are
propagated. Handleless selections are viewed the same way as no selection in
the browser. In other words, the browser doesn't know or care about selections
that have no handles. This means we had to remove the SELECTION_ESTABLISHED and
SELECTION_DISSOLVED from the SelectionEventType enum since we don’t know what
is happening with all selections. Luckily this was only used in
ContextualSearch and some tests and those instances were taken care of.

BUG=633281
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Blink handle selection handle visibility

The goal of this CL is to move all of the selection handle visibility to Blink.

Previously the touch selection handle visibility was handled all in the browser
in TouchSelectionController. This was done by the renderer passing the selection
information to the browser through OnSwapCompositorFrame using
CompositorFrameMetadata. This would get passed down to TouchSelectionController
which would decide whether or not it wants to show handles on the new
selection. This meant that the browser side needed to handle touch and split
the responsibility for handles across processes.

The new approach has Blink deciding when to show the touch handles. It does
this by keeping track of a isHandleVisible boolean in the VisibleSelection
class. We also add a boolean handle_visibility_changed_ to LayerTreeHost. This
is necessary because we always want to force a draw whenever the handle
visibility changed to notify the browser through the SwapCompositorFrame IPC.
The handles are in some sense part of the frame that should cause damage, and
this worked mostly by luck before (handle updates happened to piggyback on
selection updates).

Finally the handles are propagated to the browser in the
CompositorFrameMetadata as before except that only selection with handles are
propagated. Handleless selections are viewed the same way as no selection in
the browser. In other words, the browser doesn't know or care about selections
that have no handles. This means we had to remove the SELECTION_ESTABLISHED and
SELECTION_DISSOLVED from the SelectionEventType enum since we don’t know what
is happening with all selections. Luckily this was only used in
ContextualSearch and some tests and those instances were taken care of.

BUG=633281
==========

[amaralp, 2017-01-17 18:15:44]

The CQ bit was checked by amaralp@chromium.org

[commit-bot: I haz the power, 2017-01-17 18:16:14]

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-01-17 20:05:57]

CQ is committing da patch.

Bot data: {"patchset_id": 2090001, "attempt_start_ts": 1484676944304250,
"parent_rev": "28b1bf2e4e780f510109ca4b1d61e72f024232aa", "commit_rev":
"1107a295dcee2f4ee3f6c24f836c6adfa7b3830d"}

[commit-bot: I haz the power, 2017-01-17 20:06:37]

Description was changed from

==========
Blink handle selection handle visibility

The goal of this CL is to move all of the selection handle visibility to Blink.

Previously the touch selection handle visibility was handled all in the browser
in TouchSelectionController. This was done by the renderer passing the selection
information to the browser through OnSwapCompositorFrame using
CompositorFrameMetadata. This would get passed down to TouchSelectionController
which would decide whether or not it wants to show handles on the new
selection. This meant that the browser side needed to handle touch and split
the responsibility for handles across processes.

The new approach has Blink deciding when to show the touch handles. It does
this by keeping track of a isHandleVisible boolean in the VisibleSelection
class. We also add a boolean handle_visibility_changed_ to LayerTreeHost. This
is necessary because we always want to force a draw whenever the handle
visibility changed to notify the browser through the SwapCompositorFrame IPC.
The handles are in some sense part of the frame that should cause damage, and
this worked mostly by luck before (handle updates happened to piggyback on
selection updates).

Finally the handles are propagated to the browser in the
CompositorFrameMetadata as before except that only selection with handles are
propagated. Handleless selections are viewed the same way as no selection in
the browser. In other words, the browser doesn't know or care about selections
that have no handles. This means we had to remove the SELECTION_ESTABLISHED and
SELECTION_DISSOLVED from the SelectionEventType enum since we don’t know what
is happening with all selections. Luckily this was only used in
ContextualSearch and some tests and those instances were taken care of.

BUG=633281
==========

to

==========
Blink handle selection handle visibility

The goal of this CL is to move all of the selection handle visibility to Blink.

Previously the touch selection handle visibility was handled all in the browser
in TouchSelectionController. This was done by the renderer passing the selection
information to the browser through OnSwapCompositorFrame using
CompositorFrameMetadata. This would get passed down to TouchSelectionController
which would decide whether or not it wants to show handles on the new
selection. This meant that the browser side needed to handle touch and split
the responsibility for handles across processes.

The new approach has Blink deciding when to show the touch handles. It does
this by keeping track of a isHandleVisible boolean in the VisibleSelection
class. We also add a boolean handle_visibility_changed_ to LayerTreeHost. This
is necessary because we always want to force a draw whenever the handle
visibility changed to notify the browser through the SwapCompositorFrame IPC.
The handles are in some sense part of the frame that should cause damage, and
this worked mostly by luck before (handle updates happened to piggyback on
selection updates).

Finally the handles are propagated to the browser in the
CompositorFrameMetadata as before except that only selection with handles are
propagated. Handleless selections are viewed the same way as no selection in
the browser. In other words, the browser doesn't know or care about selections
that have no handles. This means we had to remove the SELECTION_ESTABLISHED and
SELECTION_DISSOLVED from the SelectionEventType enum since we don’t know what
is happening with all selections. Luckily this was only used in
ContextualSearch and some tests and those instances were taken care of.

BUG=633281

Review-Url: https://codereview.chromium.org/2201853002
Cr-Commit-Position: refs/heads/master@{#444119}
Committed:
https://chromium.googlesource.com/chromium/src/+/1107a295dcee2f4ee3f6c24f836c...
==========

[commit-bot: I haz the power, 2017-01-17 20:06:41]

Committed patchset #106 (id:2090001) as
https://chromium.googlesource.com/chromium/src/+/1107a295dcee2f4ee3f6c24f836c...