785466 - DebugProtocolTypes should be deleted

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Jim Blandy :jimb]

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.

Comment 1

[Jim Blandy :jimb]

Attachment: Remove DebugProtocolTypes table.

Untested; I'm rebuilding FF now.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 4

[Jim Blandy :jimb]

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

This passes the xpcshell tests.

Comment 5

[Jim Blandy :jimb]

Try: https://tbpl.mozilla.org/?tree=Try&rev=4d6e98580de4

Comment 6

[Jim Blandy :jimb]

(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).

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

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 :-)

Comment 8

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f088ffa33f0

Comment 9

[Jim Blandy :jimb]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f088ffa33f0
https://hg.mozilla.org/mozilla-central/rev/f6e4e6857c0c