Summary: | [GTK] Add webkit_web_view_load_alternate_html() to WebKit2 GTK+ API | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | gustavo, mrobinson, pnormand, webkit.review.bot, xan.lopez | ||||||
Priority: | P2 | Keywords: | Gtk | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | Linux | ||||||||
Bug Depends on: | 69252 | ||||||||
Bug Blocks: | 69255 | ||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2011-10-03 05:32:30 PDT
Created attachment 109472 [details]
Patch
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 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 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. (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. (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. (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. 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.
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 on attachment 109591 [details]
Updated patch
Looks good to me.
Committed r96614: <http://trac.webkit.org/changeset/96614> |