LiveDevelopment code cleanup

[jasonsanjose]

I hit a few minor issues when fixing the LiveDevelopment unit tests to work in brackets-shell:

• Inspector implements it's own on() and off() event handler management instead of using jQuery. The significant differences are that all handlers are triggered async and trigger() is made public and accessed by Agents.
• LiveDevelopment.open() should return a promise. Currently in LiveDevelopment-test, we have to poll LiveDevelopment until the relevant DocClass (related documents for the current live session) is loaded. Ideally open() would return a promise that resolves when this process is complete.
• LiveDevelopment installs a load event handler on Inspector which does not define a load event
• Status codes should be constants for readability and maintenance

[jasonsanjose]

@jdiehl or @joelrbrandt I think most of these are minor fix-ups except for the event handling. Can you guys recall the rationale there?

[jdiehl]

Inspector.js has its own event handling, because I wasn't sure that jQuery was the way to go in Brackets. The differences you mentioned can be fixed as following:

• Asynchronous triggers: can be ignored (this is not used anywhere)
• Agents triggering Inspector events: either make Inspector.trigger call $(exports).triggerHandler or let the agents emit their own event

I see three additional issues:

• jQuery events generate an event object that is passed as the first parameter: all Inspector event handling code must be changed to ignore this first parameter
• jQuery can only send events with a single parameter: if multiple parameters are passed, they must be wrapped in an object and the event handling code must be adapted
• jQuery uses a special naming scheme "event.nameSpace", which is in conflict with Inspector's naming scheme "domain.event": the only solution I can see (which I don't like) is to replace the . in inspector events with another character (e.g., -). This would required updating all event handling and emitting code as well as the documentation.

[jdiehl]

To address the open-should-return-a-promise issue, the live development startup process should be improved (this will also help later when supporting different debuggers):

Call LiveDevelopment.open to initiate a live connection

• get the open document
• determine an appropriate debugger for the document
• call DebuggerDiscovery.open to launch and find a connection to the debugger
  • launch the debugger if it is not running
  • open the page in the debugger if it is not opened
  • return the debugger port
• call Inspector.connect to connect to the debugger port
  • Establish the connection
• Load agents

This way the open method can be defined in a single function of LiveDevelopment, which returns a promise that resolves when the promises of all other components (DebuggerDiscovery and Inspector) are resolved.

[jdiehl]

LiveDevelopment.js:487   Inspector.on("load", _onLoad);

can be removed (_onLoad is triggered after loading the agents, not from the Inspector).

[DennisKehrig]

"jQuery can only send events with a single parameter: if multiple parameters are passed, they must be wrapped in an object and the event handling code must be adapted" (@jdiehl)

This single parameter can however be an array which gets unwrapped when calling the event handler:

$(foo).on("bar", function (event, a, b, c) { ... });
...
$(foo).triggerHandler("bar", [a, b, c]);

[DennisKehrig]

"jQuery uses a special naming scheme 'event.nameSpace', which is in conflict with Inspector's naming scheme 'domain.event': the only solution I can see (which I don't like) is to replace the . in inspector events with another character (e.g., -). This would required updating all event handling and emitting code as well as the documentation." (@jdiehl)

How about Inspector.Debugger.on("scriptParsed", ...)?

[pthiess]

Reviewed.

[jasonsanjose]

@jdiehl @DennisKehrig would either of you like to volunteer? If not, I'll take the assignment for next sprint.

[DennisKehrig]

Thanks, but I'll pass on this one!

[joelrbrandt]

@jason-sanjose: @jdiehl is on vacation, so he's unlikely to volunteer, too. :-)

@DennisKehrig: thanks for your help!

[jasonsanjose]

No worries. I'll get it. Thanks for the fast response.

[DennisKehrig]

My pleasure!

[jdiehl]

Is this still open? Can I help with anything?

[jasonsanjose]

Yep. I targeted it for this sprint. I'll defer to you if you'd like to take a first pass at it.

[jdiehl]

I should be able to work on this, tomorrow. Is the consensus to migrate to jQuery events? The remainder seems obvious enough.

[jasonsanjose]

Yep, sounds like a plan. I'll change the assignee. Thanks, Jonathan.

[jasonsanjose]

Confirmed. Closing.