Closed
Bug 1219397
Opened 9 years ago
Closed 9 years ago
Collect Firefox-Desktop-specific prefs in a subclass of GeckoInstance
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox45 fixed)
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: impossibus, Assigned: impossibus)
References
Details
Attachments
(1 file)
We want to move browser prefs that are common to firefox-ui-tests [1] and other desktop tests into Marionette.
Original discussion: https://bugzilla.mozilla.org/show_bug.cgi?id=1212608#c20
[1] https://github.com/mozilla/firefox-ui-tests/blob/50527c612f3c063ab52046d0d01f610b64f5f003/firefox_ui_harness/runners/base.py#L14
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8680204 -
Flags: review?(dburns)
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
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?
Assignee | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
This last change just addresses Comment 7.
Also, I filed Bug 1221187 re gecko.log
Comment 10•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Attachment #8680204 -
Flags: review+ → review?(dburns)
Assignee | ||
Comment 11•9 years ago
|
||
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/
Assignee | ||
Comment 12•9 years ago
|
||
Rebased to avoid merge conflict with Bug 1217557
Updated•9 years ago
|
Attachment #8680204 -
Flags: review?(dburns) → review+
Comment 13•9 years ago
|
||
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
Comment 14•9 years ago
|
||
Keywords: checkin-needed
Comment 15•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•