Implement TouchSelectionEditing controls for OOPIF.

Implement TouchSelectionEditing controls for OOPIF.

This CL revises the TouchSelection editing code to work with
cross-process iframes.

The basic design extends TouchSelectionControllerClientAura
to also implement TouchSelectionControllerManager, and it
multiplexes sub-frame's TouchSelectionControllerChildFrames
internally in a manner transparent to the
TouchSelectionController.

The manager defines an |active_client_| to be the most recent
RenderWidgetHostViewX to receive a selection bounds update in
its CompositorFrameMetadata, with the rules that and
active_client_ cannot be displayed by another view's update
if that update (i) sets the selection bounds to be empty, or (ii)
marks the bounds as !visible() (this occurs when the view loses
focus).

Since the tear-down order of the top-level RenderWidgetHostView
and its sub-frame views is not well defined, each sub-frame view
registers as an observer with the
TouchSelectionControllerClientManager and detaches itself if it
sees the manager is about to be destroyed.

At present, context menus still do not work in OOPIF subframes,
but this is being tracked in https://crbug.com/723799.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1xQLL2Hlm5IXSGmPZBRdQY8bN1spIYo9Ra-bSUVYqP5g/edit?usp=sharing

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2883653002
Cr-Commit-Position: refs/heads/master@{#474680}
Committed: https://chromium.googlesource.com/chromium/src/+/9a65831b1415a39a1614783ab3cc2faa5caa3330

[wjmaclean, 2017-05-12 21:15:25]

Description was changed from

==========
Implement TouchSelectionEditing controls for OOPIF.

<better description to come ...>

Design doc:
<link to come ...>

BUG=470662
==========

to

==========
Implement TouchSelectionEditing controls for OOPIF.

<better description to come ...>

Design doc:
<link to come ...>

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2017-05-12 21:15:52]

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

[commit-bot: I haz the power, 2017-05-12 21:17: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, 2017-05-12 21:23:21]

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

[commit-bot: I haz the power, 2017-05-12 21:23:22]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wjmaclean, 2017-05-12 22:15:46]

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

[commit-bot: I haz the power, 2017-05-12 22:17: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, 2017-05-12 22:54:05]

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

[commit-bot: I haz the power, 2017-05-12 22:54:06]

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

[wjmaclean, 2017-05-17 22:21:29]

Description was changed from

==========
Implement TouchSelectionEditing controls for OOPIF.

<better description to come ...>

Design doc:
<link to come ...>

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement TouchSelectionEditing controls for OOPIF.

This CL revises the TouchSelection editing code to work with
cross-process iframes.

The basic design extends TouchSelectionControllerClientAura
to also implement TouchSelectionControllerManager, and it
multiplexes sub-frame's TouchSelectionControllerChildFrames
internally in a manner transparent to the
TouchSelectionController.

The manager defines an |active_client_| to be the most recent
RenderWidgetHostViewX to receive a selection bounds update in
its CompositorFrameMetadata, with the rules that and
active_client_ cannot be displayed by another view's update
if that update (i) sets the selection bounds to be empty, or (ii)
marks the bounds as !visible() (this occurs when the view loses
focus).

Since the tear-down order of the top-level RenderWidgetHostView
and its sub-frame views is not well defined, each sub-frame view
registers as an observer with the
TouchSelectionControllerClientManager and detaches itself if it
sees the manager is about to be destroyed.

At present, context menus still do not work in OOPIF subframes,
but this is being tracked in https://crbug.com/723799.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1xQLL2Hlm5IXSGmPZBRdQY8bN1s...

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[wjmaclean, 2017-05-17 22:21:46]

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

[commit-bot: I haz the power, 2017-05-17 22:22:38]

Dry run: CQ is trying da patch.

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

[wjmaclean, 2017-05-17 22:23:52]

wjmaclean@chromium.org changed reviewers:
+ creis@chromium.org, kenrb@chromium.org

[wjmaclean, 2017-05-17 22:23:53]

kenrb@ - could you please take a look?

