304403 - better safe mode UE

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Mike Connor [:mconnor]]

brought up on yesterday's call, ongoing discussion with bsmedberg and beltzner.

This has two of four items implemented, the others are greyed out.  There's a
couple other non-critical tweaks here, but I'll work on those after branch.

Comment 1

[Mike Connor [:mconnor]]

Attachment: initial impl

String-complete, feature 50% implemented, bsmedberg and I each have one feature
to finish, but that can be done after branch as this doesn't involve any added
UI.

Comment 2

[Mike Connor [:mconnor]]

Attachment: more cleanup

Comment 3

[Mike Connor [:mconnor]]

Attachment: screenshot

Comment 4

[Mike Connor [:mconnor]]

Attachment: more cleanup

This adds a backup functionality to create bookmarks-backup.html if you choose
to reset bookmarks.  Prefs I'm not so worried about, but for bookmarks this
will be enhanced before beta to do the numerical incrementing a la dlmgr.

This has been tested heavily, and Beltzner has signed off on the strings.

Comment 5

[Chase Phillips]

Comment on attachment 192489 [details] [diff] [review]
more cleanup

a=[e-mail]

Comment 6

[Benjamin Smedberg]

See also my localstore patch in bug 303279

Comment 7

[Benjamin Smedberg]

ok, I think there is a major misunderstanding here: when I read this dialog, I
assume that the checkboxes will work whether I click "Make Changes and Restart"
or "Continue in Safe Mode"... and that they *should* work that way. What reasons
do we have for making the "Continue in Safe Mode" button ignore the checkboxes?

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Doesn't Safe Mode mean that all these things are always temporarily disabled?
That's what the strings were meant to reflect.

The task flow may be clearer if we made it so ...
  1. User invokes safe mode from shortcut
  2. Safe mode UI dialog opens (but browser doesn't) 
  3. IF user choses restart, then apply changes & restart in normal mode
  4. IF user choses proceed, then proceed into safe mode with temporary changes

Ideally, we'd have the [Continue in Safe Mode] button *above* the checkboxes,
making it clearer that this is an either/or proposition. I've been informed that
this is rocket science, though, and I should stop trying to make dialogs that
aren't standard 2 or 3 button ones. My creative style is seriously cramped :)

bsmedberg, what are the uses cases someone might have for entering Safe Mode but
leaving all of those boxes unchecked?

Other random thoughts:

 - the [Make Changes and Restart] button should only be enabled if at least
   one of the items is checked

 - while label changes are a pain, perhaps adding "Or, you can continue to work
in Safe Mode with all of these items temporarily disabled" would clear things up.

Comment 9

[Daniel C]

(In reply to comment #8)

> Ideally, we'd have the [Continue in Safe Mode] button *above* the checkboxes,
> making it clearer that this is an either/or proposition. I've been informed that
> this is rocket science, though, and I should stop trying to make dialogs that
> aren't standard 2 or 3 button ones. My creative style is seriously cramped :)
> 

I have to agree that from attachment 192487 [details] it is unclear whether Continuing
applies the changes temporarily, doesn't apply any changes, or will apply the
changes when the app is restarted.

Comment 10

[Mike Connor [:mconnor]]

Not an RC blocker, leaving this on my list for now.  Should be safe to leave
this until after I get back, since this is waiting on a bsmedberg bug, and for
vlad to land the improved bookmarks backup code.

Comment 11

[Benjamin Smedberg]

Asa, we at least need to fix the problem where the safe-mode window pops up
every time a new window is opened.

Comment 12

[Mike Connor [:mconnor]]

Attachment: localstore cleanup

implements deletion of localstore.rdf to reset toolbars/other customizations.

Also fixes the dialog-per-window issue, this should now open before the first
browser window.  I've been testing this in various combinations all weekend,
should work ok.

Need to talk to bsmedberg/Rob Strong about the safety of implemention disable
all in nsIExtensionManager.  This is probably a trunk improvement, but if we
can come up with a safe patch that sits beside other code, we should be able to
make it happen.  If we don't do this, I'll hide instead of disable the
checkbox.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Shouldn't the "Make changes and restart button" be disabled if no checkboxes are
checked?

Comment 14

[Mike Connor [:mconnor]]

Yeah, I'll get that in part2, whatever form it takes.

Comment 15

[Benjamin Smedberg]

Comment on attachment 197383 [details] [diff] [review]
localstore cleanup

Mike, why use a pref for this? We should be showing the safe-mode dialog from
an app-startup handler, not from browser window code at all.

Comment 16

[Asa Dotzler [:asa]]

If you come up with a very safe fix in the next couple of days, please request
approval for the patch and we'll evaluate.

Comment 17

[Benjamin Smedberg]

Asa, unless we're going to take the supplementary fix we should back out the
original patch. As it stands the behavior is simply unacceptable.

Comment 18

[Asa Dotzler [:asa]]

Mike, is part 2 coming soon?

Comment 19

[Mike Connor [:mconnor]]

Asa, the new patch hasn't been tested on branch yet since bug 310076 hasn't yet
landed there, but its ready pending testing with bsmedberg's fix on branch.  Not
at risk, it'll make it in tonight assuming Benjamin lands soon.

Comment 20

[Asa Dotzler [:asa]]

bug 310076's simple patch is in. we've got 6 hours so hustle :-)

Comment 21

[Mike Connor [:mconnor]]

Attachment: the long-awaited part 2

Comment 22

[Benjamin Smedberg]

Comment on attachment 198403 [details] [diff] [review]
the long-awaited part 2

r=me

Comment 23

[Scott MacGregor]

Comment on attachment 198403 [details] [diff] [review]
the long-awaited part 2

plussing for the branch after talking this over with mconnor.

Comment 24

[Mike Connor [:mconnor]]

fixed branch and trunk, spun off bug 311015 for the trunk work on addon disabling.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mike, when I don't check any of that options, close Firefox when it was started
and restart again, than I only get three checkboxes. The one for "Disable all
themes and extensions" will never be displayed as often as I restart Firefox. Is
it by design or do we have a issue left?

I'm using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051004
Firefox/1.6a1

Comment 26

[Mike Connor [:mconnor]]

I switched that to hidden since it won't be implemented for 1.5.  I filed bug
311015 on supporting that.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We have some new entities here which aren't known by localizers, right?

Comment 28

[Asaf Romano (gone)]

No, the strings were checked in before the l10n freeze.

Comment 29

[Adam Guthrie]

*** Bug 303282 has been marked as a duplicate of this bug. ***

Comment 30

[Flaky Jake]

I can see it's a FIXED bug but guys, how about PLUGINS? They also cause a lot of issues.