Bug 137005 – Split ResourceHandle in an abstract class and a network-specific class

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[youenn fablet]

Following on discussions that happened in https://bugs.webkit.org/show_bug.cgi?id=109199, we may want to split ResourceHandle in two classes.
- A first abstract class making the interface with existing loaders through ResourceHandleClient
- A second class (NetworkResourceHandle e.g.) that may derive from the first class to implement the actual loading through CF, soup, curl.
Once done, BlobResourceHandle should be moved to that the first abstract class, as well as data URL handling.

[youenn fablet]

Created attachment 238527 [details]
Patch

[Darin Adler]

I think the lower level class should retain the name ResourceHandle and the higher level class is the one that should have a new name. The discussion in bug 109199 seems to plan for that, and I don’t see any discussion of doing it this way.

[youenn fablet]

(In reply to comment #2)
> I think the lower level class should retain the name ResourceHandle and the higher level class is the one that should have a new name. The discussion in bug 109199 seems to plan for that, and I don’t see any discussion of doing it this way.

The number of edits between the two options seems roughly the same.
But the nature of edits is a bit different.

Introducing a new name for the abstract class requires some limited changes in the HTTP backends but has impact on other WebCore areas, like ResourceHandleClient(s), loaders, download managers.

Renaming ResourceHandle to NetworkResourceHandle has mostly an impact on HTTP backends. The current patch is probably failing to do all the necessary edits for the CURL backend, hence win compilation failure. If there are other bindings elsewhere, it may be disruptive as well.

In terms of style, ResourceHandle is already an abstract name.
It also makes sense for NetworkResourceLoader to instantiate NetworkResourceHandle.
But I am open to try it the other way. Any name suggestion?

[Darin Adler]

Maciej designed and named the ResourceHandle class. Maciej, could you comment?

[Darin Adler]

I guess we should think about whether “resource handle” and “network resource handle” make it clear what the difference is between the two classes.

[youenn fablet]

Created attachment 239204 [details]
Updated CURL binding

[youenn fablet]

Created attachment 239205 [details]
Added missing cf declaration

[youenn fablet]

The current patch introduces a number of fixme that I can carry on.
Moving BlobResourceHandle from NetworkResourceHandle to ResourceHandle is a two lines change that does not break any layout test.

[youenn fablet]

After discussion, it seems to make sense that the networking process handles all URLS, not only HTTP URLs as I initially thought.

This weakens a bit the idea of using NetworkResourceHandle as name for HTTP-specific resource handles as proposed by the current patch.

I still think it makes sense to split ResourceHandle though, so as to nicely handle data/blob urls.

Any thoughts?

[Darin Adler]

Maybe Antti has thoughts on this. He’s been commenting a lot on network process architecture lately.

[Antti Koivisto]

Comment on attachment 239205 [details]
Added missing cf declaration

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

> Source/WebCore/ChangeLog:10
> +
> +        Moved all network specific code from ResourceHandle to NetworkResourceHandle.
> +        This triggers renaming in the network code for all bindings.
> +        ResourceHandleClient API remains unchanged. 

Some "why" would be good.

> Source/WebCore/platform/network/ResourceHandle.h:108
> +    // To get access to start and platform protected routines.

Could you describe what the next steps after this patch would be? It is not entirely clear to me what the end state is going to be. For example this doesn't really make ResourceHandle abstract, there is still functionality here.

> Source/WebCore/platform/network/ResourceHandle.h:109
> +    friend NetworkResourceHandle;

Why friend?

NetworkFoo naming pattern has been used for classes belonging to WK2 network process. It is probably not the right name for what I understand is the platform implementation of an abstract base.

> Source/WebCore/platform/network/ResourceHandle.h:131
> +    virtual void clearAuthentication() { };

Unnecessary ; here and in many other places. 

Wonder why style bot is not complaining?

> Source/WebCore/platform/network/ResourceHandle.h:172
> +    virtual bool isScheduledToFail() {return false;};

Wrong spacing.

> Source/WebCore/platform/network/ResourceHandle.h:175
> +    ResourceHandleClient *m_client;

* placement

[youenn fablet]

Thanks for the review.
Please see inline.

(In reply to comment #11)
> Comment on attachment 239205 [details]
> Added missing cf declaration
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=239205&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +
> > +        Moved all network specific code from ResourceHandle to NetworkResourceHandle.
> > +        This triggers renaming in the network code for all bindings.
> > +        ResourceHandleClient API remains unchanged. 
> 
> Some "why" would be good.

Right, I could add that the target is to have a common class that would be specialized for specific handling: blob, data and network-HTTP would all have their specialized class deriving from this abstract class.

> > Source/WebCore/platform/network/ResourceHandle.h:108
> > +    // To get access to start and platform protected routines.
> 
> Could you describe what the next steps after this patch would be? It is not
> entirely clear to me what the end state is going to be. For example this
> doesn't really make ResourceHandle abstract, there is still functionality
> here.

This patch is a first step towards proper separation.
Considering the amount of code to change due to renaming, I thought it would be best to just minimize the code logic changes.

Additional patches would follow to continue the refactoring, like removing the network-specific methods (see fixme in ResourceHandle.h line 139).

After that, or in parallel, support for data URL would be added, that would solve current issues with CORS and will improve efficiency (at least for the soup backend, that would remove some unnecessary memory allocations/copies).

Another few-lines patch would be to have BlobResourceHandle deriving directly from the abstract ResourceHandle.

Another potential area (not currently in scope of what I plan to do) would be to improve readability of the current ResourceHandle: the support of various HTTP backend makes the code a bit uneasy to read.

Does that plan sound reasonnable to you?

> > Source/WebCore/platform/network/ResourceHandle.h:109
> > +    friend NetworkResourceHandle;
> 
> Why friend?

I do not remember exactly why I added it.
Further patches should anyway remove that relationship.

> NetworkFoo naming pattern has been used for classes belonging to WK2 network
> process. It is probably not the right name for what I understand is the
> platform implementation of an abstract base.

I guess HTTPResourceHandle is probably too specific as well.
What about BackendResourceHandle?

> > Source/WebCore/platform/network/ResourceHandle.h:131
> > +    virtual void clearAuthentication() { };
> 
> Unnecessary ; here and in many other places. 

OK

> Wonder why style bot is not complaining?
> 
> > Source/WebCore/platform/network/ResourceHandle.h:172
> > +    virtual bool isScheduledToFail() {return false;};
> 
> Wrong spacing.

OK

> > Source/WebCore/platform/network/ResourceHandle.h:175
> > +    ResourceHandleClient *m_client;
> 
> * placement

OK

[Antti Koivisto]

So might the plan be something along these lines?

- make ResourceHandle fully abstract
- make BlobResourceHandle not be a ResourceHandle (not sure what the factoring would be?)
- rename ResourceHandle to something more informative (PlatformNetworkConnection?)

[Alexey Proskuryakov]

> - make ResourceHandle fully abstract

Not sure what the benefit of that would be, but also don't see any downside immediately.

> - make BlobResourceHandle not be a ResourceHandle (not sure what the
> factoring would be?)

Subclassing regular ResourceHandle and overriding some of its methods is definitely a horrible design.

> - rename ResourceHandle to something more informative
> (PlatformNetworkConnection?)

Renaming may be appropriate, but it is definitely not a "network connection". This class handles authentication and redirects, which involve multiple connections.

Something like NetworkTask or NetworkLoadingTask may be more appropriate.

[Antti Koivisto]

(In reply to comment #14)
> > - make ResourceHandle fully abstract
> 
> Not sure what the benefit of that would be, but also don't see any downside
> immediately.

Hmm. Currently ResourceHandle

- is virtual (but the only subclass is BlobResourceHandle)
- has abstract client interface (ResourceHandleClient)
- has piles of platform #ifs in the header
- has generic .cpp defining some methods
- has platform specific .cpps defining other methods
- owns ResourceHandleInternal which itself has piles of inlined platform specific #ifs

It is hard to know where to even start.

I suppose the best way to start untangling this would actually be to first make it non-virtual. That is a matter of getting rid of the BlobResourceHandle but this should be done without adding even more abstraction. Blob loading would then move to higher level (somewhere around ResourceLoader).

[youenn fablet]

(In reply to comment #15)
> (In reply to comment #14)
> > > - make ResourceHandle fully abstract
> > 
> > Not sure what the benefit of that would be, but also don't see any downside
> > immediately.
> 
> Hmm. Currently ResourceHandle
> 
> - is virtual (but the only subclass is BlobResourceHandle)
> - has abstract client interface (ResourceHandleClient)
> - has piles of platform #ifs in the header
> - has generic .cpp defining some methods
> - has platform specific .cpps defining other methods
> - owns ResourceHandleInternal which itself has piles of inlined platform
> specific #ifs
> 
> It is hard to know where to even start.
> 
> I suppose the best way to start untangling this would actually be to first
> make it non-virtual. That is a matter of getting rid of the
> BlobResourceHandle but this should be done without adding even more
> abstraction. Blob loading would then move to higher level (somewhere around
> ResourceLoader).

Yes, having an abstract ResourceHandle would allow to split platform specific code into subclasses and make things much cleaner.

Starting with data URLs is probably simpler and would validate how loading should happen when handled within WebProcess/ResourceLoader.

What I am not clear is whether we want to remove ResourceLoader::m_handle or not in the long term.

If so, handling of data/blob URL loading should be handled by explicit data/blob function call within Web/ResourceLoadScheduler (more or less following archive resource loading).

If not, handling of data/blob URL could happen by instantiating within WebProcess a subclass of ResourceHandle specialized for data/blob URLs.
WebResourceLoadScheduler would then only need to start the resourceloader for data/blob URLs. AIUI, this is close to how QuickLook URLs are loaded.