Bug 92328 – [chromium] When computing surface content scale, use top level device scale

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

[chromium] When computing surface content scale, use top level device scale

[Adrienne Walker]

Created attachment 154523 [details]
Patch

[Dana Jansens]

Comment on attachment 154523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154523&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-569
> -    float contentsScale = layer->contentsScale();

can we remove CCLayerImpl::contentsScale then?

[Adrienne Walker]

(In reply to comment #2)
> (From update of attachment 154523 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154523&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-569
> > -    float contentsScale = layer->contentsScale();
> 
> can we remove CCLayerImpl::contentsScale then?

Maybe we shouldn't have layer->contentsScale at all, and setContentsScale should setContentBounds, where appropriate?

[Dana Jansens]

Comment on attachment 154523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154523&action=review

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-569
>>> -    float contentsScale = layer->contentsScale();
>> 
>> can we remove CCLayerImpl::contentsScale then?
> 
> Maybe we shouldn't have layer->contentsScale at all, and setContentsScale should setContentBounds, where appropriate?

I mean, contentsScale on LayerChromium makes sense as a hint to help create the contentBounds (tho maybe we should push that down to GraphicsLayerChromium)? But I think we should remove it from CCLayerImpl again if we can now, we plumbed it thru just for this code which is now going away here.

[Dana Jansens]

(In reply to comment #4)
> (From update of attachment 154523 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154523&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-569
> >>> -    float contentsScale = layer->contentsScale();
> >> 
> >> can we remove CCLayerImpl::contentsScale then?
> > 
> > Maybe we shouldn't have layer->contentsScale at all, and setContentsScale should setContentBounds, where appropriate?
> 
> I mean, contentsScale on LayerChromium makes sense as a hint to help create the contentBounds (tho maybe we should push that down to GraphicsLayerChromium)? But I think we should remove it from CCLayerImpl again if we can now, we plumbed it thru just for this code which is now going away here.

Or.. ya I get what you mean now.. that sounds like it could be very nice.

[Adrienne Walker]

Created attachment 154529 [details]
Remove CCLayerImpl contents scale

[Adrienne Walker]

(In reply to comment #6)
> Created an attachment (id=154529) [details]
> Remove CCLayerImpl contents scale

Yeah, removing it from the LayerChromium side *sounds* nice but is way more complicated.  Punt!

[Dana Jansens]

Comment on attachment 154529 [details]
Remove CCLayerImpl contents scale

yup! LGTM!

[Adrienne Walker]

Created attachment 154695 [details]
Rebased

[Dana Jansens]

prpl :(

[Adrienne Walker]

Created attachment 154722 [details]
Rebased

I love this patch. LGTM, too.

[Dana Jansens]

Yay green, still lgtm :)

[Dana Jansens]

kbr or senorblanco for review, as jamesr is still in catch-up mode?

[Kenneth Russell]

Comment on attachment 154722 [details]
Rebased

rs=me

[Adrienne Walker]

Comment on attachment 154722 [details]
Rebased

Thanks, kbr.  :)

[WebKit Review Bot]

Comment on attachment 154722 [details]
Rebased

Clearing flags on attachment: 154722

Committed r123814: <http://trac.webkit.org/changeset/123814>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.