[webkit-reviews] review canceled: [Bug 34875] offsetLeft broken within CSS3 columns : [Attachment 139014] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 4 12:10:26 PDT 2012
Shezan Baig <shezbaig.wk at gmail.com> has canceled Shezan Baig
<shezbaig.wk at gmail.com>'s request for review:
Bug 34875: offsetLeft broken within CSS3 columns
https://bugs.webkit.org/show_bug.cgi?id=34875
Attachment 139014: Patch
https://bugs.webkit.org/attachment.cgi?id=139014&action=review
------- Additional Comments from Shezan Baig <shezbaig.wk at gmail.com>
Thanks for reviewing!
(In reply to comment #5)
> (From update of attachment 139014 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=139014&action=review
>
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:557
> > RenderBoxModelObject* offsetPar = offsetParent();
>
> While touching this function, this should be moved into the if (offsetPar)
> below and renamed offsetParent (no need for bad abbreviation).
I've made this change in the latest patch.
>
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:588
> > + LayoutPoint topLeft = isBox() ? toRenderBox(this)->topLeftLocation() :
LayoutPoint();
>
> This should be moved inside offsetTopLeft().
The reason this is outside offsetTopLeft is because the way 'topLeft' is
calculated is different for RenderInline. To make this more obvious, my latest
patch does this slightly differently: instead of switching on 'isBox', I
modified RenderBox to override offsetLeft/offsetTop (like how we already do for
RenderInline) and pass the box's topLeftLocation.
>
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:595
> >> + return offsetTopLeft(topLeft).y();
> >
> > Great to see this simplification.
>
> I feel a bit ambivalent about that: the code sharing is superb but now you
> always get both offsets and discard half of the result. That seem wasteful
> and will slow down our implementation of offset{Top|Left} (AFAICT it's not
> used elsewhere in rendering/).
Yes, I sort of hinted this in passing in the changelog. The problem is that in
order to get correct offsetLeft for multicolumns, we *need* to keep track of
both x&y coordinates as we move up each parent (i.e. we cannot get the correct
offsetLeft if we don't have the correct y-coordinate). The code sharing is
just a nice bonus side-effect of this, it isn't the primary goal of this patch.
I thought of a couple of alternative approaches:
* Cache the result of offsetTopLeft in the renderer so that calling offsetTop
right after offsetLeft will not recalculate the result. However, I think
the space cost makes this undesirable.
* Only use offsetTopLeft if we detect that one of the parents has multiple
columns, otherwise we can fallback to the previous code. This means we only
do the extra work when we need to. However, I'm not sure if the perf
numbers (see below) justify having 3 copies of nearly identical code.
>
> To give us some perspective, what are the numbers for a run of Bindings/dom-
> attributes.html and Dromaeo before / after your patch? (just use
> Tools/Scripts/run-perf-tests [testName])
I ran dom-attributes.html twice after each build. I should point out that
dom-attributes.html only tries offsetLeft, it doesn't try offsetTop (not sure
if this is intentional or not). The results are pasted below:
Without Patch
=============
shezan at shezwk-ubuntu:~/WebKit$ run-perf-tests --platform=gtk
PerformanceTests/Bindings/dom-attributes.html
Running Bindings/dom-attributes.html (1 of 1)
RESULT Bindings: dom-attributes= 2834.8 ms
median= 2822.0 ms, stdev= 58.4992307642 ms, min= 2746.0 ms, max= 2921.0 ms
shezan at shezwk-ubuntu:~/WebKit$ run-perf-tests --platform=gtk
PerformanceTests/Bindings/dom-attributes.html
Running Bindings/dom-attributes.html (1 of 1)
RESULT Bindings: dom-attributes= 2736.8 ms
median= 2739.0 ms, stdev= 15.2367975638 ms, min= 2711.0 ms, max= 2755.0 ms
With Patch
==========
shezan at shezwk-ubuntu:~/WebKit$ run-perf-tests --platform=gtk
PerformanceTests/Bindings/dom-attributes.html
Running Bindings/dom-attributes.html (1 of 1)
RESULT Bindings: dom-attributes= 2813.2 ms
median= 2830.0 ms, stdev= 44.1062353868 ms, min= 2729.0 ms, max= 2856.0 ms
shezan at shezwk-ubuntu:~/WebKit$ run-perf-tests --platform=gtk
PerformanceTests/Bindings/dom-attributes.html
Running Bindings/dom-attributes.html (1 of 1)
RESULT Bindings: dom-attributes= 2709.6 ms
median= 2716.0 ms, stdev= 11.4995652092 ms, min= 2692.0 ms, max= 2721.0 ms
I also did Dromaeo, but it's not immediately obvious whether this tests
offsetLeft/offsetTop either. Results are:
Without patch: http://dromaeo.com/?id=170533
With patch: http://dromaeo.com/?id=170534
>
> > Source/WebCore/rendering/RenderBoxModelObject.h:66
> > + LayoutPoint offsetTopLeft(const LayoutPoint&) const;
>
> It should be protected, we don't want code outside rendering/ to start
> relying on it.
I've made this change in the latest patch.
>
> > Source/WebCore/rendering/RenderObject.h:749
> > + inline LayoutSize offsetForColumns(const LayoutPoint&) const;
>
> No need to make it 'inline' any function defined in the header is inlined by
> the compiler.
Ditto.
>
> > Source/WebCore/rendering/RenderObject.h:1140
> > +inline LayoutSize RenderObject::offsetForColumns(const LayoutPoint& point)
const
> > +{
> > + LayoutSize offset;
> > + adjustForColumns(offset, point);
> > + return offset;
> > +}
>
> This could be moved along the declaration above.
>
Ditto.
> >
LayoutTests/fast/block/positioning/offsetLeft-offsetTop-multicolumn-expected.ht
ml:32
> > + .size1 {
> > + display: inline-block;
> > + width: 10px;
> > + height: 20px;
> > + padding: 2px;
> > + background: green;
> > + }
> > + .size2 {
> > + display: inline-block;
> > + width: 20px;
> > + height: 20px;
> > + padding: 2px;
> > + background: green;
> > + }
> > + .size3 {
> > + display: inline-block;
> > + width: 40px;
> > + height: 20px;
> > + padding: 2px;
> > + background: green;
> > + }
>
> For the record, this could be simplified a lot:
> span {
> display: inline-block;
> height: 20px;
> padding: 2px;
> background-color: green;
> }
>
> span .size1
> {
> width: 10px;
> }
>
> ...
> (this would underline what is actually specific to the different class as
> size1 / 2 / 3 (FIXME!) don't really speak to me).
Ditto.
>
> >
LayoutTests/fast/block/positioning/offsetLeft-offsetTop-multicolumn.html:131
> > + cover.setAttribute("style", "background: green; " +
> > + "z-index: 10; " +
> > + "position: absolute; " +
> > + "left: " + item.offsetLeft + "px; " +
> > + "top: " + item.offsetTop + "px;");
> > + body.appendChild(cover);
>
> I don't understand why this needs to be a ref-tests. It looks like it could
> be a very simple dumpAsText() test: you are getting offsetLeft / offsetTop
> here so you can just dump them and compare them to some preset values.
I originally made it a ref-test because that was the only way I could figure
out how to test just the offsetLeft/offsetTop calculation, and not the
multicolumn layout algorithm (which I assume is covered by other tests, and not
the focus of this bug). Including the preset values as part of the test would
make this also a test of our multicolumn layout algorithm, which has the
following drawbacks:
* The test will not work on other browsers, because it turns out that different
browsers implement the multicolumn layout algorithm differently.
* If any changes are made to the webkit multicolumn layout algorithm, it will
cause this test to fail, even though this test is only supposed to check the
implementation of offsetLeft and offsetTop.
Nevertheless, my latest patch uses a dumpAsText test as requested, with the
preset values that I gathered from my test run, but it will obviously fail in
other browsers.
More information about the webkit-reviews
mailing list