Simplify sync's ErrorHandler

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: tcsc, Assigned: eoger)

Tracking

unspecified
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

a year ago
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

Updated

a year ago
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 3

a year ago
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
Reporter

Comment 4

a year ago
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)
Reporter

Comment 5

a year ago
(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 6

a year ago
mozreview-review
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 7

a year ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Reporter

Comment 11

a year ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

a year ago
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)

Comment 18

a year ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

a year ago
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

Comment 23

a year ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

a year ago
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

Comment 28

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/76c0ebfa6f08
https://hg.mozilla.org/mozilla-central/rev/157f93b585b1
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.