Bug 99081 - [GTK] Add methods to add a user stylesheet to the WebKit2 GTK+ API
Summary: [GTK] Add methods to add a user stylesheet to the WebKit2 GTK+ API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords: Gtk
Depends on: 111265
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-11 09:53 PDT by Carlos Garcia Campos
Modified: 2013-04-26 16:08 PDT (History)
9 users (show)

See Also:


Attachments
Patch (16.76 KB, patch)
2012-10-11 10:05 PDT, Carlos Garcia Campos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Try to fix mac build (17.52 KB, patch)
2012-10-11 10:26 PDT, Carlos Garcia Campos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Try to fix win build (18.28 KB, patch)
2012-10-12 09:21 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (14.57 KB, patch)
2013-03-15 18:49 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (14.78 KB, patch)
2013-03-16 13:26 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-10-11 09:53:25 PDT
Add webkit_settings_get_user_style_sheet_uri() and webkit_settings_set_user_style_sheet_uri()
Comment 1 Carlos Garcia Campos 2012-10-11 10:05:26 PDT
Created attachment 168246 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-11 10:09:19 PDT
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
Comment 3 WebKit Review Bot 2012-10-11 10:09:38 PDT
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.
Comment 4 Build Bot 2012-10-11 10:11:28 PDT
Comment on attachment 168246 [details]
Patch

Attachment 168246 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14245979
Comment 5 Martin Robinson 2012-10-11 10:17:51 PDT
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)
Comment 6 Carlos Garcia Campos 2012-10-11 10:26:19 PDT
Created attachment 168249 [details]
Try to fix mac build
Comment 7 WebKit Review Bot 2012-10-11 10:29:12 PDT
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.
Comment 8 Carlos Garcia Campos 2012-10-11 10:29:30 PDT
(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
Comment 9 Martin Robinson 2012-10-11 10:37:12 PDT
(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.
Comment 10 Carlos Garcia Campos 2012-10-11 10:40:45 PDT
(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.
Comment 11 Martin Robinson 2012-10-11 10:48:19 PDT
(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.
Comment 12 Build Bot 2012-10-11 12:07:56 PDT
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
Comment 13 Carlos Garcia Campos 2012-10-12 09:21:27 PDT
Created attachment 168435 [details]
Try to fix win build
Comment 14 WebKit Review Bot 2012-10-12 09:27:04 PDT
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.
Comment 15 Xan Lopez 2013-02-11 10:35:50 PST
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.
Comment 16 Anders Carlsson 2013-02-11 10:42:39 PST
(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.
Comment 17 Xan Lopez 2013-02-11 10:49:23 PST
(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.
Comment 18 Martin Robinson 2013-02-11 10:53:56 PST
(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?
Comment 19 Martin Robinson 2013-02-11 10:54:27 PST
(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. :)
Comment 20 Sam Weinig 2013-02-11 10:58:00 PST
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.
Comment 21 Martin Robinson 2013-03-15 18:49:16 PDT
Created attachment 193413 [details]
Patch
Comment 22 Carlos Garcia Campos 2013-03-16 02:30:35 PDT
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.
Comment 23 Carlos Garcia Campos 2013-03-16 02:40:01 PDT
(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.
Comment 24 Martin Robinson 2013-03-16 13:26:12 PDT
Created attachment 193445 [details]
Patch
Comment 25 Martin Robinson 2013-03-16 13:30:00 PDT
(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.
Comment 26 Carlos Garcia Campos 2013-03-17 02:15:55 PDT
Comment on attachment 193445 [details]
Patch

Excellent!
Comment 27 Gustavo Noronha (kov) 2013-03-17 07:57:33 PDT
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/
Comment 28 Carlos Garcia Campos 2013-03-20 01:34:31 PDT
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
Comment 29 Carlos Garcia Campos 2013-04-26 00:50:36 PDT
Patches depending on this one landed already, it's been reviewed by Gustavo and me, could a WebKit2 owner review this, please?
Comment 30 Carlos Garcia Campos 2013-04-26 00:51:22 PDT
(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
Comment 31 Benjamin Poulain 2013-04-26 13:26:18 PDT
Comment on attachment 193445 [details]
Patch

Okay for WebKit2.
Comment 32 Martin Robinson 2013-04-26 16:05:10 PDT
Committed r149219: <http://trac.webkit.org/changeset/149219>
Comment 33 Benjamin Poulain 2013-04-26 16:08:23 PDT
Comment on attachment 193445 [details]
Patch

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