895180 - Add a new Scratchpad context - remote connection

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect, P3)

Description

[Brandon Benvie [:benvie]]

Scratchpad needs a new context so the user can execute to a remotely connected DebuggerServer.

Comment 1

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

I was thinking, if a remote toolbox included the scratchpad icon on the right and had some way to pass the target to it, then we wouldn't need an explicit scratchpad context or connection screen. I haven't looked into this, but IIRC those tools (scratchpad, tilt, etc.) don't have panel definitions with isTargetSupported() and constructors that take the target as a parameter. Joe may remember why, but if we can make them more like the other tools, that would go a long way towards letting them target remote devices.

Comment 2

[Brandon Benvie [:benvie]]

Attachment: remote-scratchpad.patch

Rather than create a new context option, this patch adds a Scratchpad tab to the toolbox that uses the toolbox's target. This is an early WIP.

Comment 3

[Brandon Benvie [:benvie]]

Attachment: remote-scratchpad.patch

Comment 4

[Brandon Benvie [:benvie]]

Attachment: remote-scratchpad.patch

rebase

Comment 5

[Brandon Benvie [:benvie]]

Attachment: WIP3

Changes ScratchpadPanel.jsm to be scratchpad-panel.js, uses module loader. Something changed recently though, and the panel will not open when I add the Scratchpad as a tool. Not getting any useful debug dump either, so having a hell of a time debugging it.

Comment 6

[Brandon Benvie [:benvie]]

Attachment: WIP4

Forgot to add scratchpad-panel.js.

Comment 7

[Brandon Benvie [:benvie]]

Attachment: WIP4

fail

Comment 8

[Anton Kovalyov (:anton)]

Oh dear god this patch is going to bit rot my patch for bug 912260 so hard. Or vice versa. :)

Comment 9

[Rob Campbell [:rc] (:robcee)]

it's a race!™

Comment 10

[Brandon Benvie [:benvie]]

Going crazy trying to figure out why this won't work!

Comment 11

[Brandon Benvie [:benvie]]

Attachment: WIP5

I'm a dunce, it was working. How does computer work.

Comment 12

[Brandon Benvie [:benvie]]

Attachment: WIP6

Comment 13

[Brandon Benvie [:benvie]]

Attachment: WIP7

The last piece remaining is to update the webconsole so that when clicking on a console message link from a Scratchpad that lives in a devtools toolbox, it focuses the toolbox and switches to the Scratchpad tab.

Rob, I just wanted you to check that this is something we actually want landing. When I discussed it with other people, there seemed to be agreement that adding a Scratchpad tab for remote targets was acceptable.

Comment 14

[Brandon Benvie [:benvie]]

Attachment: WIP8

This iteration makes it so clicking the link from the webconsole that resulted from a Scratchpad tab console message will switch to the correct tab and focus the editor.

Comment 15

[Brandon Benvie [:benvie]]

Attachment: WIP9

I'm going to go ahead and upgrade this to a review, since this is complete in its current form and it would be nice to get this landed for 26.

Comment 16

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=9b9d95ccfc57

Comment 17

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 803780 [details] [diff] [review]
WIP9

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

this looks pretty clean.

We could probably use a unittest to verify that we get the correct scratchpad when clicking via the webconsole.

tagging dcamp for changes to main.js.

::: browser/devtools/main.js
@@ +231,5 @@
>    Tools.jsdebugger,
>    Tools.inspector,
>    Tools.jsprofiler,
> +  Tools.netMonitor,
> +  Tools.scratchpad

should we add this behind a pref in case we need to disable in a later release?

Would also like Dave to review the loader changes.

Comment 18

[Rob Campbell [:rc] (:robcee)]

JavaScript error: resource://gre/modules/devtools/Loader.jsm, line 86: NS_ERROR_FILE_UNRECOGNIZED_PATH: Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsILocalFile.initWithPath]
JavaScript error: chrome://browser/content/browser.xul, line 1: DeveloperToolbar is not defined

running with this patch installed. Attempting to open the Toolbar.

