Bug 98571 - [Sub pixel layout] Fast-path iframe scrolling can picks up an extra pixel
Summary: [Sub pixel layout] Fast-path iframe scrolling can picks up an extra pixel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on: 98831
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-05 15:56 PDT by Levi Weintraub
Modified: 2012-11-11 11:44 PST (History)
14 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2012-10-05 15:56:37 PDT
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 Levi Weintraub 2012-10-05 17:03:09 PDT
Created attachment 167417 [details]
Patch
Comment 2 Levi Weintraub 2012-10-05 17:32:13 PDT
FWIW, I'm happy to refactor the logical change from the refactor if preferred.
Comment 3 Levi Weintraub 2012-10-05 17:39:20 PDT
Created attachment 167424 [details]
Patch
Comment 4 WebKit Review Bot 2012-10-05 17:59:57 PDT
Comment on attachment 167424 [details]
Patch

Attachment 167424 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14177685
Comment 5 Peter Beverloo (cr-android ews) 2012-10-05 18:33:09 PDT
Comment on attachment 167424 [details]
Patch

Attachment 167424 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14196160
Comment 6 Levi Weintraub 2012-10-08 11:47:33 PDT
Created attachment 167574 [details]
Patch
Comment 7 Emil A Eklund 2012-10-08 14:13:42 PDT
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.
Comment 8 Levi Weintraub 2012-10-08 15:05:16 PDT
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!
Comment 9 Emil A Eklund 2012-10-08 15:13:46 PDT
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.
Comment 10 Levi Weintraub 2012-10-09 11:07:47 PDT
Created attachment 167789 [details]
Patch for landing
Comment 11 WebKit Review Bot 2012-10-09 11:33:45 PDT
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
Comment 12 Levi Weintraub 2012-10-09 13:51:00 PDT
Created attachment 167841 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-10-09 14:31:54 PDT
Comment on attachment 167841 [details]
Patch for landing

Clearing flags on attachment: 167841

Committed r130811: <http://trac.webkit.org/changeset/130811>
Comment 14 WebKit Review Bot 2012-10-09 14:31:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 WebKit Review Bot 2012-10-09 16:12:06 PDT
Re-opened since this is blocked by bug 98831
Comment 16 Levi Weintraub 2012-10-11 15:54:42 PDT
Created attachment 168299 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-10-11 16:47:28 PDT
Comment on attachment 168299 [details]
Patch for landing

Clearing flags on attachment: 168299

Committed r131111: <http://trac.webkit.org/changeset/131111>
Comment 18 WebKit Review Bot 2012-10-11 16:47:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Darin Adler 2012-10-11 17:34:59 PDT
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.
Comment 20 Levi Weintraub 2012-10-12 10:44:42 PDT
(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 :)
Comment 21 Simon Fraser (smfr) 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 Simon Fraser (smfr) 2012-11-11 11:44:00 PST
Why do calls to localToContainerQuad() in various repaint functions (like RenderBox::outlineBoundsForRepaint()) not use SnapOffsetForTransforms?