Bug 32196

Created attachment 44363 [details] Patch

Attachment 44363 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/loader/FrameLoader.cpp:107: Alphabetical sorting problem. [build/include_order] [4] WebCore/loader/FrameLoader.cpp:1263: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/loader/FrameLoader.cpp:1266: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/loader/FrameLoader.cpp:1268: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/platform/MIMETypeRegistry.h:33: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 5

Comment on attachment 44363 [details] Patch This patch should be written in a more platform-independent fashion with fewer #if statements. > +#include <wtf/Platform.h> There's no need to add this include. Platform.h is already included, as you can tell from the use of the #if ENABLE in the file. It's almost never necessary to add new includes of Platform.h. > - if (!settings || !settings->arePluginsEnabled() || > + if (!settings || > +#if PLATFORM(QT) > + // Qt clients 'disable' Qt plugins by not implementing them > + (!settings->arePluginsEnabled() && !MIMETypeRegistry::isQtPluginMIMEType(mimeType)) || > +#else > + !settings->arePluginsEnabled() || > +#endif Instead of calling this isQtPluginMIMEType, it should be isClientPluginMIMEType and should be called on all platforms. On non-Qt ones it can just return false. > +#include <wtf/Platform.h> No need for this. > +#if PLATFORM(QT) > + // Check to see if a mime type is a valid Java applet mime type > + static bool isQtPluginMIMEType(const String& mimeType); > +#endif Comment here is wrong (copied from Java). Then we need versions of isClientPluginMIMEType that return false on all the non-Qt platforms. I'm not sure "client plug-in" is the best name for this concept, so you can use a better one if you can think of it.

