Last Comment Bug 573538 - No options for Safe-Mode
: No options for Safe-Mode
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Startup & Profiles (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.1b3
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks: 646897
  Show dependency treegraph
 
Reported: 2010-06-21 12:19 PDT by Sascha Grage
Modified: 2011-04-04 01:21 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
needed
wanted


Attachments
patch (18.00 KB, patch)
2011-04-01 10:07 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Review
patch v2 [Checkin: comment 18] (19.29 KB, patch)
2011-04-02 17:31 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Review

Description Sascha Grage 2010-06-21 12:19:59 PDT
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) 2010-06-23 00:38:20 PDT
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 2010-08-10 07:15:00 PDT
Not blocking a3 on it but we should try to get it into one of the betas.
Comment 3 Igor Velkov 2010-08-10 08:36:35 PDT
Currently that dialog can't disable all addons for firefox trunk. At least for me.
Comment 4 Jens Hatlak (:InvisibleSmiley) 2011-04-01 09:52:07 PDT
TB porting FF's dialog in bug 641781.
Comment 5 Jens Hatlak (:InvisibleSmiley) 2011-04-01 10:07:56 PDT
Created attachment 523624 [details] [diff] [review]
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 2011-04-01 10:36:34 PDT
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) 2011-04-01 11:10:45 PDT
(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 2011-04-01 12:16:52 PDT
> 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 neil@parkwaycc.co.uk 2011-04-02 16:20:20 PDT
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 2011-04-02 16:26:08 PDT
(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) 2011-04-02 16:34:25 PDT
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 neil@parkwaycc.co.uk 2011-04-02 16:49:00 PDT
Reset toolbars, window sizes and other miscellaneous settings?
Comment 13 Jens Hatlak (:InvisibleSmiley) 2011-04-02 16:53:37 PDT
(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) 2011-04-02 17:31:10 PDT
Created attachment 523833 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-04-03 13:24:28 PDT
(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 neil@parkwaycc.co.uk 2011-04-03 13:29:31 PDT
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 neil@parkwaycc.co.uk 2011-04-03 14:22:41 PDT
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) 2011-04-03 14:30:25 PDT
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 neil@parkwaycc.co.uk 2011-04-04 01:21:49 PDT
(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.)

Note You need to log in before you can comment on or make changes to this bug.