Bug 165028 – Reduce platform ifdefs in WebKit2 custom protocols implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

There are several ifdefs for soup and cocoa that could be avoided with just some refactorings.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 299123 [details]
WIP with xcode changes

[Carlos Garcia Campos]

Created attachment 299125 [details]
WIP

[Carlos Garcia Campos]

Created attachment 299126 [details]
WIP

[Carlos Garcia Campos]

Created attachment 299129 [details]
WIP

[Carlos Garcia Campos]

Created attachment 299130 [details]
WIP

[Carlos Garcia Campos]

Created attachment 299133 [details]
Patch

[Carlos Garcia Campos]

Ping reviewers :-)

[Darin Adler]

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&.

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

(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.

[Carlos Garcia Campos]

Created attachment 302130 [details]
Rebased patch for landing

[WebKit Commit Bot]

Comment on attachment 302130 [details]
Rebased patch for landing

Clearing flags on attachment: 302130

Committed r212632: <http://trac.webkit.org/changeset/212632>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Carlos Garcia Campos]

(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.

[Ryan Haddad]

Reverted r212632 for reason:

This change appears to have caused API test failures.

Committed r212655: <http://trac.webkit.org/changeset/212655>

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 302262 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 302262 [details]
Patch for landing

Clearing flags on attachment: 302262

Committed r212724: <http://trac.webkit.org/changeset/212724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.