Bug 69254 - [GTK] Add webkit_web_view_load_alternate_html() to WebKit2 GTK+ API
Summary: [GTK] Add webkit_web_view_load_alternate_html() to 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: Nobody
URL:
Keywords: Gtk
Depends on: 69252
Blocks: 69255
  Show dependency treegraph
 
Reported: 2011-10-03 05:32 PDT by Carlos Garcia Campos
Modified: 2011-10-04 11:11 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.90 KB, patch)
2011-10-03 05:33 PDT, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
Updated patch (7.09 KB, patch)
2011-10-04 01:33 PDT, Carlos Garcia Campos
mrobinson: review+
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 2011-10-03 05:32:30 PDT
Add webkit_web_view_load_alternate_html_string() than can be used to implement default error pages in WebKitWebLoaderclient.
Comment 1 Carlos Garcia Campos 2011-10-03 05:33:58 PDT
Created attachment 109472 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-03 05:35:53 PDT
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.
Comment 3 Gustavo Noronha (kov) 2011-10-03 09:29:22 PDT
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 =)
Comment 4 Martin Robinson 2011-10-03 09:33:20 PDT
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.
Comment 5 Carlos Garcia Campos 2011-10-03 09:39:49 PDT
(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.
Comment 6 Martin Robinson 2011-10-03 09:51:16 PDT
(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.
Comment 7 Carlos Garcia Campos 2011-10-03 23:37:26 PDT
(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.
Comment 8 Carlos Garcia Campos 2011-10-04 01:33:05 PDT
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.
Comment 9 WebKit Review Bot 2011-10-04 01:35:40 PDT
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.
Comment 10 Martin Robinson 2011-10-04 10:55:07 PDT
Comment on attachment 109591 [details]
Updated patch

Looks good to me.
Comment 11 Carlos Garcia Campos 2011-10-04 11:11:08 PDT
Committed r96614: <http://trac.webkit.org/changeset/96614>