(In reply to comment #3) > (From update of attachment 44363 [details]) > This patch should be written in a more platform-independent fashion with fewer > #if statements. > > > Then we need versions of isClientPluginMIMEType that return false on all the > non-Qt platforms. > > I'm not sure "client plug-in" is the best name for this concept, so you can use > a better one if you can think of it. Is 'isNativePluginMIMEType' OK? Not sure about the unit tests to add - should I make the test available to all platforms? Or is it ok to just add it for Qt? My sense is that I need to add it for all platforms but this means changes to nearly all the DRTs to add support for a WebKitPluginsEnabled preference. My related reservation is adding so much code for platforms I can't compile myself - so the patch will only be tested on gtk and qt.

Created attachment 44873 [details] Patch Updated patch. Have only been able to test on qt and gtk.

Attachment 44873 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/loader/FrameLoader.cpp:1264: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/loader/FrameLoader.cpp:1267: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 2

Created attachment 44875 [details] Updated per stylebot

style-queue ran check-webkit-style on attachment 44875 [details] without any errors.

Hmm, this is a slight change in behaviour. Jakub, do you think that would be a problem for Arora? Tor Arne, what do you think?

(In reply to comment #9) > Hmm, this is a slight change in behaviour. Jakub, do you think that would be a > problem for Arora? This change shouldn't have any effect on Arora AFAICS. The only Qt plugin that is implemented in Arora is the substitution for Flash, which would not be affected by this patch.

While I'm here, my suggestions with regard to patch: > +bool MIMETypeRegistry::isNativePluginMIMEType(const String& mimeType) > +{ > + UNUSED_PARAM(mimeType); > + return false; > +} I believe UNUSED_PARAM is supposed to only be used in C code. In C++ you can omit the parameter name in the signature. > index 0000000..05afae4 > --- /dev/null > +++ b/LayoutTests/platform/qt/plugins/native-plugin-plugins-disabled-expected.txt > @@ -0,0 +1,20 @@ > +layer at (0,0) size 800x600 > + RenderView at (0,0) size 800x600 > +layer at (0,0) size 800x600 > + RenderBlock {HTML} at (0,0) size 800x600 > + RenderBody {BODY} at (8,8) size 784x584 > + RenderBlock {DIV} at (0,0) size 784x34 > + RenderText {#text} at (0,14) size 80x20 > + text run at (0,14) width 80: "QT Plugin: " > + RenderPartObject {OBJECT} at (80,0) size 300x30 [QT: geometry: {at (88,8) size 300x30} isHidden: 0 isSelfVisible: 1 isParentVisible: 1 mask: {at (0,0) size 300x30} ] > + RenderText {#text} at (0,0) size 0x0 > + RenderBlock {DIV} at (0,34) size 784x34 > + RenderText {#text} at (0,14) size 133x20 > + text run at (0,14) width 133: "QT Styled Widget: " > + RenderPartObject {OBJECT} at (133,0) size 300x30 [QT: geometry: {at (0,0) size 300x30} isHidden: 1 isSelfVisible: 1 isParentVisible: 1 mask: {at (0,0) size 0x0} ] > + RenderText {#text} at (0,0) size 0x0 > + RenderBlock {DIV} at (0,68) size 784x104 > + RenderText {#text} at (0,84) size 133x20 > + text run at (0,84) width 133: "Third-party plugin: " > + RenderPartObject {EMBED} at (133,0) size 100x100 > + RenderText {#text} at (0,0) size 0x0 I am not entirely sure but I think it should be possible to write a text-only test, which somehow makes use of the JSC<->QObject bindings. Like reading some properties or calling some slot? > + // Check to see if a mime type is a native plugin implemented by the > + // client (e.g. a QT Plugin). > + static bool isNativePluginMIMEType(const String& mimeType); > + > +<!-- qt plugin widget: will be created --> > +<div> > +QT Plugin: > +<object type="application/x-qt-plugin" classid="QProgressBar" name="progressbar1" height=30></object> > +</div> QT -> Qt. :) This can be especially confusing for people more familliar with QuickTime.

Seems fine to me, but as Jakub says, remove the name of the unused param instead, and all references to the toolkit Qt should be upper-case 'Q' and lower-case 't'.

Removed the [Qt] tag as this touches cross-platform code

Comment on attachment 44875 [details] Updated per stylebot I agree with Jakub and Tor Arne. I'm okay with the concept, but I think "isNativePluginMIMEType()" is a bit confusing, as usually "native" refers to "native code" in this context (I would say). Maybe a better way to distinguish between the two cases here is using application provided plugins and system provided plugins?

(In reply to comment #11) > > I am not entirely sure but I think it should be possible to write a text-only > test, which somehow makes use of the JSC<->QObject bindings. Like reading some > properties or calling some slot? > A text-only test would definitely be preferable but I've drawn a blank on implementing it - the test needs to detect the fact that an NPAPI plugin has not been created but a Qt one has been created when pluginsEnabled is false. I can't find any way of detecting that an NPAPI plugin has not been created in text-only mode.

Created attachment 49515 [details] Updated Patch Updated per all comments. As noted before, have not been able to come up with text-only test for this.

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

Comment on attachment 49515 [details] Updated Patch An alternative would be to change the signature of allowPlugins() to allowPlugin(bool, const String& mimeType)

(In reply to comment #18) > (From update of attachment 49515 [details]) > An alternative would be to change the signature of allowPlugins() to > allowPlugin(bool, const String& mimeType) Yes, m_client->allowPlugins() certainly seems to be hanging around waiting for someone to make it do something! Since the decision is mimeType related though, it seems consistent to put it in MIMETypeRegistry just like isJavaAppletMIMEType(). P.S. The patch is a little stale against trunk so I may ask you to rs the rebased version if I have to move things around. Also, I hope Ossy can check my pixel results are near enough to reality on the buildbot.

(In reply to comment #19) > (In reply to comment #18) > > (From update of attachment 49515 [details] [details]) > > An alternative would be to change the signature of allowPlugins() to > > allowPlugin(bool, const String& mimeType) > > Yes, m_client->allowPlugins() certainly seems to be hanging around waiting for > someone to make it do something! Since the decision is mimeType related though, > it seems consistent to put it in MIMETypeRegistry just like > isJavaAppletMIMEType(). Right. No problem with your current approach, was just a thought :) > P.S. The patch is a little stale against trunk so I may ask you to rs the > rebased version if I have to move things around. rs=me

> Also, I hope Ossy can check my pixel results are near enough to reality on the > buildbot. Unfortunately I can't apply your patch, could you rebase it? Hunk #1 FAILED at 1277. 1 out of 1 hunk FAILED -- saving rejects to file WebCore/loader/FrameLoader.cpp.rej Hunk #1 FAILED at 169. 1 out of 1 hunk FAILED -- saving rejects to file WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp.rej Hunk #1 FAILED at 457. 1 out of 1 hunk FAILED -- saving rejects to file WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.cpp.rej

Created attachment 51531 [details] Rebased Patch Doh! Ossy: here you go.

Comment on attachment 51531 [details] Rebased Patch r=me. Mostly based off of hausmann's previous review.

Comment on attachment 51531 [details] Rebased Patch Rejecting patch 51531 from commit-queue. Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1 Last 500 characters of output: ects/CommitQueue/WebCore/platform/mac/DragDataMac.mm -o /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/DragDataMac.o ** BUILD FAILED ** The following build commands failed: WebCore: Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/FrameLoader.o /Users/eseidel/Projects/CommitQueue/WebCore/loader/FrameLoader.cpp normal i386 c++ com.apple.compilers.gcc.4_2 (1 failure) Full output: http://webkit-commit-queue.appspot.com/results/1288030

Please remove the last parenthesis from WebCore/loader/FrameLoader.cpp at line 1281. ;) Otherwise it works for me without any pixel differences.

Comment on attachment 49515 [details] Updated Patch Cleared Simon Hausmann's review+ from obsolete attachment 49515 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 52327 [details] Updated Patch OK, this is the result of my attempts to address the observations of ap and abarth on the mailing list. I've removed the check added to MimeTypeRegistry. I've added a client->isPlugin(mimetype) function that ports can implement to specify that a given mimetype should not be considered a plugin when plugins are disabled. The reasons I have not used client->allowPlugins() or frameloader->allowPlugins() are: - the check should only be made during requestObject(); - allowPlugins() is used elsewhere in WebCore where the mimetype is not available; - allowPlugins() is called in situations where client-implemented plugins such as Qt plugins are not relevant (e.g. DOMImplementation where a file could be opened with a plugin, Page.cpp where it is getting data from the plugin);

+ virtual bool isPlugin(const String&) { return true; } It's not really the case that, by default, every MIME type is a plugin. I still think we want to pass the MIME type information with the allowPlugins() call. That will like clients selectively disable plugins by MIME types. Responding to your points: - the check should only be made during requestObject(); Ok. - allowPlugins() is used elsewhere in WebCore where the mimetype is not available; We could pass something like the null string here to indicate that we're asking about plugins in general and not about a particular plugin. Alternatively, we could have two versions of this call, one that provides a MIME type and one that doesn't. - allowPlugins() is called in situations where client-implemented plugins such as Qt plugins are not relevant (e.g. DOMImplementation where a file could be opened with a plugin, Page.cpp where it is getting data from the plugin); I guess I'm not really sure how a client-implemented plugin differs from a normal plugin. Is the point that you want to selectively disable most plugins but keep some subset (the client-implemented ones) enabled at all times?

Comment on attachment 52327 [details] Updated Patch Removing from review queue while we discuss the design.

(In reply to comment #28) > + virtual bool isPlugin(const String&) { return true; } > > It's not really the case that, by default, every MIME type is a plugin. > > I still think we want to pass the MIME type information with the allowPlugins() > call. That will like clients selectively disable plugins by MIME types. > Responding to your points: > > - the check should only be made during requestObject(); > > Ok. > > - allowPlugins() is used elsewhere in WebCore where the mimetype is not > available; > > We could pass something like the null string here to indicate that we're asking > about plugins in general and not about a particular plugin. Alternatively, we > could have two versions of this call, one that provides a MIME type and one > that doesn't. > Either of those are fine by me, I guess I just wanted to confine my change to the one case where it applied. I also had a vague notion that altering the signature of allowPlugins() to conform to just one use case, and calling it with null strings most of the time, would be frowned upon. > - allowPlugins() is called in situations where client-implemented plugins such > as Qt plugins are not relevant (e.g. DOMImplementation where a file could be > opened with a plugin, Page.cpp where it is getting data from the plugin); > > I guess I'm not really sure how a client-implemented plugin differs from a > normal plugin. Is the point that you want to selectively disable most plugins > but keep some subset (the client-implemented ones) enabled at all times? Yes, exactly. Qt plugins can be as simple as push-buttons or as complex as forms but WebCore just needs to know that they can be created even when third-party plugins are disabled. After that, it can forget about them. (If I'm wrong on this point, hopefully Simon or someone else can correct me.)

> Either of those are fine by me, I guess I just wanted to confine my change to > the one case where it applied. I also had a vague notion that altering the > signature of allowPlugins() to conform to just one use case, and calling it > with null strings most of the time, would be frowned upon. There's also the other use case of wanting to enable Flash on YouTube but disable Flash everywhere else. I think that use case is strong enough to make the allowPlugins API slightly more complicated.

(In reply to comment #30) > > There's also the other use case of wanting to enable Flash on YouTube but > disable Flash everywhere else. I think that use case is strong enough to make > the allowPlugins API slightly more complicated. Possibly just my own ignorance here but I don't think adding the mimetype to allowPlugins() will assist with that. So should I just go ahead and add the mimetype to the allowPlugins() signature and call it with null strings wherever it is unavailable? It looks like that's the way to go..

It'll be nice to have it for Gtk+ widget "plugins" too, thus one would be able to disable all but widget plugins (useful for Evolution mail client, where it's not intended to run Java or Flash in a message preview/composer). Main concern is also about loading of the plugins, because it'll be good to not load those disallowed at all, which may speed up load time (application start), as a side effect. Please think of this as well.

NPAPI Plugin support has been removed from Safari 14 onward (and for JAVA, it was dropped in 12). Further, Webkit Plugin is not support since Safari 5.1 (based on below link). Can this be marked as "RESOLVED WONTFIX"? If I am wrong, please ignore my comment. Thanks! Link - https://developer.apple.com/library/archive/documentation/InternetWeb/Conceptual/WebKit_PluginProgTopic/WebKitPluginTopics.html