WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(14)
View All
Add attachment
proposed patch, testcase, etc.
Fady Samuel
Comment 1
2011-09-28 10:54:13 PDT
Created
attachment 109037
[details]
Patch
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
Created
attachment 109063
[details]
Patch
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
Created
attachment 109064
[details]
Patch
Early Warning System Bot
Comment 9
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
Fady Samuel
Comment 10
2011-09-28 16:12:45 PDT
Created
attachment 109084
[details]
Patch
Fady Samuel
Comment 11
2011-09-28 16:25:03 PDT
Created
attachment 109085
[details]
Patch
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
Created
attachment 109094
[details]
Patch
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
Comment on
attachment 109094
[details]
Patch
Attachment 109094
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/9888457
Fady Samuel
Comment 17
2011-09-29 12:01:16 PDT
Created
attachment 109183
[details]
Patch
Collabora GTK+ EWS bot
Comment 18
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
Fady Samuel
Comment 19
2011-09-29 13:31:30 PDT
Created
attachment 109197
[details]
Patch
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
Created
attachment 109200
[details]
Patch
Fady Samuel
Comment 24
2011-09-30 10:44:07 PDT
Created
attachment 109311
[details]
Patch
Fady Samuel
Comment 25
2011-10-03 13:12:25 PDT
Created
attachment 109515
[details]
Patch
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
Created
attachment 109703
[details]
Patch
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
Created
attachment 109976
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug