Bug 73671 – [Qt] When turning off PluginsEnabled attribute then a QWebPage will still try to load plugins

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

When turning off PluginsEnabled attribute then a QWebPage will still try to load plugins.  The following patch to the Browser demo reproduces this when going to youtube.com

--- a/demos/browser/webview.cpp
+++ b/demos/browser/webview.cpp
@@ -70,6 +70,7 @@ WebPage::WebPage(QObject *parent)
     setNetworkAccessManager(BrowserApplication::networkAccessManager());
     connect(this, SIGNAL(unsupportedContent(QNetworkReply*)),
             this, SLOT(handleUnsupportedContent(QNetworkReply*)));
+    settings()->setAttribute(QWebSettings::PluginsEnabled, false);
 }
 
 BrowserMainWindow *WebPage::mainWindow()

Created attachment 121634 [details]
Proposed patch to solve the problem

I couldn't see a means to test this with the LayoutTest so if it is possible then apologies :)

[Pierre Rossi]

Comment on attachment 121634 [details]
Proposed patch to solve the problem

LGTM, but I'm not a reviewer ;)

[WebKit Review Bot]

Attachment 121634 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1
Source/WebKit/qt/ChangeLog:3:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/qt/ChangeLog:4:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/qt/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andras Becsi]

Comment on attachment 121634 [details]
Proposed patch to solve the problem

View in context: https://bugs.webkit.org/attachment.cgi?id=121634&action=review

> Source/WebKit/qt/ChangeLog:5
> [phone]  Shaw Andy  <[e-mail]>
> +
> +	Check that plugin functionality is enabled before querying the
> +	database for installed plugins	
> +

Please fix these style errors in the changelog.

[Jocelyn Turcotte]

Comment on attachment 121634 [details]
Proposed patch to solve the problem

View in context: https://bugs.webkit.org/attachment.cgi?id=121634&action=review

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1412
> +    if (m_frame && m_frame->settings() && m_frame->settings()->arePluginsEnabled() && PluginDatabase::installedPlugins()->isMIMETypeRegistered(mimeType))

It might be worth storing this in a local bool variable to slightly improve readability.
Looks good to me otherwise too.

Created attachment 177683 [details]
Updated patch based on feedback

[Pierre Rossi]

Comment on attachment 177683 [details]
Updated patch based on feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=177683&action=review

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1360
> +    if (m_frame && arePluginsEnabled)

Careful, you broke the previous logic there. The mime type check should stay, and m_frame was already tested as part of arePluginsEnabled.

[Jocelyn Turcotte]

Comment on attachment 177683 [details]
Updated patch based on feedback

View in context: https://bugs.webkit.org/attachment.cgi?id=177683&action=review

> Source/WebKit/qt/ChangeLog:6
> +        database for installed plugins.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=73671

Normally the summary have to be all on the first line, and the bug number on the second:

        Check that plugin functionality is enabled before querying the database for installed plugins.
        https://bugs.webkit.org/show_bug.cgi?id=73671

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1360
> -    if (PluginDatabase::installedPlugins()->isMIMETypeRegistered(mimeType))
> +    if (m_frame && arePluginsEnabled)

I think there is a part missing :)
And no need to check m_frame.

Created attachment 177714 [details]
Fixed patch

Created attachment 177716 [details]
Patch with fixed changelog and corrected code

Thanks Pierre, Jocelyn for pointing it out, should be all good now

[Jocelyn Turcotte]

Comment on attachment 177716 [details]
Patch with fixed changelog and corrected code

r+ed.
Please the cq flag to "?" by pressing "details" on the attachment if you would like us to submit the patch through the commit queue.

[WebKit Review Bot]

Comment on attachment 177716 [details]
Patch with fixed changelog and corrected code

Clearing flags on attachment: 177716

Committed r137122: <http://trac.webkit.org/changeset/137122>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.