[webkit-reviews] review granted: [Bug 224796] Parse `"theme_color"` in web application manifests and pass it along to `-[WKWebView themeColor]` : [Attachment 426517] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 224796: Parse `"theme_color"` in web application manifests and pass it
along to `-[WKWebView themeColor]`
https://bugs.webkit.org/show_bug.cgi?id=224796

Attachment 426517: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 426517
  --> https://bugs.webkit.org/attachment.cgi?id=426517
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.