Add webkit_settings_get_user_style_sheet_uri() and webkit_settings_set_user_style_sheet_uri()
Created attachment 168246 [details] Patch
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
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 on attachment 168246 [details] Patch Attachment 168246 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14245979
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)
Created attachment 168249 [details] Try to fix mac build
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.
(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
(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.
(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.
(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 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
Created attachment 168435 [details] Try to fix win build
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 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.
(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.
(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.
(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?
(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 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.
Created attachment 193413 [details] Patch
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.
(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.
Created attachment 193445 [details] Patch
(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 on attachment 193445 [details] Patch Excellent!
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 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
Patches depending on this one landed already, it's been reviewed by Gustavo and me, could a WebKit2 owner review this, please?
(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 on attachment 193445 [details] Patch Okay for WebKit2.
Committed r149219: <http://trac.webkit.org/changeset/149219>
Comment on attachment 193445 [details] Patch Patch landed, clearing review flag to remove it from review list.