Bug 98571 – [Sub pixel layout] Fast-path iframe scrolling can picks up an extra pixel

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

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

[Levi Weintraub]

Created attachment 167417 [details]
Patch

[Levi Weintraub]

FWIW, I'm happy to refactor the logical change from the refactor if preferred.

[Levi Weintraub]

Created attachment 167424 [details]
Patch

[WebKit Review Bot]

Comment on attachment 167424 [details]
Patch

Attachment 167424 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14177685

[Peter Beverloo (cr-android ews)]

Comment on attachment 167424 [details]
Patch

Attachment 167424 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14196160

[Levi Weintraub]

Created attachment 167574 [details]
Patch

[Emil A Eklund]

Comment on attachment 167574 [details]
Patch

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.

[Levi Weintraub]

Comment on attachment 167574 [details]
Patch

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!

[Emil A Eklund]

Comment on attachment 167574 [details]
Patch

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.

[Levi Weintraub]

Created attachment 167789 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 167789 [details]
Patch for landing

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

[Levi Weintraub]

Created attachment 167841 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 167841 [details]
Patch for landing

Clearing flags on attachment: 167841

Committed r130811: <http://trac.webkit.org/changeset/130811>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 98831

[Levi Weintraub]

Created attachment 168299 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 168299 [details]
Patch for landing

Clearing flags on attachment: 168299

Committed r131111: <http://trac.webkit.org/changeset/131111>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 168299 [details]
Patch for landing

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.

[Levi Weintraub]

(In reply to comment #19)
> (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.

Thanks for the watchful eye. I'll fix that :)

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Why do calls to localToContainerQuad() in various repaint functions (like RenderBox::outlineBoundsForRepaint()) not use SnapOffsetForTransforms?