Closed Bug 1439777 Opened 5 years ago Closed 5 years ago

Simplify sync's ErrorHandler

Categories

(Firefox :: Sync, enhancement, P1)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: tcsc, Assigned: eoger)

Details

Attachments

(2 files)

Bug 1439777 p1 - Remove X-Weave-Alert support.
59 bytes, text/x-review-board-request
markh
: review+
Details
Bug 1439777 p2 - Remove weave:ui:* related code.
59 bytes, text/x-review-board-request
markh
: review+
tcsc
: review+
Details 
It's a somewhat complex piece of code (with very complex tests) that is primarially concerned with whether or not we should be displaying errors in the UI. Since we never do this anymore, it should be simplified.
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1
Note that we will also need to update the about:sync addon which relies on weave:ui:* [0]

[0] https://github.com/mhammond/aboutsync/blob/77ef1595df68ed71d7f55ae68018d650061ca3ed/bootstrap.js#L178-L179
We could instead consider emitting weave:ui:*:error events when we'd otherwise write a sync error log. I spent some time looking into how to convert about:sync to not depend on those events, and the best I could come up with (without changing the sync code) was to listen for when we wrote an log file, and see if doing so caused a new file with error in its name to appear in our sync log directory.

A simpler solution would be to let about:sync know here https://searchfox.org/mozilla-central/source/services/sync/modules/policies.js#854 somehow (using weave:ui:* would be backwards compatible, admittedly the name is bad)
(FWIW, reporting an error in this case will catch cases where an error is logged outside of a sync -- at the moment we completely miss those)
Comment on attachment 8952983 [details]
Bug 1439777 p2 - Remove weave:ui:* related code.

https://reviewboard.mozilla.org/r/222246/#review228490

nice cleanup, thanks!
Attachment #8952983 - Flags: review?(markh) → review+
Comment on attachment 8952982 [details]
Bug 1439777 p1 - Remove X-Weave-Alert support.

https://reviewboard.mozilla.org/r/222244/#review228502
Attachment #8952982 - Flags: review?(markh) → review+
Comment on attachment 8952983 [details]
Bug 1439777 p2 - Remove weave:ui:* related code.

https://reviewboard.mozilla.org/r/222246/#review229398

It's great to see this gone.
Attachment #8952983 - Flags: review?(tchiovoloni) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8856efbc78a7
p1 - Remove X-Weave-Alert support. r=markh
https://hg.mozilla.org/integration/autoland/rev/6e83f07e8f2f
p2 - Remove weave:ui:* related code. r=markh,tcsc
Backed out 2 changesets (bug 1439777) for browser chrome in browser/components/customizableui/test/browser_remote_tabs_button.js and xpc-shell failures in services/sync/tests/unit/test_errorhandler_2.js on a CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=166018817
https://hg.mozilla.org/integration/autoland/rev/f00eccec1c8378c6468215ccfe1cd3d8a252b640
Flags: needinfo?(eoger)
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25507e523aec
Backed out 2 changesets for browser chrome in  browser/components/customizableui/test/browser_remote_tabs_button.js and xpc-shell failures in services/sync/tests/unit/test_errorhandler_2.js on a CLOSED TREE
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1af3426dc956
p1 - Remove X-Weave-Alert support. r=markh
https://hg.mozilla.org/integration/autoland/rev/d99402ad9ecc
p2 - Remove weave:ui:* related code. r=markh,tcsc
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fea7978f3af6
Backed out 2 changesets for xpcshell failures on /test_errorhandler_2.js
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76c0ebfa6f08
p1 - Remove X-Weave-Alert support. r=markh
https://hg.mozilla.org/integration/autoland/rev/157f93b585b1
p2 - Remove weave:ui:* related code. r=markh,tcsc
https://hg.mozilla.org/mozilla-central/rev/76c0ebfa6f08
https://hg.mozilla.org/mozilla-central/rev/157f93b585b1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.