Summary: | [Chromium] Expose boundsInRootViewSpace function in WebElement | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | csharp | ||||||||||||||||||||||||
Component: | New Bugs | Assignee: | csharp | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | csharp, darin, dglazkov, fishd, isherman, jamesr, webkit.review.bot | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Attachments: |
|
Description
csharp
2011-11-04 11:47:43 PDT
Created attachment 113691 [details]
Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. 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. 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? (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? (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. (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(). (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 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. (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. Comment on attachment 114500 [details] Patch Attachment 114500 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10354408 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. (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. 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 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. 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. 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 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 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. Comment on attachment 118209 [details] Patch Clearing flags on attachment: 118209 Committed r102283: <http://trac.webkit.org/changeset/102283> All reviewed patches have been landed. Closing bug. |