I still have some work to do on this, including verifying it passes existing
tests (and fixing things if it doesn't), but I think the bulk of the approach is
complete.

Making the sub-frame context menus is to be done in a follow-on CL.

[commit-bot: I haz the power, 2017-05-17 23:43:22]

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

[commit-bot: I haz the power, 2017-05-17 23:43:23]

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...)

[wjmaclean, 2017-05-18 14:48:12]

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

[commit-bot: I haz the power, 2017-05-18 14:48: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, 2017-05-18 15:16:52]

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

[commit-bot: I haz the power, 2017-05-18 15:16:53]

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_...)

[wjmaclean, 2017-05-18 15:46:19]

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

[commit-bot: I haz the power, 2017-05-18 15:46:44]

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-05-18 16:34:11]

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

[commit-bot: I haz the power, 2017-05-18 16:34:12]

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...)

[wjmaclean, 2017-05-18 17:32:02]

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

[commit-bot: I haz the power, 2017-05-18 17:34: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, 2017-05-18 20:22:58]

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

[commit-bot: I haz the power, 2017-05-18 20:22:59]

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_...)

[wjmaclean, 2017-05-23 13:38:31]

kenrb@ - ping? :-)

[kenrb, 2017-05-23 16:19:03]

A few thoughts below. It looks like this has diverged a bit from the design
document, so you will want to update that at some point.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:33: #include
"content/common/content_switches_internal.h"
Why is this include being added?

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:122: // Make
sure we're not using the zero-valued default for
This is surprising. Why wouldn't this be initialized to 1.0 in
RenderWidgetHostViewBase? That's the identity for multiplicative values,
initializing to 0 seems to be just asking for browser process crashes.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:130: // We
will only have a manarer on Aura (and eventually Android).
nit: /s/manarer/manager
Also, I'd mark this specifically as a TODO for Android to make it easier to
remember to update the comment when Android TSE OOPIF support is implemented.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:364:
selection_controller_client_.reset();
I'm trying to understand this. UnregisterFrameSinkId() is only called from the
derived class, RWHVGuest, as a way to disconnect the input routing path when the
Guest is detached. Is this path intended to be active for non-OOPIF GuestView?
If so, how does the observer get cleared from the manager?

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:201: gfx::Point
GetViewOriginInRoot() const;
This one warrants a comment explaining what it returns (and also disambiguating
'origin', which is an overloaded term in a web browser).

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/touch_selection_controller_client_child_frame.cc
(right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.cc:65:
transformed_selection.end != selection_end_) {
I'm not sure that I understand the contention problem properly, but I have a
concern about this. I presume the problem is that frames in separate processes
can have different simultaneous active selections? In that case, what happens if
the top-level frame scrolls? Does that change the selection position in both
frames? If that is true, then this would still be a race between the two RWHVs
becoming the active_frame_ in the manager.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.cc:81:
return false;
Thinking ahead a bit, is there an easy way to proxy this and SetNeedsAnimate()
to the root RWHV? I think that results in the right behavior on both Aura and
Android, as well as avoiding any #define directives. Or possibly the Aura client
could return without proxying to the internal client.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/touch_selection_controller_client_child_frame.h
(right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.h:6:
#define
CONTENT_BROWSER_FRAME_HOST_INPUT_TOUCH_SELECTION_CONTROLLER_CLIENT_CHILD_FRAME_H_
nit: 'INPUT' isn't in the path or the file name.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.h:15:
class RenderWidgetHostViewChildFrame;
I'm not sure about this, but it might be something to run by a content/ owner:
It might be better to put this class in content/browser/renderer_host, and add
an exception to content/browser/renderer_host/DEPS to allow the .cc file to
include RenderWidgetHostViewChildFrame. RWHVCF really belongs in renderer_host,
but nobody has gotten around to moving it because we have to set up some kind of
delegation to CrossProcessFrameConnector. I think it would be better to start
keeping files like this in renderer_host so we don't end up with more improper
dependencies to untangle later on.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.h:18:
// An implementation of |TouchSelectionControllerClient| to be used in Aura's
nit: This shouldn't be Aura-specific, even though it is currently only wired to
work on Aura. I would expect this class to work on any platform where the root
RWHV returns a TouchSelectionControllerClientManager.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.h
(right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:114:
// Keep tracke of which client interface to use.
nit: /s/tracke/track

https://codereview.chromium.org/2883653002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:420: virtual
TouchSelectionControllerClientManager*
A comment could be good here, mentioning that this only returns non-null on
platforms that implement touch selection, currently Aura and Android.

[wjmaclean, 2017-05-23 17:00:41]

I'll put up a new CL shortly, but please take a look at my answers to your
questions in case you have further questions.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:33: #include
"content/common/content_switches_internal.h"
On 2017/05/23 16:19:02, kenrb wrote:
> Why is this include being added?

Removed.

Let over from an earlier version of the CL.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:122: // Make
sure we're not using the zero-valued default for
On 2017/05/23 16:19:02, kenrb wrote:
> This is surprising. Why wouldn't this be initialized to 1.0 in
> RenderWidgetHostViewBase? That's the identity for multiplicative values,
> initializing to 0 seems to be just asking for browser process crashes.

I have no idea why it was initialized to zero, but it's been in the code for a
long time. I think it was assumed that some early call to GetScreenInfo() would
set this to it's correct value, but it's hard to do that here (e.g. we can get
here during the detach of an interstitial page, in which case calling
GetScreenInfo() can actually crash). But even if we initialize to 1.f in RWHVB,
we need to somehow get the actual DSF ... I've made the assumption here that if
our root view had a non-zero DSF value, then that would be safe for use here.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:130: // We
will only have a manarer on Aura (and eventually Android).
On 2017/05/23 16:19:02, kenrb wrote:
> nit: /s/manarer/manager
> Also, I'd mark this specifically as a TODO for Android to make it easier to
> remember to update the comment when Android TSE OOPIF support is implemented.

Done.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:364:
selection_controller_client_.reset();
On 2017/05/23 16:19:02, kenrb wrote:
> I'm trying to understand this. UnregisterFrameSinkId() is only called from the
> derived class, RWHVGuest, as a way to disconnect the input routing path when
the
> Guest is detached. Is this path intended to be active for non-OOPIF GuestView?
> If so, how does the observer get cleared from the manager?

Hmm, good question. TSE has never worked in GuestView before now, although it
appears to work now for non-oopif (unplanned) as well as oopif cases. So this
patway should be supported.

Thanks for noticing the lack of detaching the observer ... I've fixed that.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:201: gfx::Point
GetViewOriginInRoot() const;
On 2017/05/23 16:19:02, kenrb wrote:
> This one warrants a comment explaining what it returns (and also
disambiguating
> 'origin', which is an overloaded term in a web browser).

Done.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/touch_selection_controller_client_child_frame.cc
(right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.cc:65:
transformed_selection.end != selection_end_) {
On 2017/05/23 16:19:02, kenrb wrote:
> I'm not sure that I understand the contention problem properly, but I have a
> concern about this. I presume the problem is that frames in separate processes
> can have different simultaneous active selections? In that case, what happens
if
> the top-level frame scrolls? Does that change the selection position in both
> frames? If that is true, then this would still be a race between the two RWHVs
> becoming the active_frame_ in the manager.

I've done this on the assumption that the user is only actively selecting in one
frame at a time, and that frame should be considered active for TSE purposes.

To answer your question: yes, each frame will have its own selection, but for
TSE we will only consider one as active.

I *don't* think we should end up in a race though, at least not if we consider
user-gesture initiated selections. But this code is important since the signal
we're using, namely the selection data in CompositorFrameMetadata, is used no
only to select but also to un-select, or just to re-render a selection from blue
to grey when it is in the frame that is loosing focus. We have no guarantees
about the ordering of the focus related re-rendering events, so this code guards
against make a view the active client when it is just de-focusing *after* the
newly focused view has sent its compositor frame.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.cc:81:
return false;
On 2017/05/23 16:19:02, kenrb wrote:
> Thinking ahead a bit, is there an easy way to proxy this and SetNeedsAnimate()
> to the root RWHV? I think that results in the right behavior on both Aura and
> Android, as well as avoiding any #define directives. Or possibly the Aura
client
> could return without proxying to the internal client.

We could just have the TSE Client Manager code not forward to here, meaning the
child frame would always do whatever the root view did (which seems sensible in
and of itself). The manager has access to the root RWHV, so it could be routed
that way.

Should I make that chane in the CL, or is this follow-on work?

Just curious, which #define directives does this avoid (I can't seem to find any
related to this *in the TSE code*.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/touch_selection_controller_client_child_frame.h
(right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.h:6:
#define
CONTENT_BROWSER_FRAME_HOST_INPUT_TOUCH_SELECTION_CONTROLLER_CLIENT_CHILD_FRAME_H_
On 2017/05/23 16:19:03, kenrb wrote:
> nit: 'INPUT' isn't in the path or the file name.

Done.

(Sorry, another bit of update-cruft.)

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.h:15:
class RenderWidgetHostViewChildFrame;
On 2017/05/23 16:19:03, kenrb wrote:
> I'm not sure about this, but it might be something to run by a content/ owner:
> It might be better to put this class in content/browser/renderer_host, and add
> an exception to content/browser/renderer_host/DEPS to allow the .cc file to
> include RenderWidgetHostViewChildFrame. RWHVCF really belongs in
renderer_host,
> but nobody has gotten around to moving it because we have to set up some kind
of
> delegation to CrossProcessFrameConnector. I think it would be better to start
> keeping files like this in renderer_host so we don't end up with more improper
> dependencies to untangle later on.

creis@ - before I move this, can you weigh in please?

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.h:18:
// An implementation of |TouchSelectionControllerClient| to be used in Aura's
On 2017/05/23 16:19:02, kenrb wrote:
> nit: This shouldn't be Aura-specific, even though it is currently only wired
to
> work on Aura. I would expect this class to work on any platform where the root
> RWHV returns a TouchSelectionControllerClientManager.

Done.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.h
(right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.h:114:
// Keep tracke of which client interface to use.
On 2017/05/23 16:19:03, kenrb wrote:
> nit: /s/tracke/track

Done.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:420: virtual
TouchSelectionControllerClientManager*
On 2017/05/23 16:19:03, kenrb wrote:
> A comment could be good here, mentioning that this only returns non-null on
> platforms that implement touch selection, currently Aura and Android.

Done.

[Charlie Reis, 2017-05-23 18:39:52]

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/touch_selection_controller_client_child_frame.h
(right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.h:15:
class RenderWidgetHostViewChildFrame;
On 2017/05/23 17:00:41, wjmaclean wrote:
> On 2017/05/23 16:19:03, kenrb wrote:
> > I'm not sure about this, but it might be something to run by a content/
owner:
> > It might be better to put this class in content/browser/renderer_host, and
add
> > an exception to content/browser/renderer_host/DEPS to allow the .cc file to
> > include RenderWidgetHostViewChildFrame. RWHVCF really belongs in
> renderer_host,
> > but nobody has gotten around to moving it because we have to set up some
kind
> of
> > delegation to CrossProcessFrameConnector. I think it would be better to
start
> > keeping files like this in renderer_host so we don't end up with more
improper
> > dependencies to untangle later on.
> 
> creis@ - before I move this, can you weigh in please?

I was wondering exactly this when I glanced at the CL earlier, but then saw
RWHVCF is in frame_host as well.  I'm glad to get the context from Ken, and now
I see there's a TODO for moving RWHVCF to renderer_host in renderer_host/DEPS. 
As a result, I agree with Ken's recommendation, but I'd ask for a bug to be
filed and listed on the TODOs in the DEPS file.  Thanks!

[wjmaclean, 2017-05-24 13:42:59]

Ken, did you have any further suggestions or changes? I think the only remaining
issue is the one around the logic for setting the active client ... I think what
I have is correct, but there may be clearer ways to present it? But I think the
current version is also low risk in the sense that it shouldn't break in the
non-oopif case, since the active client will always be the top-level RWHV.

[kenrb, 2017-05-24 14:54:28]

lgtm

I'm not certain about that part either, I don't know how this works well enough
to say whether there might be cases that toggle the active_client at the wrong
time. But I agree that it is low risk because it won't break non-OOPIF, and it
makes sense to go ahead with this and see if any bugs fall out.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/touch_selection_controller_client_child_frame.cc
(right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.cc:65:
transformed_selection.end != selection_end_) {
On 2017/05/23 17:00:41, wjmaclean wrote:
> On 2017/05/23 16:19:02, kenrb wrote:
> > I'm not sure that I understand the contention problem properly, but I have a
> > concern about this. I presume the problem is that frames in separate
processes
> > can have different simultaneous active selections? In that case, what
happens
> if
> > the top-level frame scrolls? Does that change the selection position in both
> > frames? If that is true, then this would still be a race between the two
RWHVs
> > becoming the active_frame_ in the manager.
> 
> I've done this on the assumption that the user is only actively selecting in
one
> frame at a time, and that frame should be considered active for TSE purposes.
> 
> To answer your question: yes, each frame will have its own selection, but for
> TSE we will only consider one as active.
> 
> I *don't* think we should end up in a race though, at least not if we consider
> user-gesture initiated selections. But this code is important since the signal
> we're using, namely the selection data in CompositorFrameMetadata, is used no
> only to select but also to un-select, or just to re-render a selection from
blue
> to grey when it is in the frame that is loosing focus. We have no guarantees
> about the ordering of the focus related re-rendering events, so this code
guards
> against make a view the active client when it is just de-focusing *after* the
> newly focused view has sent its compositor frame.

Acknowledged.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.cc:81:
return false;
On 2017/05/23 17:00:41, wjmaclean wrote:
> On 2017/05/23 16:19:02, kenrb wrote:
> > Thinking ahead a bit, is there an easy way to proxy this and
SetNeedsAnimate()
> > to the root RWHV? I think that results in the right behavior on both Aura
and
> > Android, as well as avoiding any #define directives. Or possibly the Aura
> client
> > could return without proxying to the internal client.
> 
> We could just have the TSE Client Manager code not forward to here, meaning
the
> child frame would always do whatever the root view did (which seems sensible
in
> and of itself). The manager has access to the root RWHV, so it could be routed
> that way.
> 
> Should I make that chane in the CL, or is this follow-on work?
> 
> Just curious, which #define directives does this avoid (I can't seem to find
any
> related to this *in the TSE code*.

When this gets wired for Android it would have to be done anyway. I guess it
doesn't matter that much whether this change is done now or then.

I meant that we don't want to add any #defines here when we add Android support.
Android returns true for SupportsAnimation(), and we want this class to be
agnostic.

https://codereview.chromium.org/2883653002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2883653002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:203: // This
returns the origin of this view in the coordinates of the root
nit: "origin of this view" could be confusing, maybe "origin of this view's
bounding rect"?

[wjmaclean, 2017-05-24 17:08:30]

Patchset #9 (id:160001) has been deleted

[wjmaclean, 2017-05-24 17:10:08]

Thanks Ken ... I've revise so the ChildFrame client defers to the root view's
client. This will make sure children are consistent with the root view, and
avoid the need for any #define-s.

creis@ - please take a look?

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/touch_selection_controller_client_child_frame.cc
(right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.cc:81:
return false;
On 2017/05/24 14:54:28, kenrb wrote:
> On 2017/05/23 17:00:41, wjmaclean wrote:
> > On 2017/05/23 16:19:02, kenrb wrote:
> > > Thinking ahead a bit, is there an easy way to proxy this and
> SetNeedsAnimate()
> > > to the root RWHV? I think that results in the right behavior on both Aura
> and
> > > Android, as well as avoiding any #define directives. Or possibly the Aura
> > client
> > > could return without proxying to the internal client.
> > 
> > We could just have the TSE Client Manager code not forward to here, meaning
> the
> > child frame would always do whatever the root view did (which seems sensible
> in
> > and of itself). The manager has access to the root RWHV, so it could be
routed
> > that way.
> > 
> > Should I make that chane in the CL, or is this follow-on work?
> > 
> > Just curious, which #define directives does this avoid (I can't seem to find
> any
> > related to this *in the TSE code*.
> 
> When this gets wired for Android it would have to be done anyway. I guess it
> doesn't matter that much whether this change is done now or then.
> 
> I meant that we don't want to add any #defines here when we add Android
support.
> Android returns true for SupportsAnimation(), and we want this class to be
> agnostic.

Acknowledged.

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
File content/browser/frame_host/touch_selection_controller_client_child_frame.h
(right):

https://codereview.chromium.org/2883653002/diff/100001/content/browser/frame_...
content/browser/frame_host/touch_selection_controller_client_child_frame.h:15:
class RenderWidgetHostViewChildFrame;
On 2017/05/23 18:39:51, Charlie Reis wrote:
> On 2017/05/23 17:00:41, wjmaclean wrote:
> > On 2017/05/23 16:19:03, kenrb wrote:
> > > I'm not sure about this, but it might be something to run by a content/
> owner:
> > > It might be better to put this class in content/browser/renderer_host, and
> add
> > > an exception to content/browser/renderer_host/DEPS to allow the .cc file
to
> > > include RenderWidgetHostViewChildFrame. RWHVCF really belongs in
> > renderer_host,
> > > but nobody has gotten around to moving it because we have to set up some
> kind
> > of
> > > delegation to CrossProcessFrameConnector. I think it would be better to
> start
> > > keeping files like this in renderer_host so we don't end up with more
> improper
> > > dependencies to untangle later on.
> > 
> > creis@ - before I move this, can you weigh in please?
> 
> I was wondering exactly this when I glanced at the CL earlier, but then saw
> RWHVCF is in frame_host as well.  I'm glad to get the context from Ken, and
now
> I see there's a TODO for moving RWHVCF to renderer_host in renderer_host/DEPS.

> As a result, I agree with Ken's recommendation, but I'd ask for a bug to be
> filed and listed on the TODOs in the DEPS file.  Thanks!

Done.

https://codereview.chromium.org/2883653002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/2883653002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:203: // This
returns the origin of this view in the coordinates of the root
On 2017/05/24 14:54:28, kenrb wrote:
> nit: "origin of this view" could be confusing, maybe "origin of this view's
> bounding rect"?

Done.

[Charlie Reis, 2017-05-25 02:29:43]

This is way beyond my level of understanding, so I'm mainly deferring to Ken's
review and the tests.  (And I am requesting another test below.)  :)

Otherwise LGTM with nits.  Thanks!

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
File content/browser/renderer_host/DEPS (right):

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
content/browser/renderer_host/DEPS:50: ],
Please put this underneath Ken's TODO below, since it should also go away when
RWHVCF moves.

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:201:
// udate if the client is different and not making a change to empty, or
nit: update

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc
(right):

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura_browsertest.cc:415:
// *inside* the oopif, but not on the text, to clear.
I would recommend adding a test for this now, if possible.  There's much less
incentive to after the CL lands, and it helps us reason about whether we have
bugs in this change.  :)

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.h
(right):

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.h:19:
// implementations of TouchSelectionControllerClientManager.
Sounds generic to all clients-- should we also mention that this is for the
OOPIF case, and a bit of why we need something special for that case?

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_manager.h
(right):

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_manager.h:22:
class CONTENT_EXPORT TouchSelectionControllerClientManager {
Class level comment would help.

[wjmaclean, 2017-05-25 14:04:35]

Test added, and comments addressed.

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
File content/browser/renderer_host/DEPS (right):

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
content/browser/renderer_host/DEPS:50: ],
On 2017/05/25 02:29:43, Charlie Reis (overloaded) wrote:
> Please put this underneath Ken's TODO below, since it should also go away when
> RWHVCF moves.

Done.

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc
(right):

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_aura.cc:201:
// udate if the client is different and not making a change to empty, or
On 2017/05/25 02:29:43, Charlie Reis (overloaded) wrote:
> nit: update

Done.

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.h
(right):

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_child_frame.h:19:
// implementations of TouchSelectionControllerClientManager.
On 2017/05/25 02:29:43, Charlie Reis (overloaded) wrote:
> Sounds generic to all clients-- should we also mention that this is for the
> OOPIF case, and a bit of why we need something special for that case?

Done.

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
File
content/browser/renderer_host/input/touch_selection_controller_client_manager.h
(right):

https://codereview.chromium.org/2883653002/diff/180001/content/browser/render...
content/browser/renderer_host/input/touch_selection_controller_client_manager.h:22:
class CONTENT_EXPORT TouchSelectionControllerClientManager {
On 2017/05/25 02:29:43, Charlie Reis (overloaded) wrote:
> Class level comment would help.

Done.

[wjmaclean, 2017-05-25 14:04:42]

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

[commit-bot: I haz the power, 2017-05-25 14:05: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, 2017-05-25 14:07:11]

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

[commit-bot: I haz the power, 2017-05-25 14:07:12]

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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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_...)

[wjmaclean, 2017-05-25 15:25:08]

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

[commit-bot: I haz the power, 2017-05-25 15:25:37]

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-05-25 16:45:47]

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

[commit-bot: I haz the power, 2017-05-25 16:45:49]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2017-05-25 16:58:46]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2017-05-25 16:58:47]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2883653002/#ps220001
(title: "Rebase to master@{#474649}.")

[commit-bot: I haz the power, 2017-05-25 16:59:19]

CQ is trying da patch.

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

[Charlie Reis, 2017-05-25 17:03:57]

Thanks!  LGTM.

[commit-bot: I haz the power, 2017-05-25 17:04:02]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1495731526673450,
"parent_rev": "d3e57373452b02aabe667d9027c3e2fad6ac6562", "commit_rev":
"9a65831b1415a39a1614783ab3cc2faa5caa3330"}

[commit-bot: I haz the power, 2017-05-25 17:04:13]

Description was changed from

==========
Implement TouchSelectionEditing controls for OOPIF.

This CL revises the TouchSelection editing code to work with
cross-process iframes.

The basic design extends TouchSelectionControllerClientAura
to also implement TouchSelectionControllerManager, and it
multiplexes sub-frame's TouchSelectionControllerChildFrames
internally in a manner transparent to the
TouchSelectionController.

The manager defines an |active_client_| to be the most recent
RenderWidgetHostViewX to receive a selection bounds update in
its CompositorFrameMetadata, with the rules that and
active_client_ cannot be displayed by another view's update
if that update (i) sets the selection bounds to be empty, or (ii)
marks the bounds as !visible() (this occurs when the view loses
focus).

Since the tear-down order of the top-level RenderWidgetHostView
and its sub-frame views is not well defined, each sub-frame view
registers as an observer with the
TouchSelectionControllerClientManager and detaches itself if it
sees the manager is about to be destroyed.

At present, context menus still do not work in OOPIF subframes,
but this is being tracked in https://crbug.com/723799.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1xQLL2Hlm5IXSGmPZBRdQY8bN1s...

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement TouchSelectionEditing controls for OOPIF.

This CL revises the TouchSelection editing code to work with
cross-process iframes.

The basic design extends TouchSelectionControllerClientAura
to also implement TouchSelectionControllerManager, and it
multiplexes sub-frame's TouchSelectionControllerChildFrames
internally in a manner transparent to the
TouchSelectionController.

The manager defines an |active_client_| to be the most recent
RenderWidgetHostViewX to receive a selection bounds update in
its CompositorFrameMetadata, with the rules that and
active_client_ cannot be displayed by another view's update
if that update (i) sets the selection bounds to be empty, or (ii)
marks the bounds as !visible() (this occurs when the view loses
focus).

Since the tear-down order of the top-level RenderWidgetHostView
and its sub-frame views is not well defined, each sub-frame view
registers as an observer with the
TouchSelectionControllerClientManager and detaches itself if it
sees the manager is about to be destroyed.

At present, context menus still do not work in OOPIF subframes,
but this is being tracked in https://crbug.com/723799.

Design doc:
https://docs.google.com/a/chromium.org/document/d/1xQLL2Hlm5IXSGmPZBRdQY8bN1s...

BUG=470662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2883653002
Cr-Commit-Position: refs/heads/master@{#474680}
Committed:
https://chromium.googlesource.com/chromium/src/+/9a65831b1415a39a1614783ab3cc...
==========

[commit-bot: I haz the power, 2017-05-25 17:04:15]

Committed patchset #11 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/9a65831b1415a39a1614783ab3cc...

[EhsanK, 2017-05-29 15:41:44]

ekaramad@chromium.org changed reviewers:
+ ekaramad@chromium.org

[EhsanK, 2017-05-29 15:41:45]

https://codereview.chromium.org/2883653002/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883653002/diff/220001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:792: if
(!text_input_manager_ || !GetFocusedWidget())
Just a comment that came to my mind when I was reading this part of code to work
on https://crbug.com/714771:

RenderWidgetHostViewBase::GetFocusedWidget() uses
RenderWidgetHostDelegate::GetFocusedRenderWidgetHost() on its own
RenderWidgetHost (not that of the page).

Therefore, when render_widget_host_ && render_widget_host_->delegate() exist,
GetFocusedWidget() always returns the |render_widget_host_|.

Having said this, I believe this should change to |render_widget_host_| here and
we should only pass |this| to TextInputManager::GetTextSelection().

If the intention is to look for focused RenderWidgetHostView in page then we
should use
render_widget_host_->delegate()->GetFocusedRenderWidgetHost(RenderWidgetHostImpl::From(
frame_connector_->GetRootRenderWidgetHostView()->GetRenderWidgetHost()));

[wjmaclean, 2017-05-29 16:39:06]

https://codereview.chromium.org/2883653002/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883653002/diff/220001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:792: if
(!text_input_manager_ || !GetFocusedWidget())
On 2017/05/29 15:41:45, EhsanK wrote:
> Just a comment that came to my mind when I was reading this part of code to
work
> on https://crbug.com/714771:
> 
> RenderWidgetHostViewBase::GetFocusedWidget() uses
> RenderWidgetHostDelegate::GetFocusedRenderWidgetHost() on its own
> RenderWidgetHost (not that of the page).
> 
> Therefore, when render_widget_host_ && render_widget_host_->delegate() exist,
> GetFocusedWidget() always returns the |render_widget_host_|.
> 
> Having said this, I believe this should change to |render_widget_host_| here
and
> we should only pass |this| to TextInputManager::GetTextSelection().
> 
> If the intention is to look for focused RenderWidgetHostView in page then we
> should use
>
render_widget_host_->delegate()->GetFocusedRenderWidgetHost(RenderWidgetHostImpl::From(
> frame_connector_->GetRootRenderWidgetHostView()->GetRenderWidgetHost()));

Good to know ... I'll try this out, and revise it to match what you suggest in a
new CL. I don't know if it will help with the bug, but we'll see what happens.

[EhsanK, 2017-05-29 17:11:30]

On 2017/05/29 16:39:06, wjmaclean wrote:
>
https://codereview.chromium.org/2883653002/diff/220001/content/browser/frame_...
> File content/browser/frame_host/render_widget_host_view_child_frame.cc
(right):
> 
>
https://codereview.chromium.org/2883653002/diff/220001/content/browser/frame_...
> content/browser/frame_host/render_widget_host_view_child_frame.cc:792: if
> (!text_input_manager_ || !GetFocusedWidget())
> On 2017/05/29 15:41:45, EhsanK wrote:
> > Just a comment that came to my mind when I was reading this part of code to
> work
> > on https://crbug.com/714771:
> > 
> > RenderWidgetHostViewBase::GetFocusedWidget() uses
> > RenderWidgetHostDelegate::GetFocusedRenderWidgetHost() on its own
> > RenderWidgetHost (not that of the page).
> > 
> > Therefore, when render_widget_host_ && render_widget_host_->delegate()
exist,
> > GetFocusedWidget() always returns the |render_widget_host_|.
> > 
> > Having said this, I believe this should change to |render_widget_host_| here
> and
> > we should only pass |this| to TextInputManager::GetTextSelection().
> > 
> > If the intention is to look for focused RenderWidgetHostView in page then we
> > should use
> >
>
render_widget_host_->delegate()->GetFocusedRenderWidgetHost(RenderWidgetHostImpl::From(
> > frame_connector_->GetRootRenderWidgetHostView()->GetRenderWidgetHost()));
> 
> Good to know ... I'll try this out, and revise it to match what you suggest in
a
> new CL. I don't know if it will help with the bug, but we'll see what happens.

Yes. No it is not related to the bug actually. Thanks!