Closed Bug 1344748 Opened 7 years ago Closed 7 years ago

Set automation preferences in Marionette server

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 wontfix, firefox54 fixed, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(11 files)

Bug 1344748 - Remove B2G offline management from Marionette;
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1344748 - Set recommended prefs when Marionette starts;
59 bytes, text/x-review-board-request
impossibus
: review+
automatedtester
: review+
whimboo
: review+
Details
Bug 1344748 - Make testing/marionette/server.js a class;
59 bytes, text/x-review-board-request
impossibus
: review+
whimboo
: review+
Details
Bug 1344748 - Merge dispatcher into server.js;
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1344748 - Skip recommended Marionette prefs for Firefox tests;
59 bytes, text/x-review-board-request
impossibus
: review+
whimboo
: review+
Details
Bug 1344748 - Rename and register Marionette prefs;
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1344748 - Gate recommended prefs on a preference;
59 bytes, text/x-review-board-request
impossibus
: review+
whimboo
: review+
Details
Bug 1344748 - Remove ENABLE_MARIONETTE check from MarionetteComponent;
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1344748 - Document and make MarionetteComponent easier to read;
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1344748 - Make Marionette respect marionette.enabled pref;
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1344748 - List --marionette in CLI help;
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
Some ‘required preferences’ are currently defined in geckodriver and in the Marionette Python client.  Since none of these need to be set before the browser starts up, it should be safe for us to move them to the Marionette server.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Summary: Move required preferences into Marionette → Set automation preferences in Marionette server
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review119806

r+wc

::: testing/marionette/server.js:30
(Diff revision 3)
>  
>  this.EXPORTED_SYMBOLS = ["MarionetteServer"];
>  
>  const CONTENT_LISTENER_PREF = "marionette.contentListener";
>  
> +const RECOMMENDED_PREFS = new Map([

Is there a plan to remove any of the prefs set in marionette_driver/geckoinstance.py?

Ideally, there should be once source of truth for basic recommended prefs associated with Marionette, and it should be clear what prefs are being added or overwritten when the Marionette Client creates a profile.
Attachment #8844037 - Flags: review?(mjzffr) → review+
Comment on attachment 8844079 [details]
Bug 1344748 - Make testing/marionette/server.js a class;

https://reviewboard.mozilla.org/r/117614/#review119812

r+wc

::: commit-message-ed8f2:3
(Diff revision 2)
> +Bug 1344748 - Make testing/marionette/server.js a class; r?maja_zf
> +
> +No functional changes apart from class'ifying the file and harmonising the export symbol with the rest of the Marionette code base.

There's also a reference to MarionetteServer in tools/lint/eslint/modules.json
Attachment #8844079 - Flags: review?(mjzffr) → review+
Comment on attachment 8844080 [details]
Bug 1344748 - Merge dispatcher into server.js;

https://reviewboard.mozilla.org/r/117616/#review119814

r+wc

::: commit-message-738d7:3
(Diff revision 2)
> +Bug 1344748 - Merge dispatcher into server.js; r?maja_zf
> +
> +Merges testing/marionette/dispatcher.js into testing/marionette/server.js

There's a reference to Dispatcher in tools/lint/eslint/modules.json
Attachment #8844080 - Flags: review?(mjzffr) → review+
Comment on attachment 8844618 [details]
Bug 1344748 - Rename and register Marionette prefs;

https://reviewboard.mozilla.org/r/117980/#review119818

::: commit-message-651de:8
(Diff revision 1)
> +This change renames the following Marionette preferences:
> +
> +	marionette.defaultPrefs.enabled	marionette.enabled
> +	marionette.defaultPrefs.port		marionette.port
> +	marionette.force-local			marionette.forcelocal
> +	marionette.logging				marionette.log.level

Fix extra spacing.

::: testing/marionette/components/marionette.js:49
(Diff revision 1)
>      "nsIServerSocket",
>      "initSpecialConnection");
>  
> -Cu.import("resource://gre/modules/Log.jsm");
> -Cu.import("resource://gre/modules/Preferences.jsm");
> -Cu.import("resource://gre/modules/Services.jsm");
> +// Marionette preferences recently changed names.  This is an abstraction
> +// that first looks for the new name, but falls back to using the old name
> +// if the new oes not exist.

"oes not exist"
Attachment #8844618 - Flags: review?(mjzffr) → review+
Comment on attachment 8844619 [details]
Bug 1344748 - Gate recommended prefs on a preference;

https://reviewboard.mozilla.org/r/117982/#review119824
Attachment #8844619 - Flags: review?(mjzffr) → review+
Comment on attachment 8844619 [details]
Bug 1344748 - Gate recommended prefs on a preference;

https://reviewboard.mozilla.org/r/117982/#review119830

r+wc

::: testing/marionette/server.js:31
(Diff revision 1)
>  
>  this.EXPORTED_SYMBOLS = ["server"];
>  this.server = {};
>  
>  const CONTENT_LISTENER_PREF = "marionette.contentListener";
> +const RECOMMENDED_PREF = "marionette.prefs.recommended";

Could we avoid identifiers that only differ by an ending "s"? Too easy to misspell and misread. How about s/RECOMMENDED_PREF/MN_RECOMMENDED_PREF/?
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review119960

Sorry, but I don't see that this is the correct way to go. I just layed out some examples inline which makes me think of which other regressions we would cause.

::: testing/marionette/server.js:32
(Diff revision 3)
>  
>  const CONTENT_LISTENER_PREF = "marionette.contentListener";
>  
> +const RECOMMENDED_PREFS = new Map([
> +  // Disable automatic downloading of new releases
> +  ["app.update.auto", false],

We should not move this pref in the JS code! With that we would allow Firefox to update itself if an update check is performed at the very beginning before Marionette starts.

::: testing/marionette/server.js:35
(Diff revision 3)
> +const RECOMMENDED_PREFS = new Map([
> +  // Disable automatic downloading of new releases
> +  ["app.update.auto", false],
> +
> +  // Disable automatically upgrading Firefox
> +  ["app.update.enabled", false],

Same as above.

::: testing/marionette/server.js:52
(Diff revision 3)
> +  // whichever download test runs first doesn't show the popup
> +  // inconsistently
> +  ["browser.download.panel.shown", true],
> +
> +  // Do not show the EULA notification
> +  ["browser.EULA.override", true],

This would still show the EULA notificaiton bar during the first start of Firefox. We should not move this pref.

::: testing/marionette/server.js:56
(Diff revision 3)
> +  // Do not show the EULA notification
> +  ["browser.EULA.override", true],
> +
> +  // Turn off about:newtab and make use of about:blank instead for new
> +  // opened tabs
> +  ["browser.newtabpage.enabled", false],

This page is used at the very beginning to decide which content to show in the very first tab. I see a regression here.

::: testing/marionette/server.js:78
(Diff revision 3)
> +  ["browser.safebrowsing.blockedURIs.enabled", false],
> +  ["browser.safebrowsing.downloads.enabled", false],
> +  ["browser.safebrowsing.enabled", false],
> +  ["browser.safebrowsing.forbiddenURIs.enabled", false],
> +  ["browser.safebrowsing.malware.enabled", false],
> +  ["browser.safebrowsing.phishing.enabled", false],

Setting those prefs to false here, can still cause Firefox to download safebrowsing related files. Those prefs have to be set before Firefox gets started.

::: testing/marionette/server.js:87
(Diff revision 3)
> +
> +  // Do not restore the last open set of tabs if the browser has crashed
> +  ["browser.sessionstore.resume_from_crash", false],
> +
> +  // Don't check for the default web browser during startup
> +  ["browser.shell.checkDefaultBrowser", false],

This can cause a modal dialog to appear during the first start of Firefox because it's not set as default browser. I see a regression here.
Attachment #8844037 - Flags: review-
Comment on attachment 8844079 [details]
Bug 1344748 - Make testing/marionette/server.js a class;

https://reviewboard.mozilla.org/r/117614/#review119982

::: testing/marionette/server.js:225
(Diff revision 2)
>   *
>   * Once started, it opens a TCP socket sporting the debugger transport
>   * protocol on the provided port.  For every new client a Dispatcher is
>   * created.
> - *
> + */
> +server.Marionette = class {

Marionette isn't only about the socket server. So I feel this naming is a bit confusing.
Comment on attachment 8844619 [details]
Bug 1344748 - Gate recommended prefs on a preference;

https://reviewboard.mozilla.org/r/117982/#review119986

::: testing/marionette/prefs.js:20
(Diff revision 1)
>  // "warn", "info", "config", "debug", and "trace".
>  pref("marionette.log.level", "info");
> +
> +// Sets preferences recommended when using Firefox in automation with
> +// Marionette.
> +pref("marionette.prefs.recommended", true);

You are using an additional pref branch here with only a single item in it. I don't think this is a good idea. Instead why not simply using `marionette.recommended_prefs`? This also applies to `marionette.log.level`.
Comment on attachment 8844081 [details]
Bug 1344748 - Skip recommended Marionette prefs for Firefox tests;

https://reviewboard.mozilla.org/r/117618/#review119988

::: testing/profiles/prefs_general.js:369
(Diff revision 2)
>  user_pref("browser.formautofill.experimental", true);
> +
> +// Disable all recommended Marionette preferences for Gecko tests.
> +// The prefs recommended by Marionette are typically geared towards
> +// consumer automation; not vendor testing.
> +user_pref("marionette.prefs.recommended", false);

What about Firefox UI tests? As far as I know whether Marionette, nor mozprofile is using this file. As result any test harness (oh, that also includes external media tests, and maybe others too) will use the recommended prefs.
Comment on attachment 8844079 [details]
Bug 1344748 - Make testing/marionette/server.js a class;

https://reviewboard.mozilla.org/r/117614/#review119982

> Marionette isn't only about the socket server. So I feel this naming is a bit confusing.

I’m sorry, what is confusing?  The renaming from `MarionetteServer` to `server.Marionette`?  I could rename it `server.TCPServer`, `server.TCPListener` or similar.

See the latest revision.
Comment on attachment 8844080 [details]
Bug 1344748 - Merge dispatcher into server.js;

https://reviewboard.mozilla.org/r/117616/#review119814

> There's a reference to Dispatcher in tools/lint/eslint/modules.json

Another good catch.
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120082

::: commit-message-208ad:15
(Diff revision 3)
> +This change does not mean it is dangerous or wrong for consumers to write
> +their own preferences to the profile.  Any preferences written to the
> +profile will take precedence over this set of recommended preferences.
> +If the recommended Marionette preference has a user-defined value (i.e. it
> +is written to the profile before starting up or has manually changed),
> +that user-set value is preferred.

Could you also describe where these prefs come from (combination of what's found in geckoinstance.py, from geckodriver, anywhere else?)
Attachment #8844037 - Flags: review+
Comment on attachment 8844079 [details]
Bug 1344748 - Make testing/marionette/server.js a class;

https://reviewboard.mozilla.org/r/117614/#review119982

> I’m sorry, what is confusing?  The renaming from `MarionetteServer` to `server.Marionette`?  I could rename it `server.TCPServer`, `server.TCPListener` or similar.
> 
> See the latest revision.

That would be great, yes. Thanks.
Comment on attachment 8844618 [details]
Bug 1344748 - Rename and register Marionette prefs;

https://reviewboard.mozilla.org/r/117980/#review119818

> Fix extra spacing.

That’s odd.  Fixed.

> "oes not exist"

oes does not exist now.
Comment on attachment 8844619 [details]
Bug 1344748 - Gate recommended prefs on a preference;

https://reviewboard.mozilla.org/r/117982/#review119986

> You are using an additional pref branch here with only a single item in it. I don't think this is a good idea. Instead why not simply using `marionette.recommended_prefs`? This also applies to `marionette.log.level`.

So there is some precedence for this in about:config.  Because prefs are stored in a flat key-value map with no tree structure, more specificity is achieved through emulating a tree structure using dots, or branches.

In a tree structure, the most general thing comes first.  So for example, "browser.dom.window.dump.enabled" means to enable the `dump` function in the window’s DOM in the browser.  This is the only pref on this top-level branch and is clearer than "browser.enableWindowDumpInDOM" if we were to follow your example.

Similarly, "dom.webcomponents.enabled" and "dom.webcomponents.customelements.enabled" are well paired: "customelements" is a specificity on the "webcomponents" branch to control a sub-feature. If we consider that Marionette may have other logging settings or other preference-setting related settings in the future, following "marionette.log.level" and "marionette.prefs.recommended" does seem rather harmonious with the existing body of prefs.

Let me conjure up some examples:

- Perhaps we want to forward console.log to stdout in the future.  This could be gated under "marionette.log.console.enabled".
- When we introduce support for the Selenium log interface (to fetch various network-, performance-, and console logs from the browser) in the future we will have to limit the number of cached entries due to memory restrictions, so "marionette.log.limit.performance" or "marionette.log.limit.network" prefs would match similar settings such as "devtools.hud.loglimit.console" and "devtools.hud.loglimit.network".
- Maybe if we have a need for a custom set of preferences to be set in the future, this could be gated under "marionette.prefs.alternate" or "marionette.prefs.minimal".

But the main point is that whilst I agree shorter names are better, we shouldn’t set ourselves up so that we can’t add new things in the future we haven’t yet thought about.
Comment on attachment 8844619 [details]
Bug 1344748 - Gate recommended prefs on a preference;

https://reviewboard.mozilla.org/r/117982/#review119830

> Could we avoid identifiers that only differ by an ending "s"? Too easy to misspell and misread. How about s/RECOMMENDED_PREF/MN_RECOMMENDED_PREF/?

I agree the genitive s ending here is confusing.

I s/RECOMMENDED_PREF/RECOMMENDED/g and s/CONTENT_LISTENER_PREF/CONTENT_LISTENER/g.  My thinking is that Preferences.get(RECOMMENDED) and Preferences.get(CONTENT_LISTENER) reads perfectly fine, whereas RECOMMENDED_PREFS in for (let [k,v] of RECOMMENDED_PREFS) { … } needs more specificity due to the lack of context.

I hope you find this is more readable.  I have resolved the issue, but feel free to re-open it if you disagree.
Comment on attachment 8844081 [details]
Bug 1344748 - Skip recommended Marionette prefs for Firefox tests;

https://reviewboard.mozilla.org/r/117618/#review119988

> What about Firefox UI tests? As far as I know whether Marionette, nor mozprofile is using this file. As result any test harness (oh, that also includes external media tests, and maybe others too) will use the recommended prefs.

True, but as I said in the meeting, the prefs that _are_ used by Marionette/mozprofile/Firefox UI tests are the same as those recommended by Marionette.
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review119960

> Sorry, but I don't see that this is the correct way to go. I just layed out some examples inline which makes me think of which other regressions we would cause.

Do you still feel after our conversation yesterday that this is the wrong thing to do in principle, or just because it sets some preferences that must be set at startup?

My current thinking is to set those prefs you have pointed out must be set at startup in the profile, as before, and the rest at runtime when starting Marionette.  Does that sound reasonable to you?

> We should not move this pref in the JS code! With that we would allow Firefox to update itself if an update check is performed at the very beginning before Marionette starts.

Great to get your feedback on this.  It’s exactly what I wanted.

We should keep setting this in the profile then, which we already are.  But do you object to also setting it at runtime when Marionette starts?  There’s no harm in doing that, is there?

> This would still show the EULA notificaiton bar during the first start of Firefox. We should not move this pref.

We’re not moving the pref.  This is duplicating the prefs already set by the Mn harness.  Certainly we can never remove "browser.EULA.override" from the profile for the reasons you have already laid out.

What I suggest is that I go through an annotate the prefs that have no effect when set at runtime.  Is that acceptable?

> This page is used at the very beginning to decide which content to show in the very first tab. I see a regression here.

This is already set in http://searchfox.org/mozilla-central/source/testing/marionette/client/marionette_driver/geckoinstance.py#390 and https://github.com/mozilla/geckodriver/blob/master/src/prefs.rs#L34.

> Setting those prefs to false here, can still cause Firefox to download safebrowsing related files. Those prefs have to be set before Firefox gets started.

Duly noted.  I will annotate them.

> This can cause a modal dialog to appear during the first start of Firefox because it's not set as default browser. I see a regression here.

It doesn’t cause a regression because the pref is still set in the profile.
Comment on attachment 8844619 [details]
Bug 1344748 - Gate recommended prefs on a preference;

https://reviewboard.mozilla.org/r/117982/#review119986

> So there is some precedence for this in about:config.  Because prefs are stored in a flat key-value map with no tree structure, more specificity is achieved through emulating a tree structure using dots, or branches.
> 
> In a tree structure, the most general thing comes first.  So for example, "browser.dom.window.dump.enabled" means to enable the `dump` function in the window’s DOM in the browser.  This is the only pref on this top-level branch and is clearer than "browser.enableWindowDumpInDOM" if we were to follow your example.
> 
> Similarly, "dom.webcomponents.enabled" and "dom.webcomponents.customelements.enabled" are well paired: "customelements" is a specificity on the "webcomponents" branch to control a sub-feature. If we consider that Marionette may have other logging settings or other preference-setting related settings in the future, following "marionette.log.level" and "marionette.prefs.recommended" does seem rather harmonious with the existing body of prefs.
> 
> Let me conjure up some examples:
> 
> - Perhaps we want to forward console.log to stdout in the future.  This could be gated under "marionette.log.console.enabled".
> - When we introduce support for the Selenium log interface (to fetch various network-, performance-, and console logs from the browser) in the future we will have to limit the number of cached entries due to memory restrictions, so "marionette.log.limit.performance" or "marionette.log.limit.network" prefs would match similar settings such as "devtools.hud.loglimit.console" and "devtools.hud.loglimit.network".
> - Maybe if we have a need for a custom set of preferences to be set in the future, this could be gated under "marionette.prefs.alternate" or "marionette.prefs.minimal".
> 
> But the main point is that whilst I agree shorter names are better, we shouldn’t set ourselves up so that we can’t add new things in the future we haven’t yet thought about.

Ok, taken! Another thing would even be if we want to have another pref in the same branch like `marionette.log.foobar` we don't have to change preference names, which is always a nightmare in regards of backward compatibility. Lets just drop this issue.
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review119806

> Is there a plan to remove any of the prefs set in marionette_driver/geckoinstance.py?
> 
> Ideally, there should be once source of truth for basic recommended prefs associated with Marionette, and it should be clear what prefs are being added or overwritten when the Marionette Client creates a profile.

At some point we can go ahead and remove _some_ of the prefs set in geckoinstance.py.  But because the Marionette Python client is being used for upgrade tests, we cannot start removing duplicates until Firefox 55 ships.

Also as whimboo points out in the issues below, some of the prefs in the list here must be set in the profile because they are picked up when Firefox starts.  I will go through an annotate the prefs that Henrik have highlighted so we know this for the future.

So the tl;dr here is: Yes we want to remove duplicated prefs from geckoinstance.py in the future, but no we can’t do it right now.
Comment on attachment 8844081 [details]
Bug 1344748 - Skip recommended Marionette prefs for Firefox tests;

https://reviewboard.mozilla.org/r/117618/#review119988

> True, but as I said in the meeting, the prefs that _are_ used by Marionette/mozprofile/Firefox UI tests are the same as those recommended by Marionette.

Yes, as long as we can set our own prefs before Firefox starts all is fine.
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review119960

No, the idea is fine when doing it here. We just have to make sure that - as you pointed out - have to set the required prefs before we start Firefox. We should still go through the full list, and check which have to be taken.

> Great to get your feedback on this.  It’s exactly what I wanted.
> 
> We should keep setting this in the profile then, which we already are.  But do you object to also setting it at runtime when Marionette starts?  There’s no harm in doing that, is there?

Will this be done each time when Marionette starts? What if a test which used `enforce_gecko_prefs()` changed the value? We should not reset it here. Also we should not override the value if it has already been set in the profile itself. So a check if it's already a user pref might be worth doing.

> We’re not moving the pref.  This is duplicating the prefs already set by the Mn harness.  Certainly we can never remove "browser.EULA.override" from the profile for the reasons you have already laid out.
> 
> What I suggest is that I go through an annotate the prefs that have no effect when set at runtime.  Is that acceptable?

Yes, totally. Even it means one more place to handle default preference changes.
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review119960

> Will this be done each time when Marionette starts? What if a test which used `enforce_gecko_prefs()` changed the value? We should not reset it here. Also we should not override the value if it has already been set in the profile itself. So a check if it's already a user pref might be worth doing.

If a test uses enforce_gecko_prefs(), I assume the pref would get the user-set state.  We only set recommended preferences that are not already changed by the user.

What I refer to is the Preferences.isSet test here:

>   // set recommended preferences if they are not already user-defined
>   for (let [k,v] of RECOMMENDED_PREFS) {
>     if (!Preferences.isSet(k)) {
>       logger.debug(`Setting pref ${k} to ${v}`);
>       Preferences.set(k, v);
>     }
>   }
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120632

::: testing/marionette/server.js:105
(Diff revision 5)
> +  ["browser.safebrowsing.forbiddenURIs.enabled", false],
> +  ["browser.safebrowsing.malware.enabled", false],
> +  ["browser.safebrowsing.phishing.enabled", false],
> +
> +  // Disable updates to search engines
> +  ["browser.search.update", false],

I would prefer to see all update related prefs to be listed in the profile. You never know when the appropriate timer for an update check fires. So it could also be during startup.

::: testing/marionette/server.js:137
(Diff revision 5)
> +
> +  // Disable first run splash page on Windows 10
> +  ["browser.usedOnWindows10.introURL", ""],
> +
> +  // Disable the UI tour
> +  ["browser.uitour.enabled", false],

I believe this also needs to be defined in the profile.

::: testing/marionette/server.js:149
(Diff revision 5)
> +  ["datareporting.healthreport.service.enabled", false],
> +  ["datareporting.healthreport.service.firstRun", false],
> +  ["datareporting.healthreport.uploadEnabled", false],
> +  ["datareporting.policy.dataSubmissionEnabled", false],
> +  ["datareporting.policy.dataSubmissionPolicyAccepted", false],
> +  ["datareporting.policy.dataSubmissionPolicyBypassNotification", true],

I think it should be fine to leave those prefs here only. In the worst case we would send a single report.

Otherwise lets at least put uploadEnabled and dataSubmissionEnabled in the profile.

I wonder if we need the other prefs at all if we have those two services disabled anyway.

::: testing/marionette/server.js:164
(Diff revision 5)
> +  ["dom.max_script_run_time", 0],
> +
> +  // Only load extensions from the application and user profile
> +  // AddonManager.SCOPE_PROFILE + AddonManager.SCOPE_APPLICATION
> +  ["extensions.autoDisableScopes", 0],
> +  ["extensions.enabledScopes", 5],

Both should definitely be in the profile. Otherwise we would install system-wide installed add-ons during the first start. As example you can try this out on Ubuntu which ships some of those.

::: testing/marionette/server.js:174
(Diff revision 5)
> +  // Disable metadata caching for installed add-ons by default
> +  ["extensions.getAddons.cache.enabled", false],
> +
> +  // Disable intalling any distribution extensions or add-ons
> +  ["extensions.installDistroAddons", false],
> +  ["extensions.showMismatchUI", false],

Not totally sure but if add-ons are installed during the first startup, we would miss that.

::: testing/marionette/server.js:220
(Diff revision 5)
> +  // Make sure SNTP requests do not hit the network
> +  ["network.sntp.pools", "%(server)s"],
> +
> +  // Local documents have access to all other local docments, including
> +  // directory listings.
> +  ["security.fileuri.strict_origin_policy", false],

Can you remind me why we need this?

::: testing/marionette/server.js:297
(Diff revision 5)
>    if (this.alive) {
>      return;
>    }
> +
> +  // set recommended preferences if they are not already user-defined
> +  for (let [k,v] of RECOMMENDED_PREFS) {

nit: space after comma please.
Attachment #8844037 - Flags: review?(hskupin) → review-
Comment on attachment 8844079 [details]
Bug 1344748 - Make testing/marionette/server.js a class;

https://reviewboard.mozilla.org/r/117614/#review120644
Attachment #8844079 - Flags: review?(hskupin) → review+
Comment on attachment 8844619 [details]
Bug 1344748 - Gate recommended prefs on a preference;

https://reviewboard.mozilla.org/r/117982/#review120646

::: testing/marionette/server.js:31
(Diff revision 3)
>  
>  this.EXPORTED_SYMBOLS = ["server"];
>  this.server = {};
>  
> -const CONTENT_LISTENER_PREF = "marionette.contentListener";
> +const CONTENT_LISTENER = "marionette.contentListener";
> +const RECOMMENDED = "marionette.prefs.recommended";

We should name preferences constants like `PREF_FOO_BAR` to stay in sync with the Firefox code. With that it is also easier to distinguish what are preferences. The current code lacks this at various places and an extra check is always necessary.

And I agree that the `_PREFS` and `_PREF` suffixes were not ideal.
Attachment #8844619 - Flags: review?(hskupin) → review+
Comment on attachment 8844081 [details]
Bug 1344748 - Skip recommended Marionette prefs for Firefox tests;

https://reviewboard.mozilla.org/r/117618/#review120648

::: commit-message-0fdeb:6
(Diff revision 4)
> +Bug 1344748 - Skip recommended Marionette prefs for Firefox tests; r?maja_zf,whimboo
> +
> +The recommended Marionette preferences are not so useful when testing
> +the browser itself.  Many of the tests for Firefox have highly specific
> +requirements, and setting the recommended Marionette preferences could
> +mean a deterioration of test coverage.

I'm actually not that sure about it. For any preference overlap we would have here, it would not match. But as long as the preferences are getting defined in the profile before Marionette is used, I'm fine with this.
Attachment #8844081 - Flags: review?(hskupin) → review+
Comment on attachment 8844619 [details]
Bug 1344748 - Gate recommended prefs on a preference;

https://reviewboard.mozilla.org/r/117982/#review120646

> We should name preferences constants like `PREF_FOO_BAR` to stay in sync with the Firefox code. With that it is also easier to distinguish what are preferences. The current code lacks this at various places and an extra check is always necessary.
> 
> And I agree that the `_PREFS` and `_PREF` suffixes were not ideal.

I don’t think this is necessary.  It’s pretty clear from the context of Preferences.get(RECOMMENDED) what it does.
Comment on attachment 8844081 [details]
Bug 1344748 - Skip recommended Marionette prefs for Firefox tests;

https://reviewboard.mozilla.org/r/117618/#review120648

> I'm actually not that sure about it. For any preference overlap we would have here, it would not match. But as long as the preferences are getting defined in the profile before Marionette is used, I'm fine with this.

OK.  Do you have a suggestions for how to reword the commit message?
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120632

> I think it should be fine to leave those prefs here only. In the worst case we would send a single report.
> 
> Otherwise lets at least put uploadEnabled and dataSubmissionEnabled in the profile.
> 
> I wonder if we need the other prefs at all if we have those two services disabled anyway.

Data reporting is very unlikely to happen at startup.  I’ve addressed the other cases you have pointed out, but I’d like to keep the prefs we have to set in the profile minimal.

Let’s add a note later if it proves to be the case?

> Can you remind me why we need this?

This disables the strict file origin policy which is used when local HTML files are loaded into the browser (e.g. over the file: protocol).  The scripts and links within these files have restrictions on what they can see and do, and this removes those restrictions determined by the same-origin policy.

http://searchfox.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#231
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120082

> Could you also describe where these prefs come from (combination of what's found in geckoinstance.py, from geckodriver, anywhere else?)

Done.
Comment on attachment 8844619 [details]
Bug 1344748 - Gate recommended prefs on a preference;

https://reviewboard.mozilla.org/r/117982/#review120646

> I don’t think this is necessary.  It’s pretty clear from the context of Preferences.get(RECOMMENDED) what it does.

My proposal is to stick to naming conventions, and I think it's also what you prefer. If you see this contant used somewhere else without being an argument of a preferences method, it really doesn't tell anything.
Comment on attachment 8844081 [details]
Bug 1344748 - Skip recommended Marionette prefs for Firefox tests;

https://reviewboard.mozilla.org/r/117618/#review120648

> OK.  Do you have a suggestions for how to reword the commit message?

Maybe we call it "Marionette specific recommended preferences"? Maybe also note that Marionette is basically only used to install add-ons but for nothing else. So there is no need to set the recommended prefs.
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120632

> Data reporting is very unlikely to happen at startup.  I’ve addressed the other cases you have pointed out, but I’d like to keep the prefs we have to set in the profile minimal.
> 
> Let’s add a note later if it proves to be the case?

It's not a blocker for now, but we should really make sure we don't regress something when it gets removed from the profile settings. Keep in mind that most of the reporting features have specific times stored when the last report happened, and will report once the threshold is reached. In this case it will be `datareporting.healthreport.nextDataSubmissionTime`.
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120632

> This disables the strict file origin policy which is used when local HTML files are loaded into the browser (e.g. over the file: protocol).  The scripts and links within these files have restrictions on what they can see and do, and this removes those restrictions determined by the same-origin policy.
> 
> http://searchfox.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#231

And why does specifically Marionette need this? I don't see this pref used in `prefs_general.js` as used in all other test suites.
Comment on attachment 8845609 [details]
Bug 1344748 - Remove ENABLE_MARIONETTE check from MarionetteComponent;

https://reviewboard.mozilla.org/r/118738/#review120946

::: testing/marionette/components/moz.build:8
(Diff revision 3)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -EXTRA_COMPONENTS += ["marionette.manifest"]
> -EXTRA_PP_COMPONENTS += ["marionette.js"]
> -
> -if CONFIG["ENABLE_MARIONETTE"]:
> +EXTRA_COMPONENTS += [
> +    "marionette.js",
> +    "marionette.manifest",
> +]

Looks fine to me, but also get a review from a build system peer please.
Attachment #8845609 - Flags: review?(hskupin) → review+
Comment on attachment 8845610 [details]
Bug 1344748 - Document and make MarionetteComponent easier to read;

https://reviewboard.mozilla.org/r/118740/#review120950

::: testing/marionette/components/marionette.js:259
(Diff revision 3)
>  MarionetteComponent.prototype.init = function () {
> -  if (this.loaded || !this.enabled || !this.finalUIStartup) {
> +  if (this.running || !this.enabled || !this.finalUIStartup) {
>      return;
>    }
>  
> -  this.loaded = true;
> +  this.running = true;

What if spunning up the server fails? Should Marionette be marked as running? I don't think so.

::: testing/marionette/components/marionette.js:272
(Diff revision 3)
>      let insaneSacrificialGoat =
>          new ServerSocket(666, Ci.nsIServerSocket.KeepWhenOffline, 4);
>      insaneSacrificialGoat.asyncListen(this);
>    }
>  
> -  let so;
> +  let s;

Please use an understandable variable name here, like `listener`.
Attachment #8845610 - Flags: review?(hskupin) → review-
Comment on attachment 8845611 [details]
Bug 1344748 - Make Marionette respect marionette.enabled pref;

https://reviewboard.mozilla.org/r/118742/#review120952

::: testing/marionette/components/marionette.js:24
(Diff revision 3)
> +const PORT = "marionette.port";
> +const PORT_FALLBACK = "marionette.defaultPrefs.port";
> +const LOG_LEVEL = "marionette.log.level";
> +const LOG_LEVEL_FALLBACK = "marionette.logging";
> +const FORCE_LOCAL = "marionette.forcelocal";
> +const FORCE_LOCAL_FALLBACK = "marionette.force-local";

Similar to the other commit I would prefer the `PREF_` suffix.

::: testing/marionette/components/marionette.js:64
(Diff revision 3)
> -    return Preferences.get("marionette.enabled", fallback);
> -  },
> -
>    get port () {
> -    let fallback = Preferences.get("marionette.defaultPrefs.port", DEFAULT_PORT);
> -    return Preferences.get("marionette.port", fallback);
> +    let fallback = Preferences.get(PORT_FALLBACK, DEFAULT_PORT);
> +    return Preferences.get(PORT, fallback);

Why are you not modifying the other commit which introduces all this new code? It would reduce the number of changes in a lot of lines.

::: testing/marionette/components/marionette.js:176
(Diff revision 3)
> -    this.init();
>    }
>  };
>  
> +Object.defineProperty(MarionetteComponent.prototype, "enabled", {
> +  set (enable) {

I would use `value` here because it can also be disabled.
Attachment #8845611 - Flags: review?(hskupin) → review-
Comment on attachment 8845612 [details]
Bug 1344748 - List --marionette in CLI help;

https://reviewboard.mozilla.org/r/118744/#review120954
Attachment #8845612 - Flags: review?(hskupin) → review+
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review121036
Attachment #8844037 - Flags: review?(mjzffr) → review+
Comment on attachment 8844081 [details]
Bug 1344748 - Skip recommended Marionette prefs for Firefox tests;

https://reviewboard.mozilla.org/r/117618/#review121040
Attachment #8844081 - Flags: review?(mjzffr) → review+
Comment on attachment 8845611 [details]
Bug 1344748 - Make Marionette respect marionette.enabled pref;

https://reviewboard.mozilla.org/r/118742/#review121044

Whoops, forgot to publish this this morning.

::: testing/marionette/components/marionette.js:191
(Diff revision 3)
>    switch (topic) {
> +    case "nsPref:changed":
> +      if (Preferences.get(ENABLED)) {
> +        this.init();
> +      } else {
> +        this.uninit();

If I understand correctly, this change means we can stop Marionette mid-session by changing the `enabled` pref at run-time. What are the implications, then? Do we have appropriate error handling and cleanup? Does this imply changes to the runner as well? 

I think Henrik mentioned that we could observe the default-prefs branch instead, so that user pref changes don't affect the Marionette server.
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120632

> It's not a blocker for now, but we should really make sure we don't regress something when it gets removed from the profile settings. Keep in mind that most of the reporting features have specific times stored when the last report happened, and will report once the threshold is reached. In this case it will be `datareporting.healthreport.nextDataSubmissionTime`.

I had a look at some of these preferences around the code, and although they are documented in c.f. https://github.com/mozilla/gecko-dev/blob/86897859913403b68829dbf9a154f5a87c4b0638/toolkit/components/telemetry/docs/fhr/architecture.rst, I cannot find a usage of many of them anywhere in the Gecko source code.  That is probably an indication some of these preferences have changed names or been deprecated.  At some future point it would probably be a good idea to do a spring cleaning of these since this list, unlike geckoinstance.py, only applies to the Firefox version it is bundled with.

In any case, I will resolve this issue for the time being.  I think whenever we do move preferences out of geckoinstance.py, a more thorough look at what is there needs to be undertaken.

> And why does specifically Marionette need this? I don't see this pref used in `prefs_general.js` as used in all other test suites.

Users of WebDriver expect to be able to load all local documents on the filesystem, including files not in the document’s current working directory and directory listings.
Comment on attachment 8844619 [details]
Bug 1344748 - Gate recommended prefs on a preference;

https://reviewboard.mozilla.org/r/117982/#review120646

> My proposal is to stick to naming conventions, and I think it's also what you prefer. If you see this contant used somewhere else without being an argument of a preferences method, it really doesn't tell anything.

It is my opinion that this is completely unnecessary busywork, but I have nevertheless obliged to your request.
Comment on attachment 8844081 [details]
Bug 1344748 - Skip recommended Marionette prefs for Firefox tests;

https://reviewboard.mozilla.org/r/117618/#review120648

> Maybe we call it "Marionette specific recommended preferences"? Maybe also note that Marionette is basically only used to install add-ons but for nothing else. So there is no need to set the recommended prefs.

Thanks, fixed.
Comment on attachment 8845609 [details]
Bug 1344748 - Remove ENABLE_MARIONETTE check from MarionetteComponent;

https://reviewboard.mozilla.org/r/118738/#review120946

> Looks fine to me, but also get a review from a build system peer please.

I’m sorry, but I don’t think this warrants wasting a build peer’s time.  This is the build file for the module I am a peer of, and these changes won’t even compile without this change.

EXTRA_PP_COMPONENTS causes the file to be passed through the preprocessor before being included in the jar.  If you pass it a file that doesn’t include any preprocessable elements, such as #ifdefs, it will complain.
Comment on attachment 8845610 [details]
Bug 1344748 - Document and make MarionetteComponent easier to read;

https://reviewboard.mozilla.org/r/118740/#review120950

> What if spunning up the server fails? Should Marionette be marked as running? I don't think so.

The thinking is that we may have already set up some state that could make calling stop() meaningful before this function throws.  But I agree in principle that it shouldn’t incorrectly report the server as running if it isn’t, so I will move it last in the function.

> Please use an understandable variable name here, like `listener`.

It is a transitional non-public variable that spans three lines: do you think readers of the code are incapable of keeping its context in mind whilst reading those?

I don’t mean to be picky about this, but this issue is a complete waste of my time.
Comment on attachment 8845610 [details]
Bug 1344748 - Document and make MarionetteComponent easier to read;

https://reviewboard.mozilla.org/r/118740/#review120950

> It is a transitional non-public variable that spans three lines: do you think readers of the code are incapable of keeping its context in mind whilst reading those?
> 
> I don’t mean to be picky about this, but this issue is a complete waste of my time.

Then go with how it is right now. Dropping issue.
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120632

> Users of WebDriver expect to be able to load all local documents on the filesystem, including files not in the document’s current working directory and directory listings.

I see. So this basically adds this preference to Marionette now, and opens up way more priviliges to the process. Can you please get this through David? If he is happy with it - and we don't need another sec review (?) - we can go with it.
Comment on attachment 8845609 [details]
Bug 1344748 - Remove ENABLE_MARIONETTE check from MarionetteComponent;

https://reviewboard.mozilla.org/r/118738/#review120946

> I’m sorry, but I don’t think this warrants wasting a build peer’s time.  This is the build file for the module I am a peer of, and these changes won’t even compile without this change.
> 
> EXTRA_PP_COMPONENTS causes the file to be passed through the preprocessor before being included in the jar.  If you pass it a file that doesn’t include any preprocessable elements, such as #ifdefs, it will complain.

Ok. Fine then.
Comment on attachment 8845611 [details]
Bug 1344748 - Make Marionette respect marionette.enabled pref;

https://reviewboard.mozilla.org/r/118742/#review121044

> If I understand correctly, this change means we can stop Marionette mid-session by changing the `enabled` pref at run-time. What are the implications, then? Do we have appropriate error handling and cleanup? Does this imply changes to the runner as well? 
> 
> I think Henrik mentioned that we could observe the default-prefs branch instead, so that user pref changes don't affect the Marionette server.

Yes, this is the first step along the road to make it possible to start and stop Marionette at runtime.  Whether we have appropriate error handling and cleanup for that?  Probably not.  But this change does not imply any changes to the runner for the time being.

> I think Henrik mentioned that we could observe the default-prefs branch instead, so that user pref changes don't affect the Marionette server.

The intention with this change _is_ to let users turn Marionette on and off in an existing Firefox session, like it is possible to do for the browser toolbox.
Comment on attachment 8845611 [details]
Bug 1344748 - Make Marionette respect marionette.enabled pref;

https://reviewboard.mozilla.org/r/118742/#review122056
Attachment #8845611 - Flags: review?(hskupin) → review+
Comment on attachment 8845610 [details]
Bug 1344748 - Document and make MarionetteComponent easier to read;

https://reviewboard.mozilla.org/r/118740/#review122076
Attachment #8845610 - Flags: review?(hskupin) → review+
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120632

> I see. So this basically adds this preference to Marionette now, and opens up way more priviliges to the process. Can you please get this through David? If he is happy with it - and we don't need another sec review (?) - we can go with it.

Well it is already used in geckodriver at https://github.com/mozilla/geckodriver/blob/master/src/prefs.rs#L186, so this isn’t really introducing any new, previously unknown behaviour.  But I will request additional review from dburns.
Attachment #8844037 - Flags: review?(hskupin)
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120632

> Well it is already used in geckodriver at https://github.com/mozilla/geckodriver/blob/master/src/prefs.rs#L186, so this isn’t really introducing any new, previously unknown behaviour.  But I will request additional review from dburns.

In this case I don't care about code used outside the tree. When we look at our core code this is something new and has not been used by Marionette proper.

I would like to see another try build at least for those platforms and tests which failed that horribly last time.
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review125610

r=me, with David's agreement on the remaining issue.
Attachment #8844037 - Flags: review+
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review125618
Attachment #8844037 - Flags: review?(dburns) → review+
Comment on attachment 8844037 [details]
Bug 1344748 - Set recommended prefs when Marionette starts;

https://reviewboard.mozilla.org/r/117594/#review120632

> In this case I don't care about code used outside the tree. When we look at our core code this is something new and has not been used by Marionette proper.
> 
> I would like to see another try build at least for those platforms and tests which failed that horribly last time.

The point is that it’s (a) been reviewed by a Mozillian before (jgraham), and (b) will not be used for the automation on try.

When you say that it “has not been used by Marionette proper” before, that is only true if you think of Marionette in that context as the in-tree Firefox test we run on try.  It has, arguably, been used by end-users and public consumers of WebDriver/geckodriver for some time.  The set of recommended automation preferences will, as we agreed, not be used in-tree and their intention is to facilitate end-user automation of Firefox.

Because I now have an r+ from dburns on this, I am proceeding to close this issue.
Comment on attachment 8845611 [details]
Bug 1344748 - Make Marionette respect marionette.enabled pref;

https://reviewboard.mozilla.org/r/118742/#review120952

> Similar to the other commit I would prefer the `PREF_` suffix.

Fixed.

> Why are you not modifying the other commit which introduces all this new code? It would reduce the number of changes in a lot of lines.

Because I only thought about it after I needed to refer to PREF_ENABLED and PREF_ENABLED_FALLBACK multiple times, at which point I considered it a security risk I mistype them.  So for consistency reasons that triggered me to use constants for all other preferences that didn’t previously require this.

Dropping this issue since there’s nothing actionable in it.

> I would use `value` here because it can also be disabled.

Fixed.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f7441656f46
Remove B2G offline management from Marionette; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/d425dc57b170
Set recommended prefs when Marionette starts; r=automatedtester,maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/5e4ffba9bd16
Make testing/marionette/server.js a class; r=maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/75ed5a3cff70
Merge dispatcher into server.js; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/65aee3ceefec
Rename and register Marionette prefs; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/0701ad4bd9f6
Gate recommended prefs on a preference; r=maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/1598e2db1cf6
Skip recommended Marionette prefs for Firefox tests; r=maja_zf,whimboo
https://hg.mozilla.org/integration/autoland/rev/4985a0d47c7f
Remove ENABLE_MARIONETTE check from MarionetteComponent; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/818abd440d52
Document and make MarionetteComponent easier to read; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/728b96e6ccdd
Make Marionette respect marionette.enabled pref; r=whimboo
https://hg.mozilla.org/integration/autoland/rev/16ae5cf72cd6
List --marionette in CLI help; r=whimboo
Blocks: 1350887
Sheriffs: Please uplift to Aurora and Beta as test-only.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Can we please wait until the regression has been fixed on m-c? Thanks.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
(In reply to Henrik Skupin (:whimboo) from comment #143)
> Can we please wait until the regression has been fixed on m-c? Thanks.

Not sure what regression you are referring to.  I’m addressing the fact that the fallback/deprecated prefs aren’t being picked up in https://bugzilla.mozilla.org/show_bug.cgi?id=1350887, and I’m close to a solution there.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Sheriffs: Please uplift to Aurora and Beta as test-only.
grafting 401573:5917cc585718 "Bug 1344748 - Rename and register Marionette prefs; r=maja_zf, a=test-only"
merging testing/marionette/client/marionette_driver/geckoinstance.py                                        
merging testing/marionette/components/marionette.js                                                         
merging tools/lint/eslint/modules.json                                                                      
warning: conflicts while merging testing/marionette/components/marionette.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
Whiteboard: [checkin-needed-beta]
Setting a needinfo for myself so that I remember to rebase the patches for Beta.
Flags: needinfo?(ato)
(In reply to Andreas Tolfsen ‹:ato› from comment #144)
> Not sure what regression you are referring to.  I’m addressing the fact that
> the fallback/deprecated prefs aren’t being picked up in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1350887, and I’m close to a
> solution there.

You are aware of bug 1350887 because you filed it and you also know that it prevents tracing output. Seeing this patch uplifted to aurora now is bad because it stops us from any test failure investigation for Aurora nightly builds. 

This really shouldn't have been uplifted yet! :(
(In reply to Henrik Skupin (:whimboo) from comment #149)
> You are aware of bug 1350887 because you filed it and you also know that it
> prevents tracing output. Seeing this patch uplifted to aurora now is bad
> because it stops us from any test failure investigation for Aurora nightly
> builds. 
> 
> This really shouldn't have been uplifted yet! :(

I disagree with that sentiment since I know the fix, which was only delayed because you filed a last-minute issue on a review you weren’t flagged for.  Let’s think priorities here.
Well, it's not your only issue as we have seen with the backouts. But with the uplift to aurora you also prevent me from any introspection of update failures due to missing trace information for bug 1316564. I call this more annoying now given that this is a blocking issue for current beta releases.
(In reply to Andreas Tolfsen ‹:ato› from comment #148)
> Setting a needinfo for myself so that I remember to rebase the patches for
> Beta.

I did a successful rebase of this to beta following some conflicts, but there were a lot of sketchy changes that I don’t want to spend time figuring out if was right.  For that reason, I’m cancelling the uplift of this patch to beta.
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #151)

> Well, it's not your only issue as we have seen with the backouts.

The backouts of https://bugzilla.mozilla.org/show_bug.cgi?id=1350887
were caused by a typo.

> But with the uplift to aurora you also prevent me from any
> introspection of update failures due to missing trace information for
> bug 1316564.

I agree that is annoying, but we did discover that a bit too late.

> I call this more annoying now given that this is a blocking issue for
> current beta releases.

I wouldn’t consider this blocking for beta.  It would be “nice to
have”, but certainly not a requirement.
(In reply to Andreas Tolfsen ‹:ato› from comment #153)
> > I call this more annoying now given that this is a blocking issue for
> > current beta releases.
> 
> I wouldn’t consider this blocking for beta.  It would be “nice to
> have”, but certainly not a requirement.

Sorry for leaving info out... what I meant here was that its a blocking issue for current beta release update testing.
(In reply to Henrik Skupin (:whimboo) from comment #155)
> (In reply to Andreas Tolfsen ‹:ato› from comment #153)
> > > I call this more annoying now given that this is a blocking issue for
> > > current beta releases.
> > 
> > I wouldn’t consider this blocking for beta.  It would be “nice to
> > have”, but certainly not a requirement.
> 
> Sorry for leaving info out... what I meant here was that its a blocking
> issue for current beta release update testing.

There’s nothing in this patch that I can see that affects update tests?  It adds a set of automation prefs that aren’t used in Firefox automation, some documentation, and renames some prefs.
Please read comment 151 again. There are all the information. :) Anyway we shouldn't spend more time on arguments here, but get the regression fixed and uplifted.
No longer blocks: 1350887
Depends on: 1350887
Andreas, to be able to uplift bug 1350887 and bug 1337743 to beta, we would need those patches being uplifted. When I grafted those commits three small updates were necessary to the following changesets:

Bug 1344748 - Rename and register Marionette prefs
Bug 1344748 - Skip recommended Marionette prefs for Firefox tests
Bug 1344748 - Document and make MarionetteComponent easier to read

I just talked to Ryan on IRC and the deadline for 53 is nowish! So no way to get this uplifted anymore. :(
status-firefox53: affected → wontfix
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: