[Webkit-unassigned] [Bug 33180] [Qt] Support private browsing mode in plugins

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Robert Hogan <robert at roberthogan.net>  2010-02-17 11:32:17 PST ---
(In reply to comment #7)
> (In reply to comment #5)
> > > 
> > > > +Tests that NPNVprivateModeBool is supported by the WebKit plugin view. This test is for WebKit platforms that wish to support NPNVprivateModeBool but do not wish to implement the preference change listener required to support a cachedPrivateBrowsingEnabled property similar to the one provided by Safari and tested for in private-browsing-mode.html
> > > 
> > > I don't understand this part. It seems trivial to implement in
> > > DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp. Why not do it
> > > there instead of this extra test?
> > > 
> > > In fact, it seems like a bug right now that the cachedPrivateBrowsingEnabled
> > > variable isn't initialized in that file, where as it is initialized in
> > > TestNetscapePlugIn.subproj/main.cpp. Could that be why the test was failing for
> > > you?
> > 
> > The mac port listens for changes to the users webkit preferences and halts any
> > netscape plugins currently running if pluginsEnabled has changed to false.
> > Setting the 'cachedPrivateBrowsingMode' value rides on the coat-tails of this
> > feature - which Qt doesn't have, and possibly should. I don't think it's
> > trivial to implement though. So will leave it out of this patch if that's ok?
> 
> I think I understand now, but I don't see that the mac port stops the plugin.

I came to the conclusion from reading in WebBaseNetscapePluginView.mm:

- (void)preferencesHaveChanged:(NSNotification *)notification
{
    WebPreferences *preferences = [[self webView] preferences];

    if ([notification object] != preferences)
        return;

    BOOL arePlugInsEnabled = [preferences arePlugInsEnabled];
    if (_isStarted != arePlugInsEnabled) {
        if (arePlugInsEnabled) {
            if ([self currentWindow]) {
                [self start];
            }
        } else {
            [self stop];
            [self invalidatePluginContentRect:[self bounds]];
        }
    }

    BOOL isPrivateBrowsingEnabled = [preferences privateBrowsingEnabled];
    if (isPrivateBrowsingEnabled != _isPrivateBrowsingEnabled) {
        _isPrivateBrowsingEnabled = isPrivateBrowsingEnabled;
        [self privateBrowsingModeDidChange];
    }
}


> Instead looking at - (void)privateBrowsingModeDidChange in
> WebNetscapePluginView.mm I can see that the private browsing mode value is
> _propagated_ to the plugin (setvalue), and _that_ is what the other part of the
> private-browsing-mode.html tests.
> 
> Your patch implements the part that allows the plugin to query the browser if
> private browsing is enabled or not, and your added test verifies that.
> 
> The Mac port also implements _notifying_ the plugin if the private browsing
> mode changes, which appears to be an important feature to have.
> 

Ah OK, it actually sets the value of NPNVprivateModeBool there and then:

- (void)privateBrowsingModeDidChange
{
    if (!_isStarted)
        return;

    NPBool value = _isPrivateBrowsingEnabled;

    [self willCallPlugInFunction];
    {
        JSC::JSLock::DropAllLocks dropAllLocks(JSC::SilenceAssertionsOnly);
        if ([_pluginPackage.get() pluginFuncs]->setvalue)
            [_pluginPackage.get() pluginFuncs]->setvalue(plugin,
NPNVprivateModeBool, &value);
    }
    [self didCallPlugInFunction];
}

I was under the misapprehension that any time a plugin wants to check the value
of NPNVprivateModeBool it would have to perform a getvalue() but it will
actually receive the setvalue event from the browser and act appropriately. Got
it now.

> I agree with you that this is okay to implement in a second patch. I guess once
> the setting in WebCore changes, the page should iterate through its plugins and
> notify them via setvalue calls.

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