Comment 19

[Rob Campbell [:rc] (:robcee)]

presumably this needs another try run.

Comment 20

[Brandon Benvie [:benvie]]

Think I got hosed by something else landing, checking into it.

Comment 21

[Brandon Benvie [:benvie]]

Attachment: WIP10

Did need a rebase, but there was also some tests to fix <:|. Should be fixed. Working on adding additional test now, but in the meantime have a tryrun:

https://tbpl.mozilla.org/?tree=Try&rev=658857742b35

Comment 22

[Brandon Benvie [:benvie]]

Attachment: WIP11

Rebase. Still needs that test.

Comment 23

[Brandon Benvie [:benvie]]

Attachment: WIP12

Adds a test confirming the link behavior is as expected. Mihai, could you review the webconsole changes for this? The test I added lives in the webconsole tests and the additional behavior in the Scratchpad link click handler. Thanks!

https://tbpl.mozilla.org/?tree=Try&rev=71cd0e2412a9

Comment 24

[Mihai Sucan [:msucan]]

Comment on attachment 812936 [details] [diff] [review]
WIP12

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

Patch looks good. Please remove all trailing white space from the patch.

r+ with the comments addressed.

::: browser/devtools/framework/test/browser_toolbox_options.js
@@ +77,5 @@
>    cleanup();
>  }
>  
>  function checkTools() {
> +  let toolsPref = panelWin.document.querySelectorAll("#default-tools-box > checkbox:not([label$='*'])");

is this safe? you might want an attribute or class name on the checkbox.

::: browser/devtools/webconsole/test/browser_webconsole_scratchpad_panel_link.js
@@ +51,5 @@
> +    yield scratchpad.run();
> +
> +    info("Clicking link to switch to and focus Scratchpad");
> +
> +    let [matched] = [...(yield message)[0].matched];

Nit: the test looks good but I was surprised to see waitForMessages() called before console.log() and without a yield.

Maybe you can move the |let message = wait...| line after the scratchpad.run() call, with a yield. Might be less confusing for me / more obvious that waitForMessages() returns a promise.

@@ +53,5 @@
> +    info("Clicking link to switch to and focus Scratchpad");
> +
> +    let [matched] = [...(yield message)[0].matched];
> +    ok(matched, "Found logged message from Scratchpad");
> +    let anchor = matched.querySelector("a");

The selector should probably be a.location.

@@ +56,5 @@
> +    ok(matched, "Found logged message from Scratchpad");
> +    let anchor = matched.querySelector("a");
> +    EventUtils.synthesizeMouse(anchor, 2, 2, {}, hud.iframeWindow);
> +
> +    is(aToolbox.getCurrentPanel(), scratchpadPanel,

The mouse click is sync but, IIAMN, it is only an implementation detail that the click also changes the current panel in sync. Should the test wait for the panel selection event?

::: browser/devtools/webconsole/webconsole.js
@@ +2589,5 @@
>  
>      // Make the location clickable.
>      this._addMessageLinkCallback(locationNode, () => {
>        if (isScratchpad) {
> +        // Check for matching top level Scratchpad window.

This callback function is growing too big. Can you please move the code from the if (isScratchpad) { ... } block into its own method? Please put it in hudservice.js, WebConsole.viewSourceInScratchpad(aURL, aLine), near the other viewSource*() methods.

Thanks!

Comment 25

[Brandon Benvie [:benvie]]

Attachment: WIP13

Addresses Mihai's comments:

* Change to '@@iterator' since bug 907077 landed
* Make options toolbox add "unsupported" attribute for unsupported tool checkboxes and use this in the test
* Reorder test so `scratchpad.run()` comes before `yield waitForMessage()` to make more sense.
* Change selector in test to be "a.location"
* change test to use an event listener for "scratchpad-selected" so as to not assume synchronicity
* move logic to WebConsole#viewSourceInScratchpad
* remove trailing whitespace

Comment 26

[Brandon Benvie [:benvie]]

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

Comment 27

[Phil Ringnalda (:philor)]

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