Closed
Bug 1456324
Opened 6 years ago
Closed 6 years ago
Stop mocking nsIPluginHost manually in tests, use nsIFakePluginTag instead, make nsI*PluginTag builtinclass
Categories
(Core Graveyard :: Plug-ins, enhancement)
Core Graveyard
Plug-ins
Tracking
(firefox61 fixed)
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 | ||
Updated•6 years ago
|
Assignee: nobody → kmaglione+bmo
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•6 years ago
|
||
mozreview-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 ::: 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+
Reporter | ||
Comment 4•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
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*
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 7•6 years ago
|
||
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
Assignee | ||
Comment 8•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b66db25a4e39f94a90c8161187466b61a7a2d03 Bug 1456324: Follow-up: Fix assertion in debug builds. r=bustage
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3a593e7961a2 https://hg.mozilla.org/mozilla-central/rev/cbcb2bea2942 https://hg.mozilla.org/mozilla-central/rev/9b66db25a4e3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•