Last Comment Bug 657952 - minimize methods in nsIPluginHost
: minimize methods in nsIPluginHost
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Josh Aas
:
:
Mentors:
Depends on: 879850 658817
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-18 09:13 PDT by Josh Aas
Modified: 2013-06-09 22:11 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (43.26 KB, patch)
2011-05-18 09:25 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.1 (43.30 KB, patch)
2011-05-18 14:01 PDT, Josh Aas
jst: review+
Details | Diff | Splinter Review

Description Josh Aas 2011-05-18 09:13:23 PDT
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 2011-05-18 09:25:15 PDT
Created attachment 533304 [details] [diff] [review]
fix v1.0
Comment 2 Josh Aas 2011-05-18 14:01:01 PDT
Created attachment 533409 [details] [diff] [review]
fix v1.1

Build fixes.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2011-05-20 11:22:58 PDT
Comment on attachment 533409 [details] [diff] [review]
fix v1.1

r=jst
Comment 4 Josh Aas 2011-05-21 06:31:54 PDT
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/d3d024159f8b
Comment 5 :Irving Reid (No longer working on Firefox) 2013-06-03 12:19:15 PDT
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 [:bsmedberg] 2013-06-03 12:31:23 PDT
Nobody should be mocking pluginhost. We should mark it [builtinclass] so that doesn't work.
Comment 7 :Irving Reid (No longer working on Firefox) 2013-06-04 08:30:22 PDT
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] 2013-06-04 10:46:44 PDT
(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 [:bsmedberg] 2013-06-05 07:05:29 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.