Closed
Bug 1332557
Opened 7 years ago
Closed 7 years ago
Consider enabling sync success logs by default on Nightly
Categories
(Firefox :: Sync, defect, P2)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: markh, Assigned: bharatraghunthan9767, Mentored)
Details
(Keywords: good-first-bug)
Attachments
(3 files)
4.03 KB,
patch
|
markh
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
markh
:
review+
|
Details | Diff | Splinter Review |
59 bytes,
text/x-review-board-request
|
markh
:
review+
|
Details |
We regularly get bug reports from Nightly users. It would be helpful, and within the scope of what Nightly is for, to have services.sync.log.appender.file.logOnSuccess be true on Nightly by default - that way we could ask such users for their logs and be more likely to have something useful. (We could also argue this for Aurora, but one yak at a time ;)
Updated•7 years ago
|
Priority: -- → P2
Reporter | ||
Updated•7 years ago
|
Mentor: markh
Keywords: good-first-bug
Comment 2•7 years ago
|
||
Of course! Do you already have your Firefox build environment set up? If not, check out https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction. You'll want to add the pref to http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/services/sync/services-sync.js, and change the `moz.build` file in that directory so that it preprocesses the prefs file. Have a look at the DevTools prefs file (http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/devtools/client/preferences/devtools.js) and its build manifest for an example of how to set a Nightly-only pref. Please feel free to ask questions here, or on IRC in the #sync channel, if you get stuck. Thanks for taking this on!
Flags: needinfo?(markh)
Assignee | ||
Comment 3•7 years ago
|
||
I have taken on this bug as it was unassigned and have submitted on https://reviewboard-hg.mozilla.org/gecko/rev/dfe1120f6dbc however, it is not appearing as a MozReview Request here, so I'm attaching it separately.
Flags: needinfo?(markh)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8843560 -
Flags: feedback?(markh)
Reporter | ||
Comment 5•7 years ago
|
||
Comment on attachment 8843560 [details] [diff] [review] Enabled sync success logs by default only on Nightly That looks exactly correct, thanks.
Attachment #8843560 -
Attachment is patch: true
Attachment #8843560 -
Attachment mime type: application/javascript → text/plain
Flags: needinfo?(markh)
Attachment #8843560 -
Flags: feedback?(markh) → review+
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → bharatraghunthan9767
Keywords: checkin-needed
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8843834 -
Flags: review?(markh)
Attachment #8843834 -
Flags: checkin?(ryanvm)
Assignee | ||
Comment 8•7 years ago
|
||
Should I make obsolete the previous attachment #8843560 [details] [diff] [review] or leave it as it is?
Flags: needinfo?(markh)
Reporter | ||
Comment 9•7 years ago
|
||
Comment on attachment 8843834 [details] [diff] [review] Enabled sync success logs by default only on Nightly - submitting as diff/patch Review of attachment 8843834 [details] [diff] [review]: ----------------------------------------------------------------- Thanks - I previously looked at the reviewboard commit - you should probably work out what went wrong there as using reviewboard is much simpler for reviewers.
Attachment #8843834 -
Flags: review?(markh)
Attachment #8843834 -
Flags: review+
Attachment #8843834 -
Flags: checkin?(ryanvm)
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(markh)
Keywords: checkin-needed
Comment 10•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c651974bb5ce Enable sync success logs by default only on Nightly r=markh
Keywords: checkin-needed
Assignee | ||
Comment 11•7 years ago
|
||
Comment to attachment #8843834 [details] [diff] [review] https://reviewboard-hg.mozilla.org/gecko/rev/dfe1120f6dbc Is there anything more to be done or is the bug fixed?
Comment 12•7 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=81812063&repo=mozilla-inbound
Flags: needinfo?(bharatraghunthan9767)
Comment 13•7 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/af18a377c790 Backed out changeset c651974bb5ce for test failures in test_service_migratePrefs.js
Assignee | ||
Comment 14•7 years ago
|
||
Should I modify test_service_migratePrefs.js to test for Nightly or not, or change the code in services-sync.js (like changing order of the prefs etc.)?
Flags: needinfo?(bharatraghunthan9767) → needinfo?(markh)
Reporter | ||
Comment 15•7 years ago
|
||
Thanks for your contribution, and my apologies for not noticing the change would cause test failures. The test should be changed (even though it's now out of date) - if you add near the top of the file: Cu.import("resource://gre/modules/AppConstants.jsm"); const LOG_ON_SUCCESS_DEFAULT = AppConstants.NIGHTLY_BUILD; then change: do_check_eq(Svc.Prefs.get("log.appender.file.logOnSuccess"), false); to: do_check_eq(Svc.Prefs.get("log.appender.file.logOnSuccess"), LOG_ON_SUCCESS_DEFAULT); The test will pass. Seeing as it has been backed out once, we really need to run a try build for this, so you will need to work out how to push to mozreview, and we can trigger a test build from that. Please catch us in IRC in the #sync channel if you still have trouble pushing to mozreview. I also opened bug 1345009 to remove the code that caused the failure, so I guess if you are keen you could take that other bug first, after which no further changes would be needed here :)
Flags: needinfo?(markh)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•7 years ago
|
||
Pushed to the reviewboard in order for the patch to be landed
Reporter | ||
Comment 18•7 years ago
|
||
Try is failing with an "E" error that indicates your local tree is a week or so old and needs updating - could you please rebase your patches against a current mozilla-central, modify the commit message so it has ". r?markh" at the end, then re-push to reviewboard? You could then push you own try build too (or I'm happy to do that) - I expect try will then pass and I can push it for you. https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F has more info. Thanks for your patches and your patience!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
Please review the MozReview Request of Comment 20 not Comment 19 as it was a mistake.
Flags: needinfo?(markh)
Reporter | ||
Comment 22•7 years ago
|
||
Something strange has gone on with that patch - https://reviewboard.mozilla.org/r/118104/diff/3/ shows only one .patch file. It's not possible to review an earlier iteration of the patch, so I think you need to push a new patch with only the changes and no .patch files.
Flags: needinfo?(markh)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•7 years ago
|
||
I'm sorry, it was because my local copy of the mozilla-central repo was in a mess with multiple branches suddenly created that may have caused the problem. I hope it's fine now?
Flags: needinfo?(markh)
Reporter | ||
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8844774 [details] Bug 1332557 - Enable sync success logs by default on Nightly. https://reviewboard.mozilla.org/r/118104/#review120802 The try run looks good! :) Can you please change the commit message to "Bug 1332557 - Enable sync success logs by default on Nightly. r?markh" and re-push it, then I'll land it. Thanks!
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•7 years ago
|
||
Changed the commit message accordingly and re-pushed it. Now please try landing it.
Reporter | ||
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8844774 [details] Bug 1332557 - Enable sync success logs by default on Nightly. https://reviewboard.mozilla.org/r/118104/#review120964 Thanks!
Attachment #8844774 -
Flags: review?(markh) → review+
Comment 29•7 years ago
|
||
Pushed by mhammond@skippinet.com.au: https://hg.mozilla.org/integration/autoland/rev/b0abf85ea5e1 Enable sync success logs by default on Nightly. r=markh
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b0abf85ea5e1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(markh)
You need to log in
before you can comment on or make changes to this bug.
Description
•