Closed Bug 1235370 Opened 8 years ago Closed 8 years ago

Implement ProtocolError

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(firefox46 affected)

RESOLVED DUPLICATE of bug 1256397
Tracking Status
firefox46 --- affected

People

(Reporter: linclark, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Splitting this out from Bug #1037992

This is the latest review from Bug 1037992, comment 15, by jryans:


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

::: devtools/server/protocol.js
@@ +907,5 @@
>      console.error(err);
>      if (err.stack) {
>        dump(err.stack);
>      }
> +    if (err instanceof ProtocolError) {

Please add a test of this new protocol.js behavior.  See devtools/server/tests/unit/test_protocol_*.

@@ +1438,5 @@
> + *   Optional. Other fields to be included in the error packet.
> + * @constructor
> + */
> +var ProtocolError = Class({
> +  initialize: function (error, message = "", packet = {}) {

Hmm, any reason to prefer this style over just accepting an object for the whole thing including error and message?

Does it make sense to extend from the Error object?
Blocks: 1037992
Blocks: 1235371
Blocks: 1235375
Notes from IRC discussion with Eddy:

ProtocolError was introduced so that additional properties in an error could be communicated to the client. For example, the lastPausedUrl property [1] is used by some client code [2]. 

With the current ProtocolError patch, the additional properties won't reach the client code. This is because protocol.js only uses packet.error and package.message [3].

Rather than rewriting how protocol.js handles errors (or putting in some other work around) the current plan is to figure out where the client code depends on these custom properties. If possible, we will rewrite the client code to not depend on these custom properties. Then we could use regular Errors and wouldn't need to rewrite protocol.js to handle custom error packets.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/script.js?from=script.js%3A1230#1002
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/debugger-controller.js#378
[3] https://dxr.mozilla.org/mozilla-central/source/devtools/server/protocol.js?from=protocol.js%3A1230#1230
Assignee: nobody → lclark
Status: NEW → ASSIGNED
I am going to unlick this cookie.
Assignee: lclark → nobody
Status: ASSIGNED → NEW
Going to fix this a part of bug 1256397
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: