Bug 71579 – [Chromium] Expose boundsInRootViewSpace function in WebElement

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Exposing getRect function in WebNode for chromium

Created attachment 113691 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Ilya Sherman]

Comment on attachment 113691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113691&action=review

> Source/WebKit/chromium/ChangeLog:3
> +        Exposing getRect function in WebNode for chromium

nit: Please expand this description to explain that we are exposing this function so that we can move the Autofill UI out of WebKit, and add a link to https://code.google.com/p/chromium/issues/detail?id=51644.

Created attachment 113699 [details]
Patch

Comment on attachment 113691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113691&action=review

>> Source/WebKit/chromium/ChangeLog:3
>> +        Exposing getRect function in WebNode for chromium
> 
> nit: Please expand this description to explain that we are exposing this function so that we can move the Autofill UI out of WebKit, and add a link to https://code.google.com/p/chromium/issues/detail?id=51644.

Fixed.

[Darin Fisher (:fishd, Google)]

Comment on attachment 113699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113699&action=review

> Source/WebKit/chromium/public/WebNode.h:117
> +    WEBKIT_EXPORT WebRect getRect() const;

hmm... this function is only valid if layout is up-to-date.  you should document that fact.

also, i wonder if this is really the best name for this method.  I realize that Node uses
this name, but RenderObject calls this property absoluteBoundingBoxRect.  that may be a
nicer name as it is much more descriptive.

are the coordinates document-relative or viewport-relative?

[Ilya Sherman]

