Description
Luca Bruno
2008-04-07 09:30:09 PDT
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. Created attachment 29094 [details] Cleaned up patch from Luca Bruno in bug #18297 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. Created attachment 29216 [details]
Updated patch to include new files.
Forgot to add new files.
(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 *** Bug 24255 has been marked as a duplicate of this bug. *** 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. 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.
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.
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 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 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 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? Clearing review flag.. The patches need updating. Created attachment 29751 [details]
[1/3] Add API to load content for unreachable URL - v2
Updated patch based on kov's feedback.
Created attachment 29752 [details]
[2/3] Add default page for error reporting - v2
Updated based on kov's feedback.
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 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 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 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! (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? (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. Created attachment 29769 [details]
[1/3] Add API to load content for unreachable URL - v3
Updated per xan's feedback.
Created attachment 29770 [details]
[2/3] Add default page for error reporting - v3
Updated per xan's feedback
Created attachment 29771 [details]
[3/3] implement resource error-using methods - v2
Updated per kov's and xan's feedback.
The patches look good to me. I'll let Xan r+, though, after he sees the updates. (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. > > > + 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 on attachment 29771 [details]
[3/3] implement resource error-using methods - v2
Looks good to me.
(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? (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/. 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 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 on attachment 29771 [details]
[3/3] implement resource error-using methods - v2
r=me, but let's fix the 0/NULL mess ASAP.
Comment on attachment 29770 [details]
[2/3] Add default page for error reporting - v3
Er, I meant to review this one.
Comment on attachment 29783 [details]
[1/3] Add API to load content for unreachable URL - v4
r=me losing the G_SIGNAL_ACTION.
(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! (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 (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()). (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 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? |