Bug 150531 – Add a way to add user style sheets to WKUserContentController

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Add a way to add user style sheets to WKUserContentController

[Tim Horton]

Created attachment 263980 [details]
Patch

[WebKit Commit Bot]

Attachment 263980 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:241:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:261:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:272:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:299:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:313:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:327:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:354:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:364:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:391:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:401:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 11 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 263981 [details]
Patch

Comment on attachment 263980 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.h:40
> +@property (nonatomic, readonly) NSArray *whitelistedURLPatterns;
> +@property (nonatomic, readonly) NSArray *blacklistedURLPatterns;

These should be WK_ARRAY(NSString *) and copy even though they’re readonly.

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.h:44
> +- (instancetype)initWithSource:(NSString *)source whitelistedURLPatterns:(WK_NULLABLE NSArray *)whitelistedURLPatterns blacklistedURLPatterns:(WK_NULLABLE NSArray *)blacklistedURLPatterns forMainFrameOnly:(BOOL)forMainFrameOnly;

As should be the arguments here. Since there’s no difference between passing nil and passing an empty array, I suggest that we don’t make them optional.

[WebKit Commit Bot]

Attachment 263981 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:228:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 263981 [details]
Patch

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

> Source/WebKit2/UIProcess/API/APIUserStyleSheet.cpp:45
> +    return WebCore::URL { WebCore::URL { }, urlStringBuilder.toString() };

I think you can and should omit one or both of those WebCore::URL.

> Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:40
> +        return adoptRef(*new UserStyleSheet(userStyleSheet));

Anything to be gained by doing WTF::move here?

> Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:43
> +    UserStyleSheet(WebCore::UserStyleSheet userStyleSheet)

Should this be private?

> Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:44
> +        : m_userStyleSheet(userStyleSheet)

Anything to be gained by doing WTF::move here?

> Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:50
> +    ~UserStyleSheet()
> +    {
> +    }

I suggest omitting this since it’s what the compiler would generate if you didn’t say it explicitly.

> Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:52
> +    WebCore::UserStyleSheet& userStyleSheet() { return m_userStyleSheet; }

Do we need to support a non-const reference? I guess maybe that answer is an emphatic yes. But I’m surprised that WebCore::UserStyleSheet is apparently so small that we don’t need to move it or pass it by reference (hence likely itself a reference of some kind), yet is itself mutable and so a mutable reference to it needs to be part of the API here. Seems fishy.

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentController.mm:152
> +    _userContentControllerProxy->addUserStyleSheet(userStyleSheet->_userStyleSheet->userStyleSheet());

Something’s wrong with our naming when we make classes that are wrappers if we literally end up writing code saying “user style sheet’s user style sheet’s user style sheet”.

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.mm:43
> +static Vector<String> toWTFStrings(NSArray *strings)
> +{
> +    Vector<String> result;
> +
> +    for (NSString *string in strings)
> +        result.append(string);
> +
> +    return result;
> +}

This has to be a pattern that is repeated elsewhere. Would like to share the code. Should use reserveInitialCapacity and uncheckedAppend. Maybe some day we could use the modern syntax that tells it’s an NSArray of NSString, not an arbitrary NSArray.

> Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.mm:53
> +static RetainPtr<NSArray> toNSStrings(Vector<String> strings)
> +{
> +    RetainPtr<NSMutableArray> result = adoptNS([[NSMutableArray alloc] initWithCapacity:strings.size()]);
> +
> +    for (const auto& string : strings)
> +        [result addObject:string];
> +
> +    return result;
> +}

This has to be a pattern that is repeated elsewhere. Would like to share the code. Should probably take a const Vector<String>& rather than copying the vector.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:229
> +        EXPECT_WK_STREQ(color, value);

iOS link fails with a reference from this line of code because TestWebKitAPI::Util::toSTD(char const*) is not found, presumably not exported. Some build configuration thing?

[Tim Horton]

