Allow ErrorResult objects to be cloned

RESOLVED FIXED in Firefox 44

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks: 1 bug)

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 wontfix, firefox44 fixed, firefox45 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
In bug 1217367 we need to report a single ErrorResult to multiple consumers.  Since ErrorResults are depletable, that means we need to clone the value to each.

Current API plan is ErrorResult::CloneTo(ErrorResult& aDest).
(Assignee)

Updated

2 years ago
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Created attachment 8688120 [details] [diff] [review]
Implement ErrorResult::CloneTo(). r=bz

I tested the ErrorWithMessage case locally.
Attachment #8688120 - Flags: review?(bzbarsky)
Comment on attachment 8688120 [details] [diff] [review]
Implement ErrorResult::CloneTo(). r=bz

Please document what the method actually does.

There's no reason not to make this work with JS exceptions, right?  Just make sure that CloneTo() copies the mMightHaveUnreportedJSException state and then if IsJSException() you can aRv.ThrowJSException() using the thread-default JSContext.

r=me with those fixed.
Attachment #8688120 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 3

2 years ago
Created attachment 8689011 [details] [diff] [review]
Implement ErrorResult::CloneTo(). r=bz

Add a comment to ErrorResult.h to document the call and support for JS exceptions in the cpp.

Asking for re-review since I don't trust my jsapi knowledge for handling the JS exceptions.
Attachment #8688120 - Attachment is obsolete: true
Attachment #8689011 - Flags: review?(bzbarsky)
Comment on attachment 8689011 [details] [diff] [review]
Implement ErrorResult::CloneTo(). r=bz

r=me
Attachment #8689011 - Flags: review?(bzbarsky) → review+

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc92635ec29a
Backed out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=17457460&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d9cf8dd2ed8
Flags: needinfo?(bkelly)

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b5636e31365
(Assignee)

Updated

2 years ago
Flags: needinfo?(bkelly)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2dbaa23095e

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1b5636e31365
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

2 years ago
status-firefox43: --- → wontfix
status-firefox44: --- → affected
(Assignee)

Comment 10

2 years ago
Comment on attachment 8689011 [details] [diff] [review]
Implement ErrorResult::CloneTo(). r=bz

Note, please uplift the m-c version of the patch.  There was a small change made there to fix opt builds.

Approval Request Comment
[Feature/regressing bug #]: Service workers.
[User impact if declined]: We need this in order to uplift service worker bug 1217367.
[Describe test coverage new/current, TreeHerder]: Test coverage provided in bug 1217637.
[Risks and why]: Minimal.  While this bug defines code in dom/bindings, its only called by service worker code today.  No other features will be affected by the uplift.
[String/UUID change made/needed]: None.
Attachment #8689011 - Flags: approval-mozilla-aurora?
Comment on attachment 8689011 [details] [diff] [review]
Implement ErrorResult::CloneTo(). r=bz

Approved. Please note Ben's request on the patch to land the one from m-c.
Attachment #8689011 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 12

2 years ago
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/940121320e6e
status-firefox44: affected → fixed
You need to log in before you can comment on or make changes to this bug.