RESOLVED FIXED 224796
Parse `"theme_color"` in web application manifests and pass it along to `-[WKWebView themeColor]`
https://bugs.webkit.org/show_bug.cgi?id=224796
Summary Parse `"theme_color"` in web application manifests and pass it along to `-[WK...
Devin Rousso
Reported 2021-04-19 19:18:14 PDT
Attachments
Patch (61.37 KB, patch)
2021-04-19 19:31 PDT, Devin Rousso
no flags
Patch (61.77 KB, patch)
2021-04-19 20:57 PDT, Devin Rousso
no flags
Patch (61.72 KB, patch)
2021-04-20 14:26 PDT, Devin Rousso
no flags
Devin Rousso
Comment 1 2021-04-19 19:31:14 PDT
Devin Rousso
Comment 2 2021-04-19 20:57:43 PDT
Darin Adler
Comment 3 2021-04-19 21:45:08 PDT
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 4 2021-04-20 11:58:21 PDT
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 5 2021-04-20 13:03:04 PDT
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 6 2021-04-20 14:14:38 PDT
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
Comment 7 2021-04-20 14:26:13 PDT
EWS
Comment 8 2021-04-20 17:15:06 PDT
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
Comment 9 2021-04-23 01:56:02 PDT
Note You need to log in before you can comment on or make changes to this bug.