Closed Bug 1456324 Opened 2 years ago Closed 2 years ago

Stop mocking nsIPluginHost manually in tests, use nsIFakePluginTag instead, make nsI*PluginTag builtinclass

Categories

(Core :: Plug-ins, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: Gijs, Assigned: kmag)

References

Details

Attachments

(2 files)

Making some attributes infallible in bug 1456171 failed because we need to make the classes `builtinclass`, which in turn fails because we have tests that mock nsIPluginHost and then fake the plugintags with vanilla JS objects, which upsets xpconnect if you make nsIPluginTag builtinclass.
Assignee: nobody → kmaglione+bmo
Blocks: 1456677
Comment on attachment 8970755 [details]
Bug 1456324: Part 1 - Update tests to use nsIFakePluginTag rather than JS mocks.

https://reviewboard.mozilla.org/r/239504/#review245302

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_appversion.js:285
(Diff revision 1)
> +  ).catch(() => { /* ignore exceptions; the following test will fail anyway. */ });
> +
> +  for (let [i, addon] of ADDONS.entries()) {
>      var blocked = addons[i].blocklistState == Ci.nsIBlocklistService.STATE_BLOCKED;
> -    equal(blocked, ADDONS[i][test],
> +    equal(blocked, addon[test],
>            `Blocklist state should match expected for extension ${addons[i].id}, test ${test}`);

Nit: addon.id

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_plugin_outdated.js:19
(Diff revision 1)
>  gTestserver.registerDirectory("/data/", do_get_file("data"));
>  
> +class MockPlugin extends MockPluginTag {
> +  constructor(opts) {
> +    super(opts, opts.enabledState);
> +    this.blocklisted = opts.blocklisted;

Is there actually a point to this blocklisted = false property in this test (and the next one in this cset)? I don't see one off-hand.
Attachment #8970755 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8970756 [details]
Bug 1456324: Mark infallible nsIPluginTag getters as infallible.

https://reviewboard.mozilla.org/r/239506/#review245306

::: dom/plugins/base/nsPluginHost.cpp:308
(Diff revision 1)
>        int32_t newState = aValue.toInt32();
>        MOZ_ASSERT(newState >= 0 && newState < nsIBlocklistService::STATE_MAX,
>          "Shouldn't get an out of bounds blocklist state");
>  
>        // Check the old and new state and see if there was a change:
> -      uint32_t oldState = nsIBlocklistService::STATE_NOT_BLOCKED;
> +      uint32_t oldState = mTag->GetBlocklistState();

Is it worth filing a good first bug to update other callsites for these properties to use the infallible versions of their getters?
Attachment #8970756 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8970755 [details]
Bug 1456324: Part 1 - Update tests to use nsIFakePluginTag rather than JS mocks.

https://reviewboard.mozilla.org/r/239504/#review245302

> Is there actually a point to this blocklisted = false property in this test (and the next one in this cset)? I don't see one off-hand.

Apparently not. *shrug*
Comment on attachment 8970756 [details]
Bug 1456324: Mark infallible nsIPluginTag getters as infallible.

https://reviewboard.mozilla.org/r/239506/#review245306

> Is it worth filing a good first bug to update other callsites for these properties to use the infallible versions of their getters?

Yeah, I guess it couldn't hurt.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a593e7961a2d230a729bd9060d2c83ea7ae86e9
Bug 1456324: Part 1 - Update tests to use nsIFakePluginTag rather than JS mocks. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcb2bea294230c9dcd3edc67b5728a9ae1d9f75
Bug 1456324: Part 2 - Mark infallible nsIPluginTag getters as infallible. r=Gijs
You need to log in before you can comment on or make changes to this bug.