Expose the selection root bounds in WebView.

Expose the selection root bounds in WebView.

This will allow consumers to decide whether they want to UI related to
selection such as selection handlers in touch devices.

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

[mlamouri (slow - plz ping), 2014-03-05 00:01:10]

Unfortunately, I can't use WebWidgetTest to test this. If you have any hint in a
reliable way to test this, I would take it :)

[tkent, 2014-03-05 04:48:10]

https://codereview.chromium.org/177903010/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/177903010/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2158: if (root->shadowHost()
nit: You don't need to wrap this line.

https://codereview.chromium.org/177903010/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2164: boundingBox =
nit: You don't need to wrap this line.

https://codereview.chromium.org/177903010/diff/20001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/177903010/diff/20001/public/web/WebWidget.h#n...
public/web/WebWidget.h:200: virtual bool selectionRootBounds(WebRect& bounds)
const { return false; }
* Why do you add this function to WebWidget though only WebViewImpl has an
implementation?

* For consistency with Chromium style, please add "get" prefix, and use a
pointer for an out argument.
    virtual bool getSelectionRootBounds(WebRect* bounds) const ...

* Please document coordination system of the resultant WebRect.

[mlamouri (slow - plz ping), 2014-03-05 09:48:25]

On 2014/03/05 04:48:10, tkent wrote:
> * Why do you add this function to WebWidget though only WebViewImpl has an
> implementation?

Because it is a call that goes with selectionBounds() which is declared there.

> * For consistency with Chromium style, please add "get" prefix, and use a
> pointer for an out argument.
>     virtual bool getSelectionRootBounds(WebRect* bounds) const ...

I would be okay with that be WebWidget isn't following that convention.
Shouldn't I follow the style of the file before the project coding style?

> * Please document coordination system of the resultant WebRect.

Sure.

[mlamouri (slow - plz ping), 2014-03-05 09:52:45]

PTAL

https://codereview.chromium.org/177903010/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/177903010/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2158: if (root->shadowHost()
On 2014/03/05 04:48:10, tkent wrote:
> nit: You don't need to wrap this line.

Done.

https://codereview.chromium.org/177903010/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2164: boundingBox =
On 2014/03/05 04:48:10, tkent wrote:
> nit: You don't need to wrap this line.

Done.

https://codereview.chromium.org/177903010/diff/20001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/177903010/diff/20001/public/web/WebWidget.h#n...
public/web/WebWidget.h:200: virtual bool selectionRootBounds(WebRect& bounds)
const { return false; }
On 2014/03/05 04:48:10, tkent wrote:
> * Why do you add this function to WebWidget though only WebViewImpl has an
> implementation?
> 
> * For consistency with Chromium style, please add "get" prefix, and use a
> pointer for an out argument.
>     virtual bool getSelectionRootBounds(WebRect* bounds) const ...
> 
> * Please document coordination system of the resultant WebRect.

Done.

[timvolodine, 2014-03-05 19:41:01]

