Chromium plumbing to use the selection root bounds.

Chromium plumbing to use the selection root bounds.

This is asking the selection root bounds to Blink and send it back to
the RenderWidgetHostView implementation in the browser process.

BUG=236033
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257199

[timvolodine, 2014-03-05 19:50:46]

This approach seems to work in my preliminary testing. However more testing is
necessary especially to make sure the IPC is appropriately sent in all cases.
The corresponding Android patch:
https://codereview.chromium.org/188023002/.

https://codereview.chromium.org/186753002/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/186753002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2392: UpdateSelectionRootBounds();
Should this be android-only? Not sure if the selection issue applies to
platforms other than Android.

https://codereview.chromium.org/186753002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2518: void
RenderWidget::UpdateSelectionRootBounds() {
I think we need a better name for "UpdateSelectionRootBounds" and the
corresponding IPC. The actual use cases are bounds for clipping
insertion/selection handles. So something like UpdateVisibleBoundsForIMEHandles
seems more appropriate. wdyt?

[mlamouri (slow - plz ping), 2014-03-10 15:56:44]

https://codereview.chromium.org/186753002/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/186753002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2392: UpdateSelectionRootBounds();
On 2014/03/05 19:50:46, timvolodine wrote:
> Should this be android-only? Not sure if the selection issue applies to
> platforms other than Android.

I was considering making it Android only but I then decided to keep it as
::UpdateSelectionBounds. I do not know what are the consumers of
::UpdateSelectionBounds so maybe that analogy was a bad idea? Also, it's not
clear to me how sending those IPC could have a bad performance impact. I would
love to hear more about this.

https://codereview.chromium.org/186753002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:2518: void
RenderWidget::UpdateSelectionRootBounds() {
On 2014/03/05 19:50:46, timvolodine wrote:
> I think we need a better name for "UpdateSelectionRootBounds" and the
> corresponding IPC. The actual use cases are bounds for clipping
> insertion/selection handles. So something like
UpdateVisibleBoundsForIMEHandles
> seems more appropriate. wdyt?

I would prefer to have the name reflecting what the function actually do and it
sends selection root bounds to the browser process. I would prefer to consider
the fact that this is used by Chrome Android to know whether the selection
handlers are visible as a detail and some other consumers could use it for other
things.

[mlamouri (slow - plz ping), 2014-03-10 15:57:51]

cdn@chromium.org: Please review changes related to IPC.

[mlamouri (slow - plz ping), 2014-03-10 16:01:49]

nasko@chromium.org: Please review changes in 
- content/browser/frame_host/
- content/common/view_messages.h

(removing cdn@ as reviewer, nasko@ can review view_messages.h)

[mlamouri (slow - plz ping), 2014-03-10 16:05:08]

jochen@chromium.org: Please review changes in:
- content/browser/renderer_host/
- content/renderer/
- content/port/

[nasko, 2014-03-10 16:42:45]

kenrb@ is much better reviewer for the widget classes and is messages reviewer.

[kenrb, 2014-03-10 17:07:48]

For security review, the IPC message is fine.

Why are you adding methods to RenderWidgetHostViewGuest and
RenderWidgetHostViewChildFrame? I'm not sure if anybody has ever tried
instantiating either of those on Android before.

[mlamouri (slow - plz ping), 2014-03-10 17:50:55]

On 2014/03/10 17:07:48, kenrb wrote:
> For security review, the IPC message is fine.
> 
> Why are you adding methods to RenderWidgetHostViewGuest and
> RenderWidgetHostViewChildFrame? I'm not sure if anybody has ever tried
> instantiating either of those on Android before.

Because I changed RenderWidgetHostViewPort and those classes implement
RenderWidgetHostViewPort. For RenderWidgetHostViewChildFrame, I kept the
function empty and for RenderWidgetHostViewGuest, I forwarded the call. I
believe this is the same behaviour as SelectionBoundsChanged().

[kenrb, 2014-03-10 17:57:50]

On 2014/03/10 17:50:55, Mounir Lamouri wrote:
> On 2014/03/10 17:07:48, kenrb wrote:
> > For security review, the IPC message is fine.
> > 
> > Why are you adding methods to RenderWidgetHostViewGuest and
> > RenderWidgetHostViewChildFrame? I'm not sure if anybody has ever tried
> > instantiating either of those on Android before.
> 
> Because I changed RenderWidgetHostViewPort and those classes implement
> RenderWidgetHostViewPort. For RenderWidgetHostViewChildFrame, I kept the
> function empty and for RenderWidgetHostViewGuest, I forwarded the call. I
> believe this is the same behaviour as SelectionBoundsChanged().

RenderWidgetHostViewPort is also implemented by RenderWidgetHostViewAura,
RenderWidgetHostViewMac, and RenderWidgetHostViewGtk.

[mlamouri (slow - plz ping), 2014-03-11 12:07:08]

The Blink patch has changed and the method is now available in WebView. The new
patch is reflecting that. Also, all the plumbing is now specific to OS_ANDROID.

[kenrb, 2014-03-11 16:44:53]

lgtm for content/common/view_messages.h
Also for the RenderWidgetHostView implementations but I'm not an owner on those.

[jochen (gone - plz use gerrit), 2014-03-12 13:01:00]

lgtm with nits

https://codereview.chromium.org/186753002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl_android.cc (right):

https://codereview.chromium.org/186753002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl_android.cc:80: if (!webview() ||
handling_ime_event_) {
nit. no { }

https://codereview.chromium.org/186753002/diff/20001/content/renderer/render_...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/186753002/diff/20001/content/renderer/render_...
content/renderer/render_widget.h:538: virtual void UpdateSelectionRootBounds()
{}
function body should be in .cc file

[mlamouri (slow - plz ping), 2014-03-12 14:24:11]

jochen, nasko being away, would you be okay for me to use your review for the
parts I asked him to review?

https://codereview.chromium.org/186753002/diff/20001/content/renderer/render_...
File content/renderer/render_view_impl_android.cc (right):

https://codereview.chromium.org/186753002/diff/20001/content/renderer/render_...
content/renderer/render_view_impl_android.cc:80: if (!webview() ||
handling_ime_event_) {
On 2014/03/12 13:01:00, jochen wrote:
> nit. no { }

Done.

https://codereview.chromium.org/186753002/diff/20001/content/renderer/render_...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/186753002/diff/20001/content/renderer/render_...
content/renderer/render_widget.h:538: virtual void UpdateSelectionRootBounds()
{}
On 2014/03/12 13:01:00, jochen wrote:
> function body should be in .cc file

Done.

[mlamouri (slow - plz ping), 2014-03-12 14:29:01]

It seems that this CL is good to go as soon as the Blink CL will land.

Thanks you all for the quick reviews :)

[mlamouri (slow - plz ping), 2014-03-14 09:57:07]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-14 09:57:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/186753002/40001

[commit-bot: I haz the power, 2014-03-14 09:58:20]

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

[commit-bot: I haz the power, 2014-03-14 09:58:21]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[mlamouri (slow - plz ping), 2014-03-14 10:23:51]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-14 10:24:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/186753002/60001

[commit-bot: I haz the power, 2014-03-14 10:39:30]

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

[commit-bot: I haz the power, 2014-03-14 10:39:31]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[mlamouri (slow - plz ping), 2014-03-14 10:42:35]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-14 10:42:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/186753002/60001

[commit-bot: I haz the power, 2014-03-14 10:48:04]

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

[commit-bot: I haz the power, 2014-03-14 10:48:04]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[mlamouri (slow - plz ping), 2014-03-14 11:17:18]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-14 11:17:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/186753002/100001

[commit-bot: I haz the power, 2014-03-14 11:18:39]

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

[commit-bot: I haz the power, 2014-03-14 11:18:40]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[mlamouri (slow - plz ping), 2014-03-14 11:31:54]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-14 11:32:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/186753002/100001

[commit-bot: I haz the power, 2014-03-14 12:33:44]

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

[commit-bot: I haz the power, 2014-03-14 12:33:45]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[mlamouri (slow - plz ping), 2014-03-14 14:09:20]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-14 14:09:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/186753002/120001

[commit-bot: I haz the power, 2014-03-14 14:10:33]

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

[commit-bot: I haz the power, 2014-03-14 14:10:34]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[mlamouri (slow - plz ping), 2014-03-14 14:14:31]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-14 14:14:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/186753002/120001

[commit-bot: I haz the power, 2014-03-14 14:16:11]

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

[commit-bot: I haz the power, 2014-03-14 14:16:12]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[mlamouri (slow - plz ping), 2014-03-14 14:23:43]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-14 14:23:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/186753002/120001

[commit-bot: I haz the power, 2014-03-14 21:10:25]

Change committed as 257199

[jdduke (slow), 2014-05-29 00:52:39]

A revert of this CL has been created in
https://codereview.chromium.org/306803002/ by jdduke@chromium.org.

The reason for reverting is: The Android plumbing to use the root selection
bounds has been removed, and the
visibility calculation will soon be performed by the compositor
(see crbug.com/135959)..