[webkit-reviews] review granted: [Bug 74086] [chromium] Layer contents scale change should trigger invalidation : [Attachment 118384] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Sami Kyostila
<skyostil at google.com>'s request for review:
Bug 74086: [chromium] Layer contents scale change should trigger invalidation
https://bugs.webkit.org/show_bug.cgi?id=74086

Attachment 118384: Patch
https://bugs.webkit.org/attachment.cgi?id=118384&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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