Bug 69254 – [GTK] Add webkit_web_view_load_alternate_html() to WebKit2 GTK+ API

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

[Carlos Garcia Campos]

Add webkit_web_view_load_alternate_html_string() than can be used to implement default error pages in WebKitWebLoaderclient.

[Carlos Garcia Campos]

Created attachment 109472 [details]
Patch

[WebKit Review Bot]

Attachment 109472 [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

Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:75:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:75:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:78:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:78:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:81:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:81:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:84:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:85:  The parameter name "loader_client" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:88:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:88:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:98:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:101:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:101:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:236:  webkit_web_view_load_alternate_html_string is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 20 in 4 files


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

[Gustavo Noronha (kov)]

Comment on attachment 109472 [details]
Patch

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

Do we need two reviewers for transporting APIs from wk1 to wk2? I assume we will be reviewing the whole API set before we commit to it anyway, right?

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:231
> +

needless line =P =)

[Martin Robinson]

Comment on attachment 109472 [details]
Patch

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

Great stuff, just some minor things below.

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:236
>> + * Request loading of an alternate content for a URI that is unreachable.
>> + * Using this method will preserve the back-forward list. The URI passed in
>> + * @base_uri has to be an absolute URI.
>> + * You can monitor the status of the load operation using the
>> + * #WebKitWebLoaderClient of @web_view. See webkit_web_view_get_loader_client().
> 
> webkit_web_view_load_alternate_html_string is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

This is nice documentation. The only thing that I think it's lacking is a note about when it's appropriate to call this method. Does the user call it during a particular loader callback? Ther other quibble I have is that it seems useful outside of load failures, because it does something different with the back-forward list. I'm not exactly sure what from the documentation you have, so that could be expanded a bit. I also think if it's useful in non-failure cases unreachableURI might need a different name.

> Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:30
> +#define HTML_ALTERNATE "<html><body>Alternate Content</body></html>"

Please use static const char* for these type of strings. If they are only used in one place, be sure to place them in the innermost scope of their use.

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 109472 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109472&action=review
> 
> Great stuff, just some minor things below.
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:236
> >> + * Request loading of an alternate content for a URI that is unreachable.
> >> + * Using this method will preserve the back-forward list. The URI passed in
> >> + * @base_uri has to be an absolute URI.
> >> + * You can monitor the status of the load operation using the
> >> + * #WebKitWebLoaderClient of @web_view. See webkit_web_view_get_loader_client().
> > 
> > webkit_web_view_load_alternate_html_string is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> 
> This is nice documentation. The only thing that I think it's lacking is a note about when it's appropriate to call this method. Does the user call it during a particular loader callback? Ther other quibble I have is that it seems useful outside of load failures, because it does something different with the back-forward list. I'm not exactly sure what from the documentation you have, so that could be expanded a bit. I also think if it's useful in non-failure cases unreachableURI might need a different name.

This documentation is the same than webki1 API. The parameter is called "unreachable" in wk1 API, C API, WebKit2 and WebCore. 

> > Source/WebKit2/UIProcess/API/gtk/tests/testloading.c:30
> > +#define HTML_ALTERNATE "<html><body>Alternate Content</body></html>"
> 
> Please use static const char* for these type of strings. If they are only used in one place, be sure to place them in the innermost scope of their use.

Ok.

[Martin Robinson]

(In reply to comment #5)

> This documentation is the same than webki1 API. The parameter is called "unreachable" in wk1 API, C API, WebKit2 and WebCore. 

Check out the documentation in WebFrame.h in the Mac port. I think we should have something similar.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > This documentation is the same than webki1 API. The parameter is called "unreachable" in wk1 API, C API, WebKit2 and WebCore. 
> 
> Check out the documentation in WebFrame.h in the Mac port. I think we should have something similar.

They use unreachable name too.

[Carlos Garcia Campos]

Created attachment 109591 [details]
Updated patch

New patch adding doc comments suggested by Martin. I also renamed the function to load_html(), since it's shorter and it's obvious that it's a html string.

[WebKit Review Bot]

Attachment 109591 [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

Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:72:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:75:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:75:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:78:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:78:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:81:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:81:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:84:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:85:  The parameter name "loader_client" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:88:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:88:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:92:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:98:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:98:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:101:  The parameter name "web_view" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:101:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:241:  webkit_web_view_load_alternate_html is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 20 in 4 files


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

[Martin Robinson]

Comment on attachment 109591 [details]
Updated patch

Looks good to me.

[Carlos Garcia Campos]

Committed r96614: <http://trac.webkit.org/changeset/96614>