minimize methods in nsIPluginHost

RESOLVED FIXED

Status

()

Core
Plug-ins
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

43.30 KB, patch
jst
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
nsIPluginHost doesn't need to have so many methods in it any more. We should minimize it to the methods exposed to JS.
(Assignee)

Comment 1

6 years ago
Created attachment 533304 [details] [diff] [review]
fix v1.0
(Assignee)

Comment 2

6 years ago
Created attachment 533409 [details] [diff] [review]
fix v1.1

Build fixes.
Attachment #533304 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #533409 - Flags: review?(benjamin)
Comment on attachment 533409 [details] [diff] [review]
fix v1.1

r=jst
Attachment #533409 - Flags: review?(benjamin) → review+
(Assignee)

Comment 4

6 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/d3d024159f8b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 658817
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
Flags: needinfo?(joshmoz)
Nobody should be mocking pluginhost. We should mark it [builtinclass] so that doesn't work.
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?
Flags: needinfo?(dtownsend+bugmail)
(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.
Flags: needinfo?(dtownsend+bugmail)
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.
Depends on: 879850
(Assignee)

Updated

4 years ago
Flags: needinfo?(joshmoz)
You need to log in before you can comment on or make changes to this bug.