Bug 18344

Summary: [GTK] Error reporting
Product: WebKit Reporter: Luca Bruno <lethalman88>
Component: WebKitGTKAssignee: Jan Alonzo <jmalonzo>
Status: RESOLVED FIXED    
Severity: Enhancement CC: diegoe, gustavo, jmalonzo, pochu27, xan.lopez, zan
Priority: P2 Keywords: Gtk, Soup
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Cleaned up patch from Luca Bruno in bug #18297
none
Updated patch to include new files.
none
[1/3] Add API to load content for unreachable URL
none
[2/3] Add default page for error reporting
none
[3/3] implement resource error-using methods
none
[1/3] Add API to load content for unreachable URL - v2
none
[2/3] Add default page for error reporting - v2
none
[3/3] implement resource error-using methods - v2
none
[1/3] Add API to load content for unreachable URL - v3
none
[2/3] Add default page for error reporting - v3
xan.lopez: review+
[3/3] implement resource error-using methods - v2
xan.lopez: review+
[1/3] Add API to load content for unreachable URL - v4 xan.lopez: review+

Description Luca Bruno 2008-04-07 09:30:09 PDT
The discussion has been started already in http://bugs.webkit.org/show_bug.cgi?id=18297.
How to handle errors? GError? Creating a new object?

A patch in that bug used both for network errors.
Comment 1 Gustavo Noronha (kov) 2009-02-26 16:26:48 PST
I think GError is the way to go. Most errors in webkit are async, so we will want to have signals emitted to notify the client about them.
Comment 2 Diego Escalante Urrelo 2009-03-30 16:18:55 PDT
Created attachment 29094 [details]
Cleaned up patch from Luca Bruno in bug #18297
Comment 3 Alexander Butenko 2009-03-30 19:08:42 PDT
probably instead of message box it will be way better just to emit event for a browser. In this case browser can draw nice error page in webview. Message boxes is always annoying. 

Plus i think it will be nice to include handing of network errors like 'Permission denied', 'Not found', 'Forbidden' etc. 
Comment 4 Diego Escalante Urrelo 2009-04-02 19:51:56 PDT
Created attachment 29216 [details]
Updated patch to include new files.