some nits

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2162: &&
toHTMLInputElement(root->shadowHost())->isText()))) {
no single-line brackets

https://codereview.chromium.org/177903010/diff/60001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/177903010/diff/60001/public/web/WebWidget.h#n...
public/web/WebWidget.h:198: // the out-parameter on success. |bounds| coordinate
will be relative to the
coordinate -> coordinates

https://codereview.chromium.org/177903010/diff/60001/public/web/WebWidget.h#n...
public/web/WebWidget.h:200: // Returns false if there is no selection or on
failure.
what does failure mean here? instead maybe better to say something like: "when
there is no underlying editable element"?

[mlamouri (slow - plz ping), 2014-03-06 13:45:31]

PTAL

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2162: &&
toHTMLInputElement(root->shadowHost())->isText()))) {
On 2014/03/05 19:41:02, timvolodine wrote:
> no single-line brackets

Given the size of the condition, I think having brackets is fair.

https://codereview.chromium.org/177903010/diff/60001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/177903010/diff/60001/public/web/WebWidget.h#n...
public/web/WebWidget.h:198: // the out-parameter on success. |bounds| coordinate
will be relative to the
On 2014/03/05 19:41:02, timvolodine wrote:
> coordinate -> coordinates

Done.

https://codereview.chromium.org/177903010/diff/60001/public/web/WebWidget.h#n...
public/web/WebWidget.h:200: // Returns false if there is no selection or on
failure.
On 2014/03/05 19:41:02, timvolodine wrote:
> what does failure mean here? instead maybe better to say something like: "when
> there is no underlying editable element"?

You can have a selection even though there is no editable element. I removed
"failure" and just kept "no selection".

[timvolodine, 2014-03-06 17:02:01]

Thanks Mounir, some nits below.
This patch looks good to me, but we need owners approval.
tkent@ could you have a look please?

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2157: // If the selection is inside a form control,
the root will be a <div> that
What about textarea elements? Could you list all cases here where the
root->shadowHost() is required?

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2162: &&
toHTMLInputElement(root->shadowHost())->isText()))) {
On 2014/03/06 13:45:31, Mounir Lamouri wrote:
> On 2014/03/05 19:41:02, timvolodine wrote:
> > no single-line brackets
> 
> Given the size of the condition, I think having brackets is fair.

I was actually referring to:
http://dev.chromium.org/blink/coding-style#TOC-Indentation, item5.
Yes the condition looks a bit heavy, maybe it makes sense to put
root->shadowHost() in a local variable to make it more compact and avoid
repetitions..

[jamesr, 2014-03-06 21:55:06]

https://codereview.chromium.org/177903010/diff/80001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/177903010/diff/80001/public/web/WebWidget.h#n...
public/web/WebWidget.h:201: virtual bool selectionRootBounds(WebRect& bounds)
const { return false; }
why add this to WebWidget if you are only implementing it in WebViewImpl?  If
this is not relevant for other WebWidget implementations, then put it on WebView
(or possibly WebFrame or whatever makes sense).

[mlamouri (slow - plz ping), 2014-03-07 13:50:28]

https://codereview.chromium.org/177903010/diff/80001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/177903010/diff/80001/public/web/WebWidget.h#n...
public/web/WebWidget.h:201: virtual bool selectionRootBounds(WebRect& bounds)
const { return false; }
On 2014/03/06 21:55:07, jamesr wrote:
> why add this to WebWidget if you are only implementing it in WebViewImpl?  If
> this is not relevant for other WebWidget implementations, then put it on
WebView
> (or possibly WebFrame or whatever makes sense).

This is being used in RenderWidget and it is interacting with a WebWidget, not a
WebView. I followed the same design as WebWidget::selectionBounds(), which is
declared in WebWidget but only implemented in WebView.

[jamesr, 2014-03-08 02:02:51]

On 2014/03/07 13:50:28, Mounir Lamouri wrote:
> https://codereview.chromium.org/177903010/diff/80001/public/web/WebWidget.h
> File public/web/WebWidget.h (right):
> 
>
https://codereview.chromium.org/177903010/diff/80001/public/web/WebWidget.h#n...
> public/web/WebWidget.h:201: virtual bool selectionRootBounds(WebRect& bounds)
> const { return false; }
> On 2014/03/06 21:55:07, jamesr wrote:
> > why add this to WebWidget if you are only implementing it in WebViewImpl? 
If
> > this is not relevant for other WebWidget implementations, then put it on
> WebView
> > (or possibly WebFrame or whatever makes sense).
> 
> This is being used in RenderWidget and it is interacting with a WebWidget, not
a
> WebView.

If it should work for all RenderWidgets then you will need to implement it for
all WebWidget types, no?  If it doesn't need to do anything for Widgets that are
not Views then put the interface and the caller on the View interfaces.

> I followed the same design as WebWidget::selectionBounds(), which is
> declared in WebWidget but only implemented in WebView.

That one seems wrong to me.

[mlamouri (slow - plz ping), 2014-03-10 14:26:32]

On 2014/03/08 02:02:51, jamesr wrote:
> On 2014/03/07 13:50:28, Mounir Lamouri wrote:
> > https://codereview.chromium.org/177903010/diff/80001/public/web/WebWidget.h
> > File public/web/WebWidget.h (right):
> > 
> >
>
https://codereview.chromium.org/177903010/diff/80001/public/web/WebWidget.h#n...
> > public/web/WebWidget.h:201: virtual bool selectionRootBounds(WebRect&
bounds)
> > const { return false; }
> > On 2014/03/06 21:55:07, jamesr wrote:
> > > why add this to WebWidget if you are only implementing it in WebViewImpl? 
> If
> > > this is not relevant for other WebWidget implementations, then put it on
> > WebView
> > > (or possibly WebFrame or whatever makes sense).
> > 
> > This is being used in RenderWidget and it is interacting with a WebWidget,
not
> a
> > WebView.
> 
> If it should work for all RenderWidgets then you will need to implement it for
> all WebWidget types, no?  If it doesn't need to do anything for Widgets that
are
> not Views then put the interface and the caller on the View interfaces.

It seems that WebWidget is inherited by WebView, WebPagePopup, WebPopupMenu and
PepperWidget. WebPagePopup and WebPopupMenu simply override ::create() and
PepperWidget seems to be very specific to fullscreen related to pepper (part of
render_widget_fullscreen_pepper.cc). In other words, the only real subclass of
WebWidget is WebView. The others are empty shells or implementation solving a
very specific purpose.

In that situation, it's not clear to me what you want me to do. It seems that
what I'm doing is what is expected to me given the current design. Changing the
current design sounds orthogonal to the problem this CL is trying to solve.

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

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2157: // If the selection is inside a form control,
the root will be a <div> that
On 2014/03/06 17:02:02, timvolodine wrote:
> What about textarea elements? Could you list all cases here where the
> root->shadowHost() is required?

Done.

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2162: &&
toHTMLInputElement(root->shadowHost())->isText()))) {
On 2014/03/06 17:02:02, timvolodine wrote:
> On 2014/03/06 13:45:31, Mounir Lamouri wrote:
> > On 2014/03/05 19:41:02, timvolodine wrote:
> > > no single-line brackets
> > 
> > Given the size of the condition, I think having brackets is fair.
> 
> I was actually referring to:
> http://dev.chromium.org/blink/coding-style#TOC-Indentation, item5.
> Yes the condition looks a bit heavy, maybe it makes sense to put
> root->shadowHost() in a local variable to make it more compact and avoid
> repetitions..

I fixed the brackets. Switching to root->shadowHost() to shadowHost
unfortunately doesn't make the condition more readable.

[timvolodine, 2014-03-10 17:49:59]

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/177903010/diff/60001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2162: &&
toHTMLInputElement(root->shadowHost())->isText()))) {
On 2014/03/10 16:05:59, Mounir Lamouri wrote:
> On 2014/03/06 17:02:02, timvolodine wrote:
> > On 2014/03/06 13:45:31, Mounir Lamouri wrote:
> > > On 2014/03/05 19:41:02, timvolodine wrote:
> > > > no single-line brackets
> > > 
> > > Given the size of the condition, I think having brackets is fair.
> > 
> > I was actually referring to:
> > http://dev.chromium.org/blink/coding-style#TOC-Indentation, item5.
> > Yes the condition looks a bit heavy, maybe it makes sense to put
> > root->shadowHost() in a local variable to make it more compact and avoid
> > repetitions..
> 
> I fixed the brackets. Switching to root->shadowHost() to shadowHost
> unfortunately doesn't make the condition more readable.

The more I look at this now the more I am for putting the root->shadowHost()
into a local variable. There are 5x root->shadowHost() calls which are not
completely trivial, so this will save some CPU work (in addition to making the
code more compact).

[jamesr, 2014-03-10 20:33:19]

Widgets are a base type for something that has graphical output and can accept
user input.  This includes web contents, various types of popups, pepper
fullscreen widgets, etc.  The Widget and WidgetClient APIs expose enough API to
handle this - for instance, to accept input the widget classes need some basic
info about focus, selection+IME support, etc.  Widgets are not necessarily web
content, so any calls specific to being web content are not appropriate at the
Widget level.

Every content::RenderWidget is associated with a blink::WebWidget which has
multiple subclasses.

View is a subclass of Widget used when the content is web content handled by
Blink.  A WebView has a frame tree with documents and a large number of web
content-specific APIs.  A WebView is always backed by a WebViewImpl which is in
turn associated with a WebCore::Page.

Every content::RenderViewImpl (which inherits from content::RenderWidget) is
associated with a blink::WebView.

If you're adding new functionality, the decision of whether to put it on widget
or view is simple.  If the API only has meaning for web contents, then put the
chromium code in content::RenderViewImpl, the API on blink::WebView and the
blink implementation in blink::WebViewImpl.  If the functionality makes sense
for all widget types, not just web contents, then put the chromium code in
content::RenderWidget, blink API in blink::WebWidget, and implementations for it
in all blink::WebWidget implementations.

[jamesr, 2014-03-10 20:35:34]

https://codereview.chromium.org/177903010/diff/100001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/177903010/diff/100001/public/web/WebWidget.h#...
public/web/WebWidget.h:201: virtual bool selectionRootBounds(WebRect& bounds)
const { return false; }
not lgtm. If this only makes sense to implement for WebViews, then put the API
on WebView.

[mlamouri (slow - plz ping), 2014-03-10 21:01:25]

https://codereview.chromium.org/177903010/diff/100001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/177903010/diff/100001/public/web/WebWidget.h#...
public/web/WebWidget.h:201: virtual bool selectionRootBounds(WebRect& bounds)
const { return false; }
On 2014/03/10 20:35:35, jamesr wrote:
> not lgtm. If this only makes sense to implement for WebViews, then put the API
> on WebView.

Thanks for the explanations. I have uploaded a new version that does not touch
WebWidget.h but only WebView.h.

[jamesr, 2014-03-11 01:34:44]

Thanks, the public/web/ changes now LGTM.

Are integer coordinates going to be sufficiently precise?  If we want to
position something relative to the top-left corner of this rectangle, for
example, it'll be off by a bit for some scale/scroll combinations depending on
where the rounding happens.  What coordinate space is this in?

[mlamouri (slow - plz ping), 2014-03-11 11:27:59]

On 2014/03/11 01:34:44, jamesr wrote:
> Thanks, the public/web/ changes now LGTM.

Do you want to review the Source/web/ changes or will you L-GTM those when your
next comments will be addressed?

> Are integer coordinates going to be sufficiently precise?  If we want to
> position something relative to the top-left corner of this rectangle, for
> example, it'll be off by a bit for some scale/scroll combinations depending on
> where the rounding happens.

That's a fair point. I used integer values because it is currently meant to be
used with WebWidget::selectionBounds() and that provides two WebRect (left and
right handles). In both cases, the scaling happen on the IntRect. In one hand, I
understand that returning rounded values might lead to imprecisions depending on
when the rounding happens but on the other hand, if we return non-rounded values
we will surely end up having imprecisions compared to the values returned from
selectionBounds(). Also, it seems that contentsToWindow() is expecting IntRect.
Is there another way to get the coordinates relative to the window in float?

> What coordinate space is this in?

This is in the window coordinates I believe. It seems that it might or not
include the chrome window depending on the platform (according to
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
Again, this is in pair with WebWidget::selectionBounds(). In practice, on
Android and Linux Aura, the coordinates are relative to the content (all chrome
UI is excluded).

[timvolodine, 2014-03-11 11:54:28]

On 2014/03/11 11:27:59, Mounir Lamouri wrote:
> On 2014/03/11 01:34:44, jamesr wrote:
> > Thanks, the public/web/ changes now LGTM.
> 
> Do you want to review the Source/web/ changes or will you L-GTM those when
your
> next comments will be addressed?
> 
> > Are integer coordinates going to be sufficiently precise?  If we want to
> > position something relative to the top-left corner of this rectangle, for
> > example, it'll be off by a bit for some scale/scroll combinations depending
on
> > where the rounding happens.
> 
> That's a fair point. I used integer values because it is currently meant to be
> used with WebWidget::selectionBounds() and that provides two WebRect (left and
> right handles). In both cases, the scaling happen on the IntRect. In one hand,
I
> understand that returning rounded values might lead to imprecisions depending
on
> when the rounding happens but on the other hand, if we return non-rounded
values
> we will surely end up having imprecisions compared to the values returned from
> selectionBounds(). Also, it seems that contentsToWindow() is expecting
IntRect.
> Is there another way to get the coordinates relative to the window in float?
> 
> > What coordinate space is this in?
> 
> This is in the window coordinates I believe. It seems that it might or not
> include the chrome window depending on the platform (according to
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
> Again, this is in pair with WebWidget::selectionBounds(). In practice, on
> Android and Linux Aura, the coordinates are relative to the content (all
chrome
> UI is excluded).

FYI: When testing on Android I did not encounter any problems with round-off
errors. The root selection bounds rectangle is converted to physical pixels on
the Android side and the computed values seemed to be whole integers (or very
very close, e.g. with .99999 fractional part).

[jamesr, 2014-03-11 21:58:38]

The source/web/ changes need some tests but I'd be happy to review once you add
them.

https://codereview.chromium.org/177903010/diff/120001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/177903010/diff/120001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:856: // a text field.
this seems reasonable it's a bit subtle - you need some tests to make sure that
this is doing what you want.

[mlamouri (slow - plz ping), 2014-03-12 18:43:49]

James, PTAL at the patch with tests.

[jamesr, 2014-03-12 21:28:38]

lgtm

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

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-12 21:29:17]

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

[mlamouri (slow - plz ping), 2014-03-12 21:38:55]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2014-03-12 21:39:16]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-12 21:39:18]

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

[commit-bot: I haz the power, 2014-03-12 22:00:37]

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

[commit-bot: I haz the power, 2014-03-12 22:00:38]

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

[mlamouri (slow - plz ping), 2014-03-12 22:05:09]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-12 22:05:15]

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

[commit-bot: I haz the power, 2014-03-12 23:07:01]

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

[commit-bot: I haz the power, 2014-03-12 23:07:02]

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

[mlamouri (slow - plz ping), 2014-03-12 23:33:50]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-03-12 23:33:54]

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

[commit-bot: I haz the power, 2014-03-13 02:20:21]

Change committed as 169078

[Miguel Garcia, 2014-03-20 08:23:37]

Pass by review

https://codereview.chromium.org/177903010/diff/150001/Source/web/WebViewImpl.h
File Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/177903010/diff/150001/Source/web/WebViewImpl....
Source/web/WebViewImpl.h:297: virtual void getSelectionRootBounds(WebRect&
bounds) const OVERRIDE;
so, there is a selectionBounds method up in line 158. Would it make sense to
move this up there? Or perhaps even merge them? It is not entirely clear what
the difference is just based on the method name is it?