Bug 188028

Summary: ERROR: Unhandled web process message 'WebPage:SetUseDarkAppearance' when browsing in dark mode
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bdakin, commit-queue, ggaren, thorton, timothy, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2018-07-25 16:30:26 PDT
ERROR: Unhandled web process message 'WebPage:SetUseDarkAppearance' when browsing in dark mode.
Comment 1 Chris Dumez 2018-07-25 16:34:05 PDT
Created attachment 345800 [details]
Patch
Comment 2 Wenson Hsieh 2018-07-25 17:06:15 PDT
Comment on attachment 345800 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1316
> +    m_page->setUseDarkAppearance(effectiveAppearanceIsDark());

Doesn't this mean that the WebPageCreationParameters sent over when initializing the web process won't have the right value for `useDarkAppearance`?
Comment 3 Chris Dumez 2018-07-25 17:10:13 PDT
Comment on attachment 345800 [details]
Patch

Will check.
Comment 4 Alex Christensen 2018-07-25 17:10:57 PDT
Comment on attachment 345800 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1316
>> +    m_page->setUseDarkAppearance(effectiveAppearanceIsDark());
> 
> Doesn't this mean that the WebPageCreationParameters sent over when initializing the web process won't have the right value for `useDarkAppearance`?

We shouldn't even need to set this if it's in the WebPageCreationParameters
Comment 5 Wenson Hsieh 2018-07-25 17:24:46 PDT
Comment on attachment 345800 [details]
Patch

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

>>> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:1316
>>> +    m_page->setUseDarkAppearance(effectiveAppearanceIsDark());
>> 
>> Doesn't this mean that the WebPageCreationParameters sent over when initializing the web process won't have the right value for `useDarkAppearance`?
> 
> We shouldn't even need to set this if it's in the WebPageCreationParameters

Hm...I'm not sure I understand this comment. In WebPageProxy::creationParameters(), we have:

    parameters.useDarkAppearance = m_useDarkAppearance;

...and m_useDarkAppearance may not be the right value then if we haven't called WebPageProxy::setUseDarkAppearance yet. It seems that either:

- we should set m_useDarkAppearance to the right value but avoid sending IPC to WebPage before sending WebPageCreationParameters, or...
- maybe it doesn't matter that the useDarkAppearance bit is the right value during web process creation, since we'll just end up setting it in the next IPC message anyways and this tweak here is okay.
Comment 6 Chris Dumez 2018-07-25 18:00:20 PDT
I did not realize it was part of WebPageCreationParameters. In this case, I will avoid sending the IPC if this is called before initialization. If it is called later ton (e.g. preference changes while Safari is running), then we still want to send the IPC.
Comment 7 Chris Dumez 2018-07-25 18:31:19 PDT
(In reply to Chris Dumez from comment #6)
> I did not realize it was part of WebPageCreationParameters. In this case, I
> will avoid sending the IPC if this is called before initialization. If it is
> called later ton (e.g. preference changes while Safari is running), then we
> still want to send the IPC.

Hmm, looks like there is currently no flag telling if initializeWebPage() was called yet or not.
Comment 8 Chris Dumez 2018-07-26 11:51:42 PDT
Created attachment 345857 [details]
Patch
Comment 9 WebKit Commit Bot 2018-07-26 12:49:16 PDT
Comment on attachment 345857 [details]
Patch

Clearing flags on attachment: 345857

Committed r234268: <https://trac.webkit.org/changeset/234268>
Comment 10 WebKit Commit Bot 2018-07-26 12:49:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-07-26 12:50:26 PDT
<rdar://problem/42634537>