[chromium] renderSurface in incorrect space if owning layer has empty but non-zero bounds

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

A particular translation transform was inside an if-statement that caused it to be accidentally skipped when it shouldn't be.  The fix is simply to move it outside of the if-statement so it is not skipped.

I have a feeling the other transform that remains inside the if-statement will also get incorrectly skipped, affecting correctness in high-DPI mode.  However, that one is not as clear how to fix (if it actually is a problem in the first place?), because not-skipping it means that we might be scaling by a zero width or height, causing the entire subtree to become invisible.   It might work to have separate scale transforms for width and height, and skip them individually when appropriate, but i'm not completely sure that would work as intended either.

So yeah, if it's OK with reviewers I would like to land this simple straightforward part first, get it merged to m22 sooner, and then deal with the high-DPI part separately.

[Shawn Singh]

Created attachment 157926 [details]
Patch

tested on osx with layout and unit tests.  Also fixed one comment that used right-to-left name...  We have discussed possibly switching the naming convention of transforms to that style, but for now those transforms were being written so that their name interprets what happens to the coordinate space from left-to-right.

[Adrienne Walker]

(In reply to comment #0)
> A particular translation transform was inside an if-statement that caused it to be accidentally skipped when it shouldn't be.  The fix is simply to move it outside of the if-statement so it is not skipped.
> 
> I have a feeling the other transform that remains inside the if-statement will also get incorrectly skipped, affecting correctness in high-DPI mode.

I am less sure about that, actually.  I think the surface-creating code makes some assumptions that device scale = contentsScale(), and it's possible that this other transform might be moot.  Along with a degenerate case like this bug with bounds() != contentBounds() case, I also worry about a layer with bounds() == contentBounds() creating a surface when deviceScale != 1.

> So yeah, if it's OK with reviewers I would like to land this simple straightforward part first, get it merged to m22 sooner, and then deal with the high-DPI part separately.

Is there a separate bug for investigating the high-DPI end of things? Is this something that you can also look into?

[Adrienne Walker]

Comment on attachment 157926 [details]
Patch

R=me.  That's a really elegant test, to have all draw transforms come out to be identity.  :)

[Shawn Singh]

Thanks for the review =)

I created https://bugs.webkit.org/show_bug.cgi?id=93919 for the highDPI questions we have.  Of course please correct me if my comments there are not actually what you were worried about. =)

[Shawn Singh]

Committed r125486: <http://trac.webkit.org/changeset/125486>