Add support for NPNVprivateModeBool property in plugins. See also: https://developer.mozilla.org/En/Supporting_private_browsing_in_plugins The NPNVprivateModeBool property is supported as scriptable property privateBrowsingEnabled in the test WebKit plugin. The Mac platform also supports a cachedPrivateBrowsingEnabled property implemented in the test plugin. This allows the Layout test plugins/private-browsing-mode.html to retrieve the previous value of NPNVprivateModeBool in the test plugin. Due to the platform-specific overhead required to support this bespoke property it is not implemented as part of this patch, instead a new test, plugins/private-browsing-mode-2.html, is added to ensure that setting and resetting privateBrowsingEnabled works as expected.
Created attachment 45828 [details] Patch
style-queue ran check-webkit-style on attachment 45828 [details] without any errors.
Comment on attachment 45828 [details] Patch LGTM, but it seems that something went wrong with GTK, please have a look at that. Also, where is that successfullyParsed = true; used?
Comment on attachment 45828 [details] Patch > --- a/LayoutTests/platform/gtk/Skipped > +++ b/LayoutTests/platform/gtk/Skipped > @@ -3651,6 +3651,11 @@ plugins/plugin-javascript-access.html > # https://bugs.webkit.org/show_bug.cgi?id=30561 > plugins/private-browsing-mode.html > > +# https://bugs.webkit.org/show_bug.cgi?id=30561 > +plugins/private-browsing-mode.html It seems strange to see these two lines being added that exist alread three lines further up :) > +# https://bugs.webkit.org/show_bug.cgi?id=33180 > +plugins/private-browsing-mode-2.html > + > # Tests generating new results > plugins/embed-attributes-style.html > plugins/netscape-dom-access.html > +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? > + > + case NPNVprivateModeBool: { > + Page* page = m_parentFrame->page(); > + if (!page) > + return NPERR_GENERIC_ERROR; > + *((NPBool*)value) = (!page->settings() || page->settings()->privateBrowsingEnabled())? true : false; > + return NPERR_NO_ERROR; > + } I would leave out the ? true : false; part and the extra parentheses, it's redundant. Otherwise the patch looks great, and certainly long-hanging fruit for the other PluginView implementations, too.
> > > +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?
Created attachment 47448 [details] Updated Patch Fixed style issues. Simon - if you agree with my response to your comments this should be ok to r+. I could look at halting plugins when pluginsEnabled gets set to false separately - or maybe address that first and make it a blocker for this bug.
(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. 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. 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.
Comment on attachment 47448 [details] Updated Patch > + case NPNVprivateModeBool: { > + Page* page = m_parentFrame->page(); > + if (!page) > + return NPERR_GENERIC_ERROR; > + *((NPBool*)value) = !page->settings() || page->settings()->privateBrowsingEnabled(); > + return NPERR_NO_ERROR; > + } r=me, but when landing you may want to change the above to return NPERR_GENERIC_ERROR if !page->settings() and only return the real value to the plugin if we have a settings object.
(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.
(In reply to comment #8) > (From update of attachment 47448 [details]) > > > + case NPNVprivateModeBool: { > > + Page* page = m_parentFrame->page(); > > + if (!page) > > + return NPERR_GENERIC_ERROR; > > + *((NPBool*)value) = !page->settings() || page->settings()->privateBrowsingEnabled(); > > + return NPERR_NO_ERROR; > > + } > > r=me, but when landing you may want to change the above to return > NPERR_GENERIC_ERROR if !page->settings() and only return the real value to the > plugin if we have a settings object. Elsewhere in the code the default behaviour for security-sensitive settings is to assume that if !page->settings() then the setting is true. e.g. in WebCore/loader/DocLoader.cpp: Settings* settings = frame()->settings(); if (!settings || settings->privateBrowsingEnabled()) return;
Manually committed r55358: http://trac.webkit.org/changeset/55358
If this is a Qt-specific font, maybe it should live under platform/qt?
(In reply to comment #12) > If this is a Qt-specific font, maybe it should live under platform/qt? Hi Dimitri, Is this comment on the correct bug? If it's regarding the additions to the skiplists, I've added plugins/private-browsing-mode-2.html to any platform that already skips plugins/private-browsing-mode.html since it likely as not will fail it.
(In reply to comment #13) > (In reply to comment #12) > > If this is a Qt-specific font, maybe it should live under platform/qt? > > Hi Dimitri, > > Is this comment on the correct bug? If it's regarding the additions to the > skiplists, I've added plugins/private-browsing-mode-2.html to any platform that > already skips plugins/private-browsing-mode.html since it likely as not will > fail it. Sorry, comment fail on my part :) What I meant to say is that if this is a Qt-specific test, you could've put it in platform/qt and this way there would've been no need to update Skipped lists for other platforms. Just a suggestion.
I believe the functionality that you described implementing in a second patch was what I just landed as part of bug 35576. Qt should be able to unskip the original private browsing plug-in patch after that change.
Please follow the QtWebKit bug reporting guidelines when reporting bugs. See http://trac.webkit.org/wiki/QtWebKitBugs Specifically: - The 'QtWebKit' component should only be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit http://trac.webkit.org/wiki/QtWebKitBugs#Component - Add the keyword 'Qt' to signal that it's a Qt-related bug http://trac.webkit.org/wiki/QtWebKitBugs#Keywords