Bug 79145

The following layout tests were rebaselined in a previous change where we tracked an opaque white paint. Turns out that was in a SkCanvas layer, and the original baselines match again after this CL. So I'm including the original baselines again. compositing/overflow/clip-content-under-overflow-controls-expected.png compositing/scrollbar-painting-expected.png

Image clipping is done via a layer, so we can remove the special case code around image clipping in OpaqueRegionSkia.

Created attachment 128062 [details] Patch Ooh ok, comment #2 was a lie. The layout tests are right.

Comment on attachment 128062 [details] Patch This looks like Stephen's territory to me.

Comment on attachment 128062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128062&action=review > Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:261 > + AffineTransform restoreTransform = restoringToRootLayer ? transform : AffineTransform(); This is a little confusing to me. Shouldn't this be the other way around (root layer = empty transform, other = m_opaqueRegionTransform)? or does m_opaqueRegionTransform always represent the root layer transform? > Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:271 > + if (!deviceClip.isRect()) > + return; Are you going to need to handle non-rect regions at some point? Or does WebKit never create them?

Comment on attachment 128062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128062&action=review >> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:261 >> + AffineTransform restoreTransform = restoringToRootLayer ? transform : AffineTransform(); > > This is a little confusing to me. Shouldn't this be the other way around (root layer = empty transform, other = m_opaqueRegionTransform)? or does m_opaqueRegionTransform always represent the root layer transform? You're right I should change this. We store the root layer in "chromium layer" coordinate space. I thought I'd need to transform other canvas layers but I don't so I should store them all in the same space. Thanks. >> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:271 >> + return; > > Are you going to need to handle non-rect regions at some point? Or does WebKit never create them? I don't expect we'll bother to go there, at least any time soon. WebKit can (probably?) create them but the complexity of dealing with them gets much larger.

Created attachment 128542 [details] Patch The work to pop a layer is a bit more (extra mapRect() call), but all opaque rects are stored in the target coordinate space for consistency.

Comment on attachment 128542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128542&action=review OK with me, assuming enne is ok with it. > Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:268 > + // We have a rect in target space, move it to device space. > + if (!transform.isInvertible()) > + return; > + SkMatrix targetToDeviceTransform = transform.inverse(); > + if (!targetToDeviceTransform.mapRect(&opaqueLayerRect)) > + return; Nit: Could the cons of the LayerIter above be moved below these early-outs? Not a big deal, but kind of nice to early-out as early as possible.

Created attachment 128549 [details] Patch nit done!

Comment on attachment 128549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128549&action=review > Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:99 > + canvasPainter.skiaContext()->setTrackOpaqueRegion(false); I don't understand this change and the SkPictureCanvas updater change? Why is this needed with this patch?

Comment on attachment 128549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128549&action=review >> Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:99 >> + canvasPainter.skiaContext()->setTrackOpaqueRegion(false); > > I don't understand this change and the SkPictureCanvas updater change? Why is this needed with this patch? Ah! Because the canvasPainter has a save() and restore(). The save() is done in the constructor, so setTrackOpaqueRegion() is not true, and the tracker doesn't know about it. The restore() is done in the destructor, and if the tracker tries to execute it, then it sees unbalanced save/restore. It can work but i put an assert to catch this. Turning off tracking here makes any clean up not cause unbalance with the opaque tracking.

Comment on attachment 128549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128549&action=review >>> Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:99 >>> + canvasPainter.skiaContext()->setTrackOpaqueRegion(false); >> >> I don't understand this change and the SkPictureCanvas updater change? Why is this needed with this patch? > > Ah! Because the canvasPainter has a save() and restore(). > The save() is done in the constructor, so setTrackOpaqueRegion() is not true, and the tracker doesn't know about it. > The restore() is done in the destructor, and if the tracker tries to execute it, then it sees unbalanced save/restore. It can work but i put an assert to catch this. Turning off tracking here makes any clean up not cause unbalance with the opaque tracking. This is just a drive-by opinion, but that seems really fragile to have to case about whether flags were true at various points in time during save/restores. Is it reasonable to instead maybe not assert and early-out, or maybe perform some logic in all cases to be consistent, or maybe to set the right value up front prior to calling save() in the first place and disallow changing it at the wrong time?

Well, I was just thinking about a shorter answer to this: We want to track what is done in paintContents() only. Leaving tracking on after makes us track things we're not interested in. The code does not break if you restore() more than you save(). But I put an assert since it's not valid. GraphicsContext::restore() has LOG_ERROR when this happens there. So maybe just take the assert out?

Created attachment 128594 [details] Patch ok no longer need the setTracking(false). PlatformContextSkia will always keep track of layers so that no matter when you enable tracking, you are in a valid state. The tracking is pretty much a no-op when tracking is not on, other than append/remove from vector.

Comment on attachment 128594 [details] Patch Thanks! LGTM.

Comment on attachment 128594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128594&action=review > Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:236 > + m_opaqueRectStack.append(SkRect::MakeEmpty()); It seems like m_opaqueRect should be made empty here.

Created attachment 129530 [details] Patch Avoid the whole didSave/willRestore business, and the whole stack thing. Use whatever layers are on the SkCanvas directly when tracking a draw.

Comment on attachment 129530 [details] Patch Me likey! Much simpler. Will leave for enne to take a look.

(In reply to comment #18) > (From update of attachment 129530 [details]) > Me likey! Much simpler. Will leave for enne to take a look. I don't understand enough about SkCanvas layers to review the changes in this latest patch. This is all you, Stephen.

Comment on attachment 129530 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129530&action=review > Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:265 > + if (!targetRect.intersect(SkIntToScalar(clipBounds.fLeft), SkIntToScalar(clipBounds.fTop), SkIntToScalar(clipBounds.fRight), SkIntToScalar(clipBounds.fBottom))) > + return; Hmmm.. are you sure you want to early-return here? It seems like we won't be marking the rect either way if any one of the layers has non-intersecting bounds. Is that intentional?

(In reply to comment #20) > (From update of attachment 129530 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129530&action=review > > > Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:265 > > + if (!targetRect.intersect(SkIntToScalar(clipBounds.fLeft), SkIntToScalar(clipBounds.fTop), SkIntToScalar(clipBounds.fRight), SkIntToScalar(clipBounds.fBottom))) > > + return; > > Hmmm.. are you sure you want to early-return here? It seems like we won't be marking the rect either way if any one of the layers has non-intersecting bounds. Is that intentional? Yeh, it means the draw is being clipped out of existence so there's nothing to track. It's similar to the clip check above.

Comment on attachment 129530 [details] Patch OK then. r=me

The commit-queue encountered the following flaky tests while processing attachment 129530 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: dumi@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 129530 [details] Patch Rejecting attachment 129530 [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: ed at Tools/Scripts/update-webkit line 164. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 9 ate pixel count for frame rate control When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. Full output: http://queues.webkit.org/results/11776188

Created attachment 129702 [details] Patch for landing

Comment on attachment 129702 [details] Patch for landing Rejecting attachment 129702 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11757475

Comment on attachment 129702 [details] Patch for landing Rejecting attachment 129702 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11776478

Comment on attachment 129702 [details] Patch for landing Rejecting attachment 129702 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11766573

Created attachment 129735 [details] Patch Try to bypass this cq bot..

Comment on attachment 129735 [details] Patch Clearing flags on attachment: 129735 Committed r109514: <http://trac.webkit.org/changeset/109514>

All reviewed patches have been landed. Closing bug.