Closed
Bug 1254296
Opened 8 years ago
Closed 8 years ago
[e10s] Fix browser_CTP_plugins.js to run in e10s
Categories
(Core Graveyard :: Plug-ins, defect)
Core Graveyard
Plug-ins
Tracking
(e10s+, firefox48 fixed)
RESOLVED
FIXED
mozilla48
People
(Reporter: jaws, Assigned: jaws)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
This test passes locally for me, and appears to have been fixed by bug 899347. Pushing to tryserver now to confirm...
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b335466c130d
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38541/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38541/
Attachment #8727604 -
Flags: review?(gfritzsche)
Comment 4•8 years ago
|
||
Comment on attachment 8727604 [details] MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche https://reviewboard.mozilla.org/r/38541/#review35301 Nice, "fixed already" is the best kind of fixed.
Attachment #8727604 -
Flags: review?(gfritzsche) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Comment 5•8 years ago
|
||
The try push has multiple perma-failures. Linux opt M-e10s(bc4) 15:21:32 INFO - 388 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | part8: state menu should have 'Never Activate' selected - Got [object XULElement], expected [object XULElement] 2660 15:21:35 INFO - 390 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | part9: waited too long for plugin to activate - 2665 15:21:35 INFO - 392 INFO TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js | part10: plugin should be activated - Same failures on Linux debug M-e10s(bc1) and OSX 10.10 debug M-e10s(bc1). Passes on Windows opt/debug and OSX opt.
Keywords: checkin-needed
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8727604 [details] MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38541/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8727604 -
Flags: review?(jaws)
Assignee | ||
Updated•8 years ago
|
Attachment #8727604 -
Flags: review?(jaws)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8727604 [details] MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche >https://reviewboard.mozilla.org/r/38541/diff/#index_header
Attachment #8727604 -
Flags: review+ → review?(gfritzsche)
Assignee | ||
Comment 8•8 years ago
|
||
All green on try with 14 retriggers, https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e6e4c845df3
Updated•8 years ago
|
Attachment #8727604 -
Flags: review?(gfritzsche)
Comment 9•8 years ago
|
||
Comment on attachment 8727604 [details] MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche https://reviewboard.mozilla.org/r/38541/#review38129 This overhaul makes things much more readable, thanks. ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:433 (Diff revision 2) > windowClosed(win) { > let domWinClosedPromise = BrowserTestUtils.domWindowClosed(win); > let promises = [domWinClosedPromise]; > let winType = win.document.documentElement.getAttribute("windowtype"); > > + dump(`windowClosed for ${winType}`); Is this intentional or a left-over? ::: toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js:4 (Diff revision 2) > -var gManagerWindow; > -var gTestPluginId; > -var gPluginBrowser; > > function updateBlocklist(aCallback) { > + info("entered updateBlocklist"); Are all the `info()` calls in this file left-overs? ::: toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js:160 (Diff revision 2) > + > + yield BrowserTestUtils.removeTab(pluginTab); > > // causes appDisabled to be set > + managerWindow = yield new Promise(resolve => { > setAndUpdateBlocklist(gHttpTestRoot + "blockPluginHard.xml", is `asyncSetAndUpdateBlocklistasyncSetAndUpdateBlocklist()` potentially needed here? ::: toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js:182 (Diff revision 2) > -function part13() { > + menu = managerWindow.document.getElementById("detail-state-menulist"); > - let menu = gManagerWindow.document.getElementById("detail-state-menulist"); > is(menu.disabled, true, "part13: detail state menu should be disabled"); > > - setAndUpdateBlocklist(gHttpTestRoot + "blockNoPlugins.xml", function() { > - run_next_test(); > + yield new Promise(resolve => { > + setAndUpdateBlocklist(gHttpTestRoot + "blockNoPlugins.xml", resolve); Could we move that to a cleanup function to avoid affecting later tests in case of failures? ::: toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js:186 (Diff revision 2) > - run_next_test(); > + setAndUpdateBlocklist(gHttpTestRoot + "blockNoPlugins.xml", resolve); > }); > -} > > -function end_test() { > - Services.prefs.clearUserPref("plugins.click_to_play"); > + info("part14: about to close the window"); > + //yield BrowserTestUtils.closeWindow(managerWindow); Left-over?
Assignee | ||
Updated•8 years ago
|
Attachment #8727604 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8727604 [details] MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38541/diff/2-3/
Assignee | ||
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/38541/#review38129 > Is this intentional or a left-over? Leftover debugging, as were the info's mentioned below. They've been removed. > is `asyncSetAndUpdateBlocklistasyncSetAndUpdateBlocklist()` potentially needed here? It's not in this directory's head.js, and I don't want to duplicate it there. We could maybe move it to BrowserTestUtils, though I'm also not sure if that is the right place for such as a specific method. I'd prefer to leave this for another bug, but I'm curious as to how you think we should handle the duplication.
Comment 12•8 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11) > > is `asyncSetAndUpdateBlocklistasyncSetAndUpdateBlocklist()` potentially needed here? > > It's not in this directory's head.js, and I don't want to duplicate it > there. We could maybe move it to BrowserTestUtils, though I'm also not sure > if that is the right place for such as a specific method. I'd prefer to > leave this for another bug, but I'm curious as to how you think we should > handle the duplication. BrowserTestUtils sounds like a reasonable place to me; if that one becomes too big we could still break it into more specific modules later (PluginTestUtils, etc.)? I'm happy to leave it for another bug, this could potentially cause intermittent failures later.
Comment 13•8 years ago
|
||
Comment on attachment 8727604 [details] MozReview Request: Bug 1254296 - [e10s] Fix browser_CTP_plugins.js to run in e10s. r?gfritzsche https://reviewboard.mozilla.org/r/38541/#review38727
Attachment #8727604 -
Flags: review?(gfritzsche) → review+
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cdc495ec3550
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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
•