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

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.