Bug 69009

Created attachment 109037 [details] Patch

Comment on attachment 109037 [details] Patch How would you feel about changing the useTransforms param to be an enum, for better readability?

(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.

(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.

Comment on attachment 109037 [details] Patch r- since we're going to change things.

Created attachment 109063 [details] Patch

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); }

Created attachment 109064 [details] Patch

Comment on attachment 109064 [details] Patch Attachment 109064 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9889129

Created attachment 109084 [details] Patch

Created attachment 109085 [details] Patch

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

Comment on attachment 109085 [details] Patch Transferring r+ but please see comments on previous patch

Created attachment 109094 [details] Patch

(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.

Comment on attachment 109094 [details] Patch Attachment 109094 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9888457

Created attachment 109183 [details] Patch

Comment on attachment 109183 [details] Patch Attachment 109183 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9888612

Created attachment 109197 [details] Patch

Please don't keep putting patches up in review. You can just transfer my r+ if there are insubstantial changes.

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.

(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.

Created attachment 109200 [details] Patch

Created attachment 109311 [details] Patch

Created attachment 109515 [details] Patch

Did you mean to request review?

Comment on attachment 109515 [details] Patch Not much has changed but it finally builds on Windows!!!

(In reply to comment #26) > Did you mean to request review? Please, it should finally build on Windows now!

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.

(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.

Created attachment 109703 [details] Patch

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.

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

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

(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.

Created attachment 109976 [details] Patch

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

Created attachment 110010 [details] Patch for landing

Created attachment 110014 [details] Patch for landing

Comment on attachment 110014 [details] Patch for landing Clearing flags on attachment: 110014 Committed r96859: <http://trac.webkit.org/changeset/96859>

All reviewed patches have been landed. Closing bug.