Bug 200528 - Set WKWebView opaque based on drawsBackground in PageConfiguration
Summary: Set WKWebView opaque based on drawsBackground in PageConfiguration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-07 19:31 PDT by Timothy Hatcher
Modified: 2019-08-15 17:43 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.38 KB, patch)
2019-08-07 19:40 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2019-08-07 19:31:32 PDT
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>
Comment 1 Timothy Hatcher 2019-08-07 19:40:22 PDT
Created attachment 375781 [details]
Patch
Comment 2 WebKit Commit Bot 2019-08-08 12:04:47 PDT
Comment on attachment 375781 [details]
Patch

Clearing flags on attachment: 375781

Committed r248436: <https://trac.webkit.org/changeset/248436>
Comment 3 WebKit Commit Bot 2019-08-08 12:04:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Darin Adler 2019-08-08 12:51:24 PDT
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?
Comment 5 Timothy Hatcher 2019-08-08 12:55:35 PDT
(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.)
Comment 6 Darin Adler 2019-08-08 12:59:29 PDT
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.
Comment 7 Timothy Hatcher 2019-08-08 13:10:53 PDT
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().
Comment 8 Darin Adler 2019-08-08 16:01:51 PDT
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.
Comment 9 Timothy Hatcher 2019-08-15 17:43:23 PDT
(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.