Bug 200528 – Set WKWebView opaque based on drawsBackground in PageConfiguration

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Timothy Hatcher]

Setting opaque early puts the transparent backgroundColor in the WebPageCreationParameters and avoids a page message later, and some extra transparency/layout thrash.

<rdar://problem/53859380>

[Timothy Hatcher]

Created attachment 375781 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375781 [details]
Patch

Clearing flags on attachment: 375781

Committed r248436: <https://trac.webkit.org/changeset/248436>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 375781 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715
> +    if (!self.opaque || !pageConfiguration->drawsBackground())
> +        self.opaque = NO;

Is checking the existing value of self.opaque an important optimization?

[Timothy Hatcher]

(In reply to Darin Adler from comment #4)
> Comment on attachment 375781 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375781&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715
> > +    if (!self.opaque || !pageConfiguration->drawsBackground())
> > +        self.opaque = NO;
> 
> Is checking the existing value of self.opaque an important optimization?

This ensures a subclass overriding opaque will get NO propagated down to our setter which does internal work that might get skipped otherwise. (It also preserves the old logic.)

[Darin Adler]

Comment on attachment 375781 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715
>>> +        self.opaque = NO;
>> 
>> Is checking the existing value of self.opaque an important optimization?
> 
> This ensures a subclass overriding opaque will get NO propagated down to our setter which does internal work that might get skipped otherwise. (It also preserves the old logic.)

I don’t understand. This code skips calling the setter in the case where the getter is overridden to return NO. So how does that ensure that NO will get propagated to our setter? Sounds backwards.

[Timothy Hatcher]

Comment on attachment 375781 [details]
Patch

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

>>>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715
>>>> +        self.opaque = NO;
>>> 
>>> Is checking the existing value of self.opaque an important optimization?
>> 
>> This ensures a subclass overriding opaque will get NO propagated down to our setter which does internal work that might get skipped otherwise. (It also preserves the old logic.)
> 
> I don’t understand. This code skips calling the setter in the case where the getter is overridden to return NO. So how does that ensure that NO will get propagated to our setter? Sounds backwards.

The check is !self.opaque || !pageConfiguration->drawsBackground().

[Darin Adler]

Comment on attachment 375781 [details]
Patch

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

>>>>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715
>>>>> +        self.opaque = NO;
>>>> 
>>>> Is checking the existing value of self.opaque an important optimization?
>>> 
>>> This ensures a subclass overriding opaque will get NO propagated down to our setter which does internal work that might get skipped otherwise. (It also preserves the old logic.)
>> 
>> I don’t understand. This code skips calling the setter in the case where the getter is overridden to return NO. So how does that ensure that NO will get propagated to our setter? Sounds backwards.
> 
> The check is !self.opaque || !pageConfiguration->drawsBackground().

Got it! Thanks. I was the one who had it backwards. Now makes total sense to me. I think we could follow up and make this better in three ways:

1a) Adding a comment could be helpful since the setter here is "impure" and someone might not understand that later. Maybe making the same mistake I made.

    // There is valuable code in the setter for "opaque" that we want to call explicitly if opaque is overridden to return NO.

1b) Move the valuable code in the setter for opaque into a different method, and call that method unconditionally instead of relying on the side effect of the setter.

2) Add a test case that overrides opaque to return NO to TestWebKitAPI to catch when this is broken by a future well-meaning programmers.

[Timothy Hatcher]

(In reply to Darin Adler from comment #8)
> Comment on attachment 375781 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375781&action=review
> 
> >>>>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:715
> >>>>> +        self.opaque = NO;
> >>>> 
> >>>> Is checking the existing value of self.opaque an important optimization?
> >>> 
> >>> This ensures a subclass overriding opaque will get NO propagated down to our setter which does internal work that might get skipped otherwise. (It also preserves the old logic.)
> >> 
> >> I don’t understand. This code skips calling the setter in the case where the getter is overridden to return NO. So how does that ensure that NO will get propagated to our setter? Sounds backwards.
> > 
> > The check is !self.opaque || !pageConfiguration->drawsBackground().
> 
> Got it! Thanks. I was the one who had it backwards. Now makes total sense to
> me. I think we could follow up and make this better in three ways:
> 
> 1a) Adding a comment could be helpful since the setter here is "impure" and
> someone might not understand that later. Maybe making the same mistake I
> made.
> 
>     // There is valuable code in the setter for "opaque" that we want to
> call explicitly if opaque is overridden to return NO.
> 
> 1b) Move the valuable code in the setter for opaque into a different method,
> and call that method unconditionally instead of relying on the side effect
> of the setter.
> 
> 2) Add a test case that overrides opaque to return NO to TestWebKitAPI to
> catch when this is broken by a future well-meaning programmers.

Addressed in bug 200802.