573538 - No options for Safe-Mode

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, enhancement)

Description

[Sascha Grage]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100620 Minefield/3.7a6pre NOT Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100620 Lightning/1.1a1pre SeaMonkey/2.1a2pre

Seamonkey has no options for the safe-mode.

Reproducible: Always

Steps to Reproduce:
1. Start Seamonkey with -safe-mode 
2.
3.
Actual Results:  
No dialog with options.

Expected Results:  
Dialog with options
http://www.firefox-browser.de/MediaWiki/images/d/db/AbgesicherterModus.png

Comment 1

[Justin Wood (:Callek)]

Ok heres the intersting bits.

Using a debug build as my testbed (since trunk opt is building now): |seamonkey -safe-mode| && |seamonkey --safe-mode| spawned a no-restart SeaMonkey with the _SIDEBAR_ open. I closed it and re-ran, each time sidebar open.

|seamonkey| did spawn a restart and did not have sidebar open.

I also did get the set-as-default prompt each time (I don't have my trunk builds as default).

Comment 2

[Ian Neal]

Not blocking a3 on it but we should try to get it into one of the betas.

Comment 3

[Igor Velkov]

Currently that dialog can't disable all addons for firefox trunk. At least for me.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

TB porting FF's dialog in bug 641781.

Comment 5

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

Neil, if review takes some time, please give at least a general direction whether the strings are acceptable so I can create an extra patch to make the string freeze.

Comment 6

[Philip Chee]

Some brief comments:
> +  // Remove the pref-overrides dir, if it exists.
> +  try {

We don't have Bug 364297 Change default home page search and default search engine for Fx 2.0 series.

> +<!ENTITY resetToolbars.label          "Reset toolbars and controls">

Not sure what "controls" are but deleting local store deletes all persistent attributes. I don't know how to word it in a user facing way.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #6)
> We don't have Bug 364297 Change default home page search and default search
> engine for Fx 2.0 series.

Ah, I see. Still maybe we should leave it in here so this part is ready should we ever use it. But I'll leave that decision to Neil.

> > +<!ENTITY resetToolbars.label          "Reset toolbars and controls">
> 
> Not sure what "controls" are but deleting local store deletes all persistent
> attributes. I don't know how to word it in a user facing way.

FWIW I'd associate "controls" with toolbar items, but in that case it would be pretty redundant here.

AFAIK local store is also the place where window sizes are remembered, right? So maybe "Reset toolbars and size settings"?

Comment 8

[Philip Chee]

> Ah, I see. Still maybe we should leave it in here so this part is ready should
> we ever use it. But I'll leave that decision to Neil.

I suggested over IRC that we add a comment along the lines of:
//XXX This code currently doesn't do anything unless the rest of Bug 364297 is ported.
To prevent some developer from "optimizing" this code away later on.

> AFAIK local store is also the place where window sizes are remembered, right?
[I think "controls" here mean the window controls so window size and position.]

> So maybe "Reset toolbars and size settings"?
Any and every attribute that is persisted. (XUL persist= attribute and JS document.persist(elementID, attributeName). e.g. "Reset toolbars and settings other than those stored in user preferences"

Comment 9

[[e-mail]]

Comment on attachment 523624 [details] [diff] [review]
patch

>+   content/communicator/safeMode.js                                 (safeMode.js)
>+   content/communicator/safeMode.xul                                (safeMode.xul)
Top-level files have their own section.

>+  document.getElementById("tasks")
>+          .addEventListener("CheckboxStateChange", UpdateOKButtonState, false);
<vbox id="tasks" oncommand="UpdateOKButtonState();">

>+function UpdateOKButtonState() {
>+  document.documentElement.getButton("accept").disabled = 
>+    !document.getElementById("resetUserPrefs").checked &&
>+    !document.getElementById("deleteBookmarks").checked &&
>+    !document.getElementById("resetToolbars").checked &&
>+    !document.getElementById("disableAddons").checked &&
>+    !document.getElementById("restoreSearch").checked;
[I wonder whether !getElementsByAttribute("checked", "true").item(0) works]

>+<!ENTITY % platformCommunicatorDTD SYSTEM "chrome://communicator-platform/locale/platformCommunicatorOverlay.dtd">
Which entity are we using from here?

>+            buttonlabelextra1="&continueButton.label;"
It would be nice if this button could have initial focus.

>+            ondialogcancel="onCancel();"
Cancel normally tries to close the dialog, so we need this to return false.
[Although I'm not sure whether it would be neater for the caller to restart.]

>   _onProfileStartup: function()
>   {
>+    // check if we're in safe mode
>+    if (Services.appinfo.inSafeMode) {
>+      Services.ww.openWindow(null, "chrome://communicator/content/safeMode.xul", 
>+                             "_blank", "chrome,centerscreen,modal,resizable=no", null);
[I assume this is early enough.]
>+    }
>+
>     this._updatePrefs();
>     migrateMailnews(); // mailnewsMigrator.js
> 
>     Sanitizer.checkAndSanitize();
So, the problem here is that even if you want to restart, profile startup still gets to finish. One fix might be to use a different notification, such as profile-after-change.

Comment 10

[Ian Neal]

(In reply to comment #8)

> > So maybe "Reset toolbars and size settings"?
> Any and every attribute that is persisted. (XUL persist= attribute and JS
> document.persist(elementID, attributeName). e.g. "Reset toolbars and settings
> other than those stored in user preferences"

I think it would be very difficult to come up with something that would fit into a single line - I guess "Reset toolbars and persisted attributes" is still not very user-friendly.
Maybe we just accept nothing will be user-friendly and just keep it fairly short and explain it in help pages.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

I'll concentrate on l10n issues for now.

(In reply to comment #9)
> >+<!ENTITY % platformCommunicatorDTD SYSTEM "chrome://communicator-platform/locale/platformCommunicatorOverlay.dtd">
> Which entity are we using from here?

quitApplicationCmd.label

(In reply to comment #10)
> I think it would be very difficult to come up with something that would fit
> into a single line

I think we don't have to take care of every single case this affects but may instead concentrate on the common/most visible things this affects. IMO that's toolbars (including their contents) and window sizes, but I'm not a localstore expert so maybe there's more.

> I guess "Reset toolbars and persisted attributes" is still not very
> user-friendly.

Right.

> Maybe we just accept nothing will be user-friendly and just keep it fairly
> short and explain it in help pages.

Agreed, but keep in mind that Help cannot/should not be called this early, and that it's hard to predict when this particular topic would be written (given the nature of our development environment).

Comment 12

[[e-mail]]

Reset toolbars, window sizes and other miscellaneous settings?

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #12)
> Reset toolbars, window sizes and other miscellaneous settings?

Apart from being a bit long it's also, a little too vague. A bit scary, actually, like "may affect anything, even things you didn't know existed". I think we should really look at this from a user's perspective rather than trying to be exact or all-encompassing (I think we'd fail on both anyway).

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2 [Checkin: comment 18]

(In reply to comment #9)
> [I wonder whether !getElementsByAttribute("checked", "true").item(0) works]

It does.

> >+            ondialogcancel="onCancel();"
> Cancel normally tries to close the dialog, so we need this to return false.
> [Although I'm not sure whether it would be neater for the caller to restart.]

I don't see any difference in behavior (tried Esc and the window's close button), in both cases the application closes (i.e. startup is interrupted).

> >   _onProfileStartup: function()
> >   {
> >+    // check if we're in safe mode
> >+    if (Services.appinfo.inSafeMode) {
> >+      Services.ww.openWindow(null, "chrome://communicator/content/safeMode.xul", 
> >+                             "_blank", "chrome,centerscreen,modal,resizable=no", null);
> [I assume this is early enough.]

For showing the dialog before any other window opens? Then Yes.

> So, the problem here is that even if you want to restart, profile startup
> still gets to finish. One fix might be to use a different notification, such 
> as profile-after-change.

Unfortunately I don't know how to check whether that makes any difference, but I tried to apply what you suggested (and of course checked that the parts I could observe still work).

Comment 15

[[e-mail]]

(In reply to comment #14)
> > >+            ondialogcancel="onCancel();"
> > Cancel normally tries to close the dialog, so we need this to return false.
> > [Although I'm not sure whether it would be neater for the caller to restart.]
> I don't see any difference in behavior (tried Esc and the window's close
> button), in both cases the application closes (i.e. startup is interrupted).
Just for consistency, since "accept" prevents the window from closing (except as part of the restart process).

Comment 16

[[e-mail]]

Comment on attachment 523833 [details] [diff] [review]
patch v2 [Checkin: comment 18]

>+    !document.documentElement.getElementsByAttribute("checked", "true").item(0);
[IIRC document has getElementsByAttribute, or you could use the vbox]

>         break;
>+      case "profile-after-change":
>+        this._onProfileAfterChange();
>       case "final-ui-startup":
:-(

>+    Services.obs.addObserver(this, "profile-after-change", false);
[I wonder whether it's better to use the category method]

Comment 17

[[e-mail]]

Comment on attachment 523833 [details] [diff] [review]
patch v2 [Checkin: comment 18]

r=me with changes as discussed on IRC.

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 523833 [details] [diff] [review]
patch v2 [Checkin: comment 18]

http://hg.mozilla.org/comm-central/rev/9d9a25c4364d
with comment 16 points 1+2 addressed

Comment 19

[[e-mail]]

(In reply to comment #16)
>>+    Services.obs.addObserver(this, "profile-after-change", false);
>[I wonder whether it's better to use the category method]
FYI, this works as follows:
1. Remove the explicit calls to add/removeObserver
2. Add an extra line to SuiteCommon.manifest
This article targets extensions but it demonstrates the idea.
https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0#What_you_need_to_change
(Note that as an app component we still want app-startup registration.)