1384541 - Implement migration path from Compact Light/Dark Theme to compact mode

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[Johann Hofmann [:johannh]]

The "Compact" part of the Compact Dark and Light themes will be replaced by the new compact mode added in bug 1352358. We should find a way to migrate users using these themes to compact mode in Firefox 57.

Comment 1

[Brian Grinstead [:bgrins]]

Makes sense. Just a note that there's currently some syncing between the devtools theme and the corresponding compact theme. Both:
* if you switch from compact dark -> compact light  (or vice versa) it updates the devtools toolbox color
* if you switch the devtools toolbox color from dark to light (or vice versa) and you currently have a compact theme applied, it updates the applied compact theme

Comment 2

[Johann Hofmann [:johannh]]

Attachment: Bug 1384541 - Add a UI migration from compact themes to compact mode.

Review commit: https://reviewboard.mozilla.org/r/164310/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164310/

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8893266 [details]
Bug 1384541 - Add a UI migration from compact themes to compact mode.

https://reviewboard.mozilla.org/r/164310/#review169660

::: browser/components/nsBrowserGlue.js:2101
(Diff revision 1)
>      }
>  
> +    if (currentUIVersion < 51) {
> +      // Switch to compact UI density if the user is using a formerly compact
> +      // dark or light theme.
> +      let currentTheme = Services.prefs.getCharPref("lightweightThemes.selectedThemeID", "");

nit: second argument isn't needed since lightweightThemes.selectedThemeID is in firefox.js.

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8893266 [details]
Bug 1384541 - Add a UI migration from compact themes to compact mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164310/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d74faf4943b
Add a UI migration from compact themes to compact mode. r=dao

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing xpcshell's test_browserGlue_urlbar_defaultbehavior_migration.js and test_browserGlue_migration_loop_cleanup.js:7

https://hg.mozilla.org/integration/autoland/rev/04596e8b12adf1c30729a5d6c1fcefda8e1487e0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5d74faf4943bb6f573ffda2a5964748c96415dd5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120665488&repo=autoland

First failure:
03:19:40     INFO -  TEST-START | browser/components/places/tests/unit/test_browserGlue_urlbar_defaultbehavior_migration.js
03:19:40  WARNING -  TEST-UNEXPECTED-FAIL | browser/components/places/tests/unit/test_browserGlue_urlbar_defaultbehavior_migration.js | xpcshell return code: 0
03:19:40     INFO -  TEST-INFO took 348ms
03:19:40     INFO -  >>>>>>>
03:19:40     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
03:19:40     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
03:19:40     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
03:19:40     INFO -  running event loop
03:19:40     INFO -  browser/components/places/tests/unit/test_browserGlue_urlbar_defaultbehavior_migration.js | Starting
03:19:40     INFO -  (xpcshell/head.js) | test pending (2)
03:19:40     INFO -  "Migrate default.behavior = 0"
03:19:40     INFO -  PID 8633 | JavaScript error: jar:file:///Users/cltbld/tasks/task_1501754263/build/application/Nightly.app/Contents/Resources/browser/omni.ja!/components/nsBrowserGlue.js, line 2101: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]
03:19:40     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
03:19:40     INFO -  Unexpected exception NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]
03:19:40     INFO -  BG__migrateUI@jar:file:///Users/cltbld/tasks/task_1501754263/build/application/Nightly.app/Contents/Resources/browser/omni.ja!/components/nsBrowserGlue.js:2101:26
03:19:40     INFO -  BG_observe@jar:file:///Users/cltbld/tasks/task_1501754263/build/application/Nightly.app/Contents/Resources/browser/omni.ja!/components/nsBrowserGlue.js:414:11
03:19:40     INFO -  setupBehaviorAndMigrate@/Users/cltbld/tasks/task_1501754263/build/tests/xpcshell/tests/browser/components/places/tests/unit/test_browserGlue_urlbar_defaultbehavior_migration.js:37:3
03:19:40     INFO -  @/Users/cltbld/tasks/task_1501754263/build/tests/xpcshell/tests/browser/components/places/tests/unit/test_browserGlue_urlbar_defaultbehavior_migration.js:42:3
03:19:40     INFO -  async*asyncFunction@resource://gre/modules/Task.jsm:241:18
03:19:40     INFO -  Task_spawn@resource://gre/modules/Task.jsm:166:12
03:19:40     INFO -  _run_next_test@/Users/cltbld/tasks/task_1501754263/build/tests/xpcshell/head.js:1488:9
03:19:40     INFO -  run@/Users/cltbld/tasks/task_1501754263/build/tests/xpcshell/head.js:701:9
03:19:40     INFO -  _do_main@/Users/cltbld/tasks/task_1501754263/build/tests/xpcshell/head.js:221:3
03:19:40     INFO -  _execute_test@/Users/cltbld/tasks/task_1501754263/build/tests/xpcshell/head.js:544:5
03:19:40     INFO -  @-e:1:1
03:19:40     INFO -  exiting test
03:19:40     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]" {file: "jar:file:///Users/cltbld/tasks/task_1501754263/build/application/Nightly.app/Contents/Resources/browser/omni.ja!/components/nsBrowserGlue.js" line: 2101}]"
03:19:40     INFO -  <<<<<<<

Comment 7

[Johann Hofmann [:johannh]]

For reference, this is failing due to a pretty intransparent hack that will probably come back to bite anyone who attempts to access the current lightweight theme in future migrations (https://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/testing/xpcshell/head.js#1557). I'll add the fallback argument back and file a new bug for this.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8893266 [details]
Bug 1384541 - Add a UI migration from compact themes to compact mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164310/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57fc4b2d2982
Add a UI migration from compact themes to compact mode. r=dao

Comment 10

[Brian Grinstead [:bgrins]]

It seems we still have a situation where the default theme is applied when this migration happens, then the compact theme is applied after the fact so the density isn't switched.

Are there plans to either apply the density change immediately when a compact theme is applied, or to remove the compact themes entirely?

Comment 11

[Dão Gottwald [:dao]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Are there plans to either apply the density change immediately when a
> compact theme is applied, or to remove the compact themes entirely?

The latter. See bug 1366363.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/57fc4b2d2982

Comment 13

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/57fc4b2d2982b55f09b13e41e3334bb655561e1a
Bug 1384541 - Add a UI migration from compact themes to compact mode. r=dao

Comment 14

[ovidiu boca[:Ovidiu]]

Johann, can you please suggest us how to verify this? Thanks

Comment 15

[Johann Hofmann [:johannh]]

(In reply to ovidiu boca[:Ovidiu] from comment #14)
> Johann, can you please suggest us how to verify this? Thanks

The point is that if a user had been using the (Compact) Dark or Light Theme before the UI migration, they should be switched to the new compact mode (and keep the Dark or Light Theme) when they start Firefox.

You can set the pref browser.migration.version to 50 to simulate "before the UI migration".

Comment 16

[ovidiu boca[:Ovidiu]]

I verified this on Mac OS X 10.10, Windows 10, and Ubuntu 16.04 with Nightly 57.0a1(2017-08-18) and I can confirm the fix. 

I will also put the steps here in case that someone else needs to verify this: 


1. set the pref browser.migration.version to 50
2. switch your theme to dark or light and keep the density to normal
3. restart the browser

  Expected result: density should be compact


Thanks Johann for your help.