Bug 59020 – [chromium] Fix incorrect scissor rect for layers that render into a rendersurface

• Component: New Bugs
• Status: RESOLVED FIXED

[Adrienne Walker]

[chromium] Fix incorrect scissor rect for layers that render into a rendersurface

[Adrienne Walker]

Created attachment 90399 [details]
Patch

[Adrienne Walker]

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.  :)

[Adrienne Walker]

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.

[James Robinson]

Comment on attachment 90399 [details]
Patch

The test looks like a good candidate for layoutTestController.dumpAsText(true);

[Adrienne Walker]

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?

[James Robinson]

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.

[Adrienne Walker]

Oh, my mistake.  How does that work for ports that don't do pixel tests? Would this test just be skipped?

[James Robinson]

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.

[Adrienne Walker]

Created attachment 90790 [details]
Patch

[Adrienne Walker]

(In reply to comment #9)
> Created an attachment (id=90790) [details]
> Patch

Oh, great.  Thanks for the suggestion.  :)

[Kenneth Russell]

Comment on attachment 90790 [details]
Patch

Looks fine assuming it's been tested.

[Adrienne Walker]

Committed r85075: <http://trac.webkit.org/changeset/85075>

[Adrienne Walker]

Committed r85100: <http://trac.webkit.org/changeset/85100>

[Adrienne Walker]

This breaks deeply-nested-reflections.  Will investigate and reland.

[Adrienne Walker]

(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).

[Adrienne Walker]

Created attachment 92353 [details]
Patch

[Kenneth Russell]

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.

[Adrienne Walker]

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.

[Adrienne Walker]

(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.

[Vangelis Kokkevis]

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?

[James Robinson]

Comment on attachment 92353 [details]
Patch

R=me - please address Vangelis' comments before landing.

[Adrienne Walker]

Committed r85959: <http://trac.webkit.org/changeset/85959>

[Adrienne Walker]

Committed r85970: <http://trac.webkit.org/changeset/85970>