(In reply to comment #4)
> Comment on attachment 263980 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263980&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.h:40
> > +@property (nonatomic, readonly) NSArray *whitelistedURLPatterns;
> > +@property (nonatomic, readonly) NSArray *blacklistedURLPatterns;
> 
> These should be WK_ARRAY(NSString *) and copy even though they’re readonly.

Generics I'm cool with, but why "copy" with readonly? 

> > Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.h:44
> > +- (instancetype)initWithSource:(NSString *)source whitelistedURLPatterns:(WK_NULLABLE NSArray *)whitelistedURLPatterns blacklistedURLPatterns:(WK_NULLABLE NSArray *)blacklistedURLPatterns forMainFrameOnly:(BOOL)forMainFrameOnly;
> 
> As should be the arguments here. Since there’s no difference between passing
> nil and passing an empty array, I suggest that we don’t make them optional.

Ok! I was marching the C API but sure, that makes sense!

(In reply to comment #6)
> Comment on attachment 263981 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263981&action=review
> 
> > Source/WebKit2/UIProcess/API/APIUserStyleSheet.cpp:45
> > +    return WebCore::URL { WebCore::URL { }, urlStringBuilder.toString() };
> 
> I think you can and should omit one or both of those WebCore::URL.

Ok! This and many of the below comments also apply to APIUserScript, since that's where most of this code came from. Will consider each and apply to both places as necessary.

> > Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:40
> > +        return adoptRef(*new UserStyleSheet(userStyleSheet));
> 
> Anything to be gained by doing WTF::move here?
> 
> > Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:43
> > +    UserStyleSheet(WebCore::UserStyleSheet userStyleSheet)
> 
> Should this be private?
> 
> > Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:44
> > +        : m_userStyleSheet(userStyleSheet)
> 
> Anything to be gained by doing WTF::move here?
> 
> > Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:50
> > +    ~UserStyleSheet()
> > +    {
> > +    }
> 
> I suggest omitting this since it’s what the compiler would generate if you
> didn’t say it explicitly.
> 
> > Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:52
> > +    WebCore::UserStyleSheet& userStyleSheet() { return m_userStyleSheet; }
> 
> Do we need to support a non-const reference? I guess maybe that answer is an
> emphatic yes. But I’m surprised that WebCore::UserStyleSheet is apparently
> so small that we don’t need to move it or pass it by reference (hence likely
> itself a reference of some kind), yet is itself mutable and so a mutable
> reference to it needs to be part of the API here. Seems fishy.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKUserContentController.mm:152
> > +    _userContentControllerProxy->addUserStyleSheet(userStyleSheet->_userStyleSheet->userStyleSheet());
> 
> Something’s wrong with our naming when we make classes that are wrappers if
> we literally end up writing code saying “user style sheet’s user style
> sheet’s user style sheet”.

Agreed. So many wrappers! (_WKUserStyleSheet->API::UserStyleSheet->WebCore::UserStyleSheet). Will discuss with Dan and Anders.

> > Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.mm:43
> > +static Vector<String> toWTFStrings(NSArray *strings)
> > +{
> > +    Vector<String> result;
> > +
> > +    for (NSString *string in strings)
> > +        result.append(string);
> > +
> > +    return result;
> > +}
> 
> This has to be a pattern that is repeated elsewhere. Would like to share the
> code. Should use reserveInitialCapacity and uncheckedAppend. Maybe some day
> we could use the modern syntax that tells it’s an NSArray of NSString, not
> an arbitrary NSArray.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.mm:53
> > +static RetainPtr<NSArray> toNSStrings(Vector<String> strings)
> > +{
> > +    RetainPtr<NSMutableArray> result = adoptNS([[NSMutableArray alloc] initWithCapacity:strings.size()]);
> > +
> > +    for (const auto& string : strings)
> > +        [result addObject:string];
> > +
> > +    return result;
> > +}
> 
> This has to be a pattern that is repeated elsewhere. Would like to share the
> code. Should probably take a const Vector<String>& rather than copying the
> vector.

Definitely should :) and, I'll try to find a home for these.

> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:229
> > +        EXPECT_WK_STREQ(color, value);
> 
> iOS link fails with a reference from this line of code because
> TestWebKitAPI::Util::toSTD(char const*) is not found, presumably not
> exported. Some build configuration thing?

That's a bit weird.

(In reply to comment #7)
> (In reply to comment #4)
> > Comment on attachment 263980 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=263980&action=review
> > 
> > > Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.h:40
> > > +@property (nonatomic, readonly) NSArray *whitelistedURLPatterns;
> > > +@property (nonatomic, readonly) NSArray *blacklistedURLPatterns;
> > 
> > These should be WK_ARRAY(NSString *) and copy even though they’re readonly.
> 
> Generics I'm cool with, but why "copy" with readonly? 

Other properties in the modern API follow this pattern. The practical reason is that it allows an internal redeclaration of the property as readwrite to specify copy as well. The API reason is that it communicates to the client that the returned object is not going to change even if it happens to be of a mutable subclass.

[Tim Horton]

> > Source/WebKit2/UIProcess/API/APIUserStyleSheet.h:52
> > +    WebCore::UserStyleSheet& userStyleSheet() { return m_userStyleSheet; }
> 
> Do we need to support a non-const reference? I guess maybe that answer is an
> emphatic yes. But I’m surprised that WebCore::UserStyleSheet is apparently
> so small that we don’t need to move it or pass it by reference (hence likely
> itself a reference of some kind), yet is itself mutable and so a mutable
> reference to it needs to be part of the API here. Seems fishy.

I don't actually think we do; they're totally immutable as far as I can tell. Will remove it and see if anything falls apart. Good catch!

[Tim Horton]

Created attachment 264001 [details]
Patch

[WebKit Commit Bot]

Attachment 264001 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:228:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Created attachment 264003 [details]
try to fix iOS

[WebKit Commit Bot]

Attachment 264003 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:228:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKUserStyleSheet.mm:28:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

http://trac.webkit.org/changeset/191545

[Anders Carlsson]

Do we really have to expose the blacklist and whitelist? We don't expose them on WKUserScript.

[Tim Horton]

(In reply to comment #15)
> Do we really have to expose the blacklist and whitelist? We don't expose
> them on WKUserScript.

Errr, interesting. What do you think?

[Tim Horton]

(In reply to comment #16)
> (In reply to comment #15)
> > Do we really have to expose the blacklist and whitelist? We don't expose
> > them on WKUserScript.
> 
> Errr, interesting. What do you think?

Should I just pull those parameters out?

[Anders Carlsson]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Do we really have to expose the blacklist and whitelist? We don't expose
> > > them on WKUserScript.
> > 
> > Errr, interesting. What do you think?
> 
> Should I just pull those parameters out?

Yes.