Last Comment Bug 895180 - Add a new Scratchpad context - remote connection
: Add a new Scratchpad context - remote connection
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Scratchpad (show other bugs)
: Trunk
: All All
P3 normal with 2 votes (vote)
: Firefox 27
Assigned To: Brandon Benvie [:benvie]
:
: Julian Descottes [:jdescottes]
Mentors:
Depends on: 825039
Blocks: tb-debugger
  Show dependency treegraph
 
Reported: 2013-07-17 16:14 PDT by Brandon Benvie [:benvie]
Modified: 2013-10-04 22:14 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remote-scratchpad.patch (7.89 KB, patch)
2013-08-28 03:10 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
remote-scratchpad.patch (8.62 KB, patch)
2013-08-28 03:11 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
remote-scratchpad.patch (8.63 KB, patch)
2013-08-30 06:26 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
WIP3 (9.56 KB, patch)
2013-09-09 14:05 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
WIP4 (9.56 KB, patch)
2013-09-09 15:11 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
WIP4 (11.01 KB, patch)
2013-09-09 15:13 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
WIP5 (24.38 KB, patch)
2013-09-09 18:58 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
WIP6 (24.90 KB, patch)
2013-09-10 14:34 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
WIP7 (26.22 KB, patch)
2013-09-11 11:30 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
WIP8 (31.34 KB, patch)
2013-09-11 16:02 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
WIP9 (31.30 KB, patch)
2013-09-12 08:51 PDT, Brandon Benvie [:benvie]
rcampbell: review+
Details | Diff | Splinter Review
WIP10 (32.92 KB, patch)
2013-09-13 09:26 PDT, Brandon Benvie [:benvie]
dcamp: review+
Details | Diff | Splinter Review
WIP11 (33.00 KB, patch)
2013-09-24 12:18 PDT, Brandon Benvie [:benvie]
no flags Details | Diff | Splinter Review
WIP12 (36.56 KB, patch)
2013-10-01 19:39 PDT, Brandon Benvie [:benvie]
mihai.sucan: review+
Details | Diff | Splinter Review
WIP13 (37.88 KB, patch)
2013-10-04 12:44 PDT, Brandon Benvie [:benvie]
bbenvie: review+
Details | Diff | Splinter Review

Description User image Brandon Benvie [:benvie] 2013-07-17 16:14:48 PDT
Scratchpad needs a new context so the user can execute to a remotely connected DebuggerServer.
Comment 1 User image Panos Astithas [:past] 2013-07-19 10:48:09 PDT
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 User image Brandon Benvie [:benvie] 2013-08-28 03:10:28 PDT
Created attachment 796563 [details] [diff] [review]
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 User image Brandon Benvie [:benvie] 2013-08-28 03:11:13 PDT
Created attachment 796565 [details] [diff] [review]
remote-scratchpad.patch
Comment 4 User image Brandon Benvie [:benvie] 2013-08-30 06:26:16 PDT
Created attachment 797849 [details] [diff] [review]
remote-scratchpad.patch

rebase
Comment 5 User image Brandon Benvie [:benvie] 2013-09-09 14:05:27 PDT
Created attachment 801774 [details] [diff] [review]
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 User image Brandon Benvie [:benvie] 2013-09-09 15:11:23 PDT
Created attachment 801836 [details] [diff] [review]
WIP4

Forgot to add scratchpad-panel.js.
Comment 7 User image Brandon Benvie [:benvie] 2013-09-09 15:13:44 PDT
Created attachment 801837 [details] [diff] [review]
WIP4

fail
Comment 8 User image Anton Kovalyov (:anton) 2013-09-09 15:39:43 PDT
Oh dear god this patch is going to bit rot my patch for bug 912260 so hard. Or vice versa. :)
Comment 9 User image Rob Campbell [:rc] (:robcee) 2013-09-09 15:55:42 PDT
it's a race!™
Comment 10 User image Brandon Benvie [:benvie] 2013-09-09 17:37:39 PDT
Going crazy trying to figure out why this won't work!
Comment 11 User image Brandon Benvie [:benvie] 2013-09-09 18:58:02 PDT
Created attachment 801970 [details] [diff] [review]
WIP5

I'm a dunce, it was working. How does computer work.
Comment 12 User image Brandon Benvie [:benvie] 2013-09-10 14:34:24 PDT
Created attachment 802599 [details] [diff] [review]
WIP6
Comment 13 User image Brandon Benvie [:benvie] 2013-09-11 11:30:42 PDT
Created attachment 803186 [details] [diff] [review]
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 User image Brandon Benvie [:benvie] 2013-09-11 16:02:04 PDT
Created attachment 803352 [details] [diff] [review]
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 User image Brandon Benvie [:benvie] 2013-09-12 08:51:54 PDT
Created attachment 803780 [details] [diff] [review]
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 User image Brandon Benvie [:benvie] 2013-09-12 12:54:45 PDT
https://tbpl.mozilla.org/?tree=Try&rev=9b9d95ccfc57
Comment 17 User image Rob Campbell [:rc] (:robcee) 2013-09-13 05:53:11 PDT
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 User image Rob Campbell [:rc] (:robcee) 2013-09-13 06:05:36 PDT
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 User image Rob Campbell [:rc] (:robcee) 2013-09-13 06:08:06 PDT
presumably this needs another try run.
Comment 20 User image Brandon Benvie [:benvie] 2013-09-13 08:54:45 PDT
Think I got hosed by something else landing, checking into it.
Comment 21 User image Brandon Benvie [:benvie] 2013-09-13 09:26:36 PDT
Created attachment 804511 [details] [diff] [review]
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 User image Brandon Benvie [:benvie] 2013-09-24 12:18:12 PDT
Created attachment 809364 [details] [diff] [review]
WIP11

Rebase. Still needs that test.
Comment 23 User image Brandon Benvie [:benvie] 2013-10-01 19:39:23 PDT
Created attachment 812936 [details] [diff] [review]
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 User image Mihai Sucan [:msucan] 2013-10-03 11:33:26 PDT
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 User image Brandon Benvie [:benvie] 2013-10-04 12:44:26 PDT
Created attachment 813791 [details] [diff] [review]
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 User image Brandon Benvie [:benvie] 2013-10-04 13:47:06 PDT
https://hg.mozilla.org/integration/fx-team/rev/c99db0ab0cfa
Comment 27 User image Phil Ringnalda (:philor) 2013-10-04 22:14:54 PDT
https://hg.mozilla.org/mozilla-central/rev/c99db0ab0cfa

Note You need to log in before you can comment on or make changes to this bug.