DebugProtocolTypes should be deleted

RESOLVED FIXED in Firefox 18

Status

defect
--
trivial
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
Firefox 18
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

7 years ago
In toolkit/devtools/debugger/dbg-client.jsm, the DebugProtocolTypes table doesn't seem to do any worthwhile job.

It doesn't catch typos: referring to an undefined property will get you 'undefined', which JSON.stringify turns into 'null' --- all silently.

If we simply wrote out the packet names in the code, we'd get an unrecognizedPacketType or missingParameter error response when we make a mistake.

This is hardly critical, but it shows up in every patch anyone submits that implements a new request; it's kind of distracting. And I'll bet it makes merges fail.
Assignee

Comment 1

7 years ago
Untested; I'm rebuilding FF now.
Assignee: nobody → jimb
It's definitely not critical, but I've found that editor/IDE code completion in fact help avoid typos when completing the property name. On the other hand, I can see how the undefined/null issue can be a pain, even though I've never been bitten by it.
At minimum we should be consistent between the client and server. The server currently uses strings directly. Either remove the protocol types from the client, or add them to the server.
Assignee

Comment 4

7 years ago
Comment on attachment 655088 [details] [diff] [review]
Remove DebugProtocolTypes table.

This passes the xpcshell tests.
Attachment #655088 - Flags: review?(past)
Assignee

Comment 6

7 years ago
(In reply to Panos Astithas [:past] from comment #2)
> It's definitely not critical, but I've found that editor/IDE code completion
> in fact help avoid typos when completing the property name. On the other
> hand, I can see how the undefined/null issue can be a pain, even though I've
> never been bitten by it.

Ah, is that the rationale for DebugProtocolTypes: that IDEs will present those names for completion? (My only worry about removing it - it seems like needless abstraction - is that I didn't really understand why it was added in the first place.)

In Emacs, at least, dabbrev-expand, on M-/, will complete the current string using other strings found in the buffer. I find myself using dabbrev-expand to complete even a single character at the end of the string - simply because if it works, that confirms that that string appears elsewhere in the buffer (or, in some buffer).
Assignee

Updated

7 years ago
Status: NEW → ASSIGNED
Comment on attachment 655088 [details] [diff] [review]
Remove DebugProtocolTypes table.

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

(In reply to Jim Blandy :jimb from comment #6)
> (In reply to Panos Astithas [:past] from comment #2)
> > It's definitely not critical, but I've found that editor/IDE code completion
> > in fact help avoid typos when completing the property name. On the other
> > hand, I can see how the undefined/null issue can be a pain, even though I've
> > never been bitten by it.
> 
> Ah, is that the rationale for DebugProtocolTypes: that IDEs will present
> those names for completion? (My only worry about removing it - it seems like
> needless abstraction - is that I didn't really understand why it was added
> in the first place.)

It was mostly that, as well as a documentation aid to a newcomer.

> In Emacs, at least, dabbrev-expand, on M-/, will complete the current string
> using other strings found in the buffer. I find myself using dabbrev-expand
> to complete even a single character at the end of the string - simply
> because if it works, that confirms that that string appears elsewhere in the
> buffer (or, in some buffer).

Yeah, my editor also supports it, and that is why I will not miss this :-)
Attachment #655088 - Flags: review?(past) → review+
Assignee

Comment 8

7 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f088ffa33f0
Flags: in-testsuite-
Target Milestone: --- → Firefox 18
Assignee

Comment 9

7 years ago
Forgot to refresh the patch to incorporate conflict's resolution before pushing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e4e6857c0c

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.