Closed Bug 675125 Opened 11 years ago Closed 11 years ago

Utils.notify should pass observers the exception as subject

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: philikon, Assigned: philikon)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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: nobody → philipp
Attached patch v1 (obsolete) — Splinter Review
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-
Attached patch v2Splinter Review
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
Closed: 11 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.