<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [SOUP] Simplify custom protocols handler implementation"
href="https://bugs.webkit.org/show_bug.cgi?id=164922#c18">Comment # 18</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [SOUP] Simplify custom protocols handler implementation"
href="https://bugs.webkit.org/show_bug.cgi?id=164922">bug 164922</a>
from <span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=164922#c17">comment #17</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=295159&action=diff" name="attach_295159" title="Patch">attachment 295159</a> <a href="attachment.cgi?id=295159&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=295159&action=review">https://bugs.webkit.org/attachment.cgi?id=295159&action=review</a>
>
> 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.</span >
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.
<span class="quote">> But I approve of landing it as is.</span >
Thank you, I'll try to refactor it after the cleanup, because it will be easier.
<span class="quote">> > 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.</span >
This can be fixed for sure, I'll do it in a follow up.
<span class="quote">> > 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
> <<a href="http://scottmeyers.blogspot.com/2014/07/should-move-only-types-ever-be">http://scottmeyers.blogspot.com/2014/07/should-move-only-types-ever-be</a>-
> passed.html> for one version of the rationale.</span >
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 <a class="bz_bug_link
bz_status_RESOLVED bz_closed"
title="RESOLVED FIXED - REGRESSION: API test _WKDownload.ConvertResponseToDownload is a flaky timeout"
href="show_bug.cgi?id=164631">bug #164631</a>. 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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>