Closed
Bug 785466
Opened 9 years ago
Closed 9 years ago
DebugProtocolTypes should be deleted
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 18
People
(Reporter: jimb, Assigned: jimb)
Details
Attachments
(1 file)
8.01 KB,
patch
|
past
:
review+
|
Details | Diff | Splinter Review |
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 2•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Comment on attachment 655088 [details] [diff] [review] Remove DebugProtocolTypes table. This passes the xpcshell tests.
Attachment #655088 -
Flags: review?(past)
Assignee | ||
Comment 5•9 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=4d6e98580de4
Assignee | ||
Comment 6•9 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•9 years ago
|
Status: NEW → ASSIGNED
Comment 7•9 years ago
|
||
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•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f088ffa33f0
Flags: in-testsuite-
Target Milestone: --- → Firefox 18
Assignee | ||
Comment 9•9 years ago
|
||
Forgot to refresh the patch to incorporate conflict's resolution before pushing: https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e4e6857c0c
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6f088ffa33f0 https://hg.mozilla.org/mozilla-central/rev/f6e4e6857c0c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•