In safe-mode, Help -> Restart with Add-ons Disabled should switch to something to allow restarting in normal mode

RESOLVED FIXED in Firefox 40

Status

()

Firefox
Menus
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Unfocused, Assigned: manrajsingh, Mentored)

Tracking

(Blocks: 1 bug, {uiwanted, user-doc-needed})

unspecified
Firefox 40
uiwanted, user-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [good first bug][lang=js][lang=xul], URL)

Attachments

(3 attachments, 7 obsolete attachments)

To enter safe-mode via menu, there's Help -> Restart with Add-ons Disabled. However, when in safe-mode, that menu item stays the same. Instead, it should switch to something that allows restarting in normal mode.
Blocks: 614985
Relevant code is found in this MXR search:
https://mxr.mozilla.org/mozilla-central/search?string=safeModeRestart

I think a new <menuitem> can be added with JS toggling the visibility of the new and existing item such that they don't both appear at the same time.

Asking for UI input on the menu and confirmation dialog strings.
Blocks: 599835
Keywords: uiwanted
Whiteboard: [good first bug][mentor=MattN][lang=js][lang=xul]

Comment 2

6 years ago
Hi, is this one still a valid bug to work on? I've got a school project that requires us to work on an open source project and I was wondering if me and my partner could try this one out.
(In reply to Matthew Lai from comment #2)
> Hi, is this one still a valid bug to work on?

This is still a valid bug but I was helping someone with it last night. I didn't get it assigned to them but if you don't find another bug, you can check back in a week or so to see if there was any progress.
Status: NEW → ASSIGNED

Comment 4

6 years ago
Okay, thank you.

Comment 5

6 years ago
Hey this is vandhanaa. Maybe you can assign it to me to avoid confusion. :)
Assignee: nobody → vandhanaa91

Comment 6

6 years ago
Created attachment 614355 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/browser/

I was waiting for the UI people to get back to me regarding the Name. Meanwhile I have made the changes needed. Will submit a revised version if needed later on. 

Patch is only for /mozilla-central/browser/ folder.
Attachment #614355 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 614355 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/browser/

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

Sorry for the delay. Overall I think this is good.  I found a few minor problems (nits) but once those are fixed I think the patch will be ready for a browser peer to review. Thanks for the patch!

::: browser/base/content/browser.js
@@ +1514,5 @@
>    Cu.import("resource:///modules/TelemetryTimestamps.jsm", tmp);
>    let TelemetryTimestamps = tmp.TelemetryTimestamps;
>    TelemetryTimestamps.add("delayedStartupStarted");
>    gDelayedStartupTimeoutId = null;
> +  

Nit: Please leave the whitespace here and some other places as-is since you're not changing the adjacent code and it's preferred to not have trailing whitespace.

@@ +1566,5 @@
> +  let safeMode = document.getElementById("helpSafeMode");
> +  if(!Services.appinfo.inSafeMode)
> +  { 
> +    safeMode.label=safeMode.getAttribute("startlabel");  
> +  }

Nit: Braces on the same line as if/else please (in safeModeRestart too).

@@ +1569,5 @@
> +    safeMode.label=safeMode.getAttribute("startlabel");  
> +  }
> +  else 
> +  {
> +    safeMode.label=safeMode.getAttribute("stoplabel");

Nit: Spaces around operators (ie. =) please.

@@ +8949,5 @@
>  
>  // Prompt user to restart the browser in safe mode 
>  function safeModeRestart()
>  {
> +  if(!Services.appinfo.inSafeMode)

Since we're acting on the true and false case, I find it clearer to get rid of the ! in the condition and put the true case first.

@@ +8961,4 @@
>                       Services.prompt.BUTTON_TITLE_IS_STRING) +
>                      (Services.prompt.BUTTON_POS_1 *
>                       Services.prompt.BUTTON_TITLE_CANCEL) +
>                      Services.prompt.BUTTON_POS_0_DEFAULT;

Nit: Indent these 4 unmodified lines by 2 spaces so that it lines up again.

@@ +8976,1 @@
>      let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].

Since appStartup is used in both cases, move the declaration and initialization to the top of the function.

