Bug 92328

Summary: [chromium] When computing surface content scale, use top level device scale
Product: WebKit Reporter: Adrienne Walker <enne>
Component: New BugsAssignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, danakj, enne, jamesr, kbr, senorblanco, vollick, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 91815    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Remove CCLayerImpl contents scale
none
Rebased
none
Rebased none

Description Adrienne Walker 2012-07-25 19:07:21 PDT
[chromium] When computing surface content scale, use top level device scale
Comment 1 Adrienne Walker 2012-07-25 19:09:33 PDT
Created attachment 154523 [details]
Patch
Comment 2 Dana Jansens 2012-07-25 19:14:44 PDT
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?
Comment 3 Adrienne Walker 2012-07-25 19:24:27 PDT
(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?
Comment 4 Dana Jansens 2012-07-25 19:29:32 PDT
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.
Comment 5 Dana Jansens 2012-07-25 19:30:20 PDT
(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.
Comment 6 Adrienne Walker 2012-07-25 19:47:38 PDT
Created attachment 154529 [details]
Remove CCLayerImpl contents scale
Comment 7 Adrienne Walker 2012-07-25 19:48:08 PDT
(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!
Comment 8 Dana Jansens 2012-07-25 22:35:19 PDT
Comment on attachment 154529 [details]
Remove CCLayerImpl contents scale

yup! LGTM!
Comment 9 Adrienne Walker 2012-07-26 11:27:03 PDT
Created attachment 154695 [details]
Rebased
Comment 10 Dana Jansens 2012-07-26 12:04:08 PDT
prpl :(
Comment 11 Adrienne Walker 2012-07-26 12:31:55 PDT
Created attachment 154722 [details]
Rebased
Comment 12 vollick 2012-07-26 12:38:08 PDT
I love this patch. LGTM, too.
Comment 13 Dana Jansens 2012-07-26 13:59:07 PDT
Yay green, still lgtm :)
Comment 14 Dana Jansens 2012-07-26 15:19:50 PDT
kbr or senorblanco for review, as jamesr is still in catch-up mode?
Comment 15 Kenneth Russell 2012-07-26 15:54:40 PDT
Comment on attachment 154722 [details]
Rebased

rs=me
Comment 16 Adrienne Walker 2012-07-26 15:55:40 PDT
Comment on attachment 154722 [details]
Rebased

Thanks, kbr.  :)
Comment 17 WebKit Review Bot 2012-07-26 16:55:35 PDT
Comment on attachment 154722 [details]
Rebased

Clearing flags on attachment: 154722

Committed r123814: <http://trac.webkit.org/changeset/123814>
Comment 18 WebKit Review Bot 2012-07-26 16:55:40 PDT
All reviewed patches have been landed.  Closing bug.