Forgot to add new files.
Comment 5 Jan Alonzo 2009-04-05 00:15:16 PDT
(In reply to comment #4)
> Created an attachment (id=29216) [review]
> Updated patch to include new files.
> 
> Forgot to add new files.
> 

Diegoe, is this for review? If so can you please change the review flag to '?' ?

Thanks
Comment 6 Jan Alonzo 2009-04-07 14:22:05 PDT
*** Bug 24255 has been marked as a duplicate of this bug. ***
Comment 7 Diego Escalante Urrelo 2009-04-07 17:04:03 PDT
No, not for review, just attached the result of trying to apply the older patch. Uploaded so others can check it out if curious. But I plan to do something with this soon.
Comment 8 Jan Alonzo 2009-04-13 13:46:21 PDT
Created attachment 29436 [details]
[1/3] Add API to load content for unreachable URL

This patch adds a "load-error" signal to the WebView to signify that an error occurred while loading. It also adds a WebFrame API to load an alternate content for unreachable URLs.
Comment 9 Jan Alonzo 2009-04-13 13:49:41 PDT
Created attachment 29437 [details]
[2/3] Add default page for error reporting

This patch adds a default error page to be installed in $(datadir)/webkit-1.0/resources/error.html. This will be the default page that will be loaded if load-error is not handled.
Comment 10 Jan Alonzo 2009-04-13 13:53:21 PDT
Created attachment 29438 [details]
[3/3] implement resource error-using methods

Implement resource error-reporting methods. This patch also adds the error domain/codes that WebKitGtk will be reporting in WebKitError.h (Please see comment why it's sitting in WebCore/platform rather than in WebKit)
Comment 11 Gustavo Noronha (kov) 2009-04-13 14:46:19 PDT
Comment on attachment 29436 [details]
[1/3] Add API to load content for unreachable URL

>  #include <JavaScriptCore/APICast.h>
> +#include <glib.h>
> +#include <glib/gprintf.h>

gprintf.h is added here, and removed later in patch error-gerror, is it needed here?


> -void FrameLoaderClient::dispatchDidFailLoad(const ResourceError&)
> +void FrameLoaderClient::dispatchDidFailLoad(const ResourceError& error)
>  {
> -    g_signal_emit_by_name(m_frame, "load-done", false);

Perhaps we should do like elsewhere, and still emit this signal. Though it is not even documented, there may be users. It may be a good oportunity to document it and make it deprecated, at the same time.

> +static void webkit_web_frame_load_data(WebKitWebFrame* frame, const gchar* content, const gchar* mimeType, const gchar* encoding, const gchar* baseURL, const gchar* unreachableURL)
> +{
> +    Frame* coreFrame = core(frame);
> +    ASSERT(coreFrame);
> +
> +    KURL baseurl;
> +
> +    if (!baseURL)
> +        baseurl = blankURL();

This doesn't make sense to me. What if baseURL is there? Shouldn't you use it, then? Perhaps you are missing an else clause here, or initializing baseurl with baseURL, upon declaration?

> +    RefPtr<SharedBuffer> sharedBuffer = SharedBuffer::create(content, g_utf8_strlen(content, -1));
> +    SubstituteData substituteData(sharedBuffer.release(),
> +                                  mimeType ? String::fromUTF8(mimeType) : String::fromUTF8("text/html"),
> +                                  encoding ? String::fromUTF8(encoding) : String::fromUTF8("UTF-8"),
> +                                  baseurl,
> +                                  KURL(KURL(), String::fromUTF8(unreachableURL)));
> +
> +    coreFrame->loader()->load(request, substituteData, false);

So, does WebCore treat normal and 'alternate' loads differently, somehow? Is the fact that unreachableURL is NULL enough?

I like the patch, looks pretty good to me.
Comment 12 Gustavo Noronha (kov) 2009-04-13 14:59:09 PDT
Comment on attachment 29437 [details]
[2/3] Add default page for error reporting

> +dist_resources_DATA = \
> +	$(shell ls $(srcdir)/WebKit/gtk/resources/*.{html,css})

This seems to be modified in the next patch to have only *.html, I think it would be best to have this changed squashed into this patch.

> +    GFile* errorFile = g_file_new_for_uri("file://"DATA_DIR"/webkit-1.0/resources/error.html");

Good stuff. In the future we can use the locale to fetch a translated patch, if it exists, but I think we should not add unnecessary complexity to the initial implementation.

> +    gboolean loaded = g_file_load_contents(errorFile, NULL, &fileContent, NULL, NULL, NULL);

I think those NULL's should be 0's?

> +    webkit_web_frame_load_alternate_string(m_frame, content.utf8().data(), NULL, error.failingURL().utf8().data());

Same here.
Comment 13 Gustavo Noronha (kov) 2009-04-13 15:09:00 PDT
Comment on attachment 29438 [details]
[3/3] implement resource error-using methods

> +enum WebKitNetworkError {
> +    WebKitNetworkErrorTransport                         = 300,
> +    WebKitNetworkErrorUnknownProtocol                   = 301,
> +    WebKitNetworkErrorCancelled                         = 302,
> +    WebKitNetworkErrorFileDoesNotExist                  = 303,
> +    WebKitNetworkErrorFileCannotBeOpened                = 304,
> +    WebKitNetworkErrorHTTPMethodUnknown                 = 305,
> +};

I am missing a way of finding out what exactly was the problem: how would the client application be able to tell if the problem was caused by a DNS failure, for instance?

> +
> +// Sync'd with WebKit/mac/Misc/WebKitErrors.h
> +enum WebKitPolicyError {
> +    WebKitPolicyErrorCannotShowMIMEType                 = 100,
> +    WebKitPolicyErrorCannotShowURL                      = 101,
> +    WebKitPolicyErrorFrameLoadInterruptedByPolicyChange = 102,
> +    WebKitPolicyErrorCannotUseRestrictedPort            = 103,
> +};
> +
> +enum WebKitPluginError {
> +    WebKitPluginErrorCannotFindPlugin                   = 200,
> +    WebKitPluginErrorCannotLoadPlugin                   = 201,
> +    WebKitPluginErrorJavaUnavailable                    = 202,
> +    WebKitPluginErrorConnectionCancelled                = 203,
> +    WebKitPluginErrorWillHandleLoad                     = 204,
> +};

Also, I am a bit uneasy with handling cancelling and plugin will handle load as errors, but it may make sense. From reading the code it seems to me that in those cases, shouldFallback will return false and disallow an error page loading?
Comment 14 Jan Alonzo 2009-04-19 04:54:32 PDT
Clearing review flag.. The patches need updating.
Comment 15 Jan Alonzo 2009-04-24 12:03:49 PDT
Created attachment 29751 [details]
[1/3] Add API to load content for unreachable URL - v2

Updated patch based on kov's feedback.
Comment 16 Jan Alonzo 2009-04-24 12:09:51 PDT
Created attachment 29752 [details]
[2/3] Add default page for error reporting - v2

Updated based on kov's feedback.
Comment 17 Jan Alonzo 2009-04-24 12:15:10 PDT
Created attachment 29753 [details]
[3/3] implement resource error-using methods - v2

Updated reporting of ResourceErrors to use Soup and GIO errors rather than custom WebKitErrors (as the previous patch does).
Comment 18 Xan Lopez 2009-04-24 13:08:47 PDT
Comment on attachment 29751 [details]
[1/3] Add API to load content for unreachable URL - v2

Mmm, how come this diff is not using -p? Harder to read the patch that way.

> Index: WebKit/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp
> ===================================================================
> --- WebKit.orig/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	2009-04-24 20:49:12.000000000 +1000
> +++ WebKit/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp	2009-04-24 20:50:33.000000000 +1000
> @@ -57,6 +57,7 @@
>  #include "webkitprivate.h"
>  
>  #include <JavaScriptCore/APICast.h>
> +#include <glib.h>
>  #include <stdio.h>
>  #if PLATFORM(UNIX)
>  #include <sys/utsname.h>
> @@ -260,8 +261,11 @@
>  
>      // We can get a stopLoad() from dispose when the object is being
>      // destroyed, don't emit the signal in that case.
> -    if (!privateData->disposing)
> -        g_signal_emit_by_name(webView, "load-finished", m_frame);
> +    if (privateData->disposing)
> +        return;
> +
> +    gboolean handled;
> +    g_signal_emit_by_name(webView, "load-finished", m_frame, NULL, NULL, &handled);
>  }

Mmm, I guess you meant to emit load-error here or...? But does that make sense? /me confused

> -void FrameLoaderClient::dispatchDidFailProvisionalLoad(const ResourceError&)
> +void FrameLoaderClient::dispatchDidFailProvisionalLoad(const ResourceError& error)
>  {
> +    dispatchDidFailLoad(error);
> +
> +    // FIXME: this is here for compatibility's sake.
>      g_signal_emit_by_name(m_frame, "load-done", false);
>  }
>  

Perhaps you can add "Remove when the signal is removed" or something, to be absolutely clear.

> +static void webkit_web_frame_load_data(WebKitWebFrame* frame, const gchar* content, const gchar* mimeType, const gchar* encoding, const gchar* baseURL, const gchar* unreachableURL)
> +{
> +    Frame* coreFrame = core(frame);
> +    ASSERT(coreFrame);
> +
> +    KURL baseurl = (baseURL) ? KURL(KURL(), String::fromUTF8(baseURL)) : blankURL();

baseKURL perhaps? URL has to be capitalized properly. And the parenthesis around baseURL are not needed.

> +
> +    // FIXME: convert baseurl and unreachableURL to an absolute url

Can you briefly explain me why is this important (since you seem to pass the actual data directly) and how do you intend to do it, or how does it work without it now?
Comment 19 Xan Lopez 2009-04-24 13:16:12 PDT
Comment on attachment 29752 [details]
[2/3] Add default page for error reporting - v2

> -                                           NULL, error.failingURL().utf8().data());
> +    String content;
> +    gchar* fileContent = 0;
> +    GFile* errorFile = g_file_new_for_uri("file://"DATA_DIR"/webkit-1.0/resources/error.html");

Do not hardcode "file://", use g_filename_to_uri.

> +
> +    if (!errorFile)
> +        content = String::format("<html><body>%s</body></html>", webError->message);
> +    else {
> +        gboolean loaded = g_file_load_contents(errorFile, NULL, &fileContent, 0, 0, 0);

Either NULL or 0, but not both :) We decided to use NULL in WebKit/gtk/, so let's use that.

> +        if (!loaded)
> +            content = String::format("<html><body>%s</body></html>", webError->message);
> +        else
> +            content = String::format(fileContent, error.failingURL().utf8().data(), webError->message);
> +    }
> +
> +    webkit_web_frame_load_alternate_string(m_frame, content.utf8().data(), 0, error.failingURL().utf8().data());

Same

> +
> +    if (fileContent)
> +        g_free(fileContent);

g_free is NULL-safe.

> +
> +    if (errorFile)
> +        g_object_unref(errorFile);
> +
>      g_error_free(webError);
>  

> +#errorTitleText {
> + font-family: sans-serif;
> + font-size: 120%;
> + font-weight: bold;
> +}
> +
> +#errorMessageText {
> + font: 80% Verdana, sans-serif;
> +}

I think we should just use the font the user has set...

> +
> +</style>
> +<script type="text/javascript">
> +
> +function tryagain()
> +{
> + location.reload();
> +}
> +</script>
> +</head>
> +<body>
> +<div id="errorContainer">
> +
> +<div id="errorTitle">
> + <p id="errorTitleText">Unable to load page</p>
> +</div>
> +<div id="errorMessage">
> + <p>Problem occurred while loading the URL %s</p>
> + <p id="errorMessageText">%s</a>
> +</p>
> +</div>

We need to be able to translate the error page, but maybe that can go in another patch.
Comment 20 Xan Lopez 2009-04-24 13:25:22 PDT
Comment on attachment 29753 [details]
[3/3] implement resource error-using methods - v2

> Index: WebKit/WebCore/GNUmakefile.am
> ===================================================================
> --- WebKit.orig/WebCore/GNUmakefile.am	2009-04-24 20:49:10.000000000 +1000
> +++ WebKit/WebCore/GNUmakefile.am	2009-04-25 04:26:07.000000000 +1000
> @@ -1755,6 +1755,10 @@
>  	WebCore/plugins/gtk/xembed.h
>  endif
>  
> +webcoregtk_cppflags += \
> +	-DWTF_USE_SOUP=1 \
> +	-I$(srcdir)/WebCore/platform/network/soup
> +
>  webcoregtk_sources += \
>  	WebCore/page/gtk/AXObjectCacheAtk.cpp \
>  	WebCore/page/gtk/AccessibilityObjectAtk.cpp \
> @@ -1840,13 +1844,7 @@
>  	WebCore/platform/image-decoders/png/PNGImageDecoder.h \
>  	WebCore/platform/image-decoders/xbm/XBMImageDecoder.cpp \
>  	WebCore/platform/image-decoders/xbm/XBMImageDecoder.h \
> -	WebCore/platform/text/gtk/TextBreakIteratorInternalICUGtk.cpp
> -
> -webcore_cppflags += \
> -	-DWTF_USE_SOUP=1 \
> -	-I$(srcdir)/WebCore/platform/network/soup
> -
> -webcore_sources += \
> +	WebCore/platform/text/gtk/TextBreakIteratorInternalICUGtk.cpp \
>  	WebCore/platform/network/soup/AuthenticationChallenge.h \
>  	WebCore/platform/network/soup/CookieJarSoup.cpp \
>  	WebCore/platform/network/soup/CookieJarSoup.h \
> @@ -1856,6 +1854,7 @@
>  	WebCore/platform/network/soup/ResourceRequest.h \
>  	WebCore/platform/network/soup/ResourceResponse.h
>  
> +
>  if USE_GNOMEKEYRING
>  webcore_cppflags += \
>  	-DWTF_USE_GNOMEKEYRING=1

Stuff for another patch?


> -    gboolean handled = false;
> -    g_signal_emit_by_name(webView, "load-error", m_frame, error.failingURL().utf8().data(), webError, &handled);
> +    gboolean isHandled = false;
> +    g_signal_emit_by_name(webView, "load-error", m_frame, error.failingURL().utf8().data(), webError, &isHandled);
>  
> -    if (handled) {
> +    if (isHandled) {
>          g_error_free(webError);
>          return;

Also a different patch.

Awesome stuff!
Comment 21 Jan Alonzo 2009-04-24 14:28:48 PDT
(In reply to comment #18)
> >  
> >      // We can get a stopLoad() from dispose when the object is being
> >      // destroyed, don't emit the signal in that case.
> > -    if (!privateData->disposing)
> > -        g_signal_emit_by_name(webView, "load-finished", m_frame);
> > +    if (privateData->disposing)
> > +        return;
> > +
> > +    gboolean handled;
> > +    g_signal_emit_by_name(webView, "load-finished", m_frame, NULL, NULL, &handled);
> >  }
> 
> Mmm, I guess you meant to emit load-error here or...? But does that make sense?
> /me confused

I'm just fixing what is there for these patch sets. It's been using load-finished since so why should I change? If it doesn't seem right then I guess that's a different patch for a different bug.

> 
> > -void FrameLoaderClient::dispatchDidFailProvisionalLoad(const ResourceError&)
> > +void FrameLoaderClient::dispatchDidFailProvisionalLoad(const ResourceError& error)
> >  {
> > +    dispatchDidFailLoad(error);
> > +
> > +    // FIXME: this is here for compatibility's sake.
> >      g_signal_emit_by_name(m_frame, "load-done", false);
> >  }
> >  
> 
> Perhaps you can add "Remove when the signal is removed" or something, to be
> absolutely clear.

Ok.

> > +static void webkit_web_frame_load_data(WebKitWebFrame* frame, const gchar* content, const gchar* mimeType, const gchar* encoding, const gchar* baseURL, const gchar* unreachableURL)
> > +{
> > +    Frame* coreFrame = core(frame);
> > +    ASSERT(coreFrame);
> > +
> > +    KURL baseurl = (baseURL) ? KURL(KURL(), String::fromUTF8(baseURL)) : blankURL();
> 
> baseKURL perhaps? URL has to be capitalized properly. And the parenthesis
> around baseURL are not needed.

Ok. I'll make it baseKURL.

> 
> > +
> > +    // FIXME: convert baseurl and unreachableURL to an absolute url
> 
> Can you briefly explain me why is this important (since you seem to pass the
> actual data directly) and how do you intend to do it, or how does it work
> without it now?

I don't understand. Why it's important to pass an absolute URL or why is this method important?
Comment 22 Jan Alonzo 2009-04-24 14:32:07 PDT
(In reply to comment #19)
> (From update of attachment 29752 [details] [review])
> > -                                           NULL, error.failingURL().utf8().data());
> > +    String content;
> > +    gchar* fileContent = 0;
> > +    GFile* errorFile = g_file_new_for_uri("file://"DATA_DIR"/webkit-1.0/resources/error.html");
> 
> Do not hardcode "file://", use g_filename_to_uri.

OK.

> 
> > +
> > +    if (!errorFile)
> > +        content = String::format("<html><body>%s</body></html>", webError->message);
> > +    else {
> > +        gboolean loaded = g_file_load_contents(errorFile, NULL, &fileContent, 0, 0, 0);
> 
> Either NULL or 0, but not both :) We decided to use NULL in WebKit/gtk/, so
> let's use that.

Gustavo suggested to use 0 :( I'll change it back.

> 
> > +        if (!loaded)
> > +            content = String::format("<html><body>%s</body></html>", webError->message);
> > +        else
> > +            content = String::format(fileContent, error.failingURL().utf8().data(), webError->message);
> > +    }
> > +
> > +    webkit_web_frame_load_alternate_string(m_frame, content.utf8().data(), 0, error.failingURL().utf8().data());
> 
> Same
> 
> > +
> > +    if (fileContent)
> > +        g_free(fileContent);
> 
> g_free is NULL-safe.

Right.

> 
> > +
> > +    if (errorFile)
> > +        g_object_unref(errorFile);
> > +
> >      g_error_free(webError);
> >  
> 
> > +#errorTitleText {
> > + font-family: sans-serif;
> > + font-size: 120%;
> > + font-weight: bold;
> > +}
> > +
> > +#errorMessageText {
> > + font: 80% Verdana, sans-serif;
> > +}
> 
> I think we should just use the font the user has set...

Hmmm... ok.


Comment 23 Jan Alonzo 2009-04-24 15:49:19 PDT
Created attachment 29769 [details]
[1/3] Add API to load content for unreachable URL - v3

Updated per xan's feedback.
Comment 24 Jan Alonzo 2009-04-24 15:51:46 PDT
Created attachment 29770 [details]
[2/3] Add default page for error reporting - v3

Updated per xan's feedback
Comment 25 Jan Alonzo 2009-04-24 15:53:19 PDT
Created attachment 29771 [details]
[3/3] implement resource error-using methods - v2

Updated per kov's and xan's feedback.
Comment 26 Gustavo Noronha (kov) 2009-04-24 18:37:12 PDT
The patches look good to me. I'll let Xan r+, though, after he sees the updates.
Comment 27 Xan Lopez 2009-04-24 23:49:30 PDT
(In reply to comment #21)
> (In reply to comment #18)
> > >  
> > >      // We can get a stopLoad() from dispose when the object is being
> > >      // destroyed, don't emit the signal in that case.
> > > -    if (!privateData->disposing)
> > > -        g_signal_emit_by_name(webView, "load-finished", m_frame);
> > > +    if (privateData->disposing)
> > > +        return;
> > > +
> > > +    gboolean handled;
> > > +    g_signal_emit_by_name(webView, "load-finished", m_frame, NULL, NULL, &handled);
> > >  }
> > 
> > Mmm, I guess you meant to emit load-error here or...? But does that make sense?
> > /me confused
> 
> I'm just fixing what is there for these patch sets. It's been using
> load-finished since so why should I change? If it doesn't seem right then I
> guess that's a different patch for a different bug.
> 

OK, then I don't see what are you fixing, since the signature you are changing the signal to is wrong. 'load-finished' only has one parameter and no return value.

> > > +    // FIXME: convert baseurl and unreachableURL to an absolute url
> > 
> > Can you briefly explain me why is this important (since you seem to pass the
> > actual data directly) and how do you intend to do it, or how does it work
> > without it now?
> 
> I don't understand. Why it's important to pass an absolute URL or why is this
> method important?
> 

The former.
Comment 28 Xan Lopez 2009-04-24 23:52:34 PDT
> > > +    if (!errorFile)
> > > +        content = String::format("<html><body>%s</body></html>", webError->message);
> > > +    else {
> > > +        gboolean loaded = g_file_load_contents(errorFile, NULL, &fileContent, 0, 0, 0);
> > 
> > Either NULL or 0, but not both :) We decided to use NULL in WebKit/gtk/, so
> > let's use that.
> 
> Gustavo suggested to use 0 :( I'll change it back.

Well, we had decided to use NULL instead of 0 in all the files in WebKit/gtk/, since they are more likely to be touched by GTK+ people in general. We can change this back, but in any case what's wrong is to mix both NULL and 0 in the same patch. You seem to have changed everything to 0 now in the last patch... I'd prefer to use NULL, since that's what we have as a rule now.
Comment 29 Xan Lopez 2009-04-24 23:53:36 PDT
Comment on attachment 29771 [details]
[3/3] implement resource error-using methods - v2

Looks good to me.
Comment 30 Jan Alonzo 2009-04-25 00:27:13 PDT
(In reply to comment #28)
> > > > +    if (!errorFile)
> > > > +        content = String::format("<html><body>%s</body></html>", webError->message);
> > > > +    else {
> > > > +        gboolean loaded = g_file_load_contents(errorFile, NULL, &fileContent, 0, 0, 0);
> > > 
> > > Either NULL or 0, but not both :) We decided to use NULL in WebKit/gtk/, so
> > > let's use that.
> > 
> > Gustavo suggested to use 0 :( I'll change it back.
> 
> Well, we had decided to use NULL instead of 0 in all the files in WebKit/gtk/,
> since they are more likely to be touched by GTK+ people in general. We can
> change this back, but in any case what's wrong is to mix both NULL and 0 in the
> same patch. You seem to have changed everything to 0 now in the last patch...
> I'd prefer to use NULL, since that's what we have as a rule now.

I thought we only use NULL in gtk/webkit? But in gtk/WebCoreSupport, I think we still use 0?

Comment 31 Xan Lopez 2009-04-25 00:29:56 PDT
(In reply to comment #30)
> I thought we only use NULL in gtk/webkit? But in gtk/WebCoreSupport, I think we
> still use 0?
> 

There's quite a bit more of NULL than 0 in gtk/WebCoreSupport, and as far as I can remember the rule was meant for all the code inside gtk/.
Comment 32 Jan Alonzo 2009-04-25 00:57:32 PDT
Created attachment 29783 [details]
[1/3] Add API to load content for unreachable URL - v4

updated load-finished signal emission and added a note in the documentation of _load_alternate_string that we're expecting an absolute URL in base_url.
Comment 33 Xan Lopez 2009-04-25 01:13:03 PDT
Comment on attachment 29783 [details]
[1/3] Add API to load content for unreachable URL - v4

> +    webkit_web_view_signals[LOAD_ERROR] = g_signal_new("load-error",
> +            G_TYPE_FROM_CLASS(webViewClass),
> +            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),

I'm sorry I'm realizing now, and this is wrong elsewhere, but I don't think it makes sense that this signal is G_SIGNAL_ACTION.

r=me if you remove that.
Comment 34 Xan Lopez 2009-04-25 01:18:59 PDT
Comment on attachment 29771 [details]
[3/3] implement resource error-using methods - v2

r=me, but let's fix the 0/NULL mess ASAP.
Comment 35 Xan Lopez 2009-04-25 01:20:30 PDT
Comment on attachment 29770 [details]
[2/3] Add default page for error reporting - v3

Er, I meant to review this one.
Comment 36 Xan Lopez 2009-04-25 01:21:10 PDT
Comment on attachment 29783 [details]
[1/3] Add API to load content for unreachable URL - v4

r=me losing the G_SIGNAL_ACTION.
Comment 37 Jan Alonzo 2009-04-25 02:20:52 PDT
(In reply to comment #36)
> (From update of attachment 29783 [details] [review])
> r=me losing the G_SIGNAL_ACTION.
> 

Landed in r42864, r42865 and r42866 respectively. Thanks!
Comment 38 Zan Dobersek 2009-05-02 14:55:37 PDT
(In reply to comment #37)
> Landed in r42864, r42865 and r42866 respectively. Thanks!
> 

r42866 causes regressions, noticed in failures of these tests:
dom/xhtml/level3/core/entitygetinputencoding03.xhtml
dom/xhtml/level3/core/entitygetinputencoding04.xhtml
dom/xhtml/level3/core/entitygetxmlencoding02.xhtml
dom/xhtml/level3/core/entitygetxmlencoding03.xhtml
dom/xhtml/level3/core/entitygetxmlencoding04.xhtml
dom/xhtml/level3/core/entitygetxmlversion03.xhtml
dom/xhtml/level3/core/entitygetxmlversion04.xhtml
dom/xhtml/level3/core/nodegetbaseuri16.xhtml
dom/xhtml/level3/core/nodegetbaseuri19.xhtml
dom/xhtml/level3/core/nodegetbaseuri20.xhtml

They all time out. The problem was tracked down to changes in ResourceHandleSoup.cpp[1] - when facing an invalid URL, ResourceHandle::scheduleFailure is now called. This results in calling ResourceHandle::fireFailure, but ResourceHandle::client() is an empty client (a simple ResourceClient, nothing more specific), therefor call to ResourceHandle::client()->cannotShowUrl(this) does nothing (helpful).

There might be some other regressions, but they went on unnoticed.

The fix might be to go back to using an idle function, but the current approach (using scheduleFailure) is much better if we can get it to work.

[1]: http://trac.webkit.org/changeset/42866#file3
Comment 39 Jan Alonzo 2009-05-02 15:09:11 PDT
(In reply to comment #38)
> (In reply to comment #37)
> > Landed in r42864, r42865 and r42866 respectively. Thanks!
> > 
> 
> r42866 causes regressions, noticed in failures of these tests:
> dom/xhtml/level3/core/entitygetinputencoding03.xhtml
> dom/xhtml/level3/core/entitygetinputencoding04.xhtml
> dom/xhtml/level3/core/entitygetxmlencoding02.xhtml
> dom/xhtml/level3/core/entitygetxmlencoding03.xhtml
> dom/xhtml/level3/core/entitygetxmlencoding04.xhtml
> dom/xhtml/level3/core/entitygetxmlversion03.xhtml
> dom/xhtml/level3/core/entitygetxmlversion04.xhtml
> dom/xhtml/level3/core/nodegetbaseuri16.xhtml
> dom/xhtml/level3/core/nodegetbaseuri19.xhtml
> dom/xhtml/level3/core/nodegetbaseuri20.xhtml
> 
> They all time out. The problem was tracked down to changes in
> ResourceHandleSoup.cpp[1] - when facing an invalid URL,
> ResourceHandle::scheduleFailure is now called. This results in calling
> ResourceHandle::fireFailure, but ResourceHandle::client() is an empty client (a
> simple ResourceClient, nothing more specific), therefor call to
> ResourceHandle::client()->cannotShowUrl(this) does nothing (helpful).
> 
> There might be some other regressions, but they went on unnoticed.
> 
> The fix might be to go back to using an idle function, but the current approach
> (using scheduleFailure) is much better if we can get it to work.
> 
> [1]: http://trac.webkit.org/changeset/42866#file3
> 

Which URL in these tests are timing out and why are these tests in Skipped? We need to figure out why it's timing out or rollback the change altogether (it's not as easy as changing it to scheduleFailure()).
Comment 40 Zan Dobersek 2009-05-02 15:43:21 PDT
(In reply to comment #39)
> Which URL in these tests are timing out and why are these tests in Skipped?
>

They all seem to be timing out when loading 'external_foobr.ent'.
When grepping, it can be seen that this URL is being used in every file at line 4:
<!ENTITY ent2 SYSTEM 'external_foobr.ent'> [1]

[1] http://trac.webkit.org/browser/trunk/LayoutTests/dom/xhtml/level3/core/entitygetinputencoding04.xhtml#L4
Comment 41 Gustavo Noronha (kov) 2009-05-05 12:00:50 PDT
FWIW, the tests were disabled because the bot was down, and it was requested that when it was brought up, that we drew a new base line to start from. What do you guys think of openning another bug report to handle the regression?