::: browser/base/content/utilityOverlay.js
@@ +506,5 @@
>  {
>    // Enable/disable the "Report Web Forgery" menu item.  safebrowsing object
>    // may not exist in OSX
>    if (typeof safebrowsing != "undefined")
> +    safebrowsing.setReportPhishingMenu();    

I don't think you intended to include this file in the patch.

::: browser/config/mozconfig
@@ +4,5 @@
>  
>  ac_add_options --enable-application=browser
> +mk_add_options MOZ_OBJDIR=/home/vandhanaa/mozilla-central/ff-dbg
> +mk_add_options MOZ_MAKE_FLAGS="-j4"
> +ac_add_options --disable-optimize

These changes should go in a new .mozconfig file in the root of your source directory.
Attachment #614355 - Flags: review?(mnoorenberghe+bmo) → feedback+
Comment on attachment 614355 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/browser/

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

::: browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd
@@ +16,5 @@
>  <!ENTITY productHelp.accesskey    "H">
>  <!ENTITY helpMac.commandkey       "?">
> +<!ENTITY helpSafeMode.start.label "Restart with Add-ons Disabled…">
> +<!ENTITY helpSafeMode.start.accesskey   "R">
> +<!ENTITY helpSafeMode.stop.label "Restart with Add-ons Enabled…">

If we're not showing a dialog to confirm the restart then we shouldn't have an ellipsis here.
Created attachment 617373 [details]
Screenshot of proposed menu item

UX: What are you thoughts on this? It's just switching to the label from "Disabled" to "Enabled" when the user is in safe mode.  Is the dialog confirmation dialog required when restarting into normal mode or should we just remove the ellipsis?

Thanks.
Attachment #617373 - Flags: ui-review?(ux-review)

Comment 10

6 years ago
Created attachment 617689 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/

Hi, I have fixed the Nits you have mentioned. 

In reply to Comment 8, I have the startlabel and stoplabel contents in the ellipsis in baseMenuOverlay.dtd
If You dont want them to be here, Would you rather have me hard code the contents in baseMenuOverlay.xul ?
Attachment #614355 - Attachment is obsolete: true
Attachment #617689 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 617689 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/

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

Seems straightforward enough, fine to skip explicit ui-review in this case.
Attachment #617689 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 617373 [details]
Screenshot of proposed menu item

Removing the ellipsis and enabling the add-ons works too, I have no strong preference here. Whatever is easier. :)
Attachment #617373 - Flags: ui-review?(ux-review) → ui-review+
Comment on attachment 617689 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/

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

f+ with these changes.  You can request review on the next revision from Blair McBride (:Unfocused).

::: browser/base/content/browser.js
@@ +1566,5 @@
> +  let safeMode = document.getElementById("helpSafeMode");
> +  if(Services.appinfo.inSafeMode) { 
> +    safeMode.label = safeMode.getAttribute("stoplabel");  
> +  }
> +  else {

braces beside the else please: } else {

@@ +1579,5 @@
>      var shouldCheck = false;
>  #else
>      var shouldCheck = shell.shouldCheckDefaultBrowser;
>  #endif
> +    var willRecoverSession = false;  

Nit: There are still some whitespace changes in this file which should be reverted.

@@ +8953,5 @@
> +  
> +  if(Services.appinfo.inSafeMode) {
> +	appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
> +  }
> +  else {    

Nit: fix else here too

::: browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd
@@ +16,5 @@
>  <!ENTITY productHelp.accesskey    "H">
>  <!ENTITY helpMac.commandkey       "?">
> +<!ENTITY helpSafeMode.start.label       "Restart with Add-ons Disabled…">
> +<!ENTITY helpSafeMode.start.accesskey   "R">
> +<!ENTITY helpSafeMode.stop.label        "Restart with Add-ons Enabled…">

The strings are fine in this file, just remove the … from "Enabled…".
Attachment #617689 - Flags: feedback+
Hey VD,

You're very close to finishing this patch. Keep up the good work!  If you need any clarification or help, let us know.

Comment 15

6 years ago
Created attachment 624044 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/
Attachment #617689 - Attachment is obsolete: true
Attachment #624044 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 624044 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/

r=me but I'll have Blair double-check it in case I missed something.

VD, what is the name and email address you want to use as part of the commit? Is your Bugzilla name and email address fine?

Thanks
Attachment #624044 - Flags: review?(mnoorenberghe+bmo)
Attachment #624044 - Flags: review?(bmcbride)
Attachment #624044 - Flags: review+
Comment on attachment 624044 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/

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

Looking good, vandhanaa.

On Windows and Linux you can disable the top menubar, which will enable a Firefox button with it's own menu (this is the default on Windows Vista and newer). That Firefox menu contains its own "Restart with add-ons Disabled..." menu item - we should have that be updated too.

The menuitem is defined here:
https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-appmenu.inc#455

::: browser/base/content/browser.js
@@ +8950,5 @@
> +  let appStartup = Cc["@mozilla.org/toolkit/app-startup;1"].
> +                     getService(Ci.nsIAppStartup);
> +  
> +  if(Services.appinfo.inSafeMode) {
> +	appStartup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);

Nit: We use spaces to indent lines, not tabs like you've used here.
Attachment #624044 - Flags: review?(bmcbride) → review-

Comment 18

6 years ago
Created attachment 626431 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/

Hey guys, Sorry for the delay in submitting the patch.
You can use the same name and email while submitting the patch. :)
Attachment #624044 - Attachment is obsolete: true
Attachment #626431 - Flags: review?(mnoorenberghe+bmo)
Attachment #626431 - Flags: review?(bmcbride)
Comment on attachment 626431 [details] [diff] [review]
Patch for Buf 728813 - /mozilla-central/

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

You're making progress, VD :)

::: browser/base/content/browser.js
@@ +1564,5 @@
>    gHomeButton.updatePersonalToolbarStyle(homeButton);
>  
> +  let safeMode = document.getElementById("helpSafeMode");
> +  if(Services.appinfo.inSafeMode) { 
> +    safeMode.label = safeMode.getAttribute("stoplabel");  

You've added added a second accesskey for this menuitem, so you'll also need to change the accesskey here too.

Also, you've added the additional strings for the menuitem in the Firefox menu, but you've forgotten to make that menuitem switch between those strings here.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +325,1 @@
>  <!ENTITY appMenuSafeMode.accesskey "R">

Looks like we don't even use the string for this accesskey :) Could you remove it? (appMenuSafeMode.accesskey)
Attachment #626431 - Flags: review?(mnoorenberghe+bmo)
Attachment #626431 - Flags: review?(bmcbride)
Attachment #626431 - Flags: review-
Hey VD,

Did you have a chance to make the changes mentioned in comment 19?

==

Adding user-doc-needed so https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode gets updated when this lands.
Keywords: user-doc-needed

Updated

6 years ago
Depends on: 765609

Comment 21

6 years ago
Steps to reproduce:

clicked restart with add-ons disabled



Actual results:

Got two options
1) start in safe mode
2) reset profile


Expected results:

like in FF13 & earlier there should be an option like

3) disable all Add-ons & plugin's and start in normal mode

Resetting does not help sometimes (say just want to reset tool-bars or
bookmarks & nothing else with minimal fuss & surely that the user will not mess
up!)


> * Disable all add-ons
> ** Start in safe mode (disables extensions) and then disable the plugins
> from the add-ons manager
when using approx 20+ extensions & plugins, when we find some problem we just
disable all addons in a go(try disabling even 10 extension one by one you will
see)
& then one by one check each addon to find the problem(leaks or bugs/website
problem)


> * Reset toolbars and controls
> ** Delete localstore.rdf from the profile

As a Power  user ok but What about the general people/non-power users?(adds
unnecessary extra steps plus a risk)
plus while helping them creates extra steps(say when simply restarting &
resetting search-box does the trick without any sort of risk)


> * Delete all bookmarks except for backups
> ** set browser.bookmarks.restore_default_bookmarks to true (before start-up
> or from about:config then restart)
> * Reset all user preferences
> ** Delete prefs.js (and the "preferences" directory if it exists) from the
> profile
> * Restore default search engines
> ** Delete search* from the profile
>

Same As ABOVE!

> If there was an option which commonly solved users' problems and is better
> than resetting Firefox then we can consider a way to address that.  Could
> you file bugs (blocking this one) which you have strong arguments for?


These are the options which commonly solved users' problems and is better than
resetting Firefox , just add a button to keep the old way too by keeping the
old code( & add a 3rd button) to start the old box(many people find it easy)

(instead of going to the profile & deleting stuff as it is not easy for the casual/home user)

Comment 22

5 years ago
(In reply to Sillius Soddus from comment #21)
> Steps to reproduce:
> 
> clicked restart with add-ons disabled
> 
> 
> 
> Actual results:
> 
> Got two options
> 1) start in safe mode
> 2) reset profile
> 
> 
> Expected results:
> 
> like in FF13 & earlier there should be an option like
> 
> 3) disable all Add-ons & plugin's and start in normal mode
> 
> Resetting does not help sometimes (say just want to reset tool-bars or
> bookmarks & nothing else with minimal fuss & surely that the user will not
> mess
> up!)
> 
> 
> > * Disable all add-ons
> > ** Start in safe mode (disables extensions) and then disable the plugins
> > from the add-ons manager
> when using approx 20+ extensions & plugins, when we find some problem we just
> disable all addons in a go(try disabling even 10 extension one by one you
> will
> see)
> & then one by one check each addon to find the problem(leaks or bugs/website
> problem)
> 
> 
> > * Reset toolbars and controls
> > ** Delete localstore.rdf from the profile
> 
> As a Power  user ok but What about the general people/non-power users?(adds
> unnecessary extra steps plus a risk)
> plus while helping them creates extra steps(say when simply restarting &
> resetting search-box does the trick without any sort of risk)
> 
> 
> > * Delete all bookmarks except for backups
> > ** set browser.bookmarks.restore_default_bookmarks to true (before start-up
> > or from about:config then restart)
> > * Reset all user preferences
> > ** Delete prefs.js (and the "preferences" directory if it exists) from the
> > profile
> > * Restore default search engines
> > ** Delete search* from the profile
> >
> 
> Same As ABOVE!
> 
> > If there was an option which commonly solved users' problems and is better
> > than resetting Firefox then we can consider a way to address that.  Could
> > you file bugs (blocking this one) which you have strong arguments for?
> 
> 
> These are the options which commonly solved users' problems and is better
> than
> resetting Firefox , just add a button to keep the old way too by keeping the
> old code( & add a 3rd button) to start the old box(many people find it easy)
> 
> (instead of going to the profile & deleting stuff as it is not easy for the
> casual/home user)

Will this be implemented?
(In reply to Sillius Soddus from comment #22)
> Will this be implemented?

That's a topic for bug 765609, not this bug.
Hi VD, I wanted to check in to see if you're still working on this bug. It's been a while, and it looks like things got a bit complicated in the comments. Are you still working on it? Should I check with the bug mentor? Thanks!
Flags: needinfo?(vandhanaa91)
Hey VD, since we haven't heard from you, I'm going to remove you as the assignee on the bug for now so someone else can continue.
Assignee: vandhanaa91 → nobody
Flags: needinfo?(vandhanaa91)

Comment 26

5 years ago
We'd like to  Replace "Restart with Add-ons disabled" with "Reset Firefox" in Help Menu now https://bugzilla.mozilla.org/show_bug.cgi?id=872240
Status: ASSIGNED → NEW
Mentor: MattN+bmo@mozilla.com
Whiteboard: [good first bug][mentor=MattN][lang=js][lang=xul] → [good first bug][lang=js][lang=xul]

Comment 27

3 years ago
Hi, I'd like to work on this. Where should I start?
(In reply to Chris from comment #27)
> Hi, I'd like to work on this. Where should I start?

Hello,

You can take the existing patch, update it and address comment 19.

Thanks

Comment 29

3 years ago
Created attachment 8489075 [details] [diff] [review]
728813.patch

Main differences from the previous version:
1. Updated to the latest revision;
2. Addressed, I hope, comment 19;
3. Reworked the logic of changes a bit. I don't like the idea of adding new attributes, so I just replaced strings.
Assignee: nobody → tim_tim2000
Attachment #626431 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8489075 - Flags: review?(MattN+bmo)
Comment on attachment 8489075 [details] [diff] [review]
728813.patch

>+    let safeMode = document.getElementById("helpSafeMode");
>+    if (Services.appinfo.inSafeMode) {
>+      safeMode.label = safeMode.label.replace("start", "stop");
>+      safeMode.accesskey = safeMode.accesskey.replace("start", "stop");

This doesn't work. safeMode.label is going to be "Restart with Add-ons Disabled…" here, not what you think.
Attachment #8489075 - Flags: review?(MattN+bmo) → review-

Comment 31

3 years ago
Created attachment 8492697 [details] [diff] [review]
Second attempt

Copied a bit more from VD's patch, but truly speaking I don't like the idea of polluting element with non-standart attributes. Is it Mozilla's way?
Attachment #8489075 - Attachment is obsolete: true
Attachment #8492697 - Flags: review?(MattN+bmo)
Comment on attachment 8492697 [details] [diff] [review]
Second attempt

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

Thanks. You're really close.

(In reply to Timur Timirkhanov from comment #31)
> Copied a bit more from VD's patch, but truly speaking I don't like the idea
> of polluting element with non-standart attributes. Is it Mozilla's way?

Yes, this is how we solve this kind of problem. We don't have to worry about attribute name conflicts with third-party libraries since we don't use them on the browser window.

::: browser/base/content/browser.js
@@ +1107,5 @@
>  
>      var homeButton = document.getElementById("home-button");
>      gHomeButton.updateTooltip(homeButton);
>      gHomeButton.updatePersonalToolbarStyle(homeButton);
> +	

Nit: trailing whitespace

@@ +1115,5 @@
> +      safeMode.accesskey = safeMode.getAttribute("stopaccesskey");
> +    } else {
> +      safeMode.label = safeMode.getAttribute("startlabel");
> +      safeMode.accesskey = safeMode.getAttribute("startaccesskey");
> +	}

Indentation is off here

@@ +7111,5 @@
>  function safeModeRestart()
>  {
> +  if (Services.appinfo.inSafeMode) {
> +    Services.startup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
> +  } else {

To avoid changing the indentation of this whole block, add an early return inside the if block:

if (Services.appinfo.inSafeMode) {	
  Services.startup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
  return;
}

::: browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd
@@ +23,5 @@
>  <!ENTITY helpKeyboardShortcuts.label     "Keyboard Shortcuts">
>  <!ENTITY helpKeyboardShortcuts.accesskey "K">
>  
> +<!ENTITY helpSafeMode.start.label       "Restart with Add-ons Disabled…">
> +<!ENTITY helpSafeMode.start.accesskey   "R">

It's preferred not to change the string ID if the string isn't actually changing since that will cause extra work for localizers
Attachment #8492697 - Flags: review?(MattN+bmo) → review-

Comment 33

3 years ago
Created attachment 8507835 [details] [diff] [review]
A third attempt to close this bug

I hope I've addressed all remarks from comment #32, and sorry for a delay.
Attachment #8492697 - Attachment is obsolete: true
Attachment #8507835 - Flags: review?(gavin.sharp)
Comment on attachment 8507835 [details] [diff] [review]
A third attempt to close this bug

>+                  startaccesskey="&helpSafeMode.accesskey;"
>+                  startlabel="&helpSafeMode.label;"

you can remove this

>+    } else {
>+      safeMode.label = safeMode.getAttribute("startlabel");
>+      safeMode.accesskey = safeMode.getAttribute("startaccesskey");

and this
Comment on attachment 8507835 [details] [diff] [review]
A third attempt to close this bug

Thanks for working on this, Timur!

Dao's suggestions make sense.

I think safeModeRestart() still needs to fire quit-application-requested in the restart-without-safemode case that you're adding.

helpSafeMode.label and helpSafeMode.stop.label should be consistent about whether they have an ellipsis. It sounds like we want them both to not have one, which means you'll need to change the entity name for helpSafeMode.label (you might as well change it to helpSafeMode.start.label).

With those changes, I think this is done. It might be best for us to chat in real-time about this patch, if you have any questions. You can find me or someone else able to help on #fx-team on irc.mozilla.org (see https://wiki.mozilla.org/IRC).
Attachment #8507835 - Flags: review?(gavin.sharp) → review-
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #35)
> helpSafeMode.label and helpSafeMode.stop.label should be consistent about
> whether they have an ellipsis. It sounds like we want them both to not have
> one, which means you'll need to change the entity name for
> helpSafeMode.label (you might as well change it to helpSafeMode.start.label).

I think they are intentionally inconsistent as we show a confirmation when going into safe mode but not when leaving. It's normal for the ellipsis to indicate that there is another step.
And since the two strings aren't together, I don't think the inconsistency would actually be a problem.

Comment 38

3 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #35)
> Dao's suggestions make sense.

Agree.

> I think safeModeRestart() still needs to fire quit-application-requested in
> the restart-without-safemode case that you're adding.

I don't know what's the best wording for this. Any ideas?

> helpSafeMode.label and helpSafeMode.stop.label should be consistent about
> whether they have an ellipsis. It sounds like we want them both to not have
> one, which means you'll need to change the entity name for
> helpSafeMode.label (you might as well change it to helpSafeMode.start.label).

I think that when we add a confirmation message as you suggested, helpSafeMode.stop.label will need to have an ellipsis as well.
(In reply to Timur Timirkhanov from comment #38)
> > I think safeModeRestart() still needs to fire quit-application-requested in
> > the restart-without-safemode case that you're adding.
> 
> I don't know what's the best wording for this. Any ideas?

You need to add some code to fire the notification and check its return value before calling Services.startup.quit, similar to:
http://hg.mozilla.org/mozilla-central/annotate/21fbf1e35090/browser/base/content/browser.js#l7251

(Sorry for the very delayed response - in the future, don't hesitate to needinfo? for any questions like this.)
Assignee: tim_tim2000 → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 40

3 years ago
Hi MattN sir,

Sorry for the delayed reply. Was out of state. I went through the comments and kind of got lost after reaching comment #21. Please let me what all has been completed and from which position should I start working. Also please assign the bug to me. Thank you.
Flags: needinfo?(MattN+bmo)
Hi Manraj,

I think you can just take the existing patch (from comment 33) and read from there down. Let me know if that isn't clear enough. It will help to look through and understand the patch to make sense of the comments.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 42

3 years ago
Hi sir,

Ohkay. What I see is these tasks are left:

1. Ask for confirmation when disabling safe mode. I feel a new notification function would be needed to be created for this and added after this

https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#777

2. For consistency labels would need to be changed with ellipses for Safe Mode now.

Please correct me if I missed anything or misinterpreted something.

Also sir please assign the bug to me. Thank you.
Flags: needinfo?(MattN+bmo)
Assignee: nobody → manrajsinghgrover
Status: NEW → ASSIGNED
(In reply to Manraj Singh [:manrajsingh] from comment #42)
> Ohkay. What I see is these tasks are left:

Hello, sorry for the delay. I was away from work for a bit recently.

The confirmation isn't necessary in my opinion since the user isn't going to a more crippled mode in general. We can do the work to add a confirmation later if others disagree.

I think what's left to do here is:
* comment 34 - we don't need to have the start strings duplicated from the existing @accesskey and @label attributes since we don't need to switch back to them at any point.
* comment 39 - before the early return when we restart in regular mode, there should be a quit-application-requested notification and we need to check the return value.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 44

3 years ago
Created attachment 8596114 [details] [diff] [review]
Toggles Safe mode text. Added notification for quiting.

Hi MattN sir,

No problem sir. I believe this is what is required. I hope this is needs to be addressed in comment #39 . Please review the patch and let me know if this fixes everything or I misinterpreted the comment or any changes.
Flags: needinfo?(MattN+bmo)
Attachment #8596114 - Flags: review?(MattN+bmo)
Comment on attachment 8596114 [details] [diff] [review]
Toggles Safe mode text. Added notification for quiting.

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

This is getting close. There were some changes to safeModeRestart since the last version and I think they may have confused you.

::: browser/base/content/browser.js
@@ +7555,5 @@
> +                     .createInstance(Ci.nsISupportsPRBool);
> +  Services.obs.notifyObservers(cancelQuit, "quit-application-requested", "restart");
> +
> +  if (!cancelQuit.data) {
> +    Services.startup.restartInSafeMode(Ci.nsIAppStartup.eAttemptQuit);

The "restart-in-safe-mode" notification already handles "quit-application-requested" and restarting now[1] so the cancelQuit code above should be inside the new `if` below and the call to Services.startup.restartInSafeMode here should be removed. Inside `if (Services.appinfo.inSafeMode) {` you should just return if cancelQuit.data is true.

[1] https://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js?rev=353201769922#779

@@ +7560,5 @@
> +  }
> +
> +  if (Services.appinfo.inSafeMode) {
> +    Services.startup.quit(Ci.nsIAppStartup.eRestart |
> +                          Ci.nsIAppStartup.eAttemptQuit);

Nit: you can keep this on one line since it'll be < 100 characters long
Attachment #8596114 - Flags: review?(MattN+bmo) → feedback+
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 46

3 years ago
Hi sir,

Yeah. The two versions are now confusing me. Sir, by above changes you mean:

>function safeModeRestart() {
>  if (!cancelQuit.data) {
>    cancelQuit = Cc["@mozilla.org/supports-PRBool;1"]
>                     .createInstance(Ci.nsISupportsPRBool);
>    Services.obs.notifyObservers(cancelQuit, "quit-application-requested", "restart");
>  }
>  if (Services.appinfo.inSafeMode) {
>    if (cancelQuit.data)
>      return;
>    Services.startup.quit(Ci.nsIAppStartup.eRestart | Ci.nsIAppStartup.eAttemptQuit);
>    return;
>  }
>  Services.obs.notifyObservers(null, "restart-in-safe-mode", "");
>}

I tried this but I see it throws cancelQuit not defined error which it should. I guess you did not mean this.
Flags: needinfo?(MattN+bmo)
(In reply to Manraj Singh [:manrajsingh] from comment #46)
> I tried this but I see it throws cancelQuit not defined error which it
> should. I guess you did not mean this.

I was saying that the declaration of `cancelQuit`, the createInstance, and the notifyObservers should also be inside the `if (Services.appinfo.inSafeMode) {` block since it's not relevant to case that we aren't in safe mode.
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 48

3 years ago
Created attachment 8596760 [details] [diff] [review]
Toggles Safe mode text. Added notification for quiting.

Hi sir,

Sir I guess this is what is required. Please review the patch and let me know the changes,if any.
Attachment #8596114 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)
Attachment #8596760 - Flags: review?(MattN+bmo)
Comment on attachment 8596760 [details] [diff] [review]
Toggles Safe mode text. Added notification for quiting.

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

I've fixed the one small issue for you and pushed this so it will be in Nightly in the next day or two. Thanks for the fix! :)

::: browser/base/content/browser.js
@@ +7551,5 @@
>  
>  // Prompt user to restart the browser in safe mode
>  function safeModeRestart() {
> +  if (Services.appinfo.inSafeMode) {
> +    cancelQuit = Cc["@mozilla.org/supports-PRBool;1"]

You should put a `let` here to declare the variable explicitly.
Attachment #8596760 - Flags: review?(MattN+bmo) → review+

Comment 50

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/e1afdf84febb
https://hg.mozilla.org/integration/fx-team/rev/e1afdf84febb
Whiteboard: [good first bug][lang=js][lang=xul] → [good first bug][lang=js][lang=xul][fixed-in-fx-team]
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 52

3 years ago
Oopsie! Sorry for that.

A request sir. Could you please vouch for me on Mozillans.org? The link is mentioned below. :)

https://mozillians.org/en-US/u/manrajsingh/

Also sir, I wanted to know few things from Mozilla. Can I reach out to you on IRC?
Flags: needinfo?(MattN+bmo)
https://hg.mozilla.org/mozilla-central/rev/e1afdf84febb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][lang=xul][fixed-in-fx-team] → [good first bug][lang=js][lang=xul]
Target Milestone: --- → Firefox 40
(In reply to Manraj Singh [:manrajsingh] from comment #52)
> Oopsie! Sorry for that.

No worries.

> A request sir. Could you please vouch for me on Mozillans.org?

Done.

> Also sir, I wanted to know few things from Mozilla. Can I reach out to you
> on IRC?

Sure, feel free to reach out anytime. My IRC nickname is MattN (You can see this from my bugzilla name after the colon). Our team is in the #fx-team channel.
Flags: needinfo?(MattN+bmo)

Updated

2 years ago
See Also: → bug 1194744
You need to log in before you can comment on or make changes to this bug.