Bug 73671

Summary: [Qt] When turning off PluginsEnabled attribute then a QWebPage will still try to load plugins
Product: WebKit Reporter: andy.shaw
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, hausmann, jturcotte, pierre.rossi, webkit.review.bot
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch to solve the problem
none
Updated patch based on feedback
jturcotte: review-
Fixed patch
none
Patch with fixed changelog and corrected code none

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.