Bug 156974 - Move WebErrors from WebProcess to Shared and get rid of ErrorsGtk in WebCore
Summary: Move WebErrors from WebProcess to Shared and get rid of ErrorsGtk in WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
URL:
Keywords:
Depends on: 169672
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-25 05:56 PDT by Claudio Saavedra
Modified: 2017-04-05 00:42 PDT (History)
5 users (show)

See Also:


Attachments
Patch (118.45 KB, patch)
2016-05-20 06:26 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (129.96 KB, patch)
2016-06-03 05:11 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (130.34 KB, patch)
2016-06-10 02:24 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (129.31 KB, patch)
2016-08-12 04:48 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (129.37 KB, patch)
2016-08-12 05:07 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (129.81 KB, patch)
2016-08-12 05:23 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (130.07 KB, patch)
2016-08-12 05:49 PDT, Claudio Saavedra
sam: review-
Details | Formatted Diff | Diff
New patch (65.05 KB, patch)
2017-03-15 09:58 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (64.96 KB, patch)
2017-03-22 03:00 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix GTK+ build (64.97 KB, patch)
2017-03-22 03:25 PDT, Carlos Garcia Campos
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2016-04-25 05:56:02 PDT
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?
Comment 1 Claudio Saavedra 2016-04-25 05:57:40 PDT
Adding Darin for comments.
Comment 2 Darin Adler 2016-04-25 15:50:00 PDT
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?
Comment 3 Claudio Saavedra 2016-04-26 00:18:55 PDT
(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.
Comment 4 Darin Adler 2016-04-26 08:30:40 PDT
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?
Comment 5 Carlos Garcia Campos 2016-04-28 02:26:57 PDT
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.
Comment 6 Claudio Saavedra 2016-05-20 06:26:21 PDT
Created attachment 279481 [details]
Patch
Comment 7 Claudio Saavedra 2016-05-20 07:25:12 PDT
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.
Comment 8 Carlos Garcia Campos 2016-05-24 02:27:08 PDT
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
Comment 9 Darin Adler 2016-05-24 08:42:41 PDT
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.
Comment 10 Claudio Saavedra 2016-06-03 05:11:02 PDT
Created attachment 280439 [details]
Patch
Comment 11 Claudio Saavedra 2016-06-10 02:24:04 PDT
Created attachment 281003 [details]
Patch
Comment 12 Claudio Saavedra 2016-06-20 05:03:22 PDT
Now that this builds in all platforms, any chance to get a review?
Comment 13 Claudio Saavedra 2016-08-12 04:48:51 PDT
Created attachment 285909 [details]
Patch
Comment 14 Claudio Saavedra 2016-08-12 05:07:24 PDT
Created attachment 285910 [details]
Patch
Comment 15 Claudio Saavedra 2016-08-12 05:23:46 PDT
Created attachment 285911 [details]
Patch
Comment 16 Claudio Saavedra 2016-08-12 05:49:55 PDT
Created attachment 285912 [details]
Patch
Comment 17 Michael Catanzaro 2016-08-15 01:57:08 PDT
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.
Comment 18 Sam Weinig 2016-09-05 20:25:24 PDT
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).
Comment 19 Carlos Garcia Campos 2017-03-15 09:58:44 PDT
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.
Comment 20 Carlos Garcia Campos 2017-03-15 10:00:32 PDT
Patch doesn't apply because it depends on bug #169672
Comment 21 Michael Catanzaro 2017-03-15 11:34:45 PDT
We need an owner to review this please, maybe Darin or Sam?
Comment 22 Carlos Garcia Campos 2017-03-22 03:00:48 PDT
Created attachment 305092 [details]
Rebased patch
Comment 23 Carlos Garcia Campos 2017-03-22 03:21:07 PDT
hhm, I don't know why GTK+ EWS fails to build, it works locally.
Comment 24 Carlos Garcia Campos 2017-03-22 03:25:13 PDT
Created attachment 305093 [details]
Try to fix GTK+ build
Comment 25 Carlos Garcia Campos 2017-04-02 22:38:07 PDT
Ping reviewers.
Comment 26 Michael Catanzaro 2017-04-03 06:57:52 PDT
You need to ping owners specifically.
Comment 27 Carlos Garcia Campos 2017-04-05 00:42:04 PDT
Committed r214934: <http://trac.webkit.org/changeset/214934>