Summary: | [Sub pixel layout] Fast-path iframe scrolling can picks up an extra pixel | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Levi Weintraub <leviw> | ||||||||||||||
Component: | Layout and Rendering | Assignee: | Levi Weintraub <leviw> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | cmarcelo, dglazkov, eae, eric.carlson, eric, feature-media-reviews, fmalita, mifenton, pdr, peter+ews, schenney, simon.fraser, tkent, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 98831 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Levi Weintraub
2012-10-05 15:56:37 PDT
Created attachment 167417 [details]
Patch
FWIW, I'm happy to refactor the logical change from the refactor if preferred. Created attachment 167424 [details]
Patch
Comment on attachment 167424 [details] Patch Attachment 167424 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14177685 Comment on attachment 167424 [details] Patch Attachment 167424 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14196160 Created attachment 167574 [details]
Patch
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 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 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. Created attachment 167789 [details]
Patch for landing
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 Created attachment 167841 [details]
Patch for landing
Comment on attachment 167841 [details] Patch for landing Clearing flags on attachment: 167841 Committed r130811: <http://trac.webkit.org/changeset/130811> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 98831 Created attachment 168299 [details]
Patch for landing
Comment on attachment 168299 [details] Patch for landing Clearing flags on attachment: 168299 Committed r131111: <http://trac.webkit.org/changeset/131111> All reviewed patches have been landed. Closing bug. 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. (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 :) 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. Why do calls to localToContainerQuad() in various repaint functions (like RenderBox::outlineBoundsForRepaint()) not use SnapOffsetForTransforms? |