Closed Bug 836798 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Metro Graveyard :: General, defect)

All
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

(Whiteboard: feature=work)

Attachments

(4 files)

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.
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).
Attachment #708642 - Flags: review?(ally)
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.
Attachment #708644 - Flags: review?(ally)
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.
Attachment #708646 - Flags: review?(ally)
to match the order in the mockup (attachment 706630 [details]).
Attachment #708647 - Flags: review?(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. :)
Attachment #708642 - Flags: review?(ally) → review+
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?
Attachment #708644 - Flags: review?(ally) → review+
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 :)
Attachment #708647 - Flags: review?(ally) → review+
(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.
(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 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'.
Attachment #708646 - Flags: review?(ally) → review+
(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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 801199
Duplicate of this bug: 826925
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.