Bug 81413

Summary: Rename Node::getRect/getPixelSnappedRect and remove ContainerNode::getRect
Product: WebKit Reporter: Emil A Eklund <eae>
Component: DOMAssignee: Emil A Eklund <eae>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, darin, dglazkov, eric, fpizlo, gustavo, gyuyoung.kim, hyatt, kling, mifenton, mitz, philn, rakuco, tkent, tonikitoo, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 81016, 96226    
Bug Blocks:    
Attachments:
Description Flags
Patch for landing
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Emil A Eklund
Reported 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?
Attachments
Patch for landing (2.10 KB, patch)
2012-03-19 16:20 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (17.98 KB, patch)
2012-08-21 13:24 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (24.51 KB, patch)
2012-08-21 14:05 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (24.87 KB, patch)
2012-08-21 15:09 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (26.21 KB, patch)
2012-08-21 17:51 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch for landing (27.65 KB, patch)
2012-09-07 18:22 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Emil A Eklund
Comment 1 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+.
Emil A Eklund
Comment 2 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.
Emil A Eklund
Comment 3 2012-08-21 13:24:58 PDT
Created attachment 159753 [details] Patch
Emil A Eklund
Comment 4 2012-08-21 14:05:18 PDT
Created attachment 159765 [details] Patch
Darin Adler
Comment 5 2012-08-21 14:33:47 PDT
Comment on attachment 159765 [details] Patch Why BoxRect in these names instead of just Box?
Emil A Eklund
Comment 6 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.
Darin Adler
Comment 7 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.
Emil A Eklund
Comment 8 2012-08-21 14:47:04 PDT
That makes sense. As these are the only versions in Node I'll rename them. Thanks!
Emil A Eklund
Comment 9 2012-08-21 15:09:11 PDT
Created attachment 159775 [details] Patch
Early Warning System Bot
Comment 10 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
Emil A Eklund
Comment 11 2012-08-21 17:51:59 PDT
Created attachment 159821 [details] Patch
Emil A Eklund
Comment 12 2012-09-04 09:06:01 PDT
Darin/Hyatt any further comments on this?
Dave Hyatt
Comment 13 2012-09-07 15:40:33 PDT
Comment on attachment 159821 [details] Patch r=me
WebKit Review Bot
Comment 14 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
Emil A Eklund
Comment 15 2012-09-07 18:22:49 PDT
Created attachment 162926 [details] Patch for landing
WebKit Review Bot
Comment 16 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>
WebKit Review Bot
Comment 17 2012-09-09 17:29:06 PDT
All reviewed patches have been landed. Closing bug.
Filip Pizlo
Comment 18 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
mitz
Comment 19 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>.
mitz
Comment 20 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>.
Emil A Eklund
Comment 21 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!
mitz
Comment 22 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>.
mitz
Comment 23 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.
Antonio Gomes
Comment 24 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?
Emil A Eklund
Comment 25 2012-09-12 09:04:29 PDT
I'm looking into it at the moment, it is being tracked by 96226.
Note You need to log in before you can comment on or make changes to this bug.