[Webkit-unassigned] [Bug 103729] [GTK] Custom URI schemes stop working on Epiphany using WebKit2 after killing the web process

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=103729


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #177001|review?                     |review-
               Flag|                            |




--- Comment #2 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-12-01 02:23:26 PST ---
(From update of attachment 177001)
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]);

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.