(In reply to comment #6)
> (From update of attachment 113699 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113699&action=review
> 
> > Source/WebKit/chromium/public/WebNode.h:117
> > +    WEBKIT_EXPORT WebRect getRect() const;
> 
> hmm... this function is only valid if layout is up-to-date.  you should document that fact.

Is it fine for the function to call layout() internally?  If so, I'd prefer that we do that -- but I defer to your judgement, as you obviously have much more experience with all things WebKit API.

Created attachment 114134 [details]
Patch

Ok, I change getRect to absoluteBoundingBoxRect. 

I'm not sure if the values are document or viewport specific, the code that I'm working with in WebKit uses the values to setup a PopupContainer and I poked around it a bit but I was able to figure that out, sorry.

For the valid layout comment should I just add the following about the method?
// absoluteBoundingBoxRect is only valid if layout is up to date.

or is there a different place or way I should say it?

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> Is it fine for the function to call layout() internally?  If so, I'd prefer that we do that -- but I defer to your judgement, as you obviously have much more experience with all things WebKit API.

I believe that most of our existing APIs that return layout metrics do not internally force layout to be up-to-date.

[Ilya Sherman]

(In reply to comment #10)
> (In reply to comment #7)
> > Is it fine for the function to call layout() internally?  If so, I'd prefer that we do that -- but I defer to your judgement, as you obviously have much more experience with all things WebKit API.
> 
> I believe that most of our existing APIs that return layout metrics do not internally force layout to be up-to-date.

Fair enough.  I saw this change [1] get committed recently that made me think we might want to force layout in such methods.

[1] http://trac.webkit.org/changeset/94586/trunk/Source/WebKit/chromium/src/WebNode.cpp

Created attachment 114148 [details]
Patch

Ok, I added some comments to say that the values are document-relative and that it is only valid after a layout().

[Darin Fisher (:fishd, Google)]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > Is it fine for the function to call layout() internally?  If so, I'd prefer that we do that -- but I defer to your judgement, as you obviously have much more experience with all things WebKit API.
> > 
> > I believe that most of our existing APIs that return layout metrics do not internally force layout to be up-to-date.
> 
> Fair enough.  I saw this change [1] get committed recently that made me think we might want to force layout in such methods.
> 
> [1] http://trac.webkit.org/changeset/94586/trunk/Source/WebKit/chromium/src/WebNode.cpp

Hmm... hmm... well, Document::updateLayout() is cheap to call when there is no work to be done, so perhaps you should consider calling that.  I really haven't thought carefully enough about this issue.  Hmm...

[Darin Fisher (:fishd, Google)]

Comment on attachment 114148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114148&action=review

> Source/WebKit/chromium/public/WebNode.h:119
> +    // Note: This method only works properly after layout has occurred.

Given that we have the same comment on hasNonEmptyBoundingBox, I think it is OK to
have the same comment here.

I'm wondering what it means to return document-relative coordinates the Chromium.
I don't know that we have a good way on the Chromium side to map those coordinates
to viewport coordinates.  As a result, I'm not sure what you would do with these
coordinate values.  Are they meaningful to you when the node is contained within a
scrolled page?  What about a node in an iframe that is scrolled, contained in a
page that is also scrolled?

Hmm, I hadn't run into or though about those issues before, the test pages I worked with were very simple. I did do a pick of poking around in the code and I think that it might be possible to query the WebKit::WebFrames and the like to get enough location data to convert this value to a viewport coordinate. I'm not 100% sure, so I'll try and do some experimentation with this to see if this is possible, or if we need more functions exposed.
Regardless of my finding though I think we still need to expose this function because this seems to be the only place we can get the element specific position, regardless of its mapping, there just seems to be more work to complete the mapping then I had expected.

[Ilya Sherman]

(In reply to comment #16)
> Hmm, I hadn't run into or though about those issues before, the test pages I worked with were very simple. I did do a pick of poking around in the code and I think that it might be possible to query the WebKit::WebFrames and the like to get enough location data to convert this value to a viewport coordinate. I'm not 100% sure, so I'll try and do some experimentation with this to see if this is possible, or if we need more functions exposed.
> Regardless of my finding though I think we still need to expose this function because this seems to be the only place we can get the element specific position, regardless of its mapping, there just seems to be more work to complete the mapping then I had expected.

If the clients of this method are all interested in viewport-relative coordinates, it probably makes sense to do whatever conversion is necessary within this method.  That way, we hide the conversion complexity from clients, and we potentially need to expose fewer methods via the Chromium WebKit API.

Created attachment 114500 [details]
Patch

Ok, I did a bit of poking around and found a better function in Element to expose, boundsInWindowSpace. I did a few quick tests with it and the values it was giving seem to be what we were looking for.

[WebKit Review Bot]

Comment on attachment 114500 [details]
Patch

Attachment 114500 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10354408

[Ilya Sherman]

Comment on attachment 114500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114500&action=review

> Source/WebKit/chromium/ChangeLog:5
> +        This issue in chromium can be found at:

nit: Please capitalize "Chromium".

> Source/WebKit/chromium/ChangeLog:13
> +        (WebKit::WebNode::getRect):

nit: Please update this method name.

> Source/WebKit/chromium/public/WebElement.h:73
> +        WEBKIT_EXPORT WebRect boundsInWindowSpace();

nit: Can this method be marked const?

> Source/WebKit/chromium/src/WebElement.cpp:114
> +    return unwrap<Element>()->boundsInWindowSpace();

Oops, looks like this function just got renamed: [ http://trac.webkit.org/changeset/99778 ].

Created attachment 114557 [details]
Patch

Updated the function the to new name and fixed the problems in the ChangeLog file.

I don't think we can make this function const because one of the first things that it calls is updateLayoutIgnorePendingStylesheets, which looks like it can modify the objects.

[Ilya Sherman]

(In reply to comment #23)
> I don't think we can make this function const because one of the first things that it calls is updateLayoutIgnorePendingStylesheets, which looks like it can modify the objects.

Ah, true.  Nevermind then :)

Darin, please take another look when you have a chance.

Ping.

[Ilya Sherman]

James (Robinson) asks:

jamesr_: what does 'root view space' mean?
jamesr_: viewport space?
jamesr_: if there's a page scale applied is it scaled or unscaled?
jamesr_: sounds like a bad idea to expose an API if we aren't completely sure what it does

[James Robinson]

Comment on attachment 114557 [details]
Patch

I don't know what 'root view space' means.  I'm not actually totally sure what 'bounds' means in this context either - what happens if the element is rotated or transformed? The exact meaning of this function should be clearly documented in the API, and if necessary the function renamed to something clearer.

Created attachment 115594 [details]
Patch

Ok, I tried to provide a better explanation and name for this function. I also looked at the code and played around a bit and I am fairly certain that the value it gives will be the correct value for our viewport view.

[Darin Fisher (:fishd, Google)]

Comment on attachment 115594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115594&action=review

> Source/WebKit/chromium/public/WebElement.h:73
> +        // also called the Root View in Webkit.

nit: Webkit -> WebKit

Created attachment 116214 [details]
Patch

Comment on attachment 115594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115594&action=review

>> Source/WebKit/chromium/public/WebElement.h:73
>> +        // also called the Root View in Webkit.
> 
> nit: Webkit -> WebKit

Fixed.

[Darin Fisher (:fishd, Google)]

Comment on attachment 116214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116214&action=review

> Source/WebKit/chromium/ChangeLog:5
> +        This issue in chromium can be found at:

This ChangeLog is not formatted properly.  Sorry for not noticing before.
Please reformat like this:

   Summary text.
   [bugzilla link]

   More descriptive text, including link to crbug.com.

Created attachment 116267 [details]
Patch

Fixed the Changelog format

Created attachment 116269 [details]
Patch

ping

[WebKit Review Bot]

Comment on attachment 116269 [details]
Patch

Rejecting attachment 116269 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
o
  CXX(target) out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebBindings.o
  CXX(target) out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebDocument.o
  CXX(target) out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebElement.o
Source/WebKit/chromium/src/WebElement.cpp:34: fatal error: WebRect.h: No such file or directory
compilation terminated.
make: *** [out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebElement.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/10745077

[Ilya Sherman]

Sorry, looks like this didn't get committed earlier because I forgot to clear the r? flag when setting cq+, and now you'll need to rebase the patch.  I'll run it through the commit-queue properly once you rebase!

Created attachment 118209 [details]
Patch

Rebased and uploaded.

[WebKit Review Bot]

Comment on attachment 118209 [details]
Patch

Clearing flags on attachment: 118209

Committed r102283: <http://trac.webkit.org/changeset/102283>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.