[webkit-reviews] review denied: [Bug 119303] [GTK] Support browser credential storage : [Attachment 215514] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Brian Holt
<brian.holt at samsung.com>'s request for review:
Bug 119303: [GTK] Support browser credential storage
https://bugs.webkit.org/show_bug.cgi?id=119303

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

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


I think the default dialog should honor this setting and not show the option to
save the credentials when this is disabled.
webkit_authentication_request_can_save_credentials() should also return FALSE
when persistent credential storage is enabled, I guess. The problem of this is
that this doesn't allow to use the default dialog but with the user
implementing the credential storage.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1091
> +void ResourceHandle::setUsePersistentCredentialStorage(bool
usePersistentCredentialStorage)
> +{
> +    gUsePersistentCredentialStorage = usePersistentCredentialStorage;
> +}

All other uses of credential storage are GTK specific, so I think this should
be in a #if PLATFORM(GTK) block

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:833
> + * webkit_web_context_get_persistent_storage_enabled:

This is a very generic name, persistent storage doesn't say anything about
HTTPs authentication nor credentials.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:836
> + * Get whether persistent storage is currently enabled.

You should explain what persistent storage is here, and what happens when this
is enabled/disabled. We should also document what the default is.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1407
> +   
webkit_settings_set_enable_private_browsing(webkit_web_view_get_settings(test->
m_webView), FALSE);
> +   
webkit_web_context_set_persistent_storage_enabled(webkit_web_view_get_context(t
est->m_webView), FALSE);

How can you know if it's because of the private browsing or persisten storage
if both are disabled?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1416
> -   
webkit_settings_set_enable_private_browsing(webkit_web_view_get_settings(test->
m_webView), FALSE);
> +   
webkit_web_context_set_persistent_storage_enabled(webkit_web_view_get_context(t
est->m_webView), TRUE);

I think we want to check both, if persistent storage is enabled, but private
browsing disabled, credentials should not be stored permanently.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:1419
>     
g_assert(!webkit_authentication_request_get_proposed_credential(request));

One of the goals of this API is allow the users to implement credential sotring
by themselves. I think we should add a unit tests to check that case it's
actually possible, disabling the persistent credential storage in wk, but
providing your own credentials in the authenticate signal.

> Source/WebKit2/UIProcess/WebContext.cpp:151
> +    , m_usePersistentCredentialStorage(true)

I think this is GTK specific.

> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:147
> +    m_usePersistentCredentialStorage = usePersistentCredentialStorage;
> +   
sendToAllProcesses(Messages::WebProcess::SetUsePersistentCredentialStorage(m_us
ePersistentCredentialStorage));

You should check that the value has actually changed to avoid sending a meesage
to the WebProcess for nothing. We can also avoid the message if webkit has been
compiled without credential storage support (ENABLE(CREDENTIAL_STORAGE))