Bug 69009 – Code clean-up: Make RenderObject::absoluteBoundingBoxRect and associated methods const and make its parameter useTransforms no longer default to false

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Fady Samuel]

Code clean-up: Make RenderObject::absoluteBoundingBoxRect and associated methods const and make its parameter useTransforms no longer default

[Fady Samuel]

Created attachment 109037 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 109037 [details]
Patch

How would you feel about changing the useTransforms param to be an enum, for better readability?

[Fady Samuel]

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

(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 on attachment 109037 [details]
Patch

r- since we're going to change things.

[Fady Samuel]

Created attachment 109063 [details]
Patch

[Simon Fraser (smfr)]

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]

Created attachment 109064 [details]
Patch

[Early Warning System Bot]

Comment on attachment 109064 [details]
Patch

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

[Fady Samuel]

Created attachment 109084 [details]
Patch

[Fady Samuel]

Created attachment 109085 [details]
Patch

[Simon Fraser (smfr)]

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 on attachment 109085 [details]
Patch

Transferring r+ but please see comments on previous patch

[Fady Samuel]

Created attachment 109094 [details]
Patch

[Fady Samuel]

(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 on attachment 109094 [details]
Patch

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

[Fady Samuel]

Created attachment 109183 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 109183 [details]
Patch

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

[Fady Samuel]

Created attachment 109197 [details]
Patch

[Simon Fraser (smfr)]

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

[Darin Adler]

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]

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

Created attachment 109200 [details]
Patch

[Fady Samuel]

Created attachment 109311 [details]
Patch

[Fady Samuel]

Created attachment 109515 [details]
Patch

[Simon Fraser (smfr)]

Did you mean to request review?

[Fady Samuel]

Comment on attachment 109515 [details]
Patch

Not much has changed but it finally builds on Windows!!!

[Fady Samuel]

(In reply to comment #26)
> Did you mean to request review?

Please, it should finally build on Windows now!

[Darin Adler]

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]

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

Created attachment 109703 [details]
Patch

[WebKit Review Bot]

Comment on attachment 109703 [details]
Patch

Rejecting attachment 109703 [details] from review queue.

[e-mail] 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 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 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]

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

Created attachment 109976 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 110010 [details]
Patch for landing

[Fady Samuel]

Created attachment 110014 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 110014 [details]
Patch for landing

Clearing flags on attachment: 110014

Committed r96859: <http://trac.webkit.org/changeset/96859>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.