Add a new Scratchpad context - remote connection

RESOLVED FIXED in Firefox 27

Status

()

Firefox
Developer Tools: Scratchpad
P3
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: benvie, Assigned: benvie)

Tracking

Trunk
Firefox 27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 14 obsolete attachments)

37.88 KB, patch
benvie
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
Scratchpad needs a new context so the user can execute to a remotely connected DebuggerServer.
(Assignee)

Updated

4 years ago
Priority: -- → P3
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.
Blocks: 876636
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
Created attachment 796565 [details] [diff] [review]
remote-scratchpad.patch
Attachment #796563 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Created attachment 797849 [details] [diff] [review]
remote-scratchpad.patch

rebase
Attachment #796565 - Attachment is obsolete: true
(Assignee)

Comment 5

4 years ago
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.
Assignee: nobody → bbenvie
Attachment #797849 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
Created attachment 801836 [details] [diff] [review]
WIP4

Forgot to add scratchpad-panel.js.
Attachment #801774 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Created attachment 801837 [details] [diff] [review]
WIP4

fail
Attachment #801836 - Attachment is obsolete: true
Oh dear god this patch is going to bit rot my patch for bug 912260 so hard. Or vice versa. :)
it's a race!™
(Assignee)

Comment 10

4 years ago
Going crazy trying to figure out why this won't work!
(Assignee)

Comment 11

4 years ago
Created attachment 801970 [details] [diff] [review]
WIP5

I'm a dunce, it was working. How does computer work.
Attachment #801837 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Created attachment 802599 [details] [diff] [review]
WIP6
Attachment #801970 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
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.
Attachment #802599 - Attachment is obsolete: true
Attachment #803186 - Flags: feedback?(rcampbell)
(Assignee)

Comment 14

4 years ago
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.
Attachment #803186 - Attachment is obsolete: true
Attachment #803186 - Flags: feedback?(rcampbell)
Attachment #803352 - Flags: feedback?(rcampbell)
(Assignee)

Comment 15

4 years ago
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.
Attachment #803352 - Attachment is obsolete: true
Attachment #803352 - Flags: feedback?(rcampbell)
Attachment #803780 - Flags: review?(rcampbell)
(Assignee)

Comment 16

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9b9d95ccfc57
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.
Attachment #803780 - Flags: review?(rcampbell)
Attachment #803780 - Flags: review?(dcamp)
Attachment #803780 - Flags: review+
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.
presumably this needs another try run.
(Assignee)

Comment 20

4 years ago
Think I got hosed by something else landing, checking into it.
(Assignee)

Comment 21

4 years ago
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
Attachment #803780 - Attachment is obsolete: true
Attachment #803780 - Flags: review?(dcamp)
Attachment #804511 - Flags: review?(dcamp)

Updated

4 years ago
Attachment #804511 - Flags: review?(dcamp) → review+
(Assignee)

Comment 22

4 years ago
Created attachment 809364 [details] [diff] [review]
WIP11

Rebase. Still needs that test.
Attachment #804511 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
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
Attachment #809364 - Attachment is obsolete: true
Attachment #812936 - Flags: review?(mihai.sucan)
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!
Attachment #812936 - Flags: review?(mihai.sucan) → review+
(Assignee)

Comment 25

4 years ago
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
Attachment #812936 - Attachment is obsolete: true
Attachment #813791 - Flags: review+
(Assignee)

Comment 26

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/c99db0ab0cfa
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c99db0ab0cfa
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.