Closed Bug 1259531 Opened 4 years ago Closed 4 years ago

[e10s] Fix test_bug451286.xul to run in e10s by converting to a mochitest-browser test

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
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)
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)
Attached patch WIP Patch (obsolete) — Splinter Review
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 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+
Attachment #8734556 - Attachment is obsolete: true
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+
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)
https://hg.mozilla.org/mozilla-central/rev/82a40128b81b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.