Victor Porof [:vporof][:vp] Assignee
	Description • 12 years ago

It's all a HUGE mess right now.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 2 • 12 years ago

Yay, arrow functions!

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 3 • 12 years ago

Comment on attachment 728716 [details] [diff] [review]
v1

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

This makes my heart smile in so many ways!
Just a few minor issues to fix and it's good to go.

::: browser/devtools/debugger/DebuggerPanel.jsm
@@ -58,5 @@
> -                                   onDebuggerConnected, true);
> -
> -    // Remote debugging gets the debuggee from a RemoteTarget object.
> -    if (this.target.isRemote) {
> -      this.panelWin._remoteFlag = true;

Forgot to set this? It is being used in _isRemoteDebugger and _isChromeDebugger.

@@ +51,5 @@
> +      .then(() => this._controller.startupDebugger())
> +      .then(() => this._controller.connect())
> +      .then(() => {
> +        this.emit("ready");
> +        this.isReady = true;

I'd feel a lot safer against bug-induced re-entrancy if isReady was set before we notify listeners.

::: browser/devtools/debugger/debugger-controller.js
@@ -106,5 @@
> -   * XXX: remove all this (bug 823577)
> -   * @return boolean
> -   *         True if connection should proceed normally, false otherwise.
> -   */
> -  _prepareConnection: function DC__prepareConnection() {

If you remove this here, you need to remove the other obsolete bits that it references (remoteDebuggerPromptTitle, remoteConnectionRetries, etc.). Some of them do not appear anywhere else (e.g. remoteDebuggerConnectionFailedMessage) and I'm afraid we will neglect to get rid of them when this is gone.

@@ +131,5 @@
>     * Initializes a debugger client and connects it to the debugger server,
>     * wiring event handlers as necessary.
> +   *
> +   * @return object
> +   *         A promise that is resolved when the debugger finishes conecting.

"connecting"

::: browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js
@@ -43,5 @@
>      gDebugger.addEventListener("Debugger:SourceShown", onSourceShown);
>  
> -    gDebugger.DebuggerController.activeThread.addOneTimeListener("framesadded",
> -      function onFramesAdded() {
> -        info("frames added");

These info() calls were useful in the past, let's leave them in.

::: browser/devtools/debugger/test/browser_dbg_cmd.js
@@ +28,5 @@
>          }
>  
>          let testCommands = function(dbg, cmd) {
>            // Wait for the initial resume...
> +          dbg.panelWin.gClient.addOneTimeListener("resumed", function() {

gThreadClient corresponds to _controller.activeThread.

::: browser/devtools/debugger/test/head.js
@@ -219,5 @@
>      let debuggee = tab.linkedBrowser.contentWindow.wrappedJSObject;
> -
> -    info("Opening Browser Debugger");
> -    DebuggerUI.toggleChromeDebugger(aOnClosing, function dbgRan(process) {
> -      info("Browser Debugger has started");

These two info() calls were very helpful in hunting down an orange in the past, because the whole remote process startup is pretty opaque in the test logs. Do you mind leaving them in?

	Victor Porof [:vporof][:vp] Assignee
	Comment 4 • 12 years ago

(In reply to Panos Astithas [:past] from comment #3)
> 
> ::: browser/devtools/debugger/DebuggerPanel.jsm
> @@ -58,5 @@
> > -                                   onDebuggerConnected, true);
> > -
> > -    // Remote debugging gets the debuggee from a RemoteTarget object.
> > -    if (this.target.isRemote) {
> > -      this.panelWin._remoteFlag = true;
> 
> Forgot to set this? It is being used in _isRemoteDebugger and
> _isChromeDebugger.
> 

A chrome debugger will never know about it anyway, and _isRemoteDebugger doesn't make sense anymore (and will be removed in bug 823577), afaik we don't even check for it anywhere anymore.

> @@ +51,5 @@
> > +      .then(() => this._controller.startupDebugger())
> > +      .then(() => this._controller.connect())
> > +      .then(() => {
> > +        this.emit("ready");
> > +        this.isReady = true;
> 
> I'd feel a lot safer against bug-induced re-entrancy if isReady was set
> before we notify listeners.
> 

Whoops!

> ::: browser/devtools/debugger/debugger-controller.js
> @@ -106,5 @@
> > -   * XXX: remove all this (bug 823577)
> > -   * @return boolean
> > -   *         True if connection should proceed normally, false otherwise.
> > -   */
> > -  _prepareConnection: function DC__prepareConnection() {
> 
> If you remove this here, you need to remove the other obsolete bits that it
> references (remoteDebuggerPromptTitle, remoteConnectionRetries, etc.). Some
> of them do not appear anywhere else (e.g.
> remoteDebuggerConnectionFailedMessage) and I'm afraid we will neglect to get
> rid of them when this is gone.
> 

Yeah, bug 823577. I've been adding notes in there.

	Victor Porof [:vporof][:vp] Assignee
	Comment 5 • 12 years ago

(In reply to Panos Astithas [:past] from comment #3)

> gThreadClient corresponds to _controller.activeThread.

True, however using gThreadClient in this case is wrong. I'm not even sure why this code used to work until now, since the pertinent thing to do (and what is actually done in debug_tab_pane from head.js, hence in all of the tests) was to wait for the initial resume on gClient. The fact that browser_dbg_cmd_break.js also waits on gClient, and browser_dbg_cmd.js isn't doing anything special on startup, makes me think this just used to be a typo.

Basically, browser_dbg_cmd.js is just reimplementing debug_tab_pane.

Same goes for bug723069_editor-breakpoints.js, where initialization is a copy paste endeavor from other tests. The key thing to remember here is that waiting for "Debugger:Connected" isn't necessary anymore (and also isn't fired anymore).

	Victor Porof [:vporof][:vp] Assignee
	Comment 6 • 12 years ago

Addressed comments.

	Victor Porof [:vporof][:vp] Assignee
	Comment 7 • 12 years ago

https://hg.mozilla.org/integration/fx-team/rev/b0e59cc09e1f

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 8 • 12 years ago

(In reply to Victor Porof [:vp] from comment #5)
> (In reply to Panos Astithas [:past] from comment #3)
> 
> > gThreadClient corresponds to _controller.activeThread.
> 
> True, however using gThreadClient in this case is wrong. I'm not even sure
> why this code used to work until now, since the pertinent thing to do (and
> what is actually done in debug_tab_pane from head.js, hence in all of the
> tests) was to wait for the initial resume on gClient. The fact that
> browser_dbg_cmd_break.js also waits on gClient, and browser_dbg_cmd.js isn't
> doing anything special on startup, makes me think this just used to be a
> typo.

I looked into this to remember how things work, and apparently both ways are fine. Most xpcshell tests and the debugger frontend listen on the ThreadClient, while most mochitests listen on the DebuggerClient.

All packets are first received by the DebuggerClient, who then dispatches response packets to the appropriate clients for handling. Unsolicited events though don't correspond to a client (even though we could have maintained a mapping from actors to clients that attach to them), so by default they are handled by the DebuggerCLient. Response packets that change the thread state (ThreadStateTypes) however, receive special treatment and are being passed to the ThreadClient as well. So, in the end all listeners for these events will be notified, both those attached to the DebuggerClient as well as the ones attached to the ThreadClient.

Bug 698841 will probably limit these events to the ThreadClient, but it's not in our near-term plans.

> Basically, browser_dbg_cmd.js is just reimplementing debug_tab_pane.
> 
> Same goes for bug723069_editor-breakpoints.js, where initialization is a
> copy paste endeavor from other tests. The key thing to remember here is that
> waiting for "Debugger:Connected" isn't necessary anymore (and also isn't
> fired anymore).

Yeah, Mihai preferred to keep this test special, but I agree that we should make sure the common way t initialize tests works in all cases.

	Victor Porof [:vporof][:vp] Assignee
	Comment 9 • 12 years ago

(In reply to Panos Astithas [:past] from comment #8)
> Yeah, Mihai preferred to keep this test special, but I agree that we should
> make sure the common way t initialize tests works in all cases.

Just to be make sure we're on the same page here, bug723069_editor-breakpoints.js certainly used to be special, but after switching to promises it wasn't anymore. All tests now use mihai's approach.

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/b0e59cc09e1f