Closed
Bug 1259531
Opened 8 years ago
Closed 8 years ago
[e10s] Fix test_bug451286.xul to run in e10s by converting to a mochitest-browser test
Categories
(Toolkit :: Find Toolbar, defect)
Toolkit
Find Toolbar
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: jaws, Assigned: jaws)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
18.34 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•8 years ago
|
||
Why does it need to be a mochitest-browser test? I added a remote browser element just fine in bug 1253233, which I think might be a good start(?) Otherwise, I'd be happy to take this one. In fact, please feel free to assign any e10s findbar tests/ bugs to me.
Flags: needinfo?(jaws)
Assignee | ||
Comment 2•8 years ago
|
||
I'm almost finished with this test, just need to fix one remaining thing with it. But looking in bug 1253233 I noticed you also fixed the RemoteFinder.jsm bug that I fixed. I can upload my WIP to this bug and let you decide what to do.
Flags: needinfo?(jaws)
Assignee | ||
Comment 3•8 years ago
|
||
I'm seeing a timing issue with a hang on the second call to closeFindBarAndWait() in this test on e10s.
Attachment #8734556 -
Flags: feedback?(mdeboer)
Comment 4•8 years ago
|
||
Comment on attachment 8734556 [details] [diff] [review] WIP Patch Review of attachment 8734556 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the late reply! There was a weekend and one day of Easter PTO in between, but still... To me the test looks good; I can't reproduce the timeout you mention, so I wonder what's up with that (I'm using a checkout of todays' fx-team). AFAICT this one is ready for review... don't forget to `hg rm` the chrome test files.
Attachment #8734556 -
Flags: feedback?(mdeboer) → feedback+
Updated•8 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Updated•8 years ago
|
Attachment #8734556 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/43585/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/43585/
Attachment #8736900 -
Flags: review?(mdeboer)
Comment 6•8 years ago
|
||
Comment on attachment 8736900 [details] MozReview Request: Bug 1259531 - [e10s] Fix test_bug451286.xul to run in e10s by converting to a mochitest-browser test. r?mikedeboer https://reviewboard.mozilla.org/r/43585/#review40167 Nicely modernized version of the test! Bonus points if you land this after a green mochitest-bc try run ;-) ::: toolkit/modules/RemoteFinder.jsm:88 (Diff revision 1) > // Don't let one callback throwing stop us calling the rest > try { > l[callback].apply(l, params); > } > catch (e) { > - Cu.reportError(e); > + Cu.reportError(`Missing callback for ${callback}: ` + e); While you're here, could you put the `}` and `catch (e) {` on the same line? I totally agree with making this error state much more descriptive. Well, it was totally unhelpful before :P However, perhaps it's better to say: ```js Cu.reportError(`Missing callback for $(callback}`); Cu.reportError(e); ``` ? That way we also get all the nice stack trace printing. (BTW, I'm in love with the markdown auto-highlighting in this textarea)
Attachment #8736900 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 7•8 years ago
|
||
needinfo for felipe since this test is moving from mochitest-chrome to browser-chrome try push, https://treeherder.mozilla.org/#/jobs?repo=try&revision=b88aeed55ace
Attachment #8736900 -
Attachment is obsolete: true
Flags: needinfo?(felipc)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/82a40128b81b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 10•8 years ago
|
||
The status = fixed that you entered in the mochitest-chrome spreadsheet is what is most important to know (bug number and owner are great too). So if you mark the status as fixed on the spreadsheet I don't need to be needinfo'ed. Thanks for checking, of course :)
Flags: needinfo?(felipc)
You need to log in
before you can comment on or make changes to this bug.
Description
•