Bug 150531 - Add a way to add user style sheets to WKUserContentController
Summary: Add a way to add user style sheets to WKUserContentController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-24 09:48 PDT by Tim Horton
Modified: 2015-10-26 10:49 PDT (History)
4 users (show)

See Also:


Attachments
Patch (36.62 KB, patch)
2015-10-24 09:50 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (35.36 KB, patch)
2015-10-24 10:05 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (36.49 KB, patch)
2015-10-24 21:53 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
try to fix iOS (37.76 KB, patch)
2015-10-24 22:47 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2015-10-24 09:48:21 PDT
Add a way to add user style sheets to WKUserContentController
Comment 1 Tim Horton 2015-10-24 09:50:36 PDT
Created attachment 263980 [details]
Patch
Comment 2 WebKit Commit Bot 2015-10-24 09:52:30 PDT
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.
Comment 3 Tim Horton 2015-10-24 10:05:01 PDT
Created attachment 263981 [details]
Patch
Comment 4 mitz 2015-10-24 10:05:54 PDT
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.
Comment 5 WebKit Commit Bot 2015-10-24 10:07:57 PDT
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.
Comment 6 Darin Adler 2015-10-24 15:40:11 PDT
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?
Comment 7 Tim Horton 2015-10-24 18:21:09 PDT
(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.
Comment 8 mitz 2015-10-24 18:27:43 PDT
(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.
Comment 9 Tim Horton 2015-10-24 21:19:31 PDT
> > 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!
Comment 10 Tim Horton 2015-10-24 21:53:08 PDT
Created attachment 264001 [details]
Patch
Comment 11 WebKit Commit Bot 2015-10-24 21:56:06 PDT
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.
Comment 12 Tim Horton 2015-10-24 22:47:01 PDT
Created attachment 264003 [details]
try to fix iOS
Comment 13 WebKit Commit Bot 2015-10-24 22:49:27 PDT
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.
Comment 14 Tim Horton 2015-10-24 23:00:25 PDT
http://trac.webkit.org/changeset/191545
Comment 15 Anders Carlsson 2015-10-26 10:00:52 PDT
Do we really have to expose the blacklist and whitelist? We don't expose them on WKUserScript.
Comment 16 Tim Horton 2015-10-26 10:34:50 PDT
(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?
Comment 17 Tim Horton 2015-10-26 10:35:09 PDT
(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?
Comment 18 Anders Carlsson 2015-10-26 10:49:28 PDT
(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.