Utils.notify should pass observers the exception as subject

RESOLVED FIXED in mozilla8

Status

()

RESOLVED FIXED
7 years ago
24 days ago

People

(Reporter: philikon, Assigned: philikon)

Tracking

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

Our ErrorHandler policy (bug 659067) and Syncorro will most definitely want to inspect the error thrown by various parts of Sync. We should pass them to the appropriate observers.
(Assignee)

Updated

7 years ago
Assignee: nobody → philipp
Created attachment 549311 [details] [diff] [review]
v1

This changes the API of Utils.notify a bit. Instead of using 'subject' as a pre-determined value, we now use 'data'. That way 'subject' can be used to either pass the return value or the exception of the function that's wrapped.

Using 'data' to track, say, the engine name also matches other observers out there, e.g. weave:engine:applied.
Attachment #549311 - Flags: review?(rnewman)
Comment on attachment 549311 [details] [diff] [review]
v1

Review of attachment 549311 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, and is a sane change. However, it causes two test failures on my machine -- did you run xpcshell-tests before submitting for review?

  test_interval_triggers.js
  test_engine.js

Let me know if you can't repro locally, and I'll provide the logs.

While you're touching these tests, it occurs to me to change the obscure magic values -- "one", "baz" -- to something slightly more meaningful, like "bar_data" and "bad_data". This would make the tests a little more comprehensible at first glance.
Attachment #549311 - Flags: review?(rnewman) → review-
Created attachment 549401 [details] [diff] [review]
v2

Yeah, it was late. In fact, I went to bed thinking that I had better run all tests in the morning to be sure. I also had this indescribable feeling that we actually used the subject somewhere, and of course my gut was right.

All good now.
Attachment #549311 - Attachment is obsolete: true
Attachment #549401 - Flags: review?(rnewman)
Attachment #549401 - Flags: review?(rnewman) → review+
http://hg.mozilla.org/services/services-central/rev/ced4ba4f15e9
Status: NEW → ASSIGNED
Whiteboard: [fixed in services][qa-]
http://hg.mozilla.org/mozilla-central/rev/ced4ba4f15e9
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa-] → [qa-]
Target Milestone: --- → mozilla8
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.