[webkit-reviews] review denied: [Bug 103729] [GTK] Custom URI schemes stop working on Epiphany using WebKit2 after killing the web process : [Attachment 177001] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Joaquim Rocha
<jrocha at igalia.com>'s request for review:
Bug 103729: [GTK] Custom URI schemes stop working on Epiphany using WebKit2
after killing the web process
https://bugs.webkit.org/show_bug.cgi?id=103729

Attachment 177001: Patch
https://bugs.webkit.org/attachment.cgi?id=177001&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=177001&action=review


Thanks for the patch, it looks good, I've just a few comments.

> Source/WebKit2/ChangeLog:3
> +	   Fixes the use of URI schemes when using libsoup

This is not accurate, the bug title probably express better the problem so I
would just skip this line.

> Source/WebKit2/ChangeLog:9
> +	   When a URI scheme is registered and the WebProcess is killed,
> +	   those schemes would not work anymore after the process is
relaunched.
> +
> +	   This was observed in Epiphany and possibly affects any port that
> +	   uses libsoup.

All this text should go after the Reviewed by line, replacing the Additional
information line.

> Source/WebKit2/ChangeLog:34
> +	   Additional information of the change such as approach, rationale.
Please add per-function descriptions below (OOPS!).
> +
> +	   * Shared/WebProcessCreationParameters.cpp:
> +	   (WebKit::WebProcessCreationParameters::encode):
> +	   (WebKit::WebProcessCreationParameters::decode):
> +	   * Shared/WebProcessCreationParameters.h:
> +	   (WebProcessCreationParameters):
> +	   * UIProcess/WebContext.cpp:
> +	   (WebKit::WebContext::createNewWebProcess):
> +	   * UIProcess/soup/WebSoupRequestManagerProxy.cpp:
> +	   (WebKit::WebSoupRequestManagerProxy::registerURIScheme):
> +	   (WebKit::WebSoupRequestManagerProxy::getRegisteredURISchemes):
> +	   (WebKit):
> +	   * UIProcess/soup/WebSoupRequestManagerProxy.h:
> +	   (WebSoupRequestManagerProxy):
> +	   * WebProcess/WebProcess.cpp:
> +	   (WebKit::WebProcess::initializeWebProcess):
> +	   * WebProcess/soup/WebSoupRequestManager.h:
> +	   (WebSoupRequestManager):

As the comment says, please add per-function descriptions briefly explaining
the changes.

> Source/WebKit2/Shared/WebProcessCreationParameters.h:79
> +    Vector<String> urlSchemesRegisteredForSoup;

I would remove the ForSoup suffix.

> Source/WebKit2/UIProcess/WebContext.cpp:405
> +#if USE(SOUP)
> +    const Vector<String> &registeredUriSchemes =
m_soupRequestManagerProxy->getRegisteredURISchemes();
> +    for (Vector<String>::const_iterator it = registeredUriSchemes.begin();
it != registeredUriSchemes.end(); it++)
> +	   parameters.urlSchemesRegisteredForSoup.append(*it);
> +#endif

Instead of using an #ifdef move this code to
WebContext::platformInitializeWebProcess() in
Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp. We should do the same for EFL
in Source/WebKit2/UIProcess/efl/WebContextEfl.cpp
Can't you do a simple assignment instead of iterating the vector? 

parameters.urlSchemesRegistered =
m_soupRequestManagerProxy->getRegisteredURISchemes();

> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:64
> +    m_registeredURISchemes.append(scheme);

The API layer should make sure the same scheme is not registered twice, now
that we have a vector with the list of schemes, we could add an ASSERT to check
the scheme is not already in the Vector.

> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:97
> +Vector<String>& WebSoupRequestManagerProxy::getRegisteredURISchemes()
> +{
> +    return m_registeredURISchemes;
> +}

I think it would be better to return const Vector<String>& and the function
should be const too. The name of the function should be registeredURISchemes(),
see http://www.webkit.org/coding/coding-style.html#names-setter-getter. Also
for simple getters like this, we typically implement them in the header.

> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.h:68
> +

Remove this extra line

> Source/WebKit2/WebProcess/WebProcess.cpp:288
> +#if USE(SOUP)
> +    for (Vector<String>::const_iterator it =
parameters.urlSchemesRegisteredForSoup.begin(); it !=
parameters.urlSchemesRegisteredForSoup.end(); it++)
> +	   m_soupRequestManager.registerURIScheme(*it);
> +#endif

Instead of using #ifdef, move the code to
WebProcess::platformInitializeWebProcess() in
Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp. 
I think the vector iteration is easier to read as 

for (size_t i = 0; i < parameters.urlSchemesRegistered.size(); ++i)
    m_soupRequestManager.registerURIScheme(parameters.urlSchemesRegistered[i]);