Bug 81968 – [chromium] Force update of nonFastScrollableRegion if target CCLayerImpl has been freshly created.

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Force update of nonFastScrollableRegion if target CCLayerImpl has been freshly created.

[W. James MacLean]

Created attachment 133347 [details]
Patch

[W. James MacLean]

I'll add a test if the approach in this patch is deemed correct.

[James Robinson]

Comment on attachment 133347 [details]
Patch

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

Good catch!  This looks like the right fix, but it definitely needs test coverage before R+.

Dropping CCLayerImpl's on !visible is troublesome, we should perhaps revisit that sometimes.

> Source/WebCore/ChangeLog:8
> +        No new tests, yet. (OOPS!)

This needs tests, obviously.

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:76
> +        // FIXME: Is it valid to assume a reused CCLayerImpl will have the correct nonFastScrollableRegion?

yes, it is

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:77
> +        layer->setNonFastScrollableRegionChanged(); // We just cleared the impl layer, so don't opt-out of transferring the nonFastScrollableRegion.

the double negative isn't hard to misunderstand here. rephrase perhaps?

[W. James MacLean]

Comment on attachment 133347 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests, yet. (OOPS!)
> 
> This needs tests, obviously.

Working on one now, will upload with new patch shortly.

>> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:76
>> +        // FIXME: Is it valid to assume a reused CCLayerImpl will have the correct nonFastScrollableRegion?
> 
> yes, it is

Good to know ... FIXME comment removed.

>> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:77
>> +        layer->setNonFastScrollableRegionChanged(); // We just cleared the impl layer, so don't opt-out of transferring the nonFastScrollableRegion.
> 
> the double negative isn't hard to misunderstand here. rephrase perhaps?

Fixed.

[W. James MacLean]

Created attachment 133374 [details]
Patch

[Adrienne Walker]

(In reply to comment #3)
> (From update of attachment 133347 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133347&action=review
> 
> Good catch!  This looks like the right fix, but it definitely needs test coverage before R+.
> 
> Dropping CCLayerImpl's on !visible is troublesome, we should perhaps revisit that sometimes.

I don't really agree, but am willing to be convinced.  I think the problem here is really that there's this additional caching flag for whether or not this property should be synced or not.  Properties that simply always get synced have no issue and no extra complications.

[Adrienne Walker]

Comment on attachment 133374 [details]
Patch

Looks good to me.  Thanks for the test.

[WebKit Review Bot]

Comment on attachment 133374 [details]
Patch

Clearing flags on attachment: 133374

Committed r111803: <http://trac.webkit.org/changeset/111803>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.