[webkit-reviews] review granted: [Bug 183418] Allow WebViews to disable system appearance : [Attachment 335387] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 183418: Allow WebViews to disable system appearance
https://bugs.webkit.org/show_bug.cgi?id=183418

Attachment 335387: Patch

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




--- Comment #43 from Tim Horton <thorton at apple.com> ---
Comment on attachment 335387
  --> https://bugs.webkit.org/attachment.cgi?id=335387
Patch

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

> Source/WebCore/page/Page.h:706
> +    bool m_useSystemAppearance {false};

Spaces inside the braces!

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.h:33
> -OBJC_CLASS NSAppearence;
> +OBJC_CLASS NSAppearance;

How did this even compile before? Maybe you don't need it?

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.mm:36
> +    

This newline is wrong.

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.mm:48
> +	   m_savedSystemAppearance = nil;

Why bother? You're in the destructor.

> Source/WebCore/rendering/RenderThemeMac.mm:500
> +    return m_systemColorCache.ensure(cssValueID, [this, cssValueID,
useSystemAppearance] () -> Color {

Probably need to (perhaps in a future patch) clear this cache when things
change

> Source/WebKitLegacy/mac/WebView/WebView.mm:5195
> +#if PLATFORM(MAC)

Do you still need these PLATFORM(MAC)s?