Bug 73671 - [Qt] When turning off PluginsEnabled attribute then a QWebPage will still try to load plugins
Summary: [Qt] When turning off PluginsEnabled attribute then a QWebPage will still try...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2011-12-02 09:58 PST by andy.shaw
Modified: 2012-12-10 02:13 PST (History)
5 users (show)

See Also:


Attachments
Proposed patch to solve the problem (1.79 KB, patch)
2012-01-09 02:12 PST, andy.shaw
no flags Details | Formatted Diff | Diff
Updated patch based on feedback (1.87 KB, patch)
2012-12-04 23:56 PST, andy.shaw
jturcotte: review-
Details | Formatted Diff | Diff
Fixed patch (1.92 KB, patch)
2012-12-05 03:37 PST, andy.shaw
no flags Details | Formatted Diff | Diff
Patch with fixed changelog and corrected code (1.91 KB, patch)
2012-12-05 03:40 PST, andy.shaw
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description andy.shaw 2011-12-02 09:58:08 PST
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()
Comment 1 andy.shaw 2012-01-09 02:12:01 PST
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 :)
Comment 2 Pierre Rossi 2012-12-04 04:29:13 PST
Comment on attachment 121634 [details]
Proposed patch to solve the problem

LGTM, but I'm not a reviewer ;)
Comment 3 WebKit Review Bot 2012-12-04 04:30:38 PST
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.
Comment 4 Andras Becsi 2012-12-04 04:54:00 PST
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
> +2012-01-09  Shaw Andy  <andy.shaw@digia.com>
> +
> +	Check that plugin functionality is enabled before querying the
> +	database for installed plugins	
> +

Please fix these style errors in the changelog.
Comment 5 Jocelyn Turcotte 2012-12-04 05:56:56 PST
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.
Comment 6 andy.shaw 2012-12-04 23:56:13 PST
Created attachment 177683 [details]
Updated patch based on feedback
Comment 7 Pierre Rossi 2012-12-05 03:32:10 PST
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.
Comment 8 Jocelyn Turcotte 2012-12-05 03:36:34 PST
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.
Comment 9 andy.shaw 2012-12-05 03:37:21 PST
Created attachment 177714 [details]
Fixed patch
Comment 10 andy.shaw 2012-12-05 03:40:34 PST
Created attachment 177716 [details]
Patch with fixed changelog and corrected code
Comment 11 andy.shaw 2012-12-05 03:46:43 PST
Thanks Pierre, Jocelyn for pointing it out, should be all good now
Comment 12 Jocelyn Turcotte 2012-12-05 06:06:39 PST
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.
Comment 13 WebKit Review Bot 2012-12-10 02:13:33 PST
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>
Comment 14 WebKit Review Bot 2012-12-10 02:13:37 PST
All reviewed patches have been landed.  Closing bug.