Closed Bug 1188413 Opened 9 years ago Closed 9 years ago

Add a key shortcut or always accessible UI to reload devtools

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox44 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

patch v3
(deleted), patch
Details | Diff | Splinter Review 
We shouldn't have to open gcli to reload devtools (via `tools reload` command). We may just suggest setting devtools.loader.srcdir pref and then have a key shortcut and/or some UI within Firefox (not devtools) to allow reloading it at anytime.
It is really important to not depend on any devtools code (or very few of it)
so that we can recover if we end up breaking it!!
Attached patch wip patch (obsolete) (deleted) — — Splinter Review 
Quick patch to have SHIFT+TAB reloading tools
while working around the fact that gcli is currently broken on reload (bug 1188417).

You need patches from bug 1172010 that landed today to make reload to work.
Attached patch patch (obsolete) (deleted) — — Splinter Review 
The previous patch was incomplete.
With all the recent patch I attached in various bugs,
we can start coding on devtools without rebooting Firefox.

You can modify XUL and JS files or tools and see them being updated next time you open the toolbox.
I'm now looking via bug 1190452 in order to ensure we can do the same thing with server files.
Comment on attachment 8642537 [details] [diff] [review]
patch

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

Nice! I had started on this a while back, but was blocked by bug 1172010.

::: browser/devtools/framework/toolbox.js
@@ +502,5 @@
>      let toggleKey = this.doc.getElementById("toolbox-toggle-host-key");
>      toggleKey.addEventListener("command", this.switchToPreviousHost.bind(this), true);
>  
> +    let reloadKey = this.doc.getElementById("tools-reload-key");
> +    reloadKey.addEventListener("command", this.reload.bind(this), true);

In my WIP I have this gated on the user having already set devtools.loader.srcdir, as a small optimization for most users who won't hack on devtools:

+    if (Services.prefs.prefHasUserValue("devtools.loader.srcdir")) {

::: browser/devtools/framework/toolbox.xul
@@ +80,5 @@
>           modifiers="accel shift"/>
> +    <key id="tools-reload-key"
> +         keycode="VK_TAB"
> +         oncommand="void(0);"
> +         modifiers="shift"/>

My idea was to use Cmd/Ctrl-Alt-Shift-R, which seems more memorable ("forced reload with Alt"):

+    <key id="devtools-reload-key"
+         keycode="&toolboxReload.key;"
+         oncommand="void(0);"
+         modifiers="accel alt shift"/>
> ::: browser/devtools/framework/toolbox.xul
> @@ +80,5 @@
> >           modifiers="accel shift"/>
> > +    <key id="tools-reload-key"
> > +         keycode="VK_TAB"
> > +         oncommand="void(0);"
> > +         modifiers="shift"/>
> 
> My idea was to use Cmd/Ctrl-Alt-Shift-R, which seems more memorable ("forced
> reload with Alt"):

This is too hard for me to strike! I need my two hands to press such combinaison :o
Attached patch patch (obsolete) (deleted) — — Splinter Review 
Added the pref check, kept the same keybinding (open to change to something
else as soon as it won't require breaking my fingers to hit it)
Attachment #8639893 - Attachment is obsolete: true
Attachment #8642537 - Attachment is obsolete: true
Ryan, Brian, What do you think about the keybinding story here?

Cmd/Ctrl-Alt-Shift-R is too hard for me to type.
Shift+TAB is really a random choice I made and can easily be confusing.

Or should we make a UI button/menuitem instead (or also) (which would appear only if you have the pref)?
Assignee: nobody → poirot.alex
Flags: needinfo?(jryans)
Flags: needinfo?(bgrinstead)
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> Ryan, Brian, What do you think about the keybinding story here?
> 
> Cmd/Ctrl-Alt-Shift-R is too hard for me to type.
> Shift+TAB is really a random choice I made and can easily be confusing.
> 
> Or should we make a UI button/menuitem instead (or also) (which would appear
> only if you have the pref)?

How about ctrl+alt+r?  This is only when you've set a source dir for tools reload, so it's not going to be hit by a non-devtools dev.

In the future it'd be nice if the reload command could automatically figure out the source dir if you've run it via `./mach run`, in which case the key shortcut could be hidden behind the browser chrome debugging pref instead.   Do you think that would be possible eventually?
Flags: needinfo?(bgrinstead)
Comment on attachment 8650527 [details] [diff] [review]
patch

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

For the key, shift-tab does seem easy to confuse...  I don't have many great suggestions though.

We already use Cmd-Shift-Alt-I for browser toolbox, so I am not too bothered by Cmd-Shift-Alt-R...  Seems nice to have *some* shortcut though!

A UI button is probably overkill for a DevTools dev-only feature.

::: browser/devtools/framework/toolbox.js
@@ +1588,5 @@
>    },
>  
> +  reload: function () {
> +    const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> +    devtools.reload();

This seems a bit crazy from toolbox.js, given that the toolbox file will itself be reloaded!  But, I guess it must work or you would have put it somewhere else...
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> We already use Cmd-Shift-Alt-I for browser toolbox, so I am not too bothered
> by Cmd-Shift-Alt-R...  Seems nice to have *some* shortcut though!

Yeah ctrl+alt+shift+r is actually fine with me too
Sorry to insist to have a decent key combinations.
We are talking about a *reload* one, that hopefully you are going to hit quite often.
If we manage to improve live reload, we all are going to tend to reload more often as it's cheap!
It is a bit different from browser toolbox, which hopefully you open via --jsdebugger in you dev cycle. If I have to twist my fingers everytime I tweak a CSS or a add a new dump/log... I'll end up requiring some hand-specialized physiotherapist sessions :)

If you are all fine with ctrl+alt+shift+r, I hope you are all going to be fine with Ctlr+Alt+r ?
Note that there is more space around F5, Alt+F5, Shift+F5 seem to be free.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> A UI button is probably overkill for a DevTools dev-only feature.

I said that, because I think we should be adding a UI within devtools options panel.
If we enable chrome pref, we would display a folder-picker there to select a custom source dir.
It would make this feature discoverable and wouldn't require knowing about a hidden command in gcli to know about it.

> ::: browser/devtools/framework/toolbox.js
> @@ +1588,5 @@
> >    },
> >  
> > +  reload: function () {
> > +    const {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
> > +    devtools.reload();
> 
> This seems a bit crazy from toolbox.js, given that the toolbox file will
> itself be reloaded!  But, I guess it must work or you would have put it
> somewhere else...

It works because that's just an alias for Loader.jsm's reload.
toolbox's reload is just a listener. Loader.jsm does the job.
It is like binding reload command directly to Loader.jsm:devtools.reload.
Also it is similar to calling location.reload(), it works fine as soon as
you don't attempt to do anything alter reload call.

The only alternative would be to set the key listener in browser.xul
rather than in toolbox.xul.
(In reply to Brian Grinstead [:bgrins] from comment #7)
> In the future it'd be nice if the reload command could automatically figure
> out the source dir if you've run it via `./mach run`, in which case the key
> shortcut could be hidden behind the browser chrome debugging pref instead.  
> Do you think that would be possible eventually?

Yep! We should do that once this features works great. There is still various stuff being broken.
gcli doesn't support reload, webide is still broken when using local sources. And I haven't looked at everything yet.
(In reply to Alexandre Poirot [:ochameau] from comment #10)
> If you are all fine with ctrl+alt+shift+r, I hope you are all going to be
> fine with Ctlr+Alt+r ?
> Note that there is more space around F5, Alt+F5, Shift+F5 seem to be free.

Assuming the shortcut only works when the pref is set (making it opt-in), I am fine with any of these.  However, do check different OSes, I believe some of these may not be available on Mac vs. the others.

For example, Shift-F5 is "Open Performance Panel" on Mac.

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> > A UI button is probably overkill for a DevTools dev-only feature.
> 
> I said that, because I think we should be adding a UI within devtools
> options panel.
> If we enable chrome pref, we would display a folder-picker there to select a
> custom source dir.
> It would make this feature discoverable and wouldn't require knowing about a
> hidden command in gcli to know about it.

Ah, I see!  That seems reasonable to me.
Blocks: 1198759
Attached patch patch v2 (obsolete) (deleted) — — Splinter Review 
With Ctrl+Alt+R. Still open to reasonably easy to type shortcut.
I added a tweak in reload in order to reopen the toolbox if we asked for reload
from the toolbox, as we most likely want it to reopen on the same tab.
Attachment #8650527 - Attachment is obsolete: true
(In reply to Alexandre Poirot [:ochameau] from comment #11)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> > A UI button is probably overkill for a DevTools dev-only feature.
> 
> I said that, because I think we should be adding a UI within devtools
> options panel.
> If we enable chrome pref, we would display a folder-picker there to select a
> custom source dir.
> It would make this feature discoverable and wouldn't require knowing about a
> hidden command in gcli to know about it.

If we can get to a point where we have `./mach run` support working (as in Comment 12) then this would become unnecessary.  I don't think it's worth spending the time / effort adding a folder-picker in the meantime.  In its current, this feature is probably only interesting to a small handful of people, so they could just set the pref manually.  When it's more clear exactly what we want the workflow to be for contributors we could re-evaluate adding a UI for it.

Ideally I'd see it working like this eventually:
If the program was run via `./mach run` AND chrome debugging is true then it will 'just work' based on the path of the local build.  I guess if you've specified a custom path from the pref then it would use that path instead of the application path (and work regardless of chrome debugging pref).  If there's neither a path from the pref or a local build directory detected, then reload functionality would be disabled.

And I don't think we need to wait for it to be 100% functional before proceeding like that.  If it works most of the time at least, by enabling it for people with chrome debugging we are more likely to get DevTools devs using it and reporting bugs.
(In reply to Brian Grinstead [:bgrins] from comment #15)
> (In reply to Alexandre Poirot [:ochameau] from comment #11)
> > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> > > A UI button is probably overkill for a DevTools dev-only feature.
> > 
> > I said that, because I think we should be adding a UI within devtools
> > options panel.
> > If we enable chrome pref, we would display a folder-picker there to select a
> > custom source dir.
> > It would make this feature discoverable and wouldn't require knowing about a
> > hidden command in gcli to know about it.
> 
> If we can get to a point where we have `./mach run` support working (as in
> Comment 12) then this would become unnecessary.

./mach run is not what I'm targetting. This is just to help current workflow and help existing gecko contributors.
Hopefully we can have a contribution workflow where ./mach is not necessary!!
Being able to do ./mach run, means that you built Firefox.
It is extremelly important for me to let contributors work on devtools codebase without having to build Firefox. That's, for me, the most important thing to fix in the contribution friction program.

So such UI would be here to say: hey you can easily hack on devtools, just tell us where the sources are. May be a UI in devtools options isn't the best. May be it should be more hidden, with a dedicated panel/page opened via a link with more information. May be we should keep it as a about:config. I don't know, but at some point, if we want to communicate about this it will be easier to have a dedicated UI for it.

> Ideally I'd see it working like this eventually:
> If the program was run via `./mach run` AND chrome debugging is true then it
> will 'just work' based on the path of the local build.

Yes, that's a very good first iteration. But I easily see that as becoming painful to always have to set this pref when you clobber.

> And I don't think we need to wait for it to be 100% functional before
> proceeding like that.  If it works most of the time at least, by enabling it
> for people with chrome debugging we are more likely to get DevTools devs
> using it and reporting bugs.

Yes, this is just not working "most of the time" yet (at least, webide is completely broken).
It would be great to have more than just me playing with this pref before enabling it automagically via mach.
Attachment #8652883 - Flags: review?(past)
Comment on attachment 8652883 [details] [diff] [review]
patch v2

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

Ship It!
Attachment #8652883 - Flags: review?(past) → review+
Status: NEW → ASSIGNED
Note that Ctrl+Alt+R is also the default shortcut of Restartless Restart (https://addons.mozilla.org/en-Us/firefox/addon/restartless-restart/), which might be installed by a fair number of extension developers.
(In reply to Simon Lindholm from comment #21)
> Note that Ctrl+Alt+R is also the default shortcut of Restartless Restart
> (https://addons.mozilla.org/en-Us/firefox/addon/restartless-restart/), which
> might be installed by a fair number of extension developers.

Hmm, true, I also have this add-on installed.
Attached patch patch v3 (deleted) — — Splinter Review 
Could the macos failure be related to keycode instead of key attribute
being used in xul markup?
I don't see why it would only fail on Mac, but may be...

Otherwise, I tried with the addon installed,
the addon still works fine. It overrides the firefox key binding
(or may be both code still runs, but at least firefox restarts).

I imagine most devtools developers won't need this addon anymore,
especially since there is a UI element for this in firefox UI now.

Our goal here is to ensure you can hack devtools without rebooting Firefox.
So if anyone still needs to reboot firefox while working on devtools,
it means that we have something to improve/fix.
Attachment #8652883 - Attachment is obsolete: true
(In reply to Alexandre Poirot [:ochameau] from comment #24)
> Created attachment 8666800 [details] [diff] [review]
> patch v3
> 
> Could the macos failure be related to keycode instead of key attribute
> being used in xul markup?
> I don't see why it would only fail on Mac, but may be...

Looks like it was it.
https://hg.mozilla.org/mozilla-central/rev/e9644106d540
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
This change should probably be communicated on the devtools mailing list. I'm sure a lot of people don't know about it.
Flags: needinfo?(poirot.alex)
Blocks: 1217559
This now lives in the addon, is documented on MDN and potch will post to Hacks about the addon.
Flags: needinfo?(poirot.alex)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: