RESOLVED FIXED 69009
Code clean-up: Make RenderObject::absoluteBoundingBoxRect and associated methods const and make its parameter useTransforms no longer default to false
https://bugs.webkit.org/show_bug.cgi?id=69009
Summary Code clean-up: Make RenderObject::absoluteBoundingBoxRect and associated meth...
Fady Samuel
Reported 2011-09-28 10:50:49 PDT
Code clean-up: Make RenderObject::absoluteBoundingBoxRect and associated methods const and make its parameter useTransforms no longer default
Attachments
Patch (33.44 KB, patch)
2011-09-28 10:54 PDT, Fady Samuel
no flags
Patch (36.64 KB, patch)
2011-09-28 13:29 PDT, Fady Samuel
no flags
Patch (36.63 KB, patch)
2011-09-28 13:37 PDT, Fady Samuel
no flags
Patch (39.18 KB, patch)
2011-09-28 16:12 PDT, Fady Samuel
no flags
Patch (39.19 KB, patch)
2011-09-28 16:25 PDT, Fady Samuel
no flags
Patch (39.40 KB, patch)
2011-09-28 17:04 PDT, Fady Samuel
no flags
Patch (44.04 KB, patch)
2011-09-29 12:01 PDT, Fady Samuel
no flags
Patch (45.38 KB, patch)
2011-09-29 13:31 PDT, Fady Samuel
no flags
Patch (45.38 KB, patch)
2011-09-29 14:02 PDT, Fady Samuel
no flags
Patch (45.47 KB, patch)
2011-09-30 10:44 PDT, Fady Samuel
no flags
Patch (45.81 KB, patch)
2011-10-03 13:12 PDT, Fady Samuel
no flags
Patch (45.38 KB, patch)
2011-10-04 15:15 PDT, Fady Samuel
no flags
Patch (45.33 KB, patch)
2011-10-06 10:50 PDT, Fady Samuel
no flags
Patch for landing (38.83 KB, patch)
2011-10-06 13:48 PDT, Fady Samuel
no flags
Patch for landing (45.31 KB, patch)
2011-10-06 13:55 PDT, Fady Samuel
no flags
Fady Samuel
Comment 1 2011-09-28 10:54:13 PDT
Simon Fraser (smfr)
Comment 2 2011-09-28 11:04:46 PDT
Comment on attachment 109037 [details] Patch How would you feel about changing the useTransforms param to be an enum, for better readability?
Fady Samuel
Comment 3 2011-09-28 11:06:41 PDT
(In reply to comment #2) > (From update of attachment 109037 [details]) > How would you feel about changing the useTransforms param to be an enum, for better readability? Will do, thanks.
Fady Samuel
Comment 4 2011-09-28 11:16:58 PDT
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 109037 [details] [details]) > > How would you feel about changing the useTransforms param to be an enum, for better readability? > > Will do, thanks. To minimize going back and forth, could you please give me the definition of the enum you had in mind? Thanks.
Simon Fraser (smfr)
Comment 5 2011-09-28 12:12:35 PDT
Comment on attachment 109037 [details] Patch r- since we're going to change things.
Fady Samuel
Comment 6 2011-09-28 13:29:22 PDT
Simon Fraser (smfr)
Comment 7 2011-09-28 13:37:03 PDT
Sorry, I was mid-typing and got pulled into a meeting. Another option here is to have: absoluteBoundingBoxRect(bool useTransforms = true) absoluteBoundingBoxRectIgnoringTransforms() const { return absoluteBoundingBoxRect(false); }
Fady Samuel
Comment 8 2011-09-28 13:37:29 PDT
Early Warning System Bot
Comment 9 2011-09-28 13:55:02 PDT
Fady Samuel
Comment 10 2011-09-28 16:12:45 PDT
Fady Samuel
Comment 11 2011-09-28 16:25:03 PDT
Simon Fraser (smfr)
Comment 12 2011-09-28 16:43:42 PDT
Comment on attachment 109084 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109084&action=review > Source/WebCore/dom/Element.cpp:576 > + return renderer()->view()->frameView()->contentsToScreen(renderer()->absoluteBoundingBoxRectIgnoringTransforms()); Please add a "// FIXME: this should probably respect transforms" > Source/WebCore/html/shadow/SliderThumbElement.cpp:220 > + LayoutPoint absoluteThumbOrigin = renderBox()->absoluteBoundingBoxRectIgnoringTransforms().location(); I'd like to see a comment saying why it's OK to ignore transforms. It's not clear to me that this code works correctly with transforms. > Source/WebCore/html/shadow/TextControlInnerElements.cpp:539 > + IntRect rect = renderer()->absoluteBoundingBoxRectIgnoringTransforms(); This looks broken, please add a FIXME > Source/WebCore/rendering/RenderFileUploadControl.cpp:135 > + LayoutUnit textY = buttonRenderer->absoluteBoundingBoxRectIgnoringTransforms().y() Seems like this will never work with transforms. Add a FIXME
Simon Fraser (smfr)
Comment 13 2011-09-28 16:44:14 PDT
Comment on attachment 109085 [details] Patch Transferring r+ but please see comments on previous patch
Fady Samuel
Comment 14 2011-09-28 17:04:30 PDT
Fady Samuel
Comment 15 2011-09-28 17:26:38 PDT
(In reply to comment #14) > Created an attachment (id=109094) [details] > Patch I will commit this tomorrow morning as I don't want to break anything in the evening.
Collabora GTK+ EWS bot
Comment 16 2011-09-29 02:15:09 PDT
Fady Samuel
Comment 17 2011-09-29 12:01:16 PDT
Collabora GTK+ EWS bot
Comment 18 2011-09-29 12:55:39 PDT
Fady Samuel
Comment 19 2011-09-29 13:31:30 PDT
Simon Fraser (smfr)
Comment 20 2011-09-29 13:36:28 PDT
Please don't keep putting patches up in review. You can just transfer my r+ if there are insubstantial changes.
Darin Adler
Comment 21 2011-09-29 13:41:10 PDT
Comment on attachment 109197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109197&action=review > Source/WebCore/dom/Element.cpp:576 > + // FIXME: this should probably respect transforms WebKit comment formatting style is: // FIXME: This should probably respect transforms. But an even better comment would have a word or two about why. It get us that much closer to thinking about what kind of test case we would construct. > Source/WebCore/html/shadow/SliderThumbElement.cpp:220 > + // FIXME: this should probably respect transforms Ditto. > Source/WebCore/rendering/RenderFileUploadControl.cpp:135 > + // FIXME: make this work with transforms. Same complaint about this.
Fady Samuel
Comment 22 2011-09-29 13:50:42 PDT
(In reply to comment #21) > (From update of attachment 109197 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109197&action=review > > > Source/WebCore/dom/Element.cpp:576 > > + // FIXME: this should probably respect transforms > > WebKit comment formatting style is: > > // FIXME: This should probably respect transforms. > > But an even better comment would have a word or two about why. It get us that much closer to thinking about what kind of test case we would construct. > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:220 > > + // FIXME: this should probably respect transforms > > Ditto. > > > Source/WebCore/rendering/RenderFileUploadControl.cpp:135 > > + // FIXME: make this work with transforms. > > Same complaint about this. I think the goal is (Simon, please correct me if I'm wrong) to make sure all instances of absoluteBoundingBoxRect take transforms into account, and any call that does not is suspect. The FIXME indicates this code needs further scrutiny and not necessarily a bug. I've filed bugs for each of these FIXMEs and will investigate further and either fix the bugs or remove the comment in later patchs. Please see the four bug reports under the Blocks section.
Fady Samuel
Comment 23 2011-09-29 14:02:27 PDT
Fady Samuel
Comment 24 2011-09-30 10:44:07 PDT
Fady Samuel
Comment 25 2011-10-03 13:12:25 PDT
Simon Fraser (smfr)
Comment 26 2011-10-03 17:13:20 PDT
Did you mean to request review?
Fady Samuel
Comment 27 2011-10-03 17:35:49 PDT
Comment on attachment 109515 [details] Patch Not much has changed but it finally builds on Windows!!!
Fady Samuel
Comment 28 2011-10-03 17:36:08 PDT
(In reply to comment #26) > Did you mean to request review? Please, it should finally build on Windows now!
Darin Adler
Comment 29 2011-10-04 11:23:40 PDT
Comment on attachment 109515 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109515&action=review Doesn’t seem like a big improvements, but seems OK. > Source/WebCore/html/shadow/SliderThumbElement.cpp:43 > +#include "RenderObject.h" This include is should not be added. RenderDeprecatedFlexibleBox.h already includes it, and if we did need to include something it would be farther down the inheritance tree than RenderObject.h. > Source/WebCore/html/shadow/SliderThumbElement.cpp:220 > + // FIXME: this should probably respect transforms Format of this comment is not right. We use sentence style where the first letter is capitalized and a period ends the sentence. > Source/WebCore/html/shadow/TextControlInnerElements.cpp:41 > +#include "RenderObject.h" This include is should not be added. RenderTextControlSingleLine.h already includes it, and if we did need to include something it would be farther down the inheritance tree than RenderObject.h. > Source/WebCore/rendering/RenderFileUploadControl.cpp:135 > + // FIXME: make this work with transforms. Again, wrong format.
Fady Samuel
Comment 30 2011-10-04 14:42:35 PDT
(In reply to comment #29) > (From update of attachment 109515 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109515&action=review > > Doesn’t seem like a big improvements, but seems OK. > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:43 > > +#include "RenderObject.h" > > This include is should not be added. RenderDeprecatedFlexibleBox.h already includes it, and if we did need to include something it would be farther down the inheritance tree than RenderObject.h. > > > Source/WebCore/html/shadow/SliderThumbElement.cpp:220 > > + // FIXME: this should probably respect transforms > > Format of this comment is not right. We use sentence style where the first letter is capitalized and a period ends the sentence. > > > Source/WebCore/html/shadow/TextControlInnerElements.cpp:41 > > +#include "RenderObject.h" > > This include is should not be added. RenderTextControlSingleLine.h already includes it, and if we did need to include something it would be farther down the inheritance tree than RenderObject.h. > > > Source/WebCore/rendering/RenderFileUploadControl.cpp:135 > > + // FIXME: make this work with transforms. > > Again, wrong format. Thanks, will fix. Ultimately, this patch stemmed out of my wanting to land this fix with layout tests: https://bugs.webkit.org/show_bug.cgi?id=66062 and reveal some other bugs that will fix bit by bit. I will be adding to and exposing PopupMenuClient to window.internals to allow for testing. This change was necessary (and independently useful), so I thought I'd do this as a first step.
Fady Samuel
Comment 31 2011-10-04 15:15:50 PDT
WebKit Review Bot
Comment 32 2011-10-04 15:24:15 PDT
Comment on attachment 109703 [details] Patch Rejecting attachment 109703 [details] from review queue. fsamuel@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
WebKit Review Bot
Comment 33 2011-10-04 15:57:27 PDT
Comment on attachment 109703 [details] Patch Rejecting attachment 109703 [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: ines). patching file Source/WebKit/qt/Api/qwebframe.cpp patching file Source/WebKit/qt/ChangeLog patching file Source/WebKit2/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit2/win/WebKit2.def patching file Source/WebKit2/win/WebKit2CFLite.def patching file Source/autotools/symbols.filter Hunk #1 succeeded at 41 with fuzz 2 (offset -2 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Simon Fraser', u'--for..." exit_code: 1 Full output: http://queues.webkit.org/results/9942523
WebKit Review Bot
Comment 34 2011-10-04 16:02:07 PDT
Comment on attachment 109703 [details] Patch Rejecting attachment 109703 [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: ines). patching file Source/WebKit/qt/Api/qwebframe.cpp patching file Source/WebKit/qt/ChangeLog patching file Source/WebKit2/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit2/win/WebKit2.def patching file Source/WebKit2/win/WebKit2CFLite.def patching file Source/autotools/symbols.filter Hunk #1 succeeded at 41 with fuzz 2 (offset -2 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Simon Fraser', u'--for..." exit_code: 1 Full output: http://queues.webkit.org/results/9954221
Fady Samuel
Comment 35 2011-10-06 07:36:50 PDT
(In reply to comment #34) > (From update of attachment 109703 [details]) > Rejecting attachment 109703 [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: > ines). > patching file Source/WebKit/qt/Api/qwebframe.cpp > patching file Source/WebKit/qt/ChangeLog > patching file Source/WebKit2/ChangeLog > Hunk #1 succeeded at 1 with fuzz 3. > patching file Source/WebKit2/win/WebKit2.def > patching file Source/WebKit2/win/WebKit2CFLite.def > patching file Source/autotools/symbols.filter > Hunk #1 succeeded at 41 with fuzz 2 (offset -2 lines). > > Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Simon Fraser', u'--for..." exit_code: 1 > > Full output: http://queues.webkit.org/results/9954221 Wow, this code changes very rapidly. It's hard to get it all to apply cleanly.
Fady Samuel
Comment 36 2011-10-06 10:50:02 PDT
WebKit Review Bot
Comment 37 2011-10-06 13:18:17 PDT
Comment on attachment 109976 [details] Patch Rejecting attachment 109976 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Last 500 characters of output: -commit-queue/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Updating OpenSource Current branch master is up to date. Updating chromium port dependencies using gclient... ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/9981323
Fady Samuel
Comment 38 2011-10-06 13:48:50 PDT
Created attachment 110010 [details] Patch for landing
Fady Samuel
Comment 39 2011-10-06 13:55:52 PDT
Created attachment 110014 [details] Patch for landing
WebKit Review Bot
Comment 40 2011-10-06 14:35:39 PDT
Comment on attachment 110014 [details] Patch for landing Clearing flags on attachment: 110014 Committed r96859: <http://trac.webkit.org/changeset/96859>
WebKit Review Bot
Comment 41 2011-10-06 14:35:47 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.