WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
https://w3c.github.io/manifest/#theme_color-member
Attachments
Patch
(61.37 KB, patch)
2021-04-19 19:31 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(61.77 KB, patch)
2021-04-19 20:57 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(61.72 KB, patch)
2021-04-20 14:26 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Devin Rousso
Comment 1
2021-04-19 19:31:14 PDT
Created
attachment 426512
[details]
Patch
Devin Rousso
Comment 2
2021-04-19 20:57:43 PDT
Created
attachment 426517
[details]
Patch
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
Created
attachment 426591
[details]
Patch
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
<
rdar://problem/77062361
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug