[webkit-reviews] review granted: [Bug 211926] Add baseURL version of _WKUserStyleSheet forWKWebView : [Attachment 399419] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 211926: Add baseURL version of _WKUserStyleSheet forWKWebView
https://bugs.webkit.org/show_bug.cgi?id=211926

Attachment 399419: Patch

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




--- Comment #2 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 399419
  --> https://bugs.webkit.org/attachment.cgi?id=399419
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`?