Allow WebViews to disable system appearance
Created attachment 335229 [details] Patch
<rdar://problem/36975642>
Comment on attachment 335229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335229&action=review > Source/WebCore/css/StyleColor.cpp:40 > +Color StyleColor::colorFromKeyword(CSSValueID keyword, bool useSystemAppearance) Not necessary to do in this patch but it would probably be better to make this an enum class > Source/WebCore/css/StyleResolver.cpp:1825 > + return RenderTheme::focusRingColor(document().page()->useSystemAppearance()); Can page be null? > Source/WebCore/css/StyleResolver.cpp:1832 > + return StyleColor::colorFromKeyword(identifier, document().page()->useSystemAppearance()); Can page be null? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:87 > + context->drawFocusRing(path, 1, 1, RenderTheme::focusRingColor(element.document().page()->useSystemAppearance())); Can Page be null? > Source/WebCore/platform/mac/ThemeMac.mm:76 > + WebCore::LocalDefaultSystemAppearance localAppearence(useSystemAppearance); This local is spelled wrong. There might be others, maybe grep for "appearence" > Source/WebCore/rendering/RenderTheme.h:153 > + virtual Color platformFocusRingColor(bool useSystemAppearance) const { UNUSED_PARAM(useSystemAppearance); return Color(0, 0, 0); } You can just leave the name off the arg instead, then you don't have to UNUSED_PARAM
Comment on attachment 335229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335229&action=review >> Source/WebCore/css/StyleColor.cpp:40 >> +Color StyleColor::colorFromKeyword(CSSValueID keyword, bool useSystemAppearance) > > Not necessary to do in this patch but it would probably be better to make this an enum class Something like: enum class UseSystemAppearance { Yes, No } and then you'd use it like UseSystemAppearance::Yes.
Created attachment 335244 [details] Patch
Comment on attachment 335244 [details] Patch Attachment 335244 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6847672 Number of test failures exceeded the failure limit.
Created attachment 335246 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 335244 [details] Patch Attachment 335244 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6847809 Number of test failures exceeded the failure limit.
Created attachment 335249 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 335244 [details] Patch Attachment 335244 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6847894 Number of test failures exceeded the failure limit.
Created attachment 335250 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 335251 [details] Patch
Comment on attachment 335251 [details] Patch Attachment 335251 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6848895 Number of test failures exceeded the failure limit.
Created attachment 335256 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 335251 [details] Patch Attachment 335251 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6848887 Number of test failures exceeded the failure limit.
Created attachment 335257 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 335251 [details] Patch Attachment 335251 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6848970 Number of test failures exceeded the failure limit.
Created attachment 335258 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 335251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335251&action=review > Source/WebCore/html/canvas/CanvasStyle.cpp:-74 > - return parseColor(colorString); It’s pretty clear from the crashes that canvas can be null. > Source/WebCore/html/canvas/CanvasStyle.cpp:74 > + CSSParserContext context = CSSParserContext(canvas->document()); Not sure it’s a good idea to make a CSSParserContext here that’s missing most of its context -- we might enable features that we don’t want to, etc., because we don’t create it correctly. Right? Or is this all we need to fill one in?
Created attachment 335263 [details] Patch
Comment on attachment 335263 [details] Patch Attachment 335263 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6850889 Number of test failures exceeded the failure limit.
Created attachment 335265 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 335263 [details] Patch Attachment 335263 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6851132 Number of test failures exceeded the failure limit.
Created attachment 335269 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 335263 [details] Patch Attachment 335263 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6851095 Number of test failures exceeded the failure limit.
Created attachment 335270 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 335311 [details] Patch
Created attachment 335315 [details] Patch
Created attachment 335317 [details] Patch
Created attachment 335322 [details] Patch
Created attachment 335325 [details] Patch
Created attachment 335331 [details] Patch
Created attachment 335339 [details] Patch
Comment on attachment 335339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335339&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:89 > + if (element.document().page()) You could probably make this all a lot neater if you put a helper on document that did the null checking for you. Then you'd not need to sprinkle it throughout.
Created attachment 335365 [details] Patch
Comment on attachment 335365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335365&action=review > Source/WebCore/page/Page.h:328 > +#if PLATFORM(MAC) If you do the same treatment here as you do in Document, you can avoid more #ifs elsewhere. > Source/WebCore/platform/mac/LocalDefaultSystemAppearance.h:33 > -OBJC_CLASS NSAppearence; > +@class NSAppearance; OBJC_CLASS is generally preferred because it works in C++-only files too! There's no reason this header shouldn't be importable in C++ files. > Source/WebKit/ChangeLog:310 > +>>>>>>> .r229431 Please clean this up
Comment on attachment 335365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335365&action=review > Source/WebCore/html/canvas/CanvasStyle.cpp:118 > +CanvasStyle CanvasStyle::createFromString(const String& colorString, HTMLCanvasElement* canvas) There isn't anything you can do about this, but an OffscreenCanvas won't have a document, and thus won't be able to follow the system appearance. Although, maybe the system appearance query could be somewhere other than the document/page? Could it be on RenderTheme? > Source/WebCore/platform/mac/LocalDefaultSystemAppearance.h:33 > -OBJC_CLASS NSAppearence; > +@class NSAppearance; Why change from OBJC_CLASS? > Source/WebCore/platform/mac/LocalDefaultSystemAppearance.h:46 > RetainPtr<NSAppearance> m_savedSystemAppearance; > + bool m_didSetSystemAppearance; Rather than have the boolean, why not just check the nullness of m_savedSystemAppearance? > Source/WebCore/platform/mac/LocalDefaultSystemAppearance.mm:50 > + if (m_didSetSystemAppearance) > + [NSAppearance setCurrentAppearance:m_savedSystemAppearance.get()]; So this would become if (m_savedSystemAppearance) [NSApp....
Comment on attachment 335365 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335365&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:944 > + setStrokeStyle(CanvasStyle::createFromStringWithOverrideAlpha(color, alpha.value(), &downcast<HTMLCanvasElement>(canvasBase()))); Dino says to leave it for now, but in the future: "hand in the canvasBase() itself, and then on canvasBase() add a virtual useSystemApperance() that HTMLCanvasElement implements and grabs from Page"
Created attachment 335370 [details] Patch
Created attachment 335379 [details] Patch
Created attachment 335387 [details] Patch
Comment on attachment 335387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335387&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:6961 > + if (useSystemAppearance == m_alwaysShowsHorizontalScroller) I think you meant to compare against m_useSystemAppearance, not m_alwaysShowsHorizontalScroller
Comment on attachment 335387 [details] 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?
Created attachment 335389 [details] Patch
https://trac.webkit.org/changeset/229448/webkit
Comment on attachment 335389 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335389&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:442 > +@property (nonatomic, readwrite, setter=_setUseSystemAppearance:) BOOL _useSystemAppearance WK_API_AVAILABLE(macosx(WK_MAC_TBA)); Don't we need corresponding C API in WKPage.cpp?
(In reply to Alex Christensen from comment #46) > Comment on attachment 335389 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335389&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:442 > > +@property (nonatomic, readwrite, setter=_setUseSystemAppearance:) BOOL _useSystemAppearance WK_API_AVAILABLE(macosx(WK_MAC_TBA)); > > Don't we need corresponding C API in WKPage.cpp? Hopefully not.