Bug 69255 – [GTK] Implement default error pages in WebKit2 GTK+ API

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

[Carlos Garcia Campos]

When a page fails to load and client doesn't handle load errors, we should load a default error page with the error.

[Carlos Garcia Campos]

Created attachment 109475 [details]
Patch

[Martin Robinson]

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.

[Carlos Garcia Campos]

(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.

[Martin Robinson]

(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.

[Martin Robinson]

(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. :)

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 109594 [details]
Updated patch

Patch updated to emit load signals even when loading error page.

[WebKit Review Bot]

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.

[Gustavo Noronha (kov)]

Comment on attachment 109594 [details]
Updated patch

Attachment 109594 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9935500

[Carlos Garcia Campos]

Committed r96616: <http://trac.webkit.org/changeset/96616>