Bug 98571 - [Sub pixel layout] Fast-path iframe scrolling can picks up an extra pixel
: [Sub pixel layout] Fast-path iframe scrolling can picks up an extra pixel
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 98831
:
  Show dependency treegraph
 
Reported: 2012-10-05 15:56 PST by
Modified: 2012-11-11 11:44 PST (History)


Attachments
Patch (76.19 KB, patch)
2012-10-05 17:03 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch (74.34 KB, patch)
2012-10-05 17:39 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch (98.18 KB, patch)
2012-10-08 11:47 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (115.93 KB, patch)
2012-10-09 11:07 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (115.38 KB, patch)
2012-10-09 13:51 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (115.27 KB, patch)
2012-10-11 15:54 PST, Levi Weintraub
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-10-05 15:56:37 PST
In order to fix this, we have to extend the different modes of changing coordinates beyond MapLocalToContainer to all the coordinate-changing functions.

Downstream crbug: http://code.google.com/p/chromium/issues/detail?id=143354
------- Comment #1 From 2012-10-05 17:03:09 PST -------
Created an attachment (id=167417) [details]
Patch
------- Comment #2 From 2012-10-05 17:32:13 PST -------
FWIW, I'm happy to refactor the logical change from the refactor if preferred.
------- Comment #3 From 2012-10-05 17:39:20 PST -------
Created an attachment (id=167424) [details]
Patch
------- Comment #4 From 2012-10-05 17:59:57 PST -------
(From update of attachment 167424 [details])
Attachment 167424 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14177685
------- Comment #5 From 2012-10-05 18:33:09 PST -------
(From update of attachment 167424 [details])
Attachment 167424 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14196160
------- Comment #6 From 2012-10-08 11:47:33 PST -------
Created an attachment (id=167574) [details]
Patch
------- Comment #7 From 2012-10-08 14:13:42 PST -------
(From update of attachment 167574 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=167574&action=review

> Source/WebCore/dom/ContainerNode.cpp:749
> +        point = o->localToAbsolute(FloatPoint(), UseTransforms | SnapOffsetForTransforms);

This is much more descriptive, I like it!

> Source/WebCore/dom/Element.cpp:542
> +            renderBoxModelObject()->absoluteQuads(quads, 0);

Why are you explicitly passing zero (NULL) as wasFixed here?

> Source/WebCore/dom/MouseRelatedEvent.cpp:169
> +            FloatPoint localPos = r->absoluteToLocal(absoluteLocation(), UseTransforms | SnapOffsetForTransforms);

SnapOffsetForTransforms seems to be used in most places, how about making it the default?

> LayoutTests/fast/sub-pixel/sub-pixel-iframe-copy-on-scroll.html:13
> +// src="resources/subframe.html"

This comment doesn't make a lot of sense.
------- Comment #8 From 2012-10-08 15:05:16 PST -------
(From update of attachment 167574 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=167574&action=review

>> Source/WebCore/dom/Element.cpp:542
>> +            renderBoxModelObject()->absoluteQuads(quads, 0);
> 
> Why are you explicitly passing zero (NULL) as wasFixed here?

This is because we don't want any of the feature flags. We could have a named zero constant to specify no flags?

>> Source/WebCore/dom/MouseRelatedEvent.cpp:169
>> +            FloatPoint localPos = r->absoluteToLocal(absoluteLocation(), UseTransforms | SnapOffsetForTransforms);
> 
> SnapOffsetForTransforms seems to be used in most places, how about making it the default?

SnapOffsetForTransforms is, UseTransforms isn't as this was the previous default. It does honestly seem like using transforms should be the default and callers should be specifying explicitly that they *don't* want transforms. Perhaps that's worthwhile here or in a future patch (since it would reverse logic).

>> LayoutTests/fast/sub-pixel/sub-pixel-iframe-copy-on-scroll.html:13
>> +// src="resources/subframe.html"
> 
> This comment doesn't make a lot of sense.

Whoops, that's vestigial from when I was writing the test. Good catch!
------- Comment #9 From 2012-10-08 15:13:46 PST -------
(From update of attachment 167574 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=167574&action=review

>>> Source/WebCore/dom/Element.cpp:542
>>> +            renderBoxModelObject()->absoluteQuads(quads, 0);
>> 
>> Why are you explicitly passing zero (NULL) as wasFixed here?
> 
> This is because we don't want any of the feature flags. We could have a named zero constant to specify no flags?

Either that or adding the name of the field as a comment (just like we did in some places for the boolean parameters before).

renderBoxModelObject()->absoluteQuads(quads, 0 /* mode */);

>>> Source/WebCore/dom/MouseRelatedEvent.cpp:169
>>> +            FloatPoint localPos = r->absoluteToLocal(absoluteLocation(), UseTransforms | SnapOffsetForTransforms);
>> 
>> SnapOffsetForTransforms seems to be used in most places, how about making it the default?
> 
> SnapOffsetForTransforms is, UseTransforms isn't as this was the previous default. It does honestly seem like using transforms should be the default and callers should be specifying explicitly that they *don't* want transforms. Perhaps that's worthwhile here or in a future patch (since it would reverse logic).

Lets keep this change as small as we can and have that in a future patch.
------- Comment #10 From 2012-10-09 11:07:47 PST -------
Created an attachment (id=167789) [details]
Patch for landing
------- Comment #11 From 2012-10-09 11:33:45 PST -------
(From update of attachment 167789 [details])
Rejecting attachment 167789 [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:
WebCore::RenderObject::absoluteToLocal(const WebCore::FloatPoint&, WebCore::MapCoordinatesFlags) const
  CXX(target) out/Release/obj.target/webkit/Source/WebKit/chromium/src/WebPagePopupImpl.o
make: *** [out/Release/obj.target/webkit/Source/WebKit/chromium/src/WebInputEventConversion.o] Error 1
make: *** Waiting for unfinished jobs....

Failed to run "['Tools/Scripts/build-webkit', '--release', '--chromium', '--update-chromium']" exit_code: 2
o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/14214819
------- Comment #12 From 2012-10-09 13:51:00 PST -------
Created an attachment (id=167841) [details]
Patch for landing
------- Comment #13 From 2012-10-09 14:31:54 PST -------
(From update of attachment 167841 [details])
Clearing flags on attachment: 167841

Committed r130811: <http://trac.webkit.org/changeset/130811>
------- Comment #14 From 2012-10-09 14:31:58 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #15 From 2012-10-09 16:12:06 PST -------
Re-opened since this is blocked by bug 98831
------- Comment #16 From 2012-10-11 15:54:42 PST -------
Created an attachment (id=168299) [details]
Patch for landing
------- Comment #17 From 2012-10-11 16:47:28 PST -------
(From update of attachment 168299 [details])
Clearing flags on attachment: 168299

Committed r131111: <http://trac.webkit.org/changeset/131111>
------- Comment #18 From 2012-10-11 16:47:32 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #19 From 2012-10-11 17:34:59 PST -------
(From update of attachment 168299 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=168299&action=review

> Source/WebCore/rendering/RenderBox.h:577
> +    virtual void mapLocalToContainer(RenderLayerModelObject* repaintContainer, TransformState&, MapCoordinatesFlags mode = ApplyContainerFlip | SnapOffsetForTransforms, bool* wasFixed = 0) const OVERRIDE;

I would have omitted the argument name “mode” here. It doesn’t add anything.

> Source/WebCore/rendering/RenderBox.h:579
> +    virtual void mapAbsoluteToLocalPoint(MapCoordinatesFlags mode, TransformState&) const;

I would have omitted the argument name “mode” here. It doesn’t add anything.

> Source/WebCore/rendering/RenderBoxModelObject.h:161
> +    virtual void mapAbsoluteToLocalPoint(MapCoordinatesFlags mode, TransformState&) const OVERRIDE;

I would have omitted the argument name “mode” here. It doesn’t add anything.

> Source/WebCore/rendering/RenderInline.h:138
> +    virtual void mapLocalToContainer(RenderLayerModelObject* repaintContainer, TransformState&, MapCoordinatesFlags mode = ApplyContainerFlip | SnapOffsetForTransforms, bool* wasFixed = 0) const OVERRIDE;

I would have omitted the argument name “mode” here. It doesn’t add anything.

> Source/WebCore/rendering/RenderObject.h:712
> +    FloatPoint localToAbsolute(const FloatPoint& localPoint = FloatPoint(), MapCoordinatesFlags mode = 0) const;
> +    FloatPoint absoluteToLocal(const FloatPoint&, MapCoordinatesFlags mode = 0) const;

I would have omitted the argument name “mode” here. It doesn’t add anything.

> Source/WebCore/rendering/RenderObject.h:722
> +    FloatQuad localToContainerQuad(const FloatQuad&, RenderLayerModelObject* repaintContainer, MapCoordinatesFlags mode = 0, bool* wasFixed = 0) const;
> +    FloatPoint localToContainerPoint(const FloatPoint&, RenderLayerModelObject* repaintContainer, MapCoordinatesFlags mode = 0, bool* wasFixed = 0) const;

I would have omitted the argument name “mode” here. It doesn’t add anything.

> Source/WebCore/rendering/RenderObject.h:921
> +    virtual void mapLocalToContainer(RenderLayerModelObject* repaintContainer, TransformState&, MapCoordinatesFlags mode = ApplyContainerFlip, bool* wasFixed = 0) const;
> +    virtual void mapAbsoluteToLocalPoint(MapCoordinatesFlags mode, TransformState&) const;

I would have omitted the argument name “mode” here. It doesn’t add anything.

> Source/WebCore/rendering/svg/RenderSVGForeignObject.h:57
> +    virtual void mapLocalToContainer(RenderLayerModelObject* repaintContainer, TransformState&, MapCoordinatesFlags mode = ApplyContainerFlip | SnapOffsetForTransforms, bool* wasFixed = 0) const OVERRIDE;

I would have omitted the argument name “mode” here. It doesn’t add anything.

> Source/WebCore/rendering/svg/RenderSVGModelObject.h:61
> +    virtual void mapLocalToContainer(RenderLayerModelObject* repaintContainer, TransformState&, MapCoordinatesFlags mode = ApplyContainerFlip | SnapOffsetForTransforms, bool* wasFixed = 0) const OVERRIDE;

I would have omitted the argument name “mode” here. It doesn’t add anything.

> Source/WebCore/rendering/svg/RenderSVGRoot.h:98
> +    virtual void mapLocalToContainer(RenderLayerModelObject* repaintContainer, TransformState&, MapCoordinatesFlags mode = ApplyContainerFlip | SnapOffsetForTransforms, bool* wasFixed = 0) const OVERRIDE;

I would have omitted the argument name “mode” here. It doesn’t add anything.

> Source/WebCore/rendering/svg/RenderSVGText.h:78
> +    virtual void mapLocalToContainer(RenderLayerModelObject* repaintContainer, TransformState&, MapCoordinatesFlags mode = ApplyContainerFlip | SnapOffsetForTransforms, bool* wasFixed = 0) const OVERRIDE;

I would have omitted the argument name “mode” here. It doesn’t add anything.
------- Comment #20 From 2012-10-12 10:44:42 PST -------
(In reply to comment #19)
> (From update of attachment 168299 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168299&action=review
> 
> > Source/WebCore/rendering/RenderBox.h:577
> > +    virtual void mapLocalToContainer(RenderLayerModelObject* repaintContainer, TransformState&, MapCoordinatesFlags mode = ApplyContainerFlip | SnapOffsetForTransforms, bool* wasFixed = 0) const OVERRIDE;
> 
> I would have omitted the argument name “mode” here. It doesn’t add anything.

Thanks for the watchful eye. I'll fix that :)
------- Comment #21 From 2012-11-11 11:40:20 PST -------
I don't understand why some places use SnapOffsetForTransforms and some don't in this change, and it's causing assertions in RenderGeometryMap with a change I have.

First, RenderBox::mapLocalToContainer() does snapping even when there are no transforms involved:
    if (mode & SnapOffsetForTransforms)
        containerOffset = roundedIntSize(containerOffset);
so the mode name is confusing, or the mode is mis-applied here.

Why does LayoutState::LayoutState use SnapOffsetForTransforms?

I think RenderGeometryMap needs to be told what mode flags to use on creation, rather than always applying SnapOffsetForTransforms.
------- Comment #22 From 2012-11-11 11:44:00 PST -------
Why do calls to localToContainerQuad() in various repaint functions (like RenderBox::outlineBoundsForRepaint()) not use SnapOffsetForTransforms?