The forWKWebView version of _WKUserStyleSheet should allow a baseURL to be supplied so it can appear in Web Inspector. <rdar://problem/62074675>
Created attachment 399419 [details] Patch
Comment on attachment 399419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399419&action=review r=me, with some comments/concerns > Source/WebCore/dom/ExtensionStyleSheets.cpp:130 > + auto addStyleSheet = [&](const UserStyleSheet& userStyleSheet) { I'd make this into a private member function, something like `createCSSStyleSheetForUserStyleSheet`. > Source/WebCore/dom/ExtensionStyleSheets.cpp:135 > + if (userStyleSheet.level() == UserStyleUserLevel) I think it's better to check for `sheet->contents().isUserStyleSheet()` as that's going to give us the actual resolved level. > Source/WebCore/dom/ExtensionStyleSheets.cpp:-161 > - m_pageSpecificStyleSheets.append(WTFMove(sheet)); I think it would've also been valid to `m_injectedStyleSheetToSource.set(sheet.copyRef(), userStyleSheet.source());` above this (and do a corresponding `removeFirstMatching in `removePageSpecificUserStyleSheet`), but I like the unified approach too :) > Source/WebCore/dom/ExtensionStyleSheets.cpp:166 > + bool removedStyleSheet = m_pageSpecificStyleSheets.removeFirstMatching([userStyleSheet](auto& styleSheet) { You should use `&userStyleSheet` (or just `&`) so it captures by reference instead of making a copy. NIT: `const auto&` > Source/WebCore/dom/ExtensionStyleSheets.cpp:-241 > - for (auto& sheet : m_pageSpecificStyleSheets) > - sheet->detachFromDocument(); I wonder if this was calling `detachFromDocument` more than once on the same `RefPtr<CSSStyleSheet>`, because I think the same `RefPtr<CSSStyleSheet>` could've existed in `m_injectedUserStyleSheets`/`m_injectedAuthorStyleSheets` _and_ `m_pageSpecificStyleSheets` 🤔 > Source/WebCore/dom/ExtensionStyleSheets.h:95 > + Vector<UserStyleSheet> m_pageSpecificStyleSheets; This is the first time that we're holding on to the `UserStyleSheet` instead of a `RefPtr<CSSStyleSheet>` for it. Is this problematic? > Source/WebKit/UIProcess/API/Cocoa/_WKUserStyleSheet.h:52 > +- (instancetype)initWithSource:(NSString *)source forWKWebView:(WKWebView *)webView forMainFrameOnly:(BOOL)forMainFrameOnly baseURL:(NSURL *)baseURL level:(_WKUserStyleLevel)level userContentWorld:(_WKUserContentWorld *)userContentWorld WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); Why `baseURL` instead of `sourceURL`?
Comment on attachment 399419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399419&action=review >> Source/WebCore/dom/ExtensionStyleSheets.cpp:130 >> + auto addStyleSheet = [&](const UserStyleSheet& userStyleSheet) { > > I'd make this into a private member function, something like `createCSSStyleSheetForUserStyleSheet`. Eh, why? >> Source/WebCore/dom/ExtensionStyleSheets.cpp:135 >> + if (userStyleSheet.level() == UserStyleUserLevel) > > I think it's better to check for `sheet->contents().isUserStyleSheet()` as that's going to give us the actual resolved level. Okay >> Source/WebCore/dom/ExtensionStyleSheets.cpp:-161 >> - m_pageSpecificStyleSheets.append(WTFMove(sheet)); > > I think it would've also been valid to `m_injectedStyleSheetToSource.set(sheet.copyRef(), userStyleSheet.source());` above this (and do a corresponding `removeFirstMatching in `removePageSpecificUserStyleSheet`), but I like the unified approach too :) Nope, since `updateInjectedStyleSheetCache` clears the m_injectedStyleSheetToSource. >> Source/WebCore/dom/ExtensionStyleSheets.cpp:166 >> + bool removedStyleSheet = m_pageSpecificStyleSheets.removeFirstMatching([userStyleSheet](auto& styleSheet) { > > You should use `&userStyleSheet` (or just `&`) so it captures by reference instead of making a copy. > > NIT: `const auto&` Okay >> Source/WebCore/dom/ExtensionStyleSheets.cpp:-241 >> - sheet->detachFromDocument(); > > I wonder if this was calling `detachFromDocument` more than once on the same `RefPtr<CSSStyleSheet>`, because I think the same `RefPtr<CSSStyleSheet>` could've existed in `m_injectedUserStyleSheets`/`m_injectedAuthorStyleSheets` _and_ `m_pageSpecificStyleSheets` 🤔 I think so! >> Source/WebCore/dom/ExtensionStyleSheets.h:95 >> + Vector<UserStyleSheet> m_pageSpecificStyleSheets; > > This is the first time that we're holding on to the `UserStyleSheet` instead of a `RefPtr<CSSStyleSheet>` for it. Is this problematic? Should be fine. We hold UserStyleSheet in vectors elsewhere. >> Source/WebKit/UIProcess/API/Cocoa/_WKUserStyleSheet.h:52 >> +- (instancetype)initWithSource:(NSString *)source forWKWebView:(WKWebView *)webView forMainFrameOnly:(BOOL)forMainFrameOnly baseURL:(NSURL *)baseURL level:(_WKUserStyleLevel)level userContentWorld:(_WKUserContentWorld *)userContentWorld WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)); > > Why `baseURL` instead of `sourceURL`? To match the other methods on this class. I agree it isn't great.
Created attachment 399428 [details] Patch
Committed r261726: <https://trac.webkit.org/changeset/261726> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399428 [details].