Closed Bug 1540387 Opened 2 years ago Closed 2 years ago

Replace browser-compacttheme.js with theme experiments mechanism

Categories

(Firefox :: Theme, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: ntim, Assigned: ntim)

References

(Regressed 1 open bug)

Details

Attachments

(2 files)

With bug 1525762, the light/dark themes will be webextension themes, which means we can use the experiments mechanism to inject compacttheme.css (and potentially simplify it) and get rid of browser-compacttheme.js completely.

Assignee: nobody → ntim.bugs

So this means we'd load compacttheme.css on the fly? Would this not bring back the flicker removed in bug 1448059?

Flags: needinfo?(ntim.bugs)

(In reply to Dão Gottwald [::dao] from comment #3)

So this means we'd load compacttheme.css on the fly? Would this not bring back the flicker removed in bug 1448059?

I'm not noticing any flicker locally, and I don't expect the flicker to come back given that most of the theme is rendered by setting the CSS variables on :root in onBeforeInitialXULLayout, which wasn't the case when bug 1448059 was fixed.

Flags: needinfo?(ntim.bugs)

(In reply to Tim Nguyen :ntim from comment #4)

(In reply to Dão Gottwald [::dao] from comment #3)

So this means we'd load compacttheme.css on the fly? Would this not bring back the flicker removed in bug 1448059?

I'm not noticing any flicker locally, and I don't expect the flicker to come back given that most of the theme is rendered by setting the CSS variables on :root in onBeforeInitialXULLayout, which wasn't the case when bug 1448059 was fixed.

I don't think this answers the question. I'm assuming the stylesheet is going to be loaded asynchronously; that seems independent from the CSS variables being set.

(In reply to Dão Gottwald [::dao] from comment #5)

(In reply to Tim Nguyen :ntim from comment #4)

(In reply to Dão Gottwald [::dao] from comment #3)

So this means we'd load compacttheme.css on the fly? Would this not bring back the flicker removed in bug 1448059?

I'm not noticing any flicker locally, and I don't expect the flicker to come back given that most of the theme is rendered by setting the CSS variables on :root in onBeforeInitialXULLayout, which wasn't the case when bug 1448059 was fixed.

I don't think this answers the question. I'm assuming the stylesheet is going to be loaded asynchronously; that seems independent from the CSS variables being set.

My point is that when bug 1448059 was fixed, the stylesheet did a lot more things (set all the theme variables, etc.), which meant the flicker was a lot more noticeable. Now, the stylesheet only does some small tweaking, so the difference between having and not-having the stylesheet applied is negligeable.

But yes, the stylesheet is loaded async when LightweightThemeConsumer is inited: https://searchfox.org/mozilla-central/rev/44a212460990ffffecf50a8e972d3cbde2e7216b/toolkit/modules/LightweightThemeConsumer.jsm#291-294

Priority: -- → P5
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ee2913c76f4a
Allow built-in themes to use theme experiments. r=kmag
https://hg.mozilla.org/integration/autoland/rev/e3ca91d64e82
Replace browser-compacttheme.js with theme experiments. r=dao
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8e11c0bb2a4
Remove now obsolete browser_compacttheme.js test. r=me on a CLOSED TREE
https://hg.mozilla.org/integration/autoland/rev/0f940b496e58
De-duplicate theme experiment CSS files. r=me on a CLOSED TREE

Backed out for:

  • ESlint failure
  • build bustage
  • xpcshell failures
  • browser-chrome failures

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=239455054&revision=e3ca91d64e8248f15f9224cef7c599cc0e524b71

Failure log:

ESlint: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239455023&repo=autoland&lineNumber=282
[task 2019-04-10T20:48:28.191Z]
[task 2019-04-10T20:48:28.191Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2019-04-10T20:54:50.897Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/general/browser_compacttheme.js:31:7 | 'CompactTheme' is not defined. (no-undef)
[task 2019-04-10T20:54:50.897Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/general/browser_compacttheme.js:36:6 | 'CompactTheme' is not defined. (no-undef)
[task 2019-04-10T20:54:50.897Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/general/browser_compacttheme.js:41:6 | 'CompactTheme' is not defined. (no-undef)
[task 2019-04-10T20:54:50.897Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/general/browser_compacttheme.js:46:7 | 'CompactTheme' is not defined. (no-undef)
[taskcluster 2019-04-10 20:54:51.533Z] === Task Finished ===

Bustage: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239455054&repo=autoland&lineNumber=33868

task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - package> WARNING: Found 24 duplicated files taking 669633 bytes (uncompressed)
[task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - package> ERROR: The following duplicated files are not allowed:
[task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - package> Nightly.app/Contents/Resources/browser/modules/themes/dark/experiment.css
[task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - package> Nightly.app/Contents/Resources/browser/modules/themes/light/experiment.css
[task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - package> /builds/worker/workspace/build/src/toolkit/mozapps/installer/packager.mk:23: recipe for target 'stage-package' failed
[task 2019-04-10T21:02:44.069Z] 21:02:44 ERROR - package> make[5]: *** [stage-package] Error 1
[task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - package> make[5]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/browser/installer'
[task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - package> /builds/worker/workspace/build/src/toolkit/mozapps/installer/packager.mk:97: recipe for target 'make-package' failed
[task 2019-04-10T21:02:44.069Z] 21:02:44 ERROR - package> make[4]: *** [make-package] Error 2
[task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - package> /builds/worker/workspace/build/src/config/rules.mk:400: recipe for target 'default' failed
[task 2019-04-10T21:02:44.069Z] 21:02:44 ERROR - package> make[3]: *** [default] Error 2
[task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - package> /builds/worker/workspace/build/src/browser/build.mk:6: recipe for target 'package' failed
[task 2019-04-10T21:02:44.069Z] 21:02:44 ERROR - package> make[2]: *** [package] Error 2
[task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - /builds/worker/workspace/build/src/build/moz-automation.mk:84: recipe for target 'automation/package' failed
[task 2019-04-10T21:02:44.069Z] 21:02:44 ERROR - make[1]: *** [automation/package] Error 2
[task 2019-04-10T21:02:44.069Z] 21:02:44 INFO - make[1]: *** Waiting for unfinished jobs....

Failure push:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0f940b496e58fe766632d9917a573eb757171a51

Xpcshell log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239490498&repo=autoland&lineNumber=9803
23:16:25 INFO - TEST-START | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_shutdown.js
23:16:26 WARNING - TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_shutdown.js | xpcshell return code: 0
23:16:26 INFO - TEST-INFO took 423ms
23:16:26 INFO - >>>>>>>
23:16:26 INFO - PID 17064 | Unable to load \untrusted-startup-test-dll.dll; LoadLibraryW failed: 126[17064, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
23:16:26 INFO - PID 17064 | [17064, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
23:16:26 INFO - PID 17064 | [17064, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
23:16:26 INFO - PID 17064 | [17064, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
23:16:26 INFO - PID 17064 | [17064, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 81
23:16:26 INFO - PID 17064 | [17064, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file z:/build/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2528
23:16:26 INFO - PID 17064 | JavaScript strict warning: resource://testing-common/AddonTestUtils.jsm, line 278: ReferenceError: reference to undefined property "testScope"
23:16:26 INFO - "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "testScope"" {file: "resource://testing-common/AddonTestUtils.jsm" line: 278}]"
23:16:26 INFO - PID 17064 | [17064, Main Thread] WARNING: This method is lossy. Use GetCanonicalPath !: file z:/build/build/src/xpcom/io/nsLocalFileWin.cpp, line 3188
23:16:26 INFO - PID 17064 | [17064, Main Thread] WARNING: This method is lossy. Use GetCanonicalPath !: file z:/build/build/src/xpcom/io/nsLocalFileWin.cpp, line 3188
23:16:26 INFO - PID 17064 | [17064, Main Thread] WARNING: This method is lossy. Use GetCanonicalPath !: file z:/build/build/src/xpcom/io/nsLocalFileWin.cpp, line 3188
23:16:26 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
23:16:26 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
23:16:26 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
23:16:26 INFO - running event loop
23:16:26 INFO - xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_shutdown.js | Starting
23:16:26 INFO - (xpcshell/head.js) | test pending (2)
23:16:26 INFO - "AddonManager.getInstallForURL"
23:16:26 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2)
23:16:26 INFO - "AddonManager.getInstallForFile"
23:16:26 INFO - "AddonManager.getAddonByID"
23:16:26 INFO - "AddonManager.getAddonBySyncGUID"
23:16:26 INFO - "AddonManager.getAddonsByIDs"
23:16:26 INFO - "AddonManager.getAddonsByTypes"
23:16:26 INFO - "AddonManager.getActiveAddons"
23:16:26 INFO - "AddonManager.getAllAddons"
23:16:26 INFO - "AddonManager.getInstallsByTypes"
23:16:26 INFO - "AddonManager.getAllInstalls"
23:16:26 INFO - "AddonManager.isInstallEnabled"
23:16:26 INFO - "AddonManager.isInstallAllowed"
23:16:26 INFO - "AddonManager.installAddonFromWebpage"
23:16:26 INFO - "AddonManager.installAddonFromAOM"
23:16:26 INFO - "AddonManager.installTemporaryAddon"
23:16:26 INFO - "AddonManager.installBuiltinAddon"
23:16:26 INFO - "AddonManager.maybeInstallBuiltinAddon"
23:16:26 INFO - "AddonManagerPrivate.addonIsBuiltin"

Browser-chrome failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239494001&repo=autoland&lineNumber=3365
23:09:56 INFO - TEST-START | browser/base/content/test/static/browser_all_files_referenced.js
23:09:56 INFO - GECKO(2069) | 1554937796290 Marionette TRACE [8589934593] Frame script loaded
23:09:56 INFO - GECKO(2069) | 1554937796291 Marionette ERROR [8589934593] No reply from Marionette:Register
23:10:04 INFO - TEST-INFO | started process screencapture
23:10:05 INFO - TEST-INFO | screencapture: exit 0
23:10:05 INFO - Buffered messages logged at 23:09:56
23:10:05 INFO - Entering test bound checkAllTheFiles
23:10:05 INFO - Buffered messages logged at 23:10:04
23:10:05 INFO - indirectly whitelisted file: chrome://marionette/content/test_dialog.dtd used from chrome://marionette/content/test_dialog.xul
23:10:05 INFO - indirectly whitelisted file: chrome://global-platform/locale/intl.properties used from resource://gre/greprefs.js
23:10:05 INFO - indirectly whitelisted file: chrome://marionette/content/test.xul used from chrome://marionette/content/test_anonymous_content.xul
23:10:05 INFO - indirectly whitelisted file: chrome://marionette/content/test_nested_iframe.xul used from chrome://marionette/content/test.xul
23:10:05 INFO - indirectly whitelisted file: chrome://marionette/content/test2.xul used from chrome://marionette/content/test.xul,chrome://marionette/content/test_nested_iframe.xul
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/addonutils.js used from resource://services-sync/engines/addons.js
23:10:05 INFO - indirectly whitelisted file: resource://gre/modules/Promise-backend.js used from resource://gre/modules/Promise.jsm,resource://devtools/shared/worker/loader.js,resource://devtools/shared/Loader.jsm
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/engines/clients.js used from resource://services-sync/service.js
23:10:05 INFO - indirectly whitelisted file: chrome://pippki/content/resetpassword.js used from chrome://pippki/content/resetpassword.xul
23:10:05 INFO - indirectly whitelisted file: chrome://global/content/accessibility/content-script.js used from resource://gre/modules/accessibility/AccessFu.jsm
23:10:05 INFO - indirectly whitelisted file: chrome://global/content/remote-test-ipc.js used from chrome://global/content/test-ipc.xul
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/telemetry.js used from resource://services-sync/bookmark_repair.js,resource://services-sync/service.js
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/stages/enginesync.js used from resource://services-sync/service.js
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/addonsreconciler.js used from resource://services-sync/engines/addons.js
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/collection_validator.js used from resource://services-sync/engines/addons.js,resource://services-sync/engines/forms.js,resource://services-sync/engines/passwords.js
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/policies.js used from resource://services-sync/service.js
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/bookmark_validator.js used from resource://services-sync/engines/bookmarks.js
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/browserid_identity.js used from resource://services-sync/telemetry.js,resource://services-sync/status.js
23:10:05 INFO - indirectly whitelisted file: resource://services-common/tokenserverclient.js used from resource://services-sync/browserid_identity.js
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/status.js used from resource://services-sync/telemetry.js,resource://services-sync/policies.js,resource://services-sync/service.js
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/collection_repair.js used from resource://services-sync/engines/clients.js,resource://services-sync/bookmark_repair.js,resource://services-sync/doctor.js
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/stages/declined.js used from resource://services-sync/service.js
23:10:05 INFO - indirectly whitelisted file: chrome://pippki/content/load_device.js used from chrome://pippki/content/load_device.xul
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/engines.js used from resource://services-sync/engines/clients.js,resource://services-sync/engines/addons.js,resource://services-sync/engines/forms.js,resource://services-sync/engines/history.js,resource://services-sync/engines/tabs.js,resource://services-sync/engines/bookmarks.js,resource://services-sync/engines/extension-storage.js,resource://services-sync/engines/passwords.js,resource://services-sync/engines/prefs.js,resource://services-sync/service.js,resource://formautofill/FormAutofillSync.jsm
23:10:05 INFO - indirectly whitelisted file: resource://services-common/logmanager.js used from resource://services-sync/policies.js
23:10:05 INFO - indirectly whitelisted file: resource://gre/modules/accessibility/EventManager.jsm used from chrome://global/content/accessibility/content-script.js
23:10:05 INFO - indirectly whitelisted file: resource://gre/modules/Http.jsm used from resource://app/modules/translation/GoogleTranslator.jsm,resource://app/modules/translation/YandexTranslator.jsm,resource://app/modules/translation/BingTranslator.jsm
23:10:05 INFO - indirectly whitelisted file: resource://gre/modules/accessibility/Utils.jsm used from chrome://global/content/accessibility/content-script.js,resource://gre/modules/accessibility/Traversal.jsm,resource://gre/modules/accessibility/EventManager.jsm,resource://gre/modules/accessibility/AccessFu.jsm,resource://gre/modules/accessibility/ContentControl.jsm
23:10:05 INFO - indirectly whitelisted file: resource://gre/modules/SyncedBookmarksMirror.jsm used from resource://services-sync/engines/bookmarks.js
23:10:05 INFO - indirectly whitelisted file: resource://gre/modules/accessibility/Constants.jsm used from chrome://global/content/accessibility/content-script.js,resource://gre/modules/accessibility/Traversal.jsm,resource://gre/modules/accessibility/EventManager.jsm,resource://gre/modules/accessibility/Utils.jsm,resource://gre/modules/accessibility/ContentControl.jsm
23:10:05 INFO - indirectly whitelisted file: resource://gre/modules/accessibility/ContentControl.jsm used from chrome://global/content/accessibility/content-script.js
23:10:05 INFO - indirectly whitelisted file: chrome://passwordmgr/content/recipes.json used from resource://gre/greprefs.js
23:10:05 INFO - indirectly whitelisted file: resource://formautofill/FormAutofillSync.jsm used from resource://services-sync/service.js
23:10:05 INFO - indirectly whitelisted file: chrome://global/locale/AccessFu.properties used from resource://gre/modules/accessibility/Utils.jsm
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/bookmark_repair.js used from resource://services-sync/collection_repair.js
23:10:05 INFO - indirectly whitelisted file: resource://services-sync/doctor.js used from resource://services-sync/bookmark_repair.js,resource://services-sync/stages/enginesync.js
23:10:05 INFO - indirectly whitelisted file: resource://gre/modules/accessibility/Traversal.jsm used from resource://gre/modules/accessibility/ContentControl.jsm
23:10:05 INFO - Buffered messages finished
23:10:05 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 3, expected 0
23:10:05 INFO - Stack trace:
23:10:05 INFO - chrome://mochikit/content/browser-test.js:test_is:1325
23:10:05 INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:786
23:10:05 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1116
23:10:05 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1144
23:10:05 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1005
23:10:05 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:803
23:10:05 INFO - Not taking screenshot here: see the one that was previously logged

Flags: needinfo?(ntim.bugs)
Backout by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/710344cf9b5a
Backed out 4 changesets for xpcshell failures at toolkit/mozapps/extensions/test/xpcshell/test_shutdown.js on a CLOSED TREE
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/51b541545b94
Allow built-in themes to use theme experiments. r=kmag
https://hg.mozilla.org/integration/autoland/rev/421820a5e759
Replace browser-compacttheme.js with theme experiments. r=dao
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Depends on: 1543626
Regressions: 1580799
You need to log in before you can comment on or make changes to this bug.