Bug 81968

Summary: [chromium] Force update of nonFastScrollableRegion if target CCLayerImpl has been freshly created.
Product: WebKit Reporter: W. James MacLean <wjmaclean>
Component: New BugsAssignee: W. James MacLean <wjmaclean>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, enne, jamesr, nduca, rjkroege, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description W. James MacLean 2012-03-22 14:31:19 PDT
[chromium] Force update of nonFastScrollableRegion if target CCLayerImpl has been freshly created.
Comment 1 W. James MacLean 2012-03-22 14:33:04 PDT
Created attachment 133347 [details]
Patch
Comment 2 W. James MacLean 2012-03-22 14:36:44 PDT
I'll add a test if the approach in this patch is deemed correct.
Comment 3 James Robinson 2012-03-22 14:48:51 PDT
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?
Comment 4 W. James MacLean 2012-03-22 14:59:55 PDT
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.
Comment 5 W. James MacLean 2012-03-22 15:58:34 PDT
Created attachment 133374 [details]
Patch
Comment 6 Adrienne Walker 2012-03-22 16:53:53 PDT
(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.
Comment 7 Adrienne Walker 2012-03-22 17:05:09 PDT
Comment on attachment 133374 [details]
Patch

Looks good to me.  Thanks for the test.
Comment 8 WebKit Review Bot 2012-03-22 18:42:51 PDT
Comment on attachment 133374 [details]
Patch

Clearing flags on attachment: 133374

Committed r111803: <http://trac.webkit.org/changeset/111803>
Comment 9 WebKit Review Bot 2012-03-22 18:42:56 PDT
All reviewed patches have been landed.  Closing bug.