If that tile cache has been given a margin and the page background color changes dynamically, we fail to repaint the edge tiles that are revealed with rubber-banding. <rdar://problem/15578131>
Created attachment 220458 [details] Patch
Comment on attachment 220458 [details] Patch I don't think it's right that GraphicsLayerCA is polluted with knowledge of TiledBacking margins, and I think this happens to fix repaints of hugeRects, but I don't think there's any guarantee that background color changes are always done through whole-layer repaints.
I think we need a better idea of what paints into margin tiles before we fix this, and that will require teaching more of rendering about margin tile painting.
Created attachment 220592 [details] Patch Here's a new patch that takes an approach that Simon and I discussed today.
Attachment 220592 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.cpp', u'Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h', u'Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp', u'Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderView.cpp', u'Source/WebCore/rendering/RenderView.h', '--commit-queue']" exit_code: 1 ERROR: Source/WebCore/platform/graphics/blackberry/GraphicsLayerBlackBerry.h:93: The parameter name "shouldClip" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:99: The parameter name "shouldClip" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:46: The parameter name "shouldClip" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/rendering/RenderLayerBacking.cpp:2093: Missing space after , [whitespace/comma] [3] ERROR: Source/WebCore/rendering/RenderView.cpp:577: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:109: The parameter name "shouldClip" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/GraphicsLayer.h:359: The parameter name "shouldClip" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 7 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 220593 [details] Patch
Comment on attachment 220593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220593&action=review > Source/WebCore/rendering/RenderView.h:357 > + bool m_currentlyRepaintingRootContents; I'm not really keep on storing this state here, since it's so transient. Can't we just pass it through the repaint functions?
(In reply to comment #7) > (From update of attachment 220593 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=220593&action=review > > > Source/WebCore/rendering/RenderView.h:357 > > + bool m_currentlyRepaintingRootContents; > > I'm not really keep on storing this state here, since it's so transient. Can't we just pass it through the repaint functions? Sure.
Created attachment 220649 [details] Patch
Comment on attachment 220649 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220649&action=review > Source/WebCore/rendering/RenderLayerBacking.cpp:2097 > + FloatRect rectToRepaint = FloatRect(FloatPoint(0, 0), m_graphicsLayer->size()); > + TiledBacking* tiledBacking = this->tiledBacking(); > + rectToRepaint.move(-tiledBacking->leftMarginWidth(), -tiledBacking->topMarginHeight()); > + rectToRepaint.expand(tiledBacking->leftMarginWidth() + tiledBacking->rightMarginWidth(), > + tiledBacking->topMarginHeight() + tiledBacking->bottomMarginHeight()); I think it would be better to call a function on TiledBacking that inflates the rect. That way you could eliminate this whole tiledBackingHasMargin() condition.
Created attachment 220681 [details] Patch
Comment on attachment 220681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220681&action=review > Source/WebCore/ChangeLog:46 > + with a big enough rect and a ShouldClipToLayer value of DontClipToLayer. extra space between 'a' and 'big'. shouldn't it be "DoNotClipToLayer"? > Source/WebCore/platform/graphics/GraphicsLayer.h:353 > + DontClipToLayer, yeah. DoNot.
(In reply to comment #12) > (From update of attachment 220681 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=220681&action=review > > > Source/WebCore/ChangeLog:46 > > + with a big enough rect and a ShouldClipToLayer value of DontClipToLayer. > > extra space between 'a' and 'big'. > Will fix! > shouldn't it be "DoNotClipToLayer"? > > > Source/WebCore/platform/graphics/GraphicsLayer.h:353 > > + DontClipToLayer, > > yeah. DoNot. Will fix!
Created attachment 220683 [details] Patch New new patch that addresses Tim's two small comments.
Comment on attachment 220683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=220683&action=review > Source/WebCore/platform/graphics/ca/mac/TileController.mm:1085 > +void TileController::inflateRectForMargin(FloatRect& rect) const > +{ > + rect.move(-leftMarginWidth(), -topMarginHeight()); > + rect.expand(leftMarginWidth() + rightMarginWidth(), topMarginHeight() + bottomMarginHeight()); > +} Rather than add margins to some arbitrary rect, maybe just have it return its size with margins, since the caller is already just passing in the same rect as the TileController size. > Source/WebCore/rendering/RenderLayerBacking.cpp:2090 > + FloatRect rectToRepaint = FloatRect(FloatPoint(0, 0), m_graphicsLayer->size()); > + tiledBacking()->inflateRectForMargin(rectToRepaint); Null-check here.
Comment on attachment 220683 [details] Patch Attachment 220683 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5629190430785536 New failing tests: compositing/backface-visibility/backface-visibility-hierarchical-transform.html http/tests/canvas/philip/tests/security.pattern.canvas.strokeStyle.html compositing/composited-negative-zindex-child.html animations/change-transform-style-during-animation.html compositing/backing/no-backing-for-clip-overhang.html compositing/backing/no-backing-for-clip-overlap.html accessibility/transformed-bounds.html compositing/columns/ancestor-clipped-in-paginated.html compositing/bounds-in-flipped-writing-mode.html compositing/clip-change.html compositing/background-color/background-color-text-change.html compositing/animation/state-at-end-event-transform-layer.html http/tests/canvas/philip/tests/security.drawImage.image.html animations/animation-direction-reverse-hardware.html compositing/backface-visibility/backface-visibility-3d.html accessibility/media-element.html http/tests/canvas/philip/tests/security.drawImage.canvas.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html canvas/philip/tests/2d.clearRect.basic.html http/tests/canvas/philip/tests/security.pattern.canvas.fillStyle.html compositing/backface-visibility/backface-visibility-webgl.html compositing/background-color/background-color-text-clip.html compositing/backface-visibility/backface-visibility-image.html compositing/background-color/background-color-padding-change.html animations/animation-direction-reverse-fill-mode-hardware.html accessibility/noscript-ignored.html canvas/philip/tests/2d.clearRect+fillRect.basic.html compositing/color-matching/pdf-image-match.html compositing/clip-child-by-non-stacking-ancestor.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html
Created attachment 220747 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
(In reply to comment #15) > (From update of attachment 220683 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=220683&action=review > > > Source/WebCore/platform/graphics/ca/mac/TileController.mm:1085 > > +void TileController::inflateRectForMargin(FloatRect& rect) const > > +{ > > + rect.move(-leftMarginWidth(), -topMarginHeight()); > > + rect.expand(leftMarginWidth() + rightMarginWidth(), topMarginHeight() + bottomMarginHeight()); > > +} > > Rather than add margins to some arbitrary rect, maybe just have it return its size with margins, since the caller is already just passing in the same rect as the TileController size. > Sure, that makes sense. > > Source/WebCore/rendering/RenderLayerBacking.cpp:2090 > > + FloatRect rectToRepaint = FloatRect(FloatPoint(0, 0), m_graphicsLayer->size()); > > + tiledBacking()->inflateRectForMargin(rectToRepaint); > > Null-check here. Thanks for catching!
http://trac.webkit.org/changeset/161570