[chromium] Fix incorrect scissor rect for layers that render into a rendersurface
Created attachment 90399 [details] Patch
I will point out that those few lines of code are essentially the same as what's in ContentLayerChromium::visibleLayerRect. However, given that bug 58840 is going to need to refactor this same code again, I saw no reason to try to do surgery and add a common code path for this calculation. This comes with a test though, so we'll know when we break it next time. :)
Oops, I never CC'd anybody on this bug. No wonder it didn't get a review. Vangelis needs this commit for some of the layer sorting work.
Comment on attachment 90399 [details] Patch The test looks like a good candidate for layoutTestController.dumpAsText(true);
It's the scissor rect for drawing that's the problem, not the bounds on the layer itself. Unless I'm mistaken, that doesn't get dumped?
I meant using dumpAsText(true) to avoid creating a render tree dump, since that's irrelevant to the test. The only useful output from this test is the pixel output.
Oh, my mistake. How does that work for ports that don't do pixel tests? Would this test just be skipped?
There's still text output, it's just the dumpAsText() output. For this test it'd probably be empty and the test would pass (trivially) on platforms that don't run pixel tests.
Created attachment 90790 [details] Patch
(In reply to comment #9) > Created an attachment (id=90790) [details] > Patch Oh, great. Thanks for the suggestion. :)
Comment on attachment 90790 [details] Patch Looks fine assuming it's been tested.
Committed r85075: <http://trac.webkit.org/changeset/85075>
Committed r85100: <http://trac.webkit.org/changeset/85100>
This breaks deeply-nested-reflections. Will investigate and reland.
(In reply to comment #14) > This breaks deeply-nested-reflections. Will investigate and reland. Ah, I see what the trouble is. Before my patch and with my patch it was trying to use the layer bounds as a scissor rect but it really needs to be scissoring to the render surface bounds instead. Of course, the render surface bounds aren't known until we've iterated through the layer tree and unioned all the layer bounds that are going into that render surface. However, the scissor rect for each layer is stored on that layer itself, rather than inherited from some higher level. So, some refactoring is needed to not have to do a "scissor rect update" pass or to only store/set the scissor rect where it's really needed (on clipping layers).
Created attachment 92353 [details] Patch
Comment on attachment 92353 [details] Patch Vangelis should do the unofficial review of this patch, but my only comment is to make sure the new test doesn't turn any of the other platforms' bots red.
Ok, here's another try. updatePropertiesEtcEtcEtc is kind of entangled and hard to follow, so it took me a while to sort out how best to fix this. I have some refactoring ideas though that can wait until after this patch. Here's a quick summary of this change. Prior to this bug, the scissor rect for some layers was being set incorrectly. It just happened to work out that in the majority of cases the scissor was large enough to not clip anything accidentally, but in some cases it did, which is demonstrated by the new test case. Setting the scissor rect based on the layer bounds as both the previous patch and the code before that patch tried to do is incorrect as the bounds of a layer don't contain its sublayers. Instead, it really needs to be set to the render surface size. However, this size is unfortunately not calculated until after we're done with the whole recursive updatePropertiesEtcEtcEtc call. So instead, when rendering into a render surface, the scissor rect for the layer subtree that's going into that render surface gets cleared, because it no longer applies. An empty scissor implies no scissor at all. The fallout from that is a number of places where we need to check for empty scissor rects and use the render surface size as the visible portion of the layer instead.
(In reply to comment #17) > (From update of attachment 92353 [details]) > Vangelis should do the unofficial review of this patch, but my only comment is to make sure the new test doesn't turn any of the other platforms' bots red. As Chromium is the only port that does pixel tests and there's no text output from this test, this should work. I ran it on OSX with Safari a while back and it ran cleanly. It also didn't break anything but Chromium tests when it landed before, so I'm reasonably confident about it.
Comment on attachment 92353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92353&action=review One small comment from me, otherwise LG. Thanks for fixing this up. > LayoutTests/platform/chromium/test_expectations.txt:3984 > +BUGENNE GPU : compositing/flat-with-transformed-child.html = FAIL MISSING Since you are adding a baseline for linux, shouldn't the be PASS FAIL MISSING or maybe better use MAC WINDOWS qualifiers > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:912 > + bool isLayerVisible = layer->scissorRect().isEmpty() || layer->scissorRect().intersects(layerRect); Don't we need to check against the renderSurface's contentRect if the scissorRect is empty?
Comment on attachment 92353 [details] Patch R=me - please address Vangelis' comments before landing.
Committed r85959: <http://trac.webkit.org/changeset/85959>
Committed r85970: <http://trac.webkit.org/changeset/85970>