Bug 81413 - Rename Node::getRect/getPixelSnappedRect and remove ContainerNode::getRect
Summary: Rename Node::getRect/getPixelSnappedRect and remove ContainerNode::getRect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on: 81016 96226
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-16 14:53 PDT by Emil A Eklund
Modified: 2012-09-12 09:04 PDT (History)
17 users (show)

See Also:


Attachments
Patch for landing (2.10 KB, patch)
2012-03-19 16:20 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (17.98 KB, patch)
2012-08-21 13:24 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (24.51 KB, patch)
2012-08-21 14:05 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (24.87 KB, patch)
2012-08-21 15:09 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (26.21 KB, patch)
2012-08-21 17:51 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (27.65 KB, patch)
2012-09-07 18:22 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2012-03-16 14:53:46 PDT
Node::getRect isn't very descriptive and has the word get in the name which is generally disroucaged in WebKit. Come up with a more descriptive name and rename it.

Perhaps boundingBox or absoluteRect?
Comment 1 Emil A Eklund 2012-03-19 16:20:41 PDT
Created attachment 132704 [details]
Patch for landing

Added math.h include to FractionalLayoutUnit.h, should make the gtk bot happy. If it does I'll set cq+.
Comment 2 Emil A Eklund 2012-03-19 16:21:04 PDT
(In reply to comment #1)
> Created an attachment (id=132704) [details]
> Patch for landing
> 
> Added math.h include to FractionalLayoutUnit.h, should make the gtk bot happy. If it does I'll set cq+.

Wrong bug, please ignore patch.
Comment 3 Emil A Eklund 2012-08-21 13:24:58 PDT
Created attachment 159753 [details]
Patch
Comment 4 Emil A Eklund 2012-08-21 14:05:18 PDT
Created attachment 159765 [details]
Patch
Comment 5 Darin Adler 2012-08-21 14:33:47 PDT
Comment on attachment 159765 [details]
Patch

Why BoxRect in these names instead of just Box?
Comment 6 Emil A Eklund 2012-08-21 14:39:21 PDT
(In reply to comment #5)
> (From update of attachment 159765 [details])
> Why BoxRect in these names instead of just Box?

Trying to be consistent with existing methods that return a bounding box. Most methods we have that return a bounding box are named BoxRect, such as
contentBoxRect, clientBoxRect, borderBoxRectInRegion and absoluteBoundingBoxRect.

I'd be more than happy to rename this to just Box though if you think that is a better name. I certainly prefer it.
Comment 7 Darin Adler 2012-08-21 14:44:34 PDT
I think the relevant question is whether there is more than one type that could be used to return a box. For example, maybe there’s a quad and a rect, and the rect version is lossy. If there is not more than one type, then we should s/BoxRect/Box/g. I'd want to ask Hyatt, I guess, in case he knows something.
Comment 8 Emil A Eklund 2012-08-21 14:47:04 PDT
That makes sense. As these are the only versions in Node I'll rename them. Thanks!
Comment 9 Emil A Eklund 2012-08-21 15:09:11 PDT
Created attachment 159775 [details]
Patch
Comment 10 Early Warning System Bot 2012-08-21 16:43:31 PDT
Comment on attachment 159775 [details]
Patch

Attachment 159775 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13559061
Comment 11 Emil A Eklund 2012-08-21 17:51:59 PDT
Created attachment 159821 [details]
Patch
Comment 12 Emil A Eklund 2012-09-04 09:06:01 PDT
Darin/Hyatt any further comments on this?
Comment 13 Dave Hyatt 2012-09-07 15:40:33 PDT
Comment on attachment 159821 [details]
Patch

r=me
Comment 14 WebKit Review Bot 2012-09-07 15:59:32 PDT
Comment on attachment 159821 [details]
Patch

Rejecting attachment 159821 [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:
ed at 1608 (offset 3 lines).
patching file Source/WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/qt/Api/qwebelement.cpp
Hunk #1 succeeded at 546 (offset 1 line).
Hunk #2 succeeded at 1454 (offset 1 line).
patching file Source/WebKit/qt/Api/qwebpage.cpp
Hunk #1 succeeded at 1577 (offset -8 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'David Hyatt']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13774925
Comment 15 Emil A Eklund 2012-09-07 18:22:49 PDT
Created attachment 162926 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-09-09 17:29:00 PDT
Comment on attachment 162926 [details]
Patch for landing

Clearing flags on attachment: 162926

Committed r128006: <http://trac.webkit.org/changeset/128006>
Comment 17 WebKit Review Bot 2012-09-09 17:29:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Filip Pizlo 2012-09-09 18:00:50 PDT
(In reply to comment #17)
> All reviewed patches have been landed.  Closing bug.

I suspect this broke our MountainLion build:

http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20%28Build%29/builds/1003/steps/compile-webkit/logs/stdio
Comment 19 mitz 2012-09-09 18:02:11 PDT
(In reply to comment #16)
> (From update of attachment 162926 [details])
> Clearing flags on attachment: 162926
> 
> Committed r128006: <http://trac.webkit.org/changeset/128006>

This broke the build because SVGElement has its own non-virtual boundingBox. See for example <http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/3594/steps/compile-webkit/logs/stdio/text>.
Comment 20 mitz 2012-09-09 18:09:34 PDT
(In reply to comment #19)
> (In reply to comment #16)
> > (From update of attachment 162926 [details] [details])
> > Clearing flags on attachment: 162926
> > 
> > Committed r128006: <http://trac.webkit.org/changeset/128006>
> 
> This broke the build because SVGElement has its own non-virtual boundingBox. See for example <http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/3594/steps/compile-webkit/logs/stdio/text>.

I tried to fix this in <http://trac.webkit.org/r128009>.
Comment 21 Emil A Eklund 2012-09-09 18:11:37 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #16)
> > > (From update of attachment 162926 [details] [details] [details])
> > > Clearing flags on attachment: 162926
> > > 
> > > Committed r128006: <http://trac.webkit.org/changeset/128006>
> > 
> > This broke the build because SVGElement has its own non-virtual boundingBox. See for example <http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/3594/steps/compile-webkit/logs/stdio/text>.
> 
> I tried to fix this in <http://trac.webkit.org/r128009>.

Was just about to commit a similar fix myself, thanks!
Comment 22 mitz 2012-09-09 21:58:52 PDT
(In reply to comment #16)
> (From update of attachment 162926 [details])
> Clearing flags on attachment: 162926
> 
> Committed r128006: <http://trac.webkit.org/changeset/128006>

This caused three spatial navigation tests to fail. See for example <http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r128010%20(2654)/results.html>.
Comment 23 mitz 2012-09-09 22:01:18 PDT
(In reply to comment #22)
> (In reply to comment #16)
> > (From update of attachment 162926 [details] [details])
> > Clearing flags on attachment: 162926
> > 
> > Committed r128006: <http://trac.webkit.org/changeset/128006>
> 
> This caused three spatial navigation tests to fail. See for example <http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r128010%20(2654)/results.html>.

Filed bug 96226.
Comment 24 Antonio Gomes 2012-09-11 20:19:33 PDT
(In reply to comment #22)
> (In reply to comment #16)
> > (From update of attachment 162926 [details] [details])
> > Clearing flags on attachment: 162926
> > 
> > Committed r128006: <http://trac.webkit.org/changeset/128006>
> 
> This caused three spatial navigation tests to fail. See for example <http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r128010%20(2654)/results.html>.

If this is a rename, as the title says, it should really not fail any test, and even, leave the test skipped is not a nice solution.

Emil, do you have plans to investigate it?
Comment 25 Emil A Eklund 2012-09-12 09:04:29 PDT
I'm looking into it at the moment, it is being tracked by 96226.