Bug 71579 - [Chromium] Expose boundsInRootViewSpace function in WebElement
Summary: [Chromium] Expose boundsInRootViewSpace function in WebElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: csharp
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-04 11:47 PDT by csharp
Modified: 2011-12-07 16:33 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.14 KB, patch)
2011-11-04 11:49 PDT, csharp
no flags Details | Formatted Diff | Diff
Patch (2.31 KB, patch)
2011-11-04 12:51 PDT, csharp
no flags Details | Formatted Diff | Diff
Patch (2.36 KB, patch)
2011-11-08 12:28 PST, csharp
no flags Details | Formatted Diff | Diff
Patch (2.54 KB, patch)
2011-11-08 13:50 PST, csharp
no flags Details | Formatted Diff | Diff
Patch (2.69 KB, patch)
2011-11-10 07:57 PST, csharp
no flags Details | Formatted Diff | Diff
Patch (2.65 KB, patch)
2011-11-10 13:36 PST, csharp
no flags Details | Formatted Diff | Diff
Patch (2.89 KB, patch)
2011-11-17 08:12 PST, csharp
no flags Details | Formatted Diff | Diff
Patch (2.80 KB, patch)
2011-11-22 06:28 PST, csharp
no flags Details | Formatted Diff | Diff
Patch (2.96 KB, patch)
2011-11-22 13:11 PST, csharp
no flags Details | Formatted Diff | Diff
Patch (2.96 KB, patch)
2011-11-22 13:24 PST, csharp
no flags Details | Formatted Diff | Diff
Patch (2.94 KB, patch)
2011-12-07 07:36 PST, csharp
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description csharp 2011-11-04 11:47:43 PDT
Exposing getRect function in WebNode for chromium
Comment 1 csharp 2011-11-04 11:49:38 PDT
Created attachment 113691 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-04 11:53:23 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Ilya Sherman 2011-11-04 12:43:17 PDT
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.
Comment 4 csharp 2011-11-04 12:51:43 PDT
Created attachment 113699 [details]
Patch
Comment 5 csharp 2011-11-04 12:53:01 PDT
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.
Comment 6 Darin Fisher (:fishd, Google) 2011-11-08 11:29:59 PST
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?
Comment 7 Ilya Sherman 2011-11-08 11:55:45 PST
(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.
Comment 8 csharp 2011-11-08 12:28:24 PST
Created attachment 114134 [details]
Patch
Comment 9 csharp 2011-11-08 12:31:32 PST
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?
Comment 10 Darin Fisher (:fishd, Google) 2011-11-08 12:34:36 PST
(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.
Comment 11 Ilya Sherman 2011-11-08 12:41:23 PST
(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
Comment 12 csharp 2011-11-08 13:50:31 PST
Created attachment 114148 [details]
Patch
Comment 13 csharp 2011-11-08 13:51:55 PST
Ok, I added some comments to say that the values are document-relative and that it is only valid after a layout().
Comment 14 Darin Fisher (:fishd, Google) 2011-11-08 14:21:46 PST
(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...
Comment 15 Darin Fisher (:fishd, Google) 2011-11-08 14:24:17 PST
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?
Comment 16 csharp 2011-11-09 06:29:58 PST
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.
Comment 17 Ilya Sherman 2011-11-09 12:38:31 PST
(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.
Comment 18 csharp 2011-11-10 07:57:54 PST
Created attachment 114500 [details]
Patch
Comment 19 csharp 2011-11-10 08:00:04 PST
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.
Comment 20 WebKit Review Bot 2011-11-10 08:27:31 PST
Comment on attachment 114500 [details]
Patch

Attachment 114500 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10354408
Comment 21 Ilya Sherman 2011-11-10 13:07:15 PST
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 ].
Comment 22 csharp 2011-11-10 13:36:26 PST
Created attachment 114557 [details]
Patch
Comment 23 csharp 2011-11-10 13:38:49 PST
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.
Comment 24 Ilya Sherman 2011-11-10 14:04:03 PST
(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.
Comment 25 csharp 2011-11-15 11:35:56 PST
Ping.
Comment 26 Ilya Sherman 2011-11-16 17:47:35 PST
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
Comment 27 James Robinson 2011-11-16 17:57:05 PST
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.
Comment 28 csharp 2011-11-17 08:12:35 PST
Created attachment 115594 [details]
Patch
Comment 29 csharp 2011-11-17 08:14:25 PST
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.
Comment 30 Darin Fisher (:fishd, Google) 2011-11-21 16:45:56 PST
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
Comment 31 csharp 2011-11-22 06:28:51 PST
Created attachment 116214 [details]
Patch
Comment 32 csharp 2011-11-22 06:29:40 PST
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.
Comment 33 Darin Fisher (:fishd, Google) 2011-11-22 12:45:16 PST
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.
Comment 34 csharp 2011-11-22 13:11:00 PST
Created attachment 116267 [details]
Patch
Comment 35 csharp 2011-11-22 13:11:42 PST
Fixed the Changelog format
Comment 36 csharp 2011-11-22 13:24:21 PST
Created attachment 116269 [details]
Patch
Comment 37 csharp 2011-12-06 11:17:32 PST
ping
Comment 38 WebKit Review Bot 2011-12-06 14:19:16 PST
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
Comment 39 Ilya Sherman 2011-12-06 14:23:01 PST
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!
Comment 40 csharp 2011-12-07 07:36:04 PST
Created attachment 118209 [details]
Patch
Comment 41 csharp 2011-12-07 07:36:42 PST
Rebased and uploaded.
Comment 42 WebKit Review Bot 2011-12-07 16:33:05 PST
Comment on attachment 118209 [details]
Patch

Clearing flags on attachment: 118209

Committed r102283: <http://trac.webkit.org/changeset/102283>
Comment 43 WebKit Review Bot 2011-12-07 16:33:13 PST
All reviewed patches have been landed.  Closing bug.