Bug 74086 - [chromium] Layer contents scale change should trigger invalidation
Summary: [chromium] Layer contents scale change should trigger invalidation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-08 06:43 PST by Sami Kyostila
Modified: 2011-12-08 16:38 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.44 KB, patch)
2011-12-08 07:03 PST, Sami Kyostila
jamesr: review+
jamesr: commit-queue-
Details | Formatted Diff | Diff
Patch (4.27 KB, patch)
2011-12-08 12:19 PST, Sami Kyostila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sami Kyostila 2011-12-08 06:43:17 PST
LayerChromium::setContentsScale() should invalidate the layer contents to ensure the scale change becomes visible.
Comment 1 Sami Kyostila 2011-12-08 07:03:31 PST
Created attachment 118384 [details]
Patch
Comment 2 Adrienne Walker 2011-12-08 10:54:39 PST
Comment on attachment 118384 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:428
> +    if (!needsContentsScale() || m_contentsScale == contentsScale)

I don't understand how this interacts with the needsContentsScale() check.  Only ContentLayerChromium handles contents scale at this point.  Do we want to set the contents scale for any layer, even if it doesn't handle it?

What layer type are you trying to solve this for?
Comment 3 Sami Kyostila 2011-12-08 11:15:22 PST
Comment on attachment 118384 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:428
>> +    if (!needsContentsScale() || m_contentsScale == contentsScale)
> 
> I don't understand how this interacts with the needsContentsScale() check.  Only ContentLayerChromium handles contents scale at this point.  Do we want to set the contents scale for any layer, even if it doesn't handle it?
> 
> What layer type are you trying to solve this for?

The contents scale should only be set for layers for which needsContentScale() is true. The check that I've added is intended to avoid unnecessary invalidation in the case that the contents scale did not actually change from its current value. I haven't checked whether that actually happens; this is just a precaution.

I'm seeing the problem of missed invalidations on non-root ContentLayerChromium instances. The root layer is properly invalidated through WebViewImpl::invalidateRootLayerRect() when the page scale changes.
Comment 4 Adrienne Walker 2011-12-08 12:02:39 PST
Comment on attachment 118384 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:428
>>> +    if (!needsContentsScale() || m_contentsScale == contentsScale)
>> 
>> I don't understand how this interacts with the needsContentsScale() check.  Only ContentLayerChromium handles contents scale at this point.  Do we want to set the contents scale for any layer, even if it doesn't handle it?
>> 
>> What layer type are you trying to solve this for?
> 
> The contents scale should only be set for layers for which needsContentScale() is true. The check that I've added is intended to avoid unnecessary invalidation in the case that the contents scale did not actually change from its current value. I haven't checked whether that actually happens; this is just a precaution.
> 
> I'm seeing the problem of missed invalidations on non-root ContentLayerChromium instances. The root layer is properly invalidated through WebViewImpl::invalidateRootLayerRect() when the page scale changes.

Thanks for the explanation; this makes a lot more sense to me with that context.  :)
Comment 5 James Robinson 2011-12-08 12:09:29 PST
Comment on attachment 118384 [details]
Patch

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

Please fix the ChangeLogs + nits, otherwise R=me

> Source/WebCore/ChangeLog:10
> +        Change-Id: Ic824414f65c2214ab7bacbe50f909c4cbc9ab59d

I don't know where this comes from but it doesn't belong here.

>>>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:428
>>>> +    if (!needsContentsScale() || m_contentsScale == contentsScale)
>>> 
>>> I don't understand how this interacts with the needsContentsScale() check.  Only ContentLayerChromium handles contents scale at this point.  Do we want to set the contents scale for any layer, even if it doesn't handle it?
>>> 
>>> What layer type are you trying to solve this for?
>> 
>> The contents scale should only be set for layers for which needsContentScale() is true. The check that I've added is intended to avoid unnecessary invalidation in the case that the contents scale did not actually change from its current value. I haven't checked whether that actually happens; this is just a precaution.
>> 
>> I'm seeing the problem of missed invalidations on non-root ContentLayerChromium instances. The root layer is properly invalidated through WebViewImpl::invalidateRootLayerRect() when the page scale changes.
> 
> Thanks for the explanation; this makes a lot more sense to me with that context.  :)

this feels a lot like an issue with our type system. We have functions on LayerChromium that should only ever be called on subclasses of LayerChromium, and we have this information available at the callsite.

Something to clean up later, methinks. This looks fine.

> Source/WebKit/chromium/ChangeLog:10
> +        Change-Id: Ic824414f65c2214ab7bacbe50f909c4cbc9ab59d

Same here

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:710
> +    bool needsContentsScale() const

this is virtual, please tag it as such here for clarity
Comment 6 Sami Kyostila 2011-12-08 12:19:35 PST
Created attachment 118445 [details]
Patch

Thanks James and Enne. This patch addresses the remaining issues.
Comment 7 WebKit Review Bot 2011-12-08 16:38:02 PST
Comment on attachment 118445 [details]
Patch

Clearing flags on attachment: 118445

Committed r102395: <http://trac.webkit.org/changeset/102395>
Comment 8 WebKit Review Bot 2011-12-08 16:38:06 PST
All reviewed patches have been landed.  Closing bug.