[webkit-reviews] review denied: [Bug 95790] Allow third-party storage blocking setting to change while a page is loaded : [Attachment 163710] More tests

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 12 15:29:58 PDT 2012


Brady Eidson <beidson at apple.com> has denied Jeffrey Pfau
<jeffrey at endrift.com>'s request for review:
Bug 95790: Allow third-party storage blocking setting to change while a page is
loaded
https://bugs.webkit.org/show_bug.cgi?id=95790

Attachment 163710: More tests
https://bugs.webkit.org/attachment.cgi?id=163710&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=163710&action=review


The comment about asynchronous init doesn't necessarily need to be resolved
now, but it does need a bug filed about it.

My other comments are more pressing to me.

> Source/WebKit2/PluginProcess/PluginControllerProxy.cpp:573
> +    m_storageBlockingPolicy =
static_cast<SecurityOrigin::StorageBlockingPolicy>(storageBlockingPolicy);
> +
> +    if (!m_isPrivateBrowsingEnabled)
> +	   m_plugin->storageBlockingStateChanged(m_storageBlockingPolicy);
> +}
> +

We only update the storage blocking setting on each plug-in while private
browsing is disabled?

So what if the storage blocking setting changes while private browsing is
enabled, then the user later disables private browsing?  Do the plug-in
instances get told about the storage blocking setting then?

It feels to me like we should always pipe this through.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:959
> +void
NetscapePlugin::storageBlockingStateChanged(SecurityOrigin::StorageBlockingPoli
cy storageBlockingPolicy)
> +{
> +    privateBrowsingStateChanged(storageBlockingPolicy !=
SecurityOrigin::AllowAllStorage);
> +}

I think we should always pass through a call to storageBlockingStateChanged and
the NetscapePlugin remembers both the current private browsing and storage
blocking settings.  Whenever *either* of them changes we calculate whether or
not we need to update the plugin code or not.

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:591
> +void
PluginView::storageBlockingStateChanged(WebCore::SecurityOrigin::StorageBlockin
gPolicy storageBlockingPolicy)
> +{
> +    // The plug-in can be null here if it failed to initialize.
> +    if (!m_isInitialized || !m_plugin)
> +	   return;
> +
> +    m_plugin->storageBlockingStateChanged(storageBlockingPolicy);
> +}

In the brave new world of asynchronous plug-in initialization, we probably need
to store off the new StorageBlockingPolicy value (as well as any new private
browsing settings) so we can update the plug-in when it does actually finish
initializing.


More information about the webkit-reviews mailing list