1439777 - Simplify sync's ErrorHandler

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Thom Chiovoloni [:tcsc] (ex-moco)]

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.

Comment 1

[Edouard Oger [:eoger]]

Attachment: Bug 1439777 p1 - Remove X-Weave-Alert support.

Review commit: https://reviewboard.mozilla.org/r/222244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222244/

Comment 2

[Edouard Oger [:eoger]]

Attachment: Bug 1439777 p2 - Remove weave:ui:* related code.

Review commit: https://reviewboard.mozilla.org/r/222246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222246/

Comment 3

[Edouard Oger [:eoger]]

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

Comment 4

[Thom Chiovoloni [:tcsc] (ex-moco)]

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)

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

(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

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8952983 [details]
Bug 1439777 p2 - Remove weave:ui:* related code.

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

nice cleanup, thanks!

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8952982 [details]
Bug 1439777 p1 - Remove X-Weave-Alert support.

https://reviewboard.mozilla.org/r/222244/#review228502

Comment 8

[Edouard Oger [:eoger]]

Comment on attachment 8952982 [details]
Bug 1439777 p1 - Remove X-Weave-Alert support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222244/diff/1-2/

Comment 9

[Edouard Oger [:eoger]]

Comment on attachment 8952983 [details]
Bug 1439777 p2 - Remove weave:ui:* related code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222246/diff/1-2/

Comment 10

[Edouard Oger [:eoger]]

Opened https://github.com/mhammond/aboutsync/pull/46.

Comment 11

[Thom Chiovoloni [:tcsc] (ex-moco)]

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.

Comment 12

[Edouard Oger [:eoger]]

Comment on attachment 8952982 [details]
Bug 1439777 p1 - Remove X-Weave-Alert support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222244/diff/2-3/

Comment 13

[Edouard Oger [:eoger]]

Comment on attachment 8952983 [details]
Bug 1439777 p2 - Remove weave:ui:* related code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222246/diff/2-3/

Comment 14

[Edouard Oger [:eoger]]

Comment on attachment 8952982 [details]
Bug 1439777 p1 - Remove X-Weave-Alert support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222244/diff/3-4/

Comment 15

[Edouard Oger [:eoger]]

Comment on attachment 8952983 [details]
Bug 1439777 p2 - Remove weave:ui:* related code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222246/diff/3-4/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Stefan Hindli [:stefan_hindli]]

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

Comment 18

[Pulsebot]

Backout by [e-mail]:
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 19

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c782e07566772caba7e1b1c803f4018eaea55ed

Comment 20

[Edouard Oger [:eoger]]

Comment on attachment 8952982 [details]
Bug 1439777 p1 - Remove X-Weave-Alert support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222244/diff/4-5/

Comment 21

[Edouard Oger [:eoger]]

Comment on attachment 8952983 [details]
Bug 1439777 p2 - Remove weave:ui:* related code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222246/diff/4-5/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fea7978f3af6
Backed out 2 changesets for xpcshell failures on /test_errorhandler_2.js

Comment 24

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d4fa0cbcbe81ab3249f9345ca07fe3628a75b80

Comment 25

[Edouard Oger [:eoger]]

Comment on attachment 8952982 [details]
Bug 1439777 p1 - Remove X-Weave-Alert support.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222244/diff/5-6/

Comment 26

[Edouard Oger [:eoger]]

Comment on attachment 8952983 [details]
Bug 1439777 p2 - Remove weave:ui:* related code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222246/diff/5-6/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/76c0ebfa6f08
https://hg.mozilla.org/mozilla-central/rev/157f93b585b1