1459299 - Add pref setting methods to GeckoSessionTestRule

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P2)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Add pref setting methods so tests can change specific Gecko behavior.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1459299 - 1. Add evaluateChromeJS for chrome privilege code;

Add GeckoSessionTestRule.evaluateChromeJS for JS code that requires
chrome privileges, such as setting prefs.

Review commit: https://reviewboard.mozilla.org/r/242060/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242060/

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1459299 - 2. Add pref setting methods for tests;

Add GeckoSessionTestRule.setPrefsUntilTestEnd and
GeckoSessionTestRule.setPrefsDuringNextWait so tests can easily set
prefs to get specific behavior.

Review commit: https://reviewboard.mozilla.org/r/242062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242062/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1459299 - 3. Clear parent process ChromeActor on window closing;

To represent the parent process, we use a ChromeActor/TabActor that
targets a random chrome window. However, if that chrome window later
closes, the ChromeActor will stop working. In that case, we should clear
the cached ChromeActor and create a new one that targets a different
chrome window.

Review commit: https://reviewboard.mozilla.org/r/242064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242064/

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8973690 [details]
Bug 1459299 - 3. Clear parent process ChromeActor on window closing;

https://reviewboard.mozilla.org/r/242064/#review248050

Thanks, this looks reasonable to me! :)

Ideally we will improve this code at some stage so that the ChromeActor here isn't handled as a specialized one-off case like this, but this seems good as an incremental step.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8973688 [details]
Bug 1459299 - 1. Add evaluateChromeJS for chrome privilege code;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242060/diff/1-2/

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8973689 [details]
Bug 1459299 - 2. Add pref setting methods for tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242062/diff/1-2/

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8973690 [details]
Bug 1459299 - 3. Clear parent process ChromeActor on window closing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242064/diff/1-2/

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8973688 [details]
Bug 1459299 - 1. Add evaluateChromeJS for chrome privilege code;

https://reviewboard.mozilla.org/r/242060/#review248658

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8973689 [details]
Bug 1459299 - 2. Add pref setting methods for tests;

https://reviewboard.mozilla.org/r/242062/#review248664

I like the idea of being able to set prefs for tests, but I'm not a fan of the way we're doing it here. We should just use PrefsHelper (or a new thing that's better) instead of abusing the debugger protocol like this.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8973689 [details]
Bug 1459299 - 2. Add pref setting methods for tests;

https://reviewboard.mozilla.org/r/242062/#review248704

Jim and I talked some, and while I still think that this is not the best approach, he feels strongly that it is, so I'm inclined to relent for the time being.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8973688 [details]
Bug 1459299 - 1. Add evaluateChromeJS for chrome privilege code;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242060/diff/2-3/

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8973689 [details]
Bug 1459299 - 2. Add pref setting methods for tests;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242062/diff/2-3/

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8973690 [details]
Bug 1459299 - 3. Clear parent process ChromeActor on window closing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242064/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9248d8d84487
1. Add evaluateChromeJS for chrome privilege code; r=snorp
https://hg.mozilla.org/integration/autoland/rev/af4cad4221fa
2. Add pref setting methods for tests; r=snorp
https://hg.mozilla.org/integration/autoland/rev/a603214b9905
3. Clear parent process ChromeActor on window closing; r=jryans

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9248d8d84487
https://hg.mozilla.org/mozilla-central/rev/af4cad4221fa
https://hg.mozilla.org/mozilla-central/rev/a603214b9905