657952 - minimize methods in nsIPluginHost

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

nsIPluginHost doesn't need to have so many methods in it any more. We should minimize it to the methods exposed to JS.

Comment 1

[Josh Aas]

Attachment: fix v1.0

Comment 2

[Josh Aas]

Attachment: fix v1.1

Build fixes.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 533409 [details] [diff] [review]
fix v1.1

r=jst

Comment 4

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/d3d024159f8b

Comment 5

[:Irving Reid (No longer working on Firefox)]

I have tracked some test failures in my local build down to this change. Several tests in our suite register a JavaScript implemented test mock for the nsIPluginHost service; this causes failures after the c++ code uses <static_cast> to convert the plugin host service pointer, for example at https://hg.mozilla.org/mozilla-central/annotate/8b1bfcf0ce6e/uriloader/exthandler/nsExternalHelperAppService.cpp#l2602

 nsCOMPtr<nsIPluginHost> pluginHostCOM(do_GetService(MOZ_PLUGIN_HOST_CONTRACTID, &rv));
 nsPluginHost* pluginHost = static_cast<nsPluginHost*>(pluginHostCOM.get());
 if (NS_SUCCEEDED(rv)) {
   if (NS_SUCCEEDED(pluginHost->IsPluginEnabledForExtension(flatExt.get(), mimeType))) { 


According to DXR, tests that mock "@mozilla.org/plugin/host;1" are:

http://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js#l435

http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_pluginchange.js#l121

http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_bug455906.js#l182

http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_bug514327_3.js#l102

http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_bug449027.js#l430

http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_duplicateplugins.js#l96

Comment 6

[Benjamin Smedberg]

Nobody should be mocking pluginhost. We should mark it [builtinclass] so that doesn't work.

Comment 7

[:Irving Reid (No longer working on Firefox)]

Benjamin, why not? Making our services mock-able can make a huge difference to testability.


Dave, you wrote at least one of the tests that mocks pluginhost: https://hg.mozilla.org/mozilla-central/rev/95ece7200b56

Can you see a way to work around this?

Comment 8

[Dave Townsend [:mossop] (PTO until Aug 21st)]

(In reply to :Irving Reid from comment #7)
> Benjamin, why not? Making our services mock-able can make a huge difference
> to testability.
> 
> 
> Dave, you wrote at least one of the tests that mocks pluginhost:
> https://hg.mozilla.org/mozilla-central/rev/95ece7200b56
> 
> Can you see a way to work around this?

Nothing particularly nice, no.

Comment 9

[Benjamin Smedberg]

Pluginhost is not a standalone component; it is tightly bound up with nsObjectLoadingContent/nsPluginInstanceOwner and the nsPluginTag implementation.

I can certainly see us adding a testing mode to blocklist/EM to use an alternate contractID for the pluginhost just in xpcshell tests for those components. That would certainly be more reasonable than trying to replace the real pluginhost.