Bug 69009 - Code clean-up: Make RenderObject::absoluteBoundingBoxRect and associated methods const and make its parameter useTransforms no longer default to false
Summary: Code clean-up: Make RenderObject::absoluteBoundingBoxRect and associated meth...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
Depends on:
Blocks: 69047 69051 69053 69052
  Show dependency treegraph
 
Reported: 2011-09-28 10:50 PDT by Fady Samuel
Modified: 2011-10-06 14:35 PDT (History)
7 users (show)

See Also:


Attachments
Patch (33.44 KB, patch)
2011-09-28 10:54 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (36.64 KB, patch)
2011-09-28 13:29 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (36.63 KB, patch)
2011-09-28 13:37 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (39.18 KB, patch)
2011-09-28 16:12 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (39.19 KB, patch)
2011-09-28 16:25 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (39.40 KB, patch)
2011-09-28 17:04 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (44.04 KB, patch)
2011-09-29 12:01 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (45.38 KB, patch)
2011-09-29 13:31 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (45.38 KB, patch)
2011-09-29 14:02 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (45.47 KB, patch)
2011-09-30 10:44 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (45.81 KB, patch)
2011-10-03 13:12 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (45.38 KB, patch)
2011-10-04 15:15 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (45.33 KB, patch)
2011-10-06 10:50 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch for landing (38.83 KB, patch)
2011-10-06 13:48 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch for landing (45.31 KB, patch)
2011-10-06 13:55 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 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
Comment 1 Fady Samuel 2011-09-28 10:54:13 PDT
Created attachment 109037 [details]
Patch
Comment 2 Simon Fraser (smfr) 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?
Comment 3 Fady Samuel 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.
Comment 4 Fady Samuel 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.
Comment 5 Simon Fraser (smfr) 2011-09-28 12:12:35 PDT
Comment on attachment 109037 [details]
Patch

r- since we're going to change things.
Comment 6 Fady Samuel 2011-09-28 13:29:22 PDT
Created attachment 109063 [details]
Patch
Comment 7 Simon Fraser (smfr) 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); }
Comment 8 Fady Samuel 2011-09-28 13:37:29 PDT
Created attachment 109064 [details]
Patch
Comment 9 Early Warning System Bot 2011-09-28 13:55:02 PDT
Comment on attachment 109064 [details]
Patch

Attachment 109064 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9889129
Comment 10 Fady Samuel 2011-09-28 16:12:45 PDT
Created attachment 109084 [details]
Patch
Comment 11 Fady Samuel 2011-09-28 16:25:03 PDT
Created attachment 109085 [details]
Patch
Comment 12 Simon Fraser (smfr) 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
Comment 13 Simon Fraser (smfr) 2011-09-28 16:44:14 PDT
Comment on attachment 109085 [details]
Patch

Transferring r+ but please see comments on previous patch
Comment 14 Fady Samuel 2011-09-28 17:04:30 PDT
Created attachment 109094 [details]
Patch
Comment 15 Fady Samuel 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.
Comment 16 Collabora GTK+ EWS bot 2011-09-29 02:15:09 PDT
Comment on attachment 109094 [details]
Patch

Attachment 109094 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9888457
Comment 17 Fady Samuel 2011-09-29 12:01:16 PDT
Created attachment 109183 [details]
Patch
Comment 18 Collabora GTK+ EWS bot 2011-09-29 12:55:39 PDT
Comment on attachment 109183 [details]
Patch

Attachment 109183 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9888612
Comment 19 Fady Samuel 2011-09-29 13:31:30 PDT
Created attachment 109197 [details]
Patch
Comment 20 Simon Fraser (smfr) 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.
Comment 21 Darin Adler 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.
Comment 22 Fady Samuel 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.
Comment 23 Fady Samuel 2011-09-29 14:02:27 PDT
Created attachment 109200 [details]
Patch
Comment 24 Fady Samuel 2011-09-30 10:44:07 PDT
Created attachment 109311 [details]
Patch
Comment 25 Fady Samuel 2011-10-03 13:12:25 PDT
Created attachment 109515 [details]
Patch
Comment 26 Simon Fraser (smfr) 2011-10-03 17:13:20 PDT
Did you mean to request review?
Comment 27 Fady Samuel 2011-10-03 17:35:49 PDT
Comment on attachment 109515 [details]
Patch

Not much has changed but it finally builds on Windows!!!
Comment 28 Fady Samuel 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!
Comment 29 Darin Adler 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.
Comment 30 Fady Samuel 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.
Comment 31 Fady Samuel 2011-10-04 15:15:50 PDT
Created attachment 109703 [details]
Patch
Comment 32 WebKit Review Bot 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.
Comment 33 WebKit Review Bot 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
Comment 34 WebKit Review Bot 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
Comment 35 Fady Samuel 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.
Comment 36 Fady Samuel 2011-10-06 10:50:02 PDT
Created attachment 109976 [details]
Patch
Comment 37 WebKit Review Bot 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
Comment 38 Fady Samuel 2011-10-06 13:48:50 PDT
Created attachment 110010 [details]
Patch for landing
Comment 39 Fady Samuel 2011-10-06 13:55:52 PDT
Created attachment 110014 [details]
Patch for landing
Comment 40 WebKit Review Bot 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>
Comment 41 WebKit Review Bot 2011-10-06 14:35:47 PDT
All reviewed patches have been landed.  Closing bug.