Convert test_autocomplete_change_after_focus.html to mochitest-plain for e10s coverage

RESOLVED FIXED in Firefox 49

Status

()

Toolkit
Autocomplete
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(e10s+, firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
See lengthy summary.
Flags: qe-verify-
Flags: firefox-backlog+

Updated

2 years ago
Blocks: 984139
tracking-e10s: ? → +
(Assignee)

Updated

2 years ago
Summary: Convert test_autocomplete_change_after_focus.html test_autocomplete_with_composition_on_input.html to mochitest-browser tests for e10s coverage → Convert test_autocomplete_change_after_focus.html to mochitest-browser tests for e10s coverage
(Assignee)

Comment 1

2 years ago
Created attachment 8738132 [details] [diff] [review]
Convert test to a mochitest-browser one
Attachment #8738132 - Flags: review?(MattN+bmo)
(Assignee)

Updated

2 years ago
Component: Form Manager → Password Manager
(Assignee)

Updated

2 years ago
Summary: Convert test_autocomplete_change_after_focus.html to mochitest-browser tests for e10s coverage → Convert test_autocomplete_change_after_focus.html to a mochitest-browser test for e10s coverage
Comment on attachment 8738132 [details] [diff] [review]
Convert test to a mochitest-browser one

Review of attachment 8738132 [details] [diff] [review]:
-----------------------------------------------------------------

By moving this to b-c we lose test coverage outside of Fx (e.g. Thunderbird). It would be simpler and less of a change to just wrap the body of `setup` in an anonymous function passed to loadChromeScript then have handleCompletion send a message back to the child to call runTest.
Attachment #8738132 - Flags: review?(MattN+bmo)
(Assignee)

Comment 3

2 years ago
(In reply to Matthew N. [:MattN] from comment #2)
> By moving this to b-c we lose test coverage outside of Fx (e.g.
> Thunderbird). It would be simpler and less of a change to just wrap the body
> of `setup` in an anonymous function passed to loadChromeScript then have
> handleCompletion send a message back to the child to call runTest.

Sorry for the late reply, but is the goal of this bug now that the test can be run without hitting CPOW usage errors, or am I supposed to make sure that this test can also be run in a remote iframe?
This particular test is not disabled atm, so that part is covered, but how does your instruction above add e10s coverage?
I can only see calling `loadChromeScript` on the global message manager here, but I'm not sure what that would solve.
Is there another example in-tree?
Flags: needinfo?(MattN+bmo)
Apologies for the late reply too.

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> (In reply to Matthew N. [:MattN] from comment #2)
> > By moving this to b-c we lose test coverage outside of Fx (e.g.
> > Thunderbird). It would be simpler and less of a change to just wrap the body
> > of `setup` in an anonymous function passed to loadChromeScript then have
> > handleCompletion send a message back to the child to call runTest.
> 
> Sorry for the late reply, but is the goal of this bug now that the test can
> be run without hitting CPOW usage errors, or am I supposed to make sure that
> this test can also be run in a remote iframe?

Maybe the former but perhaps closer to the latter (I think I'm confused by your choices) but you don't have to deal with iframes yourself as mochitest-plain already runs in a remote browser when running with e10s. The problem with mochitest-chrome is that even when the browser has e10s enabled, the test harness tab is loaded in a non-remote browser. The problem is less about CPOWs for this test and more about not accessing form history from the content process. You need to run the the form history in the parent process and the easiest way to do that from mochitest-plain is with loadChromeScript (see below).

> This particular test is not disabled atm, so that part is covered, but how
> does your instruction above add e10s coverage?

I guess I forgot to explicitly say that it should be moved to mochitest-plain instead. That's how we get e10s coverage.

> I can only see calling `loadChromeScript` on the global message manager
> here, but I'm not sure what that would solve.
> Is there another example in-tree?

See http://hg.mozilla.org/mozilla-central/rev/d823452bea13 where I converted another m-c to m-p. `runInParent` there is a wrapper for `loadChromeScript`: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/pwmgr_common.js?rev=ea109c3f6b3c&mark=313#306
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 5

2 years ago
Created attachment 8742725 [details] [diff] [review]
Patch v2: convert test_autocomplete_change_after_focus.html from m-chrome to m-plain

Matt, thanks so much for your explanation - wouldn't have gotten far without it ;-)
Even though this might seem like a complete rewrite, most of the changes are due to 
1) converting to `add_task`
2) jumping from content - chrome - content.
Attachment #8738132 - Attachment is obsolete: true
Attachment #8742725 - Flags: review?(MattN+bmo)
Comment on attachment 8742725 [details] [diff] [review]
Patch v2: convert test_autocomplete_change_after_focus.html from m-chrome to m-plain

Review of attachment 8742725 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Mike, moving this over to Dolske so maybe I can have time to work on some patches of my own one of these days. I've been getting ~6 reviews/feedback per day…
Attachment #8742725 - Flags: review?(MattN+bmo) → review?(dolske)
Component: Password Manager → Autocomplete
Summary: Convert test_autocomplete_change_after_focus.html to a mochitest-browser test for e10s coverage → Convert test_autocomplete_change_after_focus.html to mochitest-plain for e10s coverage
Comment on attachment 8742725 [details] [diff] [review]
Patch v2: convert test_autocomplete_change_after_focus.html from m-chrome to m-plain

Review of attachment 8742725 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8742725 - Flags: review?(dolske) → review+
(Assignee)

Comment 8

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/9ad67a907cae9644493ecbabae4529602ca3fa72
Bug 1261306 - convert test_autocomplete_change_after_focus.html from m-chrome to m-plain. r=dolske
backed out for test failures like  https://treeherder.mozilla.org/logviewer.html#?job_id=8865042&repo=fx-team
Flags: needinfo?(mdeboer)

Comment 10

2 years ago
Backout:
https://hg.mozilla.org/integration/fx-team/rev/4ac3b4265ce2
(Assignee)

Comment 11

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/7353d8e2cef652857f730e3b9ce39425e388e832
Bug 1261306 - convert test_autocomplete_change_after_focus.html from m-chrome to m-plain. r=dolske
(Assignee)

Updated

2 years ago
Flags: needinfo?(mdeboer)

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7353d8e2cef6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.