Bug 165028

Summary: Reduce platform ifdefs in WebKit2 custom protocols implementation
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, andersca, commit-queue, darin, mcatanzaro, ryanhaddad, sam
Priority: P2 Keywords: Gtk
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=164922
Attachments:
Description Flags
WIP patch
none
WIP with xcode changes
none
WIP
none
WIP
none
WIP
none
WIP
none
Patch
darin: review+
Rebased patch for landing
none
Patch for landing none

Description Carlos Garcia Campos 2016-11-22 01:47:20 PST
There are several ifdefs for soup and cocoa that could be avoided with just some refactorings.
Comment 1 Carlos Garcia Campos 2016-11-22 01:52:09 PST
Created attachment 295332 [details]
WIP patch

This patch adds CustomProtocolManagerProxy.cpp that is cross-platform and adds an API::CustomProtocolManagerClient implementation for cocoa. As usual, I need help with the Xcode changes, I could have removed the deleted files myself, but since there are new files someone with a Mac will have to edit the xcode file in any case.
Comment 2 Carlos Garcia Campos 2017-01-18 01:11:52 PST
Created attachment 299123 [details]
WIP with xcode changes
Comment 3 Carlos Garcia Campos 2017-01-18 01:37:39 PST
Created attachment 299125 [details]
WIP
Comment 4 Carlos Garcia Campos 2017-01-18 02:14:16 PST
Created attachment 299126 [details]
WIP
Comment 5 Carlos Garcia Campos 2017-01-18 02:30:10 PST
Created attachment 299129 [details]
WIP
Comment 6 Carlos Garcia Campos 2017-01-18 02:59:59 PST
Created attachment 299130 [details]
WIP
Comment 7 Carlos Garcia Campos 2017-01-18 05:03:30 PST
Created attachment 299133 [details]
Patch
Comment 8 Carlos Garcia Campos 2017-02-13 23:50:06 PST
Ping reviewers :-)
Comment 9 Darin Adler 2017-02-14 10:53:05 PST
Comment on attachment 299133 [details]
Patch

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

> Source/WebKit2/UIProcess/Cocoa/CustomProtocolManagerClient.h:46
> +    void startLoading(CustomProtocolManagerProxy&, uint64_t /*customProtocolID*/, const WebCore::ResourceRequest&) override;
> +    void stopLoading(CustomProtocolManagerProxy&, uint64_t /*customProtocolID*/) override;
> +    void invalidate(CustomProtocolManagerProxy&) override;

WebKit coding style says to use final, not override.

> Source/WebKit2/UIProcess/Cocoa/CustomProtocolManagerClient.mmSource/WebKit2/UIProcess/Network/CustomProtocols/mac/CustomProtocolManagerProxyMac.mm:143
> +    // FIXME: iterate the loader map and remove loaders for the given manager.

Why is it safe to leave this unimplemented?

> Source/WebKit2/UIProcess/WebProcessPool.cpp:371
> +    for (const auto& scheme : globalURLSchemesWithCustomProtocolHandlers())
> +        parameters.urlSchemesRegisteredForCustomProtocols.append(scheme);
> +
> +    for (const auto& scheme : m_urlSchemesRegisteredForCustomProtocols)
> +        parameters.urlSchemesRegisteredForCustomProtocols.append(scheme);

I suggest just auto& rather than const auto&.
Comment 10 Carlos Garcia Campos 2017-02-15 08:40:55 PST
(In reply to comment #9)
> Comment on attachment 299133 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299133&action=review

Thanks for the review!

> > Source/WebKit2/UIProcess/Cocoa/CustomProtocolManagerClient.h:46
> > +    void startLoading(CustomProtocolManagerProxy&, uint64_t /*customProtocolID*/, const WebCore::ResourceRequest&) override;
> > +    void stopLoading(CustomProtocolManagerProxy&, uint64_t /*customProtocolID*/) override;
> > +    void invalidate(CustomProtocolManagerProxy&) override;
> 
> WebKit coding style says to use final, not override.

Even for a class already marked as final?
 
> > Source/WebKit2/UIProcess/Cocoa/CustomProtocolManagerClient.mmSource/WebKit2/UIProcess/Network/CustomProtocols/mac/CustomProtocolManagerProxyMac.mm:143
> > +    // FIXME: iterate the loader map and remove loaders for the given manager.
> 
> Why is it safe to leave this unimplemented?

Because it's a refactoring, this is already unimplemented with current code. I was afraid of breaking something trying to implement this, since I'm not familiar with how custom protocols work in Mac and I don't have a way to test it. So, I think it's better if someone who knows this code can implement this afterwards.

> > Source/WebKit2/UIProcess/WebProcessPool.cpp:371
> > +    for (const auto& scheme : globalURLSchemesWithCustomProtocolHandlers())
> > +        parameters.urlSchemesRegisteredForCustomProtocols.append(scheme);
> > +
> > +    for (const auto& scheme : m_urlSchemesRegisteredForCustomProtocols)
> > +        parameters.urlSchemesRegisteredForCustomProtocols.append(scheme);
> 
> I suggest just auto& rather than const auto&.

Ok.
Comment 11 Michael Catanzaro 2017-02-15 09:11:38 PST
(In reply to comment #10)
> > WebKit coding style says to use final, not override.
> 
> Even for a class already marked as final?

I strongly prefer override to final because it's much more descriptive and it doesn't seem particularly useful to prohibit overriding particular methods. But yes, we discussed it on webkit-dev a while back and IIRC agreed to use final in all cases except where the function needs to be overridden a second time, for better or for worse. We've been ignoring that in our GTK+ patches but should probably start using it.
Comment 12 Carlos Garcia Campos 2017-02-20 02:48:53 PST
Created attachment 302130 [details]
Rebased patch for landing
Comment 13 WebKit Commit Bot 2017-02-20 04:29:45 PST
Comment on attachment 302130 [details]
Rebased patch for landing

Clearing flags on attachment: 302130

Committed r212632: <http://trac.webkit.org/changeset/212632>
Comment 14 WebKit Commit Bot 2017-02-20 04:29:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Ryan Haddad 2017-02-20 10:09:52 PST
This change appears to have caused 21 API test failures/timeouts:

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/3724
Comment 16 Carlos Garcia Campos 2017-02-20 10:14:58 PST
(In reply to comment #15)
> This change appears to have caused 21 API test failures/timeouts:
> 
> https://build.webkit.org/builders/
> Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/3724

hmm, I'm sorry, I'm surprised it causes failures in tests other than the custom protocols one, that is failing too. Anyway, feel free to roll it out, and I'll try to find time to look at it again in more detail.
Comment 17 Ryan Haddad 2017-02-20 10:18:34 PST
Reverted r212632 for reason:

This change appears to have caused API test failures.

Committed r212655: <http://trac.webkit.org/changeset/212655>
Comment 18 Carlos Garcia Campos 2017-02-21 08:24:27 PST
Ok, found the issue, git rebase messed it up, adding setCustomProtocolManagerClient() call to WebProcessPool::updateProcessSuppressionState() instead of WebProcessPool::platformInitialize() :-/ I'll submit a new patch for landing.
Comment 19 Carlos Garcia Campos 2017-02-21 08:28:03 PST
Created attachment 302262 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2017-02-21 10:26:37 PST
Comment on attachment 302262 [details]
Patch for landing

Clearing flags on attachment: 302262

Committed r212724: <http://trac.webkit.org/changeset/212724>
Comment 21 WebKit Commit Bot 2017-02-21 10:26:42 PST
All reviewed patches have been landed.  Closing bug.