740948 - Scratchpad should provide a quick way to reload the tab and re-run the code

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, enhancement, P3)

Description

[Cedric Vivier [:cedricv]]

I propose the following addition to the Execute menu (Run is Ctrl-R):

Reload tab and run  Ctrl-Shift-R


This could help when doing 'intrusive' prototyping with the Scratchpad (eg. doing massive DOM changes).

Thoughts?

Comment 1

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

it could be useful, though we'd have to disable that in Browser context. Not sure it warrants its own menu item or not.

Comment 2

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #1)
> it could be useful, though we'd have to disable that in Browser context. Not
> sure it warrants its own menu item or not.

FWIW, I don't think it deserves a place in the context menu (actually I have a bit of a mixed opinion with our current use of the context menu with duplication of most of Execute).
But the Execute menu otoh is far from feeling crowded imho.

Kevin, what do you think?

Comment 3

[Kevin Dangoor]

I agree that this seems reasonable for the Execute menu.

One thing to consider here is that when we pull the tools together into a combined or separate window interface (and if that includes Scratchpad in some form), users will want to be able to reload the page from within the tools. That might change our thoughts around keyboard shortcuts here.

Comment 4

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

We still want this. Good first bug?

Filter on BLACKEAGLE?!

Comment 5

[sam]

i'm looking to make my first contribution and saw this bug as a great opportunity. anyone willing to help me out?

Comment 6

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

(In reply to sam from comment #5)
> i'm looking to make my first contribution and saw this bug as a great
> opportunity. anyone willing to help me out?

Awesome!

First, Panos said he'd volunteer to mentor for this bug, so I don't want to step on his toes, but I can give you a few pointers to get started.

Source code is located in http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/.

Pay particular attention to scratchpad.js therein.

Check out: https://developer.mozilla.org/en-US/docs/Developer_Guide, notably the https://developer.mozilla.org/En/Developer_Guide/Source_Code and https://developer.mozilla.org/En/Developer_Guide/Build_Instructions.

If I haven't scared you off yet, join us in irc://irc.mozilla.org/#devtools and say hi. We're happy to help you get setup and hacking.

Comment 7

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> (In reply to sam from comment #5)
> > i'm looking to make my first contribution and saw this bug as a great
> > opportunity. anyone willing to help me out?
> 
> Awesome!
> 
> First, Panos said he'd volunteer to mentor for this bug, so I don't want to
> step on his toes, but I can give you a few pointers to get started.
> 
> Source code is located in
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/.
> 
> Pay particular attention to scratchpad.js therein.
> 
> Check out: https://developer.mozilla.org/en-US/docs/Developer_Guide, notably
> the https://developer.mozilla.org/En/Developer_Guide/Source_Code and
> https://developer.mozilla.org/En/Developer_Guide/Build_Instructions.
> 
> If I haven't scared you off yet, join us in irc://irc.mozilla.org/#devtools
> and say hi. We're happy to help you get setup and hacking.

Yes, as Rob says feel free to ask for help if you encounter any obstacles you can't overcome on your own.

Comment 8

[Amod [:AMoz]]

Sir, I would like to contribute to this bug. If the bug is not assigned to anybody then I would be highly obliged if u assign the bug to me...Will u explain the bug in detail ?

Comment 9

[Anton Kovalyov (:anton)]

Attachment: Proposed patch (no tests yet).

Attached is a proposed (working) patch. It adds a new menu item "Reload And Run" that is enabled only within the content context. Clicking on it will reload the page and run the entire editor as soon as Window object is ready.

(Initially I planned to use the 'load' event but then figured that this way we will prevent people from executing their Scratchpad code before the page is fully loaded. And if they want to wait for that they can always use window.addEventListener('load', ...); themselves)

I'd like to ask for feedback while I'm working on a patch:

 1. Is the overall approach reasonable? 
 2. This is the first time I'm modifying XUL and other non-JS files. Does everything look good there?

Thanks!

Comment 10

[Heather Arthur [:harth]]

(In reply to Anton Kovalyov (:anton) from comment #9)
> Created attachment 660507 [details] [diff] [review]
> Proposed patch (no tests yet).
> 
> Attached is a proposed (working) patch. It adds a new menu item "Reload And
> Run" that is enabled only within the content context. Clicking on it will
> reload the page and run the entire editor as soon as Window object is ready.
> 
> (Initially I planned to use the 'load' event but then figured that this way
> we will prevent people from executing their Scratchpad code before the page
> is fully loaded. And if they want to wait for that they can always use
> window.addEventListener('load', ...); themselves)

The first time you write a scratchpad or open one you wouldn't want an window onload handler. Once you've switched to reloading and running, you'd have to add an onload handler. It seems like you'd have to switch between these, and adding+removing an onload handler or using boilerplate wouldn't be ideal, imo.

>  2. This is the first time I'm modifying XUL and other non-JS files. Does
> everything look good there?

Looks good to me!

It seems like this could use a keyboard shortcut. Otherwise you could reload the page, switch back to scratchpad and re-run the scratchpad in about as much time using the keyboard.

Comment 11

[Anton Kovalyov (:anton)]

Attachment: Proposed patch with tests.

Attached is my patch ready for review. Per Heather's comment we decided to wait for the 'load' event instead of DOMWindowCreated to avoid confusion. Also added a shortcut for Reload And Run (command-E).

Comment 12

[Heather Arthur [:harth]]

Comment on attachment 660583 [details] [diff] [review]
Proposed patch with tests.

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

Looks good, nice test, thanks!

Just a couple nits and removing of event listeners, otherwise good to go.

::: browser/devtools/scratchpad/scratchpad.js
@@ +451,4 @@
>    },
>  
>    /**
> +   * Reload the current page and execute the entire editor content when

nit: break up lines that go over 80 chars.

::: browser/devtools/scratchpad/test/browser_scratchpad_bug740948_reload_and_run.js
@@ +13,5 @@
> +{
> +  waitForExplicitFinish();
> +  Services.prefs.setBoolPref(DEVTOOLS_CHROME_ENABLED, true);
> +
> +	gBrowser.selectedTab = gBrowser.addTab();

Looks like there are some tabs in this test, two-space indentation for js.

@@ +51,5 @@
> +  sp.setText(EDITOR_TEXT);
> +
> +  let browser = gBrowser.selectedBrowser;
> +
> +  browser.addEventListener("DOMWindowCreated", function () {

Remove this event listener after you catch the event.

@@ +52,5 @@
> +
> +  let browser = gBrowser.selectedBrowser;
> +
> +  browser.addEventListener("DOMWindowCreated", function () {
> +    browser.contentWindow.addEventListener("foo", function () {

Remove this even listener too after catching the event.

Comment 13

[Anton Kovalyov (:anton)]

Attachment: Final patch

Comment 14

[Anton Kovalyov (:anton)]

Browser Chrome Test Summary
	Passed: 6888
	Failed: 0
	Todo: 9

*** End BrowserChrome Test Results ***

Comment 15

[Heather Arthur [:harth]]

As discussed in IRC, we're going with the original shortcut key that was mentioned in the bug, Ctrl-Shift-R.

http://hg.mozilla.org/integration/fx-team/rev/793faea87850

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/793faea87850