Bug 99081 – [GTK] Add methods to add a user stylesheet to the WebKit2 GTK+ API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

Add webkit_settings_get_user_style_sheet_uri() and webkit_settings_set_user_style_sheet_uri()

[Carlos Garcia Campos]

Created attachment 168246 [details]
Patch

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1077:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1079:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 11 files


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

[Build Bot]

Comment on attachment 168246 [details]
Patch

Attachment 168246 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14245979

[Martin Robinson]

Comment on attachment 168246 [details]
Patch

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

Seems reasonable. It'd be nice to get the Apple guys to take a quick look at the new platform-independent WebKit2 parts.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1082
> +    g_object_class_install_property(gObjectClass,
> +                                    PROP_USER_STYLE_SHEET_URI,
> +                                    g_param_spec_string("user-style-sheet-uri",
> +                                                        _("User Style Sheet URI"),
> +                                                        _("The URI of a style sheet that is applied to every page."),
> +                                                        0,
> +                                                        readWriteConstructParamFlags));

This can be written as:

g_object_class_install_property(gObjectClass,PROP_USER_STYLE_SHEET_URI, g_param_spec_string(
        "user-style-sheet-uri",
         _("User Style Sheet URI"),
         _("The URI of a style sheet that is applied to every page."),
         0,
         readWriteConstructParamFlags));

and it's still fairly readable.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2755
> +    if (!g_strcmp0(priv->userStyleSheetURI.data(), uri))

Seems that you can just do:

if (priv->userStyleSheetURI.data() == uri)

[Carlos Garcia Campos]

Created attachment 168249 [details]
Try to fix mac build

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1077:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1079:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 12 files


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

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 168246 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168246&action=review
> 
> Seems reasonable. It'd be nice to get the Apple guys to take a quick look at the new platform-independent WebKit2 parts.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1082
> > +    g_object_class_install_property(gObjectClass,
> > +                                    PROP_USER_STYLE_SHEET_URI,
> > +                                    g_param_spec_string("user-style-sheet-uri",
> > +                                                        _("User Style Sheet URI"),
> > +                                                        _("The URI of a style sheet that is applied to every page."),
> > +                                                        0,
> > +                                                        readWriteConstructParamFlags));
> 
> This can be written as:
> 
> g_object_class_install_property(gObjectClass,PROP_USER_STYLE_SHEET_URI, g_param_spec_string(
>         "user-style-sheet-uri",
>          _("User Style Sheet URI"),
>          _("The URI of a style sheet that is applied to every page."),
>          0,
>          readWriteConstructParamFlags));
> 
> and it's still fairly readable.

And very inconsistent with the rest of the code in this file and all other files. We should add an exception to the style checker at least for GObject signal and properties declarations. 

> > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2755
> > +    if (!g_strcmp0(priv->userStyleSheetURI.data(), uri))
> 
> Seems that you can just do:
> 
> if (priv->userStyleSheetURI.data() == uri)

We are using this in all other properties, but I don't remember why :-P

[Martin Robinson]

(In reply to comment #8)

> And very inconsistent with the rest of the code in this file and all other files. We should add an exception to the style checker at least for GObject signal and properties declarations. 

It seems silly to make an exception for one function just because it takes  along argument string. I'd rather that the rule was changed for everything.

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > And very inconsistent with the rest of the code in this file and all other files. We should add an exception to the style checker at least for GObject signal and properties declarations. 
> 
> It seems silly to make an exception for one function just because it takes  along argument string. I'd rather that the rule was changed for everything.

I think consistency in coding style is important, I wouldn't mind using the new style rule in new files, but I would rather keep the existing style in existing files instead of mixing coding styles.

[Martin Robinson]

(In reply to comment #10)

> I think consistency in coding style is important, I wouldn't mind using the new style rule in new files, but I would rather keep the existing style in existing files instead of mixing coding styles.

What we do in WebKit typically is that new code follows new style rules, so it's similar. Perhaps we can figure out a way to make it nicer to declare properties. We could potentially add some helper functions to make the declaration easier to read.

The other thing we could do is try to get the rule changed. I'm not sure they realized how much code followed this style when they made the initial change.

[Build Bot]

Comment on attachment 168249 [details]
Try to fix mac build

Attachment 168249 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14253700

[Carlos Garcia Campos]

Created attachment 168435 [details]
Try to fix win build

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitSettings.h"
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1077:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1079:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 13 files


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

[Xan Lopez]

Comment on attachment 168435 [details]
Try to fix win build

This all looks pretty great to me. I'm fine landing it as it is wrt the style issue, but we should fix it or figure out a way of making it shut up in the future.

[Anders Carlsson]

(In reply to comment #15)
> (From update of attachment 168435 [details])
> This all looks pretty great to me. I'm fine landing it as it is wrt the style issue, but we should fix it or figure out a way of making it shut up in the future.

This code touches WebKit2 code and needs to be reviewed by a WebKit2 owner. Please don't r+ WebKit2 patches in the future.

[Xan Lopez]

(In reply to comment #16)
> This code touches WebKit2 code and needs to be reviewed by a WebKit2 owner. Please don't r+ WebKit2 patches in the future.

Ah, yes, forgot about that.

Either way change the r+ for "The GTK+ bits look OK to me". A quick assessment of whether the core changes would be acceptable or not would be appreciated.

[Martin Robinson]

(In reply to comment #5)

> This code touches WebKit2 code and needs to be reviewed by a WebKit2 owner. Please don't r+ WebKit2 patches in the future.

I think we first asked a review from an Apple team member exactly 4 months ago today. Do you mind review it?

[Martin Robinson]

(In reply to comment #18)

> I think we first asked a review from an Apple team member exactly 4 months ago today. Do you mind review it?

Sorry, do you mind _reviewing_ it. :)

[Sam Weinig]

Comment on attachment 168435 [details]
Try to fix win build

We already support setting user style sheets via the WKPageGroupAddUserStyleSheet().  I don't see a reason to add another way in WebPreferencesStore.

[Martin Robinson]

Created attachment 193413 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 193413 [details]
Patch

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

Thanks! The patch looks good to me in general, I have a just a few questions, though.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.cpp:243
> + * webkit_web_view_group_add_user_stylesheet:

I've noticed that stylesheet is used as StyleSheet when camelized in many places, should we use style_sheet? I don't think it's really important, just wondering, I'm fine with stylesheet.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.cpp:258
> +void webkit_web_view_group_add_user_stylesheet(WebKitWebViewGroup* group, const char* source, const char* baseURI, GList* whitelist, GList* blacklist, WebKitInjectedContentFrames injectedFrames)

Can style sheets contain null characters? In such case a char * wouldn't be appropriate for this API. We could use a GString or even a GInputStream if that is the case. 
Since whitelist and blacklist are immutable and always contain char *, maybe it would be easier to use const GStrv (null-terminated array of strings) using const char * const *, and it would also be consistent with other APIs like webkit_file_chooser_request_select_files, webkit_web_context_set_preferred_languages, etc.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.h:56
> + * @WEBKIT_INJECTED_CONTENT_FRAMES_ALL: Content will be injected all frames.

*into* all frames?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:103
> +    CString uri = kServer->getURIForPath(path);

This is unused.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:123
> +        // We must use a wildcard for the host here until http://wkbug.com/112476 is fixed.

Should this be FIXME:?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:164
> +    g_assert(!stylesheetInjectedForPath(test, randomPath));

Here you could check also that for any other random path the stylesheet is applied.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:179
> +    // It's important to clean up the environment before other tests.
> +    prepareForNewStylesheetTest(group, &whitelist, &blacklist);

Maybe this should be renamed to something like clearUserStyleSheetData() or something similar, it's a bit weird that is called prepare for test and you don't run a test after it.

[Carlos Garcia Campos]

(In reply to comment #22)
> Can style sheets contain null characters? In such case a char * wouldn't be appropriate for this API. We could use a GString or even a GInputStream if that is the case. 
> Since whitelist and blacklist are immutable and always contain char *, maybe it would be easier to use const GStrv (null-terminated array of strings) using const char * const *, and it would also be consistent with other APIs like webkit_file_chooser_request_select_files, webkit_web_context_set_preferred_languages, etc.

Btw, another advantage of using GStrv is that it allows the user to pass a stack allocated array of static strings, without having to convert it into a GList.

[Martin Robinson]

Created attachment 193445 [details]
Patch

[Martin Robinson]

(In reply to comment #22)
> (From update of attachment 193413 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193413&action=review
> 
> Thanks! The patch looks good to me in general, I have a just a few questions, though.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.cpp:243
> > + * webkit_web_view_group_add_user_stylesheet:
> 
> I've noticed that stylesheet is used as StyleSheet when camelized in many places, should we use style_sheet? I don't think it's really important, just wondering, I'm fine with stylesheet.

Sure. Wikipedia seems to use both, but "style sheet" is more common.


> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.cpp:258
> > +void webkit_web_view_group_add_user_stylesheet(WebKitWebViewGroup* group, const char* source, const char* baseURI, GList* whitelist, GList* blacklist, WebKitInjectedContentFrames injectedFrames)
> 
> Can style sheets contain null characters? In such case a char * wouldn't be appropriate for this API. We could use a GString or even a GInputStream if that is the case. 

I'm not entirely certain if style sheets can contain null characters, but the CSS spec allows for using Unicode character escapes, so clients can use them if they really need to include null characters. The WebCore and WebKit2 API all use strings, so char* is very natural.

> Since whitelist and blacklist are immutable and always contain char *, maybe it would be easier to use const GStrv (null-terminated array of strings) using const char * const *, and it would also be consistent with other APIs like webkit_file_chooser_request_select_files, webkit_web_context_set_preferred_languages, etc.

Sure. I was debating using GStrv, but GList* seemed more common in our API. I've switched to GStrv. I prefer it as well.
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.h:56
> > + * @WEBKIT_INJECTED_CONTENT_FRAMES_ALL: Content will be injected all frames.
> 
> *into* all frames?

Fixed. :)
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:103
> > +    CString uri = kServer->getURIForPath(path);
> 
> This is unused.

And now it is gone. It was left over from working around the URL pattern matching brokenness.
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:123
> > +        // We must use a wildcard for the host here until http://wkbug.com/112476 is fixed.
> 
> Should this be FIXME:?

Sure.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:164
> > +    g_assert(!stylesheetInjectedForPath(test, randomPath));
> 
> Here you could check also that for any other random path the stylesheet is applied.

Good idea!
 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebViewGroup.cpp:179
> > +    // It's important to clean up the environment before other tests.
> > +    prepareForNewStylesheetTest(group, &whitelist, &blacklist);
> 
> Maybe this should be renamed to something like clearUserStyleSheetData() or something similar, it's a bit weird that is called prepare for test and you don't run a test after it.

I renamed it to reflect what it does more literally. Should be an improvement.

[Carlos Garcia Campos]

Comment on attachment 193445 [details]
Patch

Excellent!

[Gustavo Noronha (kov)]

Comment on attachment 193445 [details]
Patch

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

The API looks good to me!

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.cpp:252
> + * Inject an external style sheet into pages. It is possible to only apply to the style sheet
> + * to some URIs by passing non-null values for @whitelist or @blacklist. Passing a %NULL

s/to the style/the style/

[Carlos Garcia Campos]

Comment on attachment 193445 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewGroup.cpp:249
> + * @injected_frames: a #WebKitInjectedFrames describing to which frames the style_sheet should apply

#WebKitInjectedFrames -> #WebKitInjectedContentFrames

[Carlos Garcia Campos]

Patches depending on this one landed already, it's been reviewed by Gustavo and me, could a WebKit2 owner review this, please?

[Carlos Garcia Campos]

(In reply to comment #29)
> Patches depending on this one landed already, it's been reviewed by Gustavo and me, could a WebKit2 owner review this, please?

Of course I meant patches blocking this one :-P

[Benjamin Poulain]

Comment on attachment 193445 [details]
Patch

Okay for WebKit2.

[Martin Robinson]

Committed r149219: <http://trac.webkit.org/changeset/149219>

[Benjamin Poulain]

Comment on attachment 193445 [details]
Patch

Patch landed, clearing review flag to remove it from review list.