Bug 101779

Created attachment 173358 [details] Patch

Merge failures, could you try to update and upload a new patch? I'd like to see what the EWS bots think of it.

Created attachment 173366 [details] Patch

Created attachment 173369 [details] Patch

(In reply to comment #4) > Created an attachment (id=173369) [details] > Patch Now with 100% more applying!

Comment on attachment 173366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173366&action=review I like this change, simplifies things quite a bit and might even be a performance win. > Source/WebCore/rendering/RenderBox.cpp:1272 > +void RenderBox::mapLocalToContainer(const RenderLayerModelObject* repaintContainer, TransformState& transformState, MapCoordinatesFlags mode, bool* wasFixed, LayoutSize accumulatedOffset) const Why are you passing this by value? To avoid having to create a local copy? > Source/WebCore/rendering/RenderBox.cpp:1278 > + return transformState.move((mode & SnapOffsetForTransforms) ? roundedIntSize(accumulatedOffset) : accumulatedOffset, preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform); This is pretty hard to follow with the nested ternary conditions. How about you make one of them an actual if statement or an inline function? > Source/WebCore/rendering/RenderBox.cpp:-1309 > - bool preserve3D = mode & UseTransforms && (o->style()->preserves3D() || style()->preserves3D()); Can you expand on why you don't need to special case preserve3d anymore?

Comment on attachment 173366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173366&action=review >> Source/WebCore/rendering/RenderBox.cpp:1272 >> +void RenderBox::mapLocalToContainer(const RenderLayerModelObject* repaintContainer, TransformState& transformState, MapCoordinatesFlags mode, bool* wasFixed, LayoutSize accumulatedOffset) const > > Why are you passing this by value? To avoid having to create a local copy? That's right. We could alternatively create a local variable at the start of each implementation, or make all versions of this protected and a public version that instantiates a local variable and passes by reference. >> Source/WebCore/rendering/RenderBox.cpp:1278 >> + return transformState.move((mode & SnapOffsetForTransforms) ? roundedIntSize(accumulatedOffset) : accumulatedOffset, preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform); > > This is pretty hard to follow with the nested ternary conditions. How about you make one of them an actual if statement or an inline function? I can't make the entire line an inline function as the virtual function is defined in RenderObject.h, but I could have the ternary resolution inlined there. I'd prefer that to a slew of if/else in these implementations. >> Source/WebCore/rendering/RenderBox.cpp:-1309 >> - bool preserve3D = mode & UseTransforms && (o->style()->preserves3D() || style()->preserves3D()); > > Can you expand on why you don't need to special case preserve3d anymore? Look up! It just moved higher in the function.

Comment on attachment 173369 [details] Patch Attachment 173369 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14792209

(In reply to comment #7) > (From update of attachment 173366 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=173366&action=review > > >> Source/WebCore/rendering/RenderBox.cpp:1272 > >> +void RenderBox::mapLocalToContainer(const RenderLayerModelObject* repaintContainer, TransformState& transformState, MapCoordinatesFlags mode, bool* wasFixed, LayoutSize accumulatedOffset) const > > > > Why are you passing this by value? To avoid having to create a local copy? > > That's right. We could alternatively create a local variable at the start of each implementation, or make all versions of this protected and a public version that instantiates a local variable and passes by reference. Makes sense, I prefer they way you have it over having a local copy. > >> Source/WebCore/rendering/RenderBox.cpp:1278 > >> + return transformState.move((mode & SnapOffsetForTransforms) ? roundedIntSize(accumulatedOffset) : accumulatedOffset, preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform); > > > > This is pretty hard to follow with the nested ternary conditions. How about you make one of them an actual if statement or an inline function? > > I can't make the entire line an inline function as the virtual function is defined in RenderObject.h, but I could have the ternary resolution inlined there. I'd prefer that to a slew of if/else in these implementations. That would be great if you could make it work, there is quite a bit of repeated code as it is and it isn't terribly readable. > >> Source/WebCore/rendering/RenderBox.cpp:-1309 > >> - bool preserve3D = mode & UseTransforms && (o->style()->preserves3D() || style()->preserves3D()); > > > > Can you expand on why you don't need to special case preserve3d anymore? > > Look up! It just moved higher in the function. Ah, of course. Thanks!

Comment on attachment 173369 [details] Patch Attachment 173369 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14787312

Comment on attachment 173369 [details] Patch Attachment 173369 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14786339

Comment on attachment 173369 [details] Patch Attachment 173369 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14792213

Comment on attachment 173369 [details] Patch Attachment 173369 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14787322

Comment on attachment 173369 [details] Patch Attachment 173369 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14777613

Created attachment 173418 [details] Patch

Comment on attachment 173418 [details] Patch Why isn't the accumulated offset part of the TransformState?

(In reply to comment #16) > (From update of attachment 173418 [details]) > Why isn't the accumulated offset part of the TransformState? That would probably make for a cleaner change, you're right. I'll take a stab on Monday, thanks!

Comment on attachment 173418 [details] Patch Maybe TransformState should work in LayoutUnits, and only use floats when it has to convert through transforms? Note that there are pure-float clients of TransformState though (like GraphicsLayerCA).

I'm trying to use RenderGeometryMap in more places and running into cases where mapping via renderers and mapping via RenderGeometryMap don't match. Some of this is because of the SnapOffsetForTransforms confusion (bug 101864). I'd like to be able to use the same RenderGeometryMap to map both repaint rects and points. I'm not sure if SnapOffsetForTransforms should be different for those two operations, but if so, then we somehow need to allow RenderGeometryMap to map with SnapOffsetForTransforms on or off.

One important point here is that if you touch the mapToContainer methods, you probably also need to fix up RenderGeometryMap.

(In reply to comment #20) > One important point here is that if you touch the mapToContainer methods, you probably also need to fix up RenderGeometryMap. RenderGeometryMap indeed needed to be fixed as well. Sadly, making this change in TransformState results in a solution that's less clean, with consumers of that class needing more awareness of its state or a parallel data structure for accumulating and returning proper values.

Created attachment 176578 [details] Patch

Comment on attachment 176578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176578&action=review I don't see how this works correctly with RenderGeometryMap, since I'd expect the pushMappingToContainer() methods to require similar changes to mapLocalToContainer() (ideally the code would be shared). > LayoutTests/ChangeLog:9 > + Update the sub-pixel-iframe-copy-on-scroll test to also test a deeply-nested iframe to ensure > + we're properly pixel snapping mapped coordinates. What a about a test with interleaved transforms and subpixel pushers?

Comment on attachment 176578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176578&action=review You're right about pushMappingToContainer. RenderBox and RenderInline's implementations both needed updates. >> LayoutTests/ChangeLog:9 >> + we're properly pixel snapping mapped coordinates. > > What a about a test with interleaved transforms and subpixel pushers? Will do.

Created attachment 177550 [details] Patch

Comment on attachment 177550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177550&action=review A general problem I have with this code is that painting and coordinate mapping don't share a common function for pixel snapping, so it's really hard to know if they are doing the same thing, and whether RenderGeometryMap matches what they both do. > Source/WebCore/ChangeLog:66 > + * platform/graphics/LayoutSize.h: > + (LayoutSize): > + (WebCore::LayoutSize::expand): > + * rendering/RenderBox.cpp: > + (WebCore::RenderBox::mapLocalToContainer): > + (WebCore::RenderBox::pushMappingToContainer): > + * rendering/RenderBox.h: > + (RenderBox): > + * rendering/RenderGeometryMap.cpp: > + (WebCore::RenderGeometryMap::mapToContainer): > + * rendering/RenderInline.cpp: > + (WebCore::RenderInline::mapLocalToContainer): > + (WebCore::RenderInline::pushMappingToContainer): > + * rendering/RenderInline.h: > + (RenderInline): > + * rendering/RenderObject.cpp: > + (WebCore::RenderObject::mapLocalToContainer): > + * rendering/RenderObject.h: > + (RenderObject): > + (WebCore::RenderObject::adjustOffsetForSnapping): > + * rendering/RenderView.cpp: > + (WebCore::RenderView::mapLocalToContainer): > + * rendering/RenderView.h: > + (RenderView): > + * rendering/svg/RenderSVGForeignObject.cpp: > + (WebCore::RenderSVGForeignObject::mapLocalToContainer): > + * rendering/svg/RenderSVGForeignObject.h: > + (RenderSVGForeignObject): > + * rendering/svg/RenderSVGInline.cpp: > + (WebCore::RenderSVGInline::mapLocalToContainer): > + * rendering/svg/RenderSVGInline.h: > + (RenderSVGInline): > + * rendering/svg/RenderSVGModelObject.cpp: > + (WebCore::RenderSVGModelObject::mapLocalToContainer): > + * rendering/svg/RenderSVGModelObject.h: > + (RenderSVGModelObject): > + * rendering/svg/RenderSVGRoot.cpp: > + (WebCore::RenderSVGRoot::mapLocalToContainer): > + * rendering/svg/RenderSVGRoot.h: > + (RenderSVGRoot): > + * rendering/svg/RenderSVGText.cpp: > + (WebCore::RenderSVGText::mapLocalToContainer): > + * rendering/svg/RenderSVGText.h: > + (RenderSVGText): > + * rendering/svg/SVGRenderSupport.cpp: > + (WebCore::SVGRenderSupport::mapLocalToContainer): > + * rendering/svg/SVGRenderSupport.h: > + (SVGRenderSupport): Please clean up this list. Per-function comments are recommended. > Source/WebCore/rendering/RenderBox.cpp:1388 > - return; > + return transformState.move(adjustOffsetForSnapping(mode, adjustedOffset), preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform); It's a shame that all the implementations of mapLocalToContainer() have to handle this boundary condition. I think it would be better to push this out to functions that initiate calls to the recursive mapLocalToContainer() (there should be few). Those callers will always flatten, since they are the end of the mapping chain. > Source/WebCore/rendering/RenderBox.cpp:1422 > + transformState.move(adjustOffsetForSnapping(mode, adjustedOffset), preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform); It's a shame to call move() when adjustOffsetForSnapping() might be zero. I think it would be better to get adjustOffsetForSnapping(), and multiply it into t, then just move once. > Source/WebCore/rendering/RenderBox.cpp:1479 > if (shouldUseTransformFromContainer(container)) { > TransformationMatrix t; > getTransformFromContainer(container, containerOffset, t); > + adjustmentForSkippedAncestor = adjustOffsetForSnapping(geometryMap.mapCoordinatesFlags(), adjustmentForSkippedAncestor); > t.translateRight(adjustmentForSkippedAncestor.width(), adjustmentForSkippedAncestor.height()); Why doesn't pushMappingToContainer() have all the other snapping logic that mapLocalToContainer() does, e.g. for the non-transform case?

Comment on attachment 177550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177550&action=review >> Source/WebCore/rendering/RenderBox.cpp:1388 >> + return transformState.move(adjustOffsetForSnapping(mode, adjustedOffset), preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform); > > It's a shame that all the implementations of mapLocalToContainer() have to handle this boundary condition. I think it would be better to push this out to functions that initiate calls to the recursive mapLocalToContainer() (there should be few). Those callers will always flatten, since they are the end of the mapping chain. Okay. I didn't want to muddy the contract, but I'm happy to make that change. >> Source/WebCore/rendering/RenderBox.cpp:1422 >> + transformState.move(adjustOffsetForSnapping(mode, adjustedOffset), preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform); > > It's a shame to call move() when adjustOffsetForSnapping() might be zero. I think it would be better to get adjustOffsetForSnapping(), and multiply it into t, then just move once. Okay. >> Source/WebCore/rendering/RenderBox.cpp:1479 >> t.translateRight(adjustmentForSkippedAncestor.width(), adjustmentForSkippedAncestor.height()); > > Why doesn't pushMappingToContainer() have all the other snapping logic that mapLocalToContainer() does, e.g. for the non-transform case? We snap before transforms, or at the end, whichever comes first. You'll notice the snapping otherwise handled in RenderGeometryMap::mapToContainer.

(In reply to comment #27) > >> Source/WebCore/rendering/RenderBox.cpp:1479 > >> t.translateRight(adjustmentForSkippedAncestor.width(), adjustmentForSkippedAncestor.height()); > > > > Why doesn't pushMappingToContainer() have all the other snapping logic that mapLocalToContainer() does, e.g. for the non-transform case? > > We snap before transforms, or at the end, whichever comes first. You'll notice the snapping otherwise handled in RenderGeometryMap::mapToContainer. Do the fast paths in RGM do the right thing?

Created attachment 178306 [details] Patch

Comment on attachment 178306 [details] Patch Attachment 178306 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15210327

Comment on attachment 178306 [details] Patch Attachment 178306 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15175892

Created attachment 178312 [details] Patch

Comment on attachment 178312 [details] Patch Attachment 178312 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15185594

Created attachment 178596 [details] Patch

(In reply to comment #28) > Do the fast paths in RGM do the right thing? They do now :) And we pass all tests in Debug.

Comment on attachment 178596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178596&action=review > Source/WebCore/rendering/RenderBox.cpp:1418 > + bool preserve3D = mode & UseTransforms && ((o && o->style()->preserves3D()) || style()->preserves3D()); |o| cannot be 0 here as we check it above. Please remove the added NULL check. > Source/WebCore/rendering/RenderBox.cpp:1427 > + applyAccumulatedOffset(transformState, accumulatedOffset, mode); > + LayoutSize adjustedContainerOffset = adjustOffsetForSnapping(mode, containerOffset); > + transformState.move(adjustedContainerOffset.width(), adjustedContainerOffset.height(), TransformState::AccumulateTransform); Shouldn't this be simplified to applyAccumulatedOffset(transformState, accumulatedOffset + containerOffset, mode) ? (not sure if that's correct as the first line uses TransformState::FlattenTransform and the last one TransformState::AccumulateTransform) > Source/WebCore/rendering/RenderGeometryMap.cpp:51 > + accumulatedOffset = (mapCoordinatesFlags & SnapOffsetForTransforms) ? roundedIntSize(accumulatedOffset) : accumulatedOffset; Shouldn't this use adjustOffsetForSnapping? > Source/WebCore/rendering/RenderGeometryMap.cpp:109 > + applyAccumulatedOffset(transformState, accumulatedOffset, m_mapCoordinatesFlags); > + LayoutSize containerOffset = adjustOffsetForSnapping(m_mapCoordinatesFlags, currentStep.m_offset); > + transformState.move(containerOffset.width(), containerOffset.height(), accumulate); Again could this be simplified to one applyAccumulatedOffset call? > Source/WebCore/rendering/RenderInline.cpp:1168 > + accumulatedOffset.expand(layoutState->m_paintOffset + accumulatedOffset); Wouldn't that double the accumulated offset? > Source/WebCore/rendering/RenderObject.cpp:2067 > + accumulatedOffset = adjustOffsetForSnapping(mapCoordinatesFlags, accumulatedOffset); Shouldn't this use adjustOffsetForSnapping? > Source/WebCore/rendering/RenderObject.cpp:2081 > +void RenderObject::mapLocalToContainerInternal(const RenderLayerModelObject* repaintContainer, TransformState& transformState, LayoutSize& accumulatedOffset, MapCoordinatesFlags mode, bool* wasFixed) const Not a huge fan of the name ('internal' is kinda pointless here). How about mapLocalToContainerAccumulatingOffset? (couldn't think of something better or shorter) > Source/WebCore/rendering/RenderObject.cpp:2088 > - return; > + return transformState.move(adjustOffsetForSnapping(mode, accumulatedOffset)); How can this compile? mapLocalToContainerInternal is void. > Source/WebCore/rendering/RenderView.cpp:231 > + transformState.move(adjustOffsetForSnapping(mode, m_frameView->scrollOffsetForFixedPosition() + accumulatedOffset)); > + accumulatedOffset = LayoutSize(); Can't this be written: accumulatedOffset.expand(m_frameView->scrollOffsetForFixedPosition()) applyAccumulatedOffset(transformState, accumulatedOffset, mode); > Source/WebCore/rendering/svg/RenderSVGText.cpp:116 > - SVGRenderSupport::mapLocalToContainer(this, repaintContainer, transformState, mode & SnapOffsetForTransforms, wasFixed); > + SVGRenderSupport::mapLocalToContainer(this, repaintContainer, transformState, accumulatedOffset, mode, wasFixed); The ChangeLog should mention why it is fine to remove mode & SnapOffsetForTransforms from all the SVG classes, along with keep the default value for MapCoordinatesFlags to ApplyContainerFlip | SnapOffsetForTransforms.

Comment on attachment 178596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178596&action=review Why aren't the pixel results using mock scrollbars? It's very hard for me to tell whether this patch is correct. I'd feel more confident if you had a bunch of tests that hit assertions with the wrong version of the patch, but no longer do so. r- for the apparently incorrect flattening. > Source/WebCore/ChangeLog:42 > + (WebCore::RenderObject::mapLocalToContainer): Broke mapLocalToContainer into this public function and > + mapLocalToContainerInternal. The internal flavors track an accumulated offset which is used to recreate the pixel > + snapping that's performed during paint. This public function calls the internal one, then applies any leftover > + accumulated offset before returning the TransformState to the caller. mapLocalToContainer was already the "internal" method. I agree with Julian that some renaming might help. > Source/WebCore/rendering/RenderBox.cpp:1425 > + applyAccumulatedOffset(transformState, accumulatedOffset, mode); applyAccumulatedOffset() is flattening here, since the default last param is 'false'. How can that be correct? > Source/WebCore/rendering/RenderBox.cpp:1429 > - transformState.move(containerOffset.width(), containerOffset.height(), preserve3D ? TransformState::AccumulateTransform : TransformState::FlattenTransform); > + accumulatedOffset.expand(containerOffset); So it seems like you're carrying along all of the non-transform offsets in accumulatedOffset, which kinda defeats the purpose of TransformState. This seems like a cop-out.

Created attachment 179318 [details] Patch

Managed to simplify things greatly and move everything into TransformState. I'd missed a couple things that prevented me from getting this solution right before. This also *removes* SnapOffsetForTransforms, which should make everyone pretty happy :)

Comment on attachment 179318 [details] Patch Attachment 179318 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15315386

Created attachment 179325 [details] Patch

Comment on attachment 179325 [details] Patch Attachment 179325 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15313424

Created attachment 179366 [details] Patch

Comment on attachment 179366 [details] Patch I like this so much better!.

Comment on attachment 179366 [details] Patch Attachment 179366 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15315675 New failing tests: fast/dom/Window/webkitConvertPoint.html transforms/3d/point-mapping/3d-point-mapping-deep.html accessibility/radio-button-group-members.html

Created attachment 179660 [details] Patch for landing

Comment on attachment 179660 [details] Patch for landing Clearing flags on attachment: 179660 Committed r137847: <http://trac.webkit.org/changeset/137847>

All reviewed patches have been landed. Closing bug.