Closed Bug 1219397 Opened 6 years ago Closed 6 years ago

Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: impossibus, Assigned: impossibus)

References

Details

Attachments

(1 file)

Bug 1219397 - Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance; r?automatedtester
Attachment #8680204 - Flags: review?(dburns)
https://reviewboard.mozilla.org/r/23597/#review21143

::: testing/marionette/driver/marionette_driver/geckoinstance.py:200
(Diff revision 1)
> +        'marionette.logging': False,

This line is stil questionable as talked on IRC yesterday. If that is enabled by default we might follow that, but the problem is that the Marionette logging is way too detailed even in INFO level. I think it would be good to check if we can change Marionette logging so it only spits out those line for DEBUG level? Not sure about any affects for other harnesses.

::: testing/marionette/driver/marionette_driver/geckoinstance.py:205
(Diff revision 1)
> +        'toolkit.telemetry.enabled': False,

Also a lot of those prefs are not desktop only. Entries like telemetry or geolocation should be present for all applications. 

The pref focusmanager.testmode gets set in GeckoInstance already.

As long as David is not around this week it might make sense to ask Jonathan for review.
Attachment #8680204 - Flags: review?(dburns)
Comment on attachment 8680204 [details]
MozReview Request: Bug 1219397 - Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance; r?automatedtester

https://reviewboard.mozilla.org/r/23597/#review21353

::: testing/marionette/driver/marionette_driver/geckoinstance.py:186
(Diff revision 1)
> +        'browser.warnOnQuit': False,

There are a number of duplicates with `GeckoInstance`. If they are the same we should probably just leave it in there and only have ones that are different here.
Comment on attachment 8680204 [details]
MozReview Request: Bug 1219397 - Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance; r?automatedtester

Bug 1219397 - Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance; r?automatedtester
Attachment #8680204 - Flags: review?(dburns)
Thanks for the reviews.
I removed the duplicates and moved the telemetry entry up into GeckoInstance. I'm not sure what else is not desktop specific: feedback welcome.

As for the log-level for marionette logging, I will ask around tomorrow and maybe file a bug.
https://reviewboard.mozilla.org/r/23597/#review21479

::: testing/marionette/driver/marionette_driver/geckoinstance.py:41
(Diff revision 2)
> +                      'toolkit.telemetry.enabled': False,}

I would suggest to put the closing bracket into the next line, so if more prefs are getting added you will only see the addition and not an update to line 41.

::: testing/marionette/driver/marionette_driver/geckoinstance.py:195
(Diff revision 2)
> +        'marionette.logging': False,

If we go with False here we should add a comment why we do that given that it overrides the default logging value of GeckoInstance. I would still propose to discuss if we can reduce the amount of Marionette logging in INFO level.

::: testing/marionette/driver/marionette_driver/geckoinstance.py:212
(Diff revision 2)
> +        'fxdesktop': DesktopInstance}

Same here for the closing bracket.

Some drive-by notes. Otherwise it looks fine to me. All of those prefs should be desktop related but apply to various apps like Firefox, Thunderbird, and SeaMonkey. So good for other subclassing if necessary later. Or shall we already have a FirefoxInstance?
Comment on attachment 8680204 [details]
MozReview Request: Bug 1219397 - Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance; r?automatedtester

Bug 1219397 - Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance; r?automatedtester
This last change just addresses Comment 7.

Also, I filed Bug 1221187 re gecko.log
Comment on attachment 8680204 [details]
MozReview Request: Bug 1219397 - Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance; r?automatedtester

https://reviewboard.mozilla.org/r/23597/#review21565
Attachment #8680204 - Flags: review?(dburns) → review+
Attachment #8680204 - Flags: review+ → review?(dburns)
Comment on attachment 8680204 [details]
MozReview Request: Bug 1219397 - Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23597/diff/3-4/
Rebased to avoid merge conflict with Bug 1217557
Attachment #8680204 - Flags: review?(dburns) → review+
Comment on attachment 8680204 [details]
MozReview Request: Bug 1219397 - Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance; r?automatedtester

https://reviewboard.mozilla.org/r/23597/#review21947
https://hg.mozilla.org/mozilla-central/rev/ffd5d064dead
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.