Closed Bug 785466 Opened 9 years ago Closed 9 years ago

DebugProtocolTypes should be deleted

Categories

(DevTools :: Debugger, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: jimb, Assigned: jimb)

Details

Attachments

(1 file)

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.
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.
Comment on attachment 655088 [details] [diff] [review]
Remove DebugProtocolTypes table.

This passes the xpcshell tests.
Attachment #655088 - Flags: review?(past)
(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).
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f088ffa33f0
Flags: in-testsuite-
Target Milestone: --- → Firefox 18
Forgot to refresh the patch to incorporate conflict's resolution before pushing:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e4e6857c0c
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.