Bug 224796 – Parse `"theme_color"` in web application manifests and pass it along to `-[WKWebView themeColor]`

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Devin Rousso]

https://w3c.github.io/manifest/#theme_color-member

[Devin Rousso]

Created attachment 426512 [details]
Patch

[Devin Rousso]

Created attachment 426517 [details]
Patch

[Darin Adler]

Comment on attachment 426517 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3857
> +    themeColorChanged();

Would be better to not call themeColorChanged() if m_metaElementThemeColor happens to be equal to the current m_applicationManifestThemeColor, but the new m_metaElementThemeColor is invalid.

An even better way to code this would be to check themeColor() before and after setting m_metaElementThemeColor to see if it changed or not. That way if it’s the same color for a different reason, we would not make a themeColorChanged() call.

> Source/WebCore/dom/Document.cpp:3987
> +    themeColorChanged();

Would be better to call themeColorChanged() only if m_metaElementThemeColor is not valid or doesn't happen to be equal to m_applicationManifestThemeColor.

An even better way to do this would be to check themeColor() before and after setting m_applicationManifestThemeColor to see if it changed or not. That way if it’s the same color for a different reason, we would not make a themeColorChanged() call.

> Source/WebCore/dom/Document.cpp:3991
> +}
> +
> +
> +#endif // ENABLE(APPLICATION_MANIFEST)

Extra blank line here.

> Source/WebCore/dom/Document.h:920
> +#endif // ENABLE(APPLICATION_MANIFEST)

This comment is not helpful; in a case like this we don’t need to comment each endif (see above). I suggest omitting it.

> Source/WebCore/loader/cache/CachedApplicationManifest.h:43
> +    Optional<struct ApplicationManifest> process(const URL& manifestURL, const URL& documentURL, RefPtr<Document> = nullptr);

Not sure I understand why that argument is a RefPtr<Document> rather than a Document*.

> Source/WebCore/page/ChromeClient.h:235
> +    virtual void pageExtendedBackgroundColorDidChange() const { }

I don’t see the change to the code calling this function.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:38
> +#endif // PLATFORM(IOS_FAMILY)

No need for a comment here.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:42
> +#endif // PLATFORM(MAC)

No need for a comment here.

[Devin Rousso]

Comment on attachment 426517 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/dom/Document.cpp:3857
>> +    themeColorChanged();
> 
> Would be better to not call themeColorChanged() if m_metaElementThemeColor happens to be equal to the current m_applicationManifestThemeColor, but the new m_metaElementThemeColor is invalid.
> 
> An even better way to code this would be to check themeColor() before and after setting m_metaElementThemeColor to see if it changed or not. That way if it’s the same color for a different reason, we would not make a themeColorChanged() call.

Ha!  I actually had this originally, but thought it would be more efficient to do what I did.  I totally forgot about the case where the color is invalidated.  Great catch!

>> Source/WebCore/loader/cache/CachedApplicationManifest.h:43
>> +    Optional<struct ApplicationManifest> process(const URL& manifestURL, const URL& documentURL, RefPtr<Document> = nullptr);
> 
> Not sure I understand why that argument is a RefPtr<Document> rather than a Document*.

I thought WebKit is making a push to use `Ref`/`RefPtr` instead of raw pointers more often (if not always) now?

>> Source/WebCore/page/ChromeClient.h:235
>> +    virtual void pageExtendedBackgroundColorDidChange() const { }
> 
> I don’t see the change to the code calling this function.

The first is called in `Document::themeColorChanged` (newly split out from the old `Document::processThemeColor`, now renamed `Document::processMetaElementThemeColor`).
The second is called in `RenderLayerCompositor::rootBackgroundColorOrTransparencyChanged`.

[Darin Adler]

Comment on attachment 426517 [details]
Patch

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

>>> Source/WebCore/loader/cache/CachedApplicationManifest.h:43
>>> +    Optional<struct ApplicationManifest> process(const URL& manifestURL, const URL& documentURL, RefPtr<Document> = nullptr);
>> 
>> Not sure I understand why that argument is a RefPtr<Document> rather than a Document*.
> 
> I thought WebKit is making a push to use `Ref`/`RefPtr` instead of raw pointers more often (if not always) now?

I believe that the answer to that is for local variables, yes. For function arguments, no. Talk to Ryosuke to find out our overall strategy and let’s make sure this is written down so we all understand it.

[Devin Rousso]

Comment on attachment 426517 [details]
Patch

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

>>>> Source/WebCore/loader/cache/CachedApplicationManifest.h:43
>>>> +    Optional<struct ApplicationManifest> process(const URL& manifestURL, const URL& documentURL, RefPtr<Document> = nullptr);
>>> 
>>> Not sure I understand why that argument is a RefPtr<Document> rather than a Document*.
>> 
>> I thought WebKit is making a push to use `Ref`/`RefPtr` instead of raw pointers more often (if not always) now?
> 
> I believe that the answer to that is for local variables, yes. For function arguments, no. Talk to Ryosuke to find out our overall strategy and let’s make sure this is written down so we all understand it.

Had a chat with @Ryosuke Niwa offline and he pointed me to <https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html>.  I'll change this to `Document*`.

[Devin Rousso]

Created attachment 426591 [details]
Patch

[EWS]

Committed r276333 (236813@main): <https://commits.webkit.org/236813@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426591 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77062361>