Bug 211926

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].