Closed Bug 1302202 Opened 3 years ago Closed 3 years ago

Fix split console keypress handler

Categories

(DevTools :: Debugger, defect, P1)

defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: jlast, Assigned: jlast)

Details

Attachments

(1 file, 4 obsolete files)

The keypress handler relies on a reference to the threadclient which is different in the new panel.
Assignee: nobody → jlaster
Priority: -- → P1
Attached patch fix-console-expandy.patch (obsolete) — Splinter Review
Attachment #8790434 - Flags: review?(jlong)
Comment on attachment 8790434 [details] [diff] [review]
fix-console-expandy.patch

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

What the benefit of this over just doing `this.threadClient.state === "paused"` in toolbox.js? It feels a little weird to go into the JS panel just for it to reference `this.toolbox` again :)

If you wanted a more generic `isPaused` method it could go straight on the thread client, but not sure it's worth the work.
Attached patch fix-console-expandy2.patch (obsolete) — Splinter Review
Attachment #8790434 - Attachment is obsolete: true
Attachment #8790434 - Flags: review?(jlong)
Attached patch fix-console-expandy2.patch (obsolete) — Splinter Review
Attachment #8790469 - Attachment is obsolete: true
Attachment #8790471 - Flags: review?(jlong)
Attached patch fix-console-expandy3.patch (obsolete) — Splinter Review
Attachment #8790471 - Attachment is obsolete: true
Attachment #8790471 - Flags: review?(jlong)
Attachment #8790808 - Flags: review?(jlong)
Comment on attachment 8790808 [details] [diff] [review]
fix-console-expandy3.patch

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

::: devtools/client/framework/toolbox.js
@@ +551,5 @@
>        this.toggleSplitConsole();
>        // If the debugger is paused, don't let the ESC key stop any pending
>        // navigation.
>        let jsdebugger = this.getPanel("jsdebugger");
> +      if (jsdebugger && this._threadClient.state == "paused") {

You don't need access to `jsdebugger` at all anymore, I think this can be:

if(this.threadClient.state === "paused") {}

There's also a getter `threadClient` (non-prefixed) but I don't care either way about that.
that's a nice improvement. thanks
Attachment #8790808 - Attachment is obsolete: true
Attachment #8790808 - Flags: review?(jlong)
Comment on attachment 8790811 [details] [diff] [review]
fix-console-expandy4.patch

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

Looks good, thanks! :)

Not sure if this needs a try run. Might as well do one just in case, as this is toolbox-level code, if you don't mind. I'll probably push an update end of day today so this should make it in.
Attachment #8790811 - Flags: review+
Pushed by jlong@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/27c6658b2a90
Fix splitconsole keypress handler. r=jlongster
https://hg.mozilla.org/mozilla-central/rev/27c6658b2a90
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.