Bug 69255 - [GTK] Implement default error pages in WebKit2 GTK+ API
Summary: [GTK] Implement default error pages in 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: 69254
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-03 05:37 PDT by Carlos Garcia Campos
Modified: 2011-10-04 11:13 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.76 KB, patch)
2011-10-03 05:45 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (6.54 KB, patch)
2011-10-04 01:47 PDT, Carlos Garcia Campos
mrobinson: review+
gns: commit-queue-
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:37:12 PDT
When a page fails to load and client doesn't handle load errors, we should load a default error page with the error.
Comment 1 Carlos Garcia Campos 2011-10-03 05:45:48 PDT
Created attachment 109475 [details]
Patch
Comment 2 Martin Robinson 2011-10-03 09:36:31 PDT
Comment on attachment 109475 [details]
Patch

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

Looks fine to me, but I'm not sure if it's the right thing to do to "hide" the loading of the error page from the API.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:72
> +    WebKitWebLoaderClient* client = WEBKIT_WEB_LOADER_CLIENT(clientInfo);
> +    if (client->priv->loadingErrorPage)
> +        return;
> +
>      gboolean returnValue;
> -    g_signal_emit(WEBKIT_WEB_LOADER_CLIENT(clientInfo), signals[PROVISIONAL_LOAD_STARTED], 0, &returnValue);
> +    g_signal_emit(client, signals[PROVISIONAL_LOAD_STARTED], 0, &returnValue);

Are you sure it's good to hide inner workings of the error page load from the API?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebLoaderClient.cpp:84
> +    WebKitWebLoaderClient* client = WEBKIT_WEB_LOADER_CLIENT(clientInfo);
> +    if (client->priv->loadingErrorPage)
> +        return;
> +
>      gboolean returnValue;

Ditto.
Comment 3 Carlos Garcia Campos 2011-10-03 09:42:01 PDT
(In reply to comment #2)
> (From update of attachment 109475 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109475&action=review
> 
> Looks fine to me, but I'm not sure if it's the right thing to do to "hide" the loading of the error page from the API.
> 

It's not hidden, provisional-load-failed and load-failed signals documentation say that a custom error page will be loaded if the user doesn't handle the signal. The behaviour is exactly the same than webkit1 API.
Comment 4 Martin Robinson 2011-10-03 09:54:27 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 109475 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=109475&action=review
> > 
> > Looks fine to me, but I'm not sure if it's the right thing to do to "hide" the loading of the error page from the API.
> > 
> 
> It's not hidden, provisional-load-failed and load-failed signals documentation say that a custom error page will be loaded if the user doesn't handle the signal. The behaviour is exactly the same than webkit1 API.

I was referring to the loading of the custom error page. I was referring to the fact that you cannot observe the loading signals as the custom error page loads. Note that this does not happen on the Mac port. I think we should follow the approach of firing the signals. At least the conservative decision is to fire them now and let the UA track if it is loading an error page.

I'm guessing the original decision was made to not break Epiphany. Since we are rewriting this code in Epiphany anyway, we can make whatever decision we want. My tendency is to be more similar to other ports.
Comment 5 Martin Robinson 2011-10-03 09:55:06 PDT
(In reply to comment #4)

> I was referring to the loading of the custom error page

Sorry. I was *not* referring to the loading of the custom error page. :)
Comment 6 Carlos Garcia Campos 2011-10-03 23:28:50 PDT
(In reply to comment #4) 
> I was referring to the loading of the custom error page. I was referring to the fact that you cannot observe the loading signals as the custom error page loads. Note that this does not happen on the Mac port. I think we should follow the approach of firing the signals. At least the conservative decision is to fire them now and let the UA track if it is loading an error page.
> 
> I'm guessing the original decision was made to not break Epiphany. Since we are rewriting this code in Epiphany anyway, we can make whatever decision we want. My tendency is to be more similar to other ports.

For me, the fact that the error message is a web page is an implementation detail, that's why I kept the wk1 behaviour of not exposing the load process of error pages to the user. But I agree, we can just emit the signals for now and take a final decision later, instead of the opposite.
Comment 7 Carlos Garcia Campos 2011-10-04 01:47:23 PDT
Created attachment 109594 [details]
Updated patch

Patch updated to emit load signals even when loading error page.
Comment 8 WebKit Review Bot 2011-10-04 01:50:10 PDT
Attachment 109594 [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/WebKitError.cpp:27:  webkit_network_error_quark is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitError.cpp:38:  webkit_policy_error_quark is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitError.cpp:49:  webkit_plugin_error_quark is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitError.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/API/gtk/WebKitError.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Gustavo Noronha (kov) 2011-10-04 03:57:00 PDT
Comment on attachment 109594 [details]
Updated patch

Attachment 109594 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9935500
Comment 10 Carlos Garcia Campos 2011-10-04 11:13:31 PDT
Committed r96616: <http://trac.webkit.org/changeset/96616>