I think it can enhance the button functions, and it is useful in some cases.

Comment on attachment 8413081 [details] [diff] [review] addRestartMenu Review of attachment 8413081 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/globalOverlay.js @@ +60,5 @@ > > var appStartup = Components.classes['@mozilla.org/toolkit/app-startup;1']. > getService(Components.interfaces.nsIAppStartup); > > + if (action='restart') This is not a comparison, but an assignment. You should replace `=` with ` == ` (note that there should be spaces around the double-equals operator).

Comment on attachment 8413144 [details] [diff] [review] addRestartMenu v2 >+ <menupopup id="PanelUIquitContextMenu"> >+ <menuitem command="cmd_restartApplication" >+ accesskey="&panelUIquitRestartMenu.accesskey;" >+ label="&panelUIquitRestartMenu.label;"/> PanelUIquit -> PanelUIQuit >+<!ENTITY panelUIquitRestartMenu.label "Restart &brandShortName;"> >+<!ENTITY panelUIquitRestartMenu.accesskey "e"> Why e? >-function goQuitApplication() >+function goQuitApplication(action) > { > if (!canQuitApplication()) > return false; > > var appStartup = Components.classes['@mozilla.org/toolkit/app-startup;1']. > getService(Components.interfaces.nsIAppStartup); > >- appStartup.quit(Components.interfaces.nsIAppStartup.eAttemptQuit); >+ if (action == 'restart') >+ appStartup.quit(Components.interfaces.nsIAppStartup.eAttemptQuit | >+ Components.interfaces.nsIAppStartup.eRestart); How about an "aQuitModifier" parameter instead of the action string?

(In reply to Dão Gottwald [:dao] from comment #3) > PanelUIquit -> PanelUIQuit >> Ok, just follow the #PanelUI-quit before. > Why e? >> for customizeMenu.removeFromMenu.accesskey "R" on above, >> I fear conflict or careless misuse with habits, maybe. > How about an "aQuitModifier" parameter instead of the action string? >> Ok, I do not mind. Are there any other suggestions?

I wanted to UI-review this, but I can't find the change with the patch applied. Could you post a screenshot or direct me to where exactly that context menu is to be found? Thanks!

right-click the Exit button in the menu panel.

OK, that looks good. It didn't show up for me however. Is it possible that this is platform specific? (I applied the patch on Mac).

(In reply to Philipp Sackl [:phlsa] from comment #8) > OK, that looks good. > It didn't show up for me however. Is it possible that this is platform > specific? (I applied the patch on Mac). I don't know, I just tested it in Win8.1 for Nightly build. But it should works, I did not do any platform specific things. Maybe need more tests.

(In reply to Philipp Sackl [:phlsa] from comment #8) > It didn't show up for me however. Use a clean profile? if not, maybe startupcache or else things. or use DOM Inspector to check the id="PanelUI-quit", sure it have 'context' attr.

(In reply to YF (Yang) from comment #10) > (In reply to Philipp Sackl [:phlsa] from comment #8) > > It didn't show up for me however. > Use a clean profile? if not, maybe startupcache or else things. > or use DOM Inspector to check the id="PanelUI-quit", sure it have 'context' > attr. I tried a clean profile and a fresh clobber build, but it still didn't show up. Since this is a technical issue and not a UI issue, perhaps whoever is reviewing this has an idea how that is happening.

Comment on attachment 8413311 [details] [diff] [review] addRestartMenu v2.1 A content menu item on a menu panel button seems like a very strange interaction to me (and not at all discoverable), but I will defer to Philipp on that one. I think Jared (or another Australis reviewer) is a better choice to review the code.

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #12) > Comment on attachment 8413311 [details] [diff] [review] > addRestartMenu v2.1 > > A content menu item on a menu panel button seems like a very strange > interaction to me (and not at all discoverable), but I will defer to Philipp > on that one. I agree. For me this falls into the »doesn't hurt and might be a pleasant surprise for some«-category. Since there already is a patch it looked like low-hanging fruit, but the OS X issue might complicate things.

(In reply to Philipp Sackl [:phlsa] from comment #13) > (In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #12) > > Comment on attachment 8413311 [details] [diff] [review] > > addRestartMenu v2.1 > > > > A content menu item on a menu panel button seems like a very strange > > interaction to me (and not at all discoverable), but I will defer to Philipp > > on that one. > > I agree. For me this falls into the »doesn't hurt and might be a pleasant > surprise for some«-category. Since there already is a patch it looked like > low-hanging fruit, but the OS X issue might complicate things. Then, what about through Shift+Click the Exit button to restart the application?

I think the issue here is two-fold: 1) People shouldn't have a need to "restart" the browser. Can you provide some typical cases that you run in to where this feature is needed? 2) If something like this is determined necessary, then most likely a dropdown arrow or a subview would be the better approach, but then it is a question of adding visual noise or a second click for normal exiting. Note that (1) is a prerequisite for (2). Until then, "Restartless Restart" is a good add-on that provides this functionality.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15) > I think the issue here is two-fold: > > 1) People shouldn't have a need to "restart" the browser. Can you provide > some typical cases that you run in to where this feature is needed? Just an accidental ideas, I think that we can provide a restart feature in very low level of interference by the Exit button. I know the "Restartless Restart", it is great, but not for some special cases, such as always a new clean profile for tests. > 2) If something like this is determined necessary, then most likely a > dropdown arrow or a subview would be the better approach, but then it is a > question of adding visual noise or a second click for normal exiting. I know this may not be quite necessary for everyone, but it is intimate, practical and low interference, like as bug 862558. and downloads of the "Restartless Restart" may be able to prove some things, some people need this feature. Low risk + useful, why not?

(In reply to YF (Yang) from comment #16) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15) > I know the "Restartless Restart", it is great, but not for some special > cases, such as always a new clean profile for tests. Additional, I remember the Developer Toolbar (Shift+F2) - 'restart' command just a moment ago, it is can be used for this case. But I still consider that this will benefit users and low-interference, especially for non-keyboard workflow.

(In reply to YF (Yang) from comment #16) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15) > > I think the issue here is two-fold: > > > > 1) People shouldn't have a need to "restart" the browser. Can you provide > > some typical cases that you run in to where this feature is needed? > > Just an accidental ideas, I think that we can provide a restart feature in > very low level of interference by the Exit button. > I know the "Restartless Restart", it is great, but not for some special > cases, such as always a new clean profile for tests. I would also find this useful, although I caution that you and I are most likely not representative of most people who use Firefox. Since this is useful for things like a "clean profile for tests", it may be better to try to build in this feature to the test harnesses (for example by auto-installing the Restartless Restart add-on). Going that route would have more benefits, as there may be other test-specific functionality that people will want (auto-installing DOM Inspector, auto-enabling chrome and add-on debugging, to name a few). > > 2) If something like this is determined necessary, then most likely a > > dropdown arrow or a subview would be the better approach, but then it is a > > question of adding visual noise or a second click for normal exiting. > > I know this may not be quite necessary for everyone, but it is intimate, > practical and low interference, like as bug 862558. and downloads of the > "Restartless Restart" may be able to prove some things, some people need > this feature. Low risk + useful, why not? Being easy to implement and low interference aren't the only standards that we have to measure against. A feature like this will require documentation, translations, QA support, and possible follow-up work. I appreciate the time that you have spent towards making this change, but I don't think it is the right change to make for all people that use Firefox.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18) jaws, thanks for you reply, I agree with you now. I'm going to learn how to configure add-on for default installation. Do not worry, just a little frustration, I understand your consideration. Thanks all.