Closed Bug 1385452 Opened 4 years ago Closed 4 years ago

Add a keyboard shortcut in local builds to restart the browser

Categories

(Firefox :: General, enhancement)

Product:
Firefox
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1385452 - Add a keyboard shortcut in local builds to restart the browser;
59 bytes, text/x-review-board-request
mossop
: review+
Details 
./mach watch [0] will allow for an improved local development workflow in which the frontend changes are built as they are saved on the file system.

So instead of a common current workflow:

1. mach build faster
2. start firefox
3. edit files
4. quit firefox
5. goto 1

It can be something like this:

1. mach watch
2. start firefox
3. edit files
4. restart firefox (via keyboard shortcut)
5. goto 3

A keyboard shortcut isn't strictly necessary but it means that you don't need to flip back to the terminal anymore (step 2).

There is a (now incompatible) "Restartless restart" addon [1] that did something similar with the cmd/ctrl + alt + r shortcut.

[0]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Incremental_builds_with_filesystem_watching
[1]: https://addons.mozilla.org/en-US/firefox/addon/restartless-restart/
We can make features like this conditional on build settings and only enable the feature for local/developer builds. Let me know if you need help with that.

We could also do things like bake the restart logic into `mach run`. If you can deposit something in the Firefox profile, `mach run` could look for that signal after the process exits and restart Firefox automatically.
We also need to make sure that -purgecaches is passed to Firefox, otherwise certain changes from watch don't seem to be picked up on the next |mach run|. I'm assuming it's safe to make that the default behavior for |mach run| with artifact builds though.
(In reply to Gregory Szorc [:gps] from comment #1)
> We can make features like this conditional on build settings and only enable
> the feature for local/developer builds. Let me know if you need help with
> that.

Thanks, I've pushed up a work in progress patch. I'm wrapping the logic in a `!AppConstants.MOZ_OFFICIAL` condition, but maybe it would be better to have a new build setting. What do you think?

> We could also do things like bake the restart logic into `mach run`. If you
> can deposit something in the Firefox profile, `mach run` could look for that
> signal after the process exits and restart Firefox automatically.

A couple things I noticed that the current patch does well using the call to nsIAppStartup.quit is that (a) the restart is fast and (b) it restores the previously opened tabs. Whereas if I quit firefox then `mach run` again it forgets my opened tabs and loads my home page (I presume moving this logic out into mach run would have similar behavior, but maybe there's a relatively simple fix for that).
Flags: needinfo?(gps)
This triggers some inconsistent behavior in the terminal on OSX that can also be seen when toggling e10s in about:preferences or with a simpler STR:

1) ./mach run
2) Paste this into browser console:

  Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup)
    .quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart);

3) ctrl+c in the terminal

The running Firefox doesn't get killed but you can enter commands again. So if you enter `./mach run`, you predictably get the "only one copy of Firefox can be open at a time" error.
Yeah, I dunno that much about how Firefox decides what to do on start/restart. Presumably there's a way to control that outside of Firefox (or could be if someone writes the patches).

Considering Firefox has a remote control protocol and we can literally inject chrome JS into a running Firefox from Python, I think it makes sense to focus on what we can do from within Firefox itself. If we need to change `mach run` to use marionette so we can do fancy things, we'll do that.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #1)
> We can make features like this conditional on build settings and only enable
> the feature for local/developer builds. Let me know if you need help with
> that.

Yes, I think it would be good to preprocess on something more clear and searchable than `ifndef MOZ_OFFICIAL_BRANDING`.  Maybe we can add a new build setting like MOZ_LOCAL_DEV. Can you help me figure out how to do that? It looks like MOZ_OFFICIAL_BRANDING is defined in old-configure, but I also see a MOZILLA_OFFICIAL in moz.configure.
Flags: needinfo?(gps)
MOZILLA_OFFICIAL has more to do with branding.

glandium: Clearly identifying "is this a dev build" has come up a few times recently. Could we expose something explicit so consumers don't key off MOZILLA_RELEASE or things like whether omni.ja is flat (like what the OS X sandbox is doing, ugh). I think I'd be ok with a variable that is implied unless --enable-release or MOZ_AUTOMATION are in play.
Flags: needinfo?(gps) → needinfo?(mh+mozilla)
Went ahead and requested review with MOZ_OFFICIAL_BRANDING, but that can be switched to another build option if we decide to add one.
Huh no, MOZ_OFFICIAL_BRANDING is strictly worse than MOZILLA_OFFICIAL.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #12)
> Huh no, MOZ_OFFICIAL_BRANDING is strictly worse than MOZILLA_OFFICIAL.

Is !MOZILLA_OFFICIAL a reliable way to identify "is this a dev build"?
Flags: needinfo?(mh+mozilla)
(In reply to Brian Grinstead [:bgrins] from comment #13)
> (In reply to Mike Hommey [:glandium] from comment #12)
> > Huh no, MOZ_OFFICIAL_BRANDING is strictly worse than MOZILLA_OFFICIAL.
> 
> Is !MOZILLA_OFFICIAL a reliable way to identify "is this a dev build"?

It's not _reliable_ -- that's what some of the discussion in these tickets has been about -- but it's what we've done in a few places.  (In particular, to purge caches on Android.)
Comment on attachment 8891554 [details]
Bug 1385452 - Add a keyboard shortcut in local builds to restart the browser;

https://reviewboard.mozilla.org/r/162668/#review171330

Other than not being sure that this is the right environment variable to use this seems fine.

::: browser/base/content/browser-development-helpers.js:19
(Diff revision 3)
> +    let env = Cc["@mozilla.org/process/environment;1"]
> +            .getService(Components.interfaces.nsIEnvironment);

We generally put the period at the end of the first line and line up getService with Cc on lines like this.

::: browser/base/content/browser-development-helpers.js:23
(Diff revision 3)
> +    Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup)
> +        .quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart);

Services.startup.quit...
Attachment #8891554 - Flags: review?(dtownsend) → review+
(In reply to Dave Townsend [:mossop] from comment #15)
> Other than not being sure that this is the right environment variable to use
> this seems fine.

Switched this to MOZILLA_OFFICIAL (which is the same thing used for the browser toolbox settings)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
See Also: → 1388471
(In reply to Nick Alexander :nalexander [parental leave until September 15th] from comment #14)
> (In reply to Brian Grinstead [:bgrins] from comment #13)
> > (In reply to Mike Hommey [:glandium] from comment #12)
> > > Huh no, MOZ_OFFICIAL_BRANDING is strictly worse than MOZILLA_OFFICIAL.
> > 
> > Is !MOZILLA_OFFICIAL a reliable way to identify "is this a dev build"?
> 
> It's not _reliable_ -- that's what some of the discussion in these tickets
> has been about -- but it's what we've done in a few places.  (In particular,
> to purge caches on Android.)

I filed Bug 1388471 to continue that discussion
Flags: needinfo?(mh+mozilla)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02bb58ce4347
Add a keyboard shortcut in local builds to restart the browser;r=mossop
See Also: → 1388552
https://hg.mozilla.org/mozilla-central/rev/02bb58ce4347
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Blocks: 1389169
Depends on: 1438487
See Also: → 1656787
You need to log in before you can comment on or make changes to this bug.