Bug 156974 – Move WebErrors from WebProcess to Shared and get rid of ErrorsGtk in WebCore

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Claudio Saavedra]

The GTK+ and EFL ports have a WebCore/platform layer for errors, which basically wraps around ResourceError and friends. This is then used from the UI, web, etc. processes.

Then there is WebErrors in WebCoreSupport, which also wrap around ResourceError (although in the GTK+ and EFL port these use the above-mentioned errors as an intermediate layer). However, this is only available to the web process.

I would like to unify these into one set of WebErrors, available to processes. I suspect that a reasonable place for this would be Shared/WebErrors.h. This would involve moving the existing WebErrors in WebCoreSupport and merging them with the errors that the EFL and GTK+ ports have in WebCore.

Does this sound OK or am I missing something?

[Claudio Saavedra]

Adding Darin for comments.

[Darin Adler]

I’m not familiar with any of this. I’ll have to read over the code to find out what this is about. Why isn’t this needed for the Cocoa ports? Maybe they use NSError?

[Claudio Saavedra]

(In reply to comment #2)
> I’m not familiar with any of this. I’ll have to read over the code to find
> out what this is about. Why isn’t this needed for the Cocoa ports? Maybe
> they use NSError?

This is needed for Cocoa ports as well, just that the Cocoa ports use the WebErrors in WebCoreSupport, which underneath use NSError when possible or custom ResourceError otherwise. I'd have to move the Cocoa errors to Shared, which is why I'm asking if the plan sounds ok.

[Darin Adler]

Anders, Sam, I think one of you would be the expert on this, right? Do we want to use these objects in both the UI and web processes?

[Carlos Garcia Campos]

Let me explain the situation. In WebCore FrameLoaderClient has pure virtual methods to allow the loader report errors for a given ResourceRequest/Response:

virtual ResourceError cancelledError(const ResourceRequest&) = 0;
virtual ResourceError blockedError(const ResourceRequest&) = 0;
virtual ResourceError blockedByContentBlockerError(const ResourceRequest&) = 0;
virtual ResourceError cannotShowURLError(const ResourceRequest&) = 0;
virtual ResourceError interruptedForPolicyChangeError(const ResourceRequest&) = 0;
virtual ResourceError cannotShowMIMETypeError(const ResourceResponse&) = 0;
virtual ResourceError fileDoesNotExistError(const ResourceResponse&) = 0;
virtual ResourceError pluginWillHandleLoadError(const ResourceResponse&) = 0;

This is implemented by wk1 ports and wk2 in their WebCoreSupport FrameLoaderclient implementation. The WebKit2 implementation uses WebErrors as a wrapper to leave the FrameLoaderClient implementation cross-platform. EFL and GTK+ use yet another abstraction, and the WebError in WebKit2 is just a wrapper around the Errors in WebCore platform. We use those errors in the UI process because we are exposing them in our public API, that's why we have the WebCore abstraction. But the network process is also using those errors, see for example NetworkLoad::cannotShowURL() which uses WebKit::cannotShowURLError(). That's kind of a layering violation, and the reason why we thought they could be moved to Shared. However, now that I have looked in more detail at this, I think we should have a common API in WebCore platform layer and use it everywhere. The only reason to have them in WebCoreSupport was because they were used to implement FrameLoaderClient, but those methods don't really need to be in the FrameLoaderClient. They are just creating ResourceErrors from a ResourceRequest/Response without using anything from the frame nor the frame loader. So, my proposal would be:

 - Remove the error methods from the FrameLoaderclient.
 - Move the common interface to WebCore platform.
 - Use the error methods from WebCore platform everywhere.

[Claudio Saavedra]

Created attachment 279481 [details]
Patch

[Claudio Saavedra]

Comment on attachment 279481 [details]
Patch

I have not done the needed changes to the win port yet, so it will probably break the build. Please review the idea anyway, I don't want to work further on this if it's going in the wrong direction.

[Carlos Garcia Campos]

Comment on attachment 279481 [details]
Patch

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

This looks good to me in general, this is exactly what I thought when I looked in detail at the errors code.

> Source/WebCore/PlatformEfl.cmake:85
> +    platform/Errors.cpp

This should be added to the CMakeList.txt file since all platforms should provide their implementation, no? The methods in frame client were all pure virtual.

> Source/WebCore/platform/unix/ErrorsUnix.h:46
> +// Sync'd with Mac's WebKit Errors.

Why don't we make these enums common to all ports? We are all sharing the same numbers, making them common we don't need to worry about keeping them in sync anymore. We could also avoid having specific header for ErrorsUnix.h

[Darin Adler]

I think the confusion here comes from the original architecture of WebKit. WebCore itself is not supposed to create public API; WebKit adds the public API on top of underpinnings inside WebCore. The one notable exception is the bindings.

So the desire to have common error codes in WebCore that are part of the API of WebKit on all platforms conflicts with this design goal. I am not sure exactly how to achieve this. Perhaps error codes should be another exception.

[Claudio Saavedra]

Created attachment 280439 [details]
Patch

[Claudio Saavedra]

Created attachment 281003 [details]
Patch

[Claudio Saavedra]

Now that this builds in all platforms, any chance to get a review?

[Claudio Saavedra]

Created attachment 285909 [details]
Patch

[Claudio Saavedra]

Created attachment 285910 [details]
Patch

[Claudio Saavedra]

Created attachment 285911 [details]
Patch

[Claudio Saavedra]

Created attachment 285912 [details]
Patch

[Michael Catanzaro]

You got a review here:

(In reply to comment #9)
> I think the confusion here comes from the original architecture of WebKit.
> WebCore itself is not supposed to create public API; WebKit adds the public
> API on top of underpinnings inside WebCore. The one notable exception is the
> bindings.
> 
> So the desire to have common error codes in WebCore that are part of the API
> of WebKit on all platforms conflicts with this design goal. I am not sure
> exactly how to achieve this. Perhaps error codes should be another exception.

Clearly some further discussion is needed on this.

[Sam Weinig]

Comment on attachment 285912 [details]
Patch

I don't see the value of doing this, at least at this time.  It introduces layering violations by making WebCore depend on WebKit concepts (e.g. the webKitErrorDomain).

[Carlos Garcia Campos]

Created attachment 304508 [details]
New patch

This is the opposite approach, instead of unifying errors in WebCore, they are removed from WebCore and unified in WebKit2, but moved to Shared and making it possible to share more code.

[Carlos Garcia Campos]

Patch doesn't apply because it depends on bug #169672

[Michael Catanzaro]

We need an owner to review this please, maybe Darin or Sam?

[Carlos Garcia Campos]

Created attachment 305092 [details]
Rebased patch

[Carlos Garcia Campos]

hhm, I don't know why GTK+ EWS fails to build, it works locally.

[Carlos Garcia Campos]

Created attachment 305093 [details]
Try to fix GTK+ build

[Carlos Garcia Campos]

Ping reviewers.

[Michael Catanzaro]

You need to ping owners specifically.

[Carlos Garcia Campos]

Committed r214934: <http://trac.webkit.org/changeset/214934>