[Webkit-unassigned] [Bug 164922] [SOUP] Simplify custom protocols handler implementation

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 21 23:47:28 PST 2016


--- Comment #18 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #17)
> Comment on attachment 295159 [details]
> Patch
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295159&action=review
> There’s s surprising amount of SOUP-specific #if in all these classes. As a
> direction for WebKit2 seems like a step in the wrong direction; would be
> nice to find a more elegant way of doing it.

Yes, I agree, but I would say platform-specific #if, not just soup, in general in custom protocols implementation. I think this is because the way it works and the way it's exposed to the API users is different between soup based ports and Apple ones. For example, in Cocoa custom protocols are always registered globally by WKBrowsingContextController, while soup based ports register the custom protocols per WebProcessPool, so I used ifdefs to save the registered protocols in each WebProcessPool to avoid having them duplicated in Cocoa, but there's nothing soup specific there. In any case, platform ifdefs can be reduced with just a few refactoring I think, I'll check it in more detail.

> But I approve of landing it as is.

Thank you, I'll try to refactor it after the cleanup, because it will be easier.

> > Source/WebKit2/UIProcess/Network/CustomProtocols/CustomProtocolManagerProxy.h:67
> > +#if USE(SOUP)
> > +    void didReceiveResponse(uint64_t customProtocolID, const WebCore::ResourceResponse&);
> > +    void didLoadData(uint64_t customProtocolID, const IPC::DataReference&);
> > +    void didFailWithError(uint64_t customProtocolID, const WebCore::ResourceError&);
> > +    void didFinishLoading(uint64_t customProtocolID);
> > +#endif
> This doesn’t seem quite right to me; I don’t have strong objections to
> landing it, but it seems like the wrong direction long term. The people I
> would ask about alternatives are Sam and Anders.

This can be fixed for sure, I'll do it in a follow up.

> > Source/WebKit2/UIProcess/WebProcessPool.h:140
> > +    void setCustomProtocolManagerClient(std::unique_ptr<API::CustomProtocolManagerClient>);
> A function like this should take unique_ptr&&, not just unique_ptr. See
> <http://scottmeyers.blogspot.com/2014/07/should-move-only-types-ever-be-
> passed.html> for one version of the rationale.

I followed the current pattern of all other clients in WebProcessPool, but I agree anyway, I'll change it. There's another side effect, though, to do it that way that is important to know when doing this, I learned it recently the hard way in bug #164631. When using the sink paramater there's no move constructor for the parameter so that when the value is moved the ownership is transferred, but the moved value is not set to nullptr, you have to use exchange or swap in that particular case if you need to ensure the value is nullptr after the move.

You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161122/90e987ea/attachment-0001.html>

More information about the webkit-unassigned mailing list