Closed
Bug 1150959
Opened 9 years ago
Closed 9 years ago
Report sw js errors more aggressively to js console
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: dougt, Assigned: nsm)
References
Details
Attachments
(1 file)
2.80 KB,
patch
|
bholley
:
review+
baku
:
feedback+
|
Details | Diff | Splinter Review |
this.addEventListener("activate", function(evt) { console.log("SW onactivate"); if (clients.claim) evt.waitUntil(clients.claim()); }); Since we don't support claim right now, this code silently fails. What we should try to do is log these failures to make developers lives easier.
Assignee | ||
Updated•9 years ago
|
Blocks: ServiceWorkers-B2G
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8591089 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Comment 3•9 years ago
|
||
Comment on attachment 8591089 [details] [diff] [review] Log rejected Promise error message to browser console Review of attachment 8591089 [details] [diff] [review]: ----------------------------------------------------------------- it looks good to me, but I prefer to have a review from somebody with more JS API knowledge than what I have.
Attachment #8591089 -
Flags: review?(amarchesini) → feedback+
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8591089 [details] [diff] [review] Log rejected Promise error message to browser console Bobby could you review jsapi usage.
Attachment #8591089 -
Flags: review?(bobbyholley)
Comment 5•9 years ago
|
||
Comment on attachment 8591089 [details] [diff] [review] Log rejected Promise error message to browser console Review of attachment 8591089 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerManager.cpp @@ +1074,5 @@ > + JS::Rooted<JSObject*> obj(aCx, workerPrivate->GlobalScope()->GetWrapper()); > + JS::Rooted<JS::Value> val(aCx, aValue); > + JS::ExposeValueToActiveJS(val); > + > + JSAutoCompartment ac(aCx, obj); Shouldn't we already be in this compartment? Can we jut assert it, and drop the subsequent wrap? There's only one global on workers, right? @@ +1082,5 @@ > + } > + > + js::ErrorReport report(aCx); > + if (NS_WARN_IF(!report.init(aCx, val))) { > + JS_ClearPendingException(aCx); This pattern is broken - it might limp along, but the best way is to pass an AutoJSAPI& instead of a JSContext*, for which the caller calls TakeOwnershipOfErrorReporting. That might be a bit much to do in this bug though. @@ +1087,5 @@ > + return; > + } > + > + nsRefPtr<xpc::ErrorReport> xpcReport = new xpc::ErrorReport(); > + xpcReport->Init(report.report(), report.message(), false /* isChrome */, 0 /* window id */); I'm used to styling this as |/* aArgumentName = */ value|, but maybe this is another convention that I'm not familiar with.
Attachment #8591089 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/de714e5d0a13
https://hg.mozilla.org/mozilla-central/rev/de714e5d0a13
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•