836798 - Work - Clean up the options flyout, update text, and implement new layout

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Implement the remaining changes from story bug 831958.  See attachment 706630 [details] for details, and bug 831958 comment 2 through 4 for clarification

In scope:

* Change to the homepage/startup UI and options.
* Remove the "Allow cookies" and "Language" prefs.

Out of scope, covered by other stories:

* Changes to the privacy / history prefs.
* Changes to the sync prefs.

I broke this up into a series of smaller patches which I hope will make the review simpler.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: 1/4: remove unwanted options

Remove the "Language" pref (used only for multilocale builds, which we don't do on Windows) and the "Allow cookies" pref (per Asa in bug 831958 comment 4).

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: 2/4: remove the prefs-messages notificationbox

In multilocale builds, we had to show a "restart the browser" notification when the Language pref changed.  Without this pref, we no longer need this notificationbox widget.

Review note: Inside the <notificationbox>, only the indentation is changed.  I double-checked this with "hg qdiff -w" and you can too if you like.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: 3/4: change the startup options

Per bug 831958, we no longer let the user set a custom homepage.  Instead, the user can choose whether to restore tabs or just show the start page at startup.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: 4/4: Move "Startup" and "Always show tabs" to the top

to match the order in the mockup (attachment 706630 [details]).

Comment 5

[Allison Naaktgeboren :ally]

Comment on attachment 708642 [details] [diff] [review]
1/4: remove unwanted options

looks good

- bug 597296 no longer applies to metro (you removed the code here) Should we comment on that bug with respect to this one? (Given that 597296 is marked fixed, perhaps the comment was out of date & there is no relation)

- I don't see a good way to write a test for this, so you're off the hook there. :)

Comment 6

[Allison Naaktgeboren :ally]

Comment on attachment 708644 [details] [diff] [review]
2/4: remove the prefs-messages notificationbox

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

r+ with the following concern addressed

-notificationRestart.update=Add-ons updated. Restart to complete changes.
-notificationRestart.blocked=Unsafe add-ons installed. Restart to disable.

Did you mean to remove these notifications? If we are removing these here, how are we notifying the user of these addon related updates?

Comment 7

[Allison Naaktgeboren :ally]

Comment on attachment 708647 [details] [diff] [review]
4/4: Move "Startup" and "Always show tabs" to the top

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

I'm not sure you really need me for this one :)

Comment 8

[Matt Brubeck (:mbrubeck)]

(In reply to :Ally Naaktgeboren from comment #6)
> -notificationRestart.update=Add-ons updated. Restart to complete changes.
> -notificationRestart.blocked=Unsafe add-ons installed. Restart to disable.
> 
> Did you mean to remove these notifications? If we are removing these here,
> how are we notifying the user of these addon related updates?

We already removed all the add-on manager code, including the code that showed these notifications, in https://hg.mozilla.org/projects/elm/rev/e59e34c28f7f (bug 783681).  This is just removing the unused strings.

Comment 9

[Allison Naaktgeboren :ally]

(In reply to Matt Brubeck (:mbrubeck) from comment #8)
> We already removed all the add-on manager code, including the code that
> showed these notifications, in
> https://hg.mozilla.org/projects/elm/rev/e59e34c28f7f (bug 783681).  This is
> just removing the unused strings.

Concern addressed. I did not know we had removed the addon manager code.

Comment 10

[Allison Naaktgeboren :ally]

Comment on attachment 708646 [details] [diff] [review]
3/4: change the startup options

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

one quick nit & then you're all set

+<!ENTITY homepage.startPage                        "start page">
+<!ENTITY homepage.sessionRestore                   "tabs from last time">
For consistency, please capitalize the 's' and the 't'.

Comment 11

[Matt Brubeck (:mbrubeck)]

(In reply to :Ally Naaktgeboren from comment #10)
> For consistency, please capitalize the 's' and the 't'.

Done.

http://hg.mozilla.org/projects/elm/rev/69c10d1ff3b1
http://hg.mozilla.org/projects/elm/rev/a8b8e4559eed
http://hg.mozilla.org/projects/elm/rev/439dcc3801b5
http://hg.mozilla.org/projects/elm/rev/adf8fb23ed24