Closed Bug 1458682 Opened 7 years ago Closed 7 years ago

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js

Categories

(WebExtensions :: Themes, enhancement)

enhancement
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

(Whiteboard: [Thunderbird-testfailure: X all])

Attachments

(2 files, 3 obsolete files)

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js M-C last good: 87bd488c19f620d726b8363d47c8a320ba M-C first bad: f877359308b17e691209e1afb7193b8e86 https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=87bd488c19f620d726b8363d47c8a320ba&tochange=f877359308b17e691209e1afb7193b8e86 This looks like bug 1432508: https://hg.mozilla.org/mozilla-central/rev/a662fe1cc8ba I can't really see why this is failing, log here: https://taskcluster-artifacts.net/dojFFFAMTBWl7gFz5nJ2BQ/0/public/logs/live_backing.log Andrew and Tim: What do we need to do to get this test to pass when run in Thunderbird? Or should we just disable the new subtest? Richard, you've worked with WE Themes, is there something we need to port?
Flags: needinfo?(richard.marti)
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(aswan)
Tim on IRC: 22:33:41 - ntim: jorgk: have you tried the functionality the test is testing manually ? 22:35:13 - ntim: jorgk: extension manifests can have localized strings, this is extending the feature to WE themes 22:36:26 - ntim: jorgk: it's in toolkit so I wouldn't see why this is broken for TB, but it's still worth testing manually 22:37:16 - ntim: jorgk: you can test the "theme-light"/"theme-dark" sample extensions from here: https://reviewboard.mozilla.org/r/210014/diff/4#index_header
I see nothing we need to port. And default_locale is optional, so it should work without it too.
Flags: needinfo?(richard.marti)
Tim tested with a localised WE theme in TB and it worked. So since this appears to be a timing issue which makes an Xpcshell test fail, all I can think about is that TB's initialisation of LW themes in event "final-ui-startup" might not run at all or too late in an Xpcshell tests.
Sorry, event "chrome-document-global-created" creates the LightweightThemeConsumer.
Here's a log from running the test locally. At the end, when running xx, we see: 0:04.31 INFO xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_webextension_theme.js | Starting default_locale_themes 0:04.31 INFO (xpcshell/head.js) | test default_locale_themes pending (2) 0:04.32 INFO (xpcshell/head.js) | test run_next_test 5 finished (2) 0:04.36 pid:1172 JavaScript strict warning: resource://gre/modules/ExtensionCommon.jsm, line 1607: ReferenceError: reference to undefined property "placeholders" 0:04.36 pid:1172 1525335660419 addons.webextension.<unknown> WARN Loading extension 'null': Reading manifest: Error processing theme.accentcolor: An unexpected property was found in the WebExtension manifest. 0:04.36 pid:1172 1525335660420 addons.webextension.<unknown> WARN Loading extension 'null': Reading manifest: Error processing theme.textcolor: An unexpected property was found in the WebExtension manifest. 0:04.37 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "placeholders"" {file: "resource://gre/modules/ExtensionCommon.jsm" line: 1607}]" 0:04.37 INFO "CONSOLE_MESSAGE: (info) 1525335660419 addons.webextension.<unknown> WARN Loading extension 'null': Reading manifest: Error processing theme.accentcolor: An unexpected property was found in the WebExtension manifest." 0:04.37 INFO "CONSOLE_MESSAGE: (info) 1525335660420 addons.webextension.<unknown> WARN Loading extension 'null': Reading manifest: Error processing theme.textcolor: An unexpected property was found in the WebExtension manifest." 0:04.39 pid:1172 1525335660457 addons.xpi WARN Invalid XPI: Error: Cannot find id for addon c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\temp\\generated-extension.xpi (resource://gre/modules/addons/XPIInstall.jsm:1772:19) JS Stack trace: loadManifest@XPIInstall.jsm:1772:19 So it doesn't appear to like theme.accentcolor and theme.textcolor and something is wrong with property "placeholders". Tim? Richard?
Flags: needinfo?(richard.marti)
OK, Tim suggested on IRC to fix the test theme, it needs to have "color" added like so; theme: { "colors": { "accentcolor": "black", "textcolor": "white", }} That fixes the "Error processing theme.accentcolor" error, but not the overall test failure. Again from a local log: 0:04.30 pid:1188 JavaScript strict warning: resource://gre/modules/ExtensionCommon.jsm, line 1607: ReferenceError: reference to undefined property "placeholders" 0:04.36 INFO "CONSOLE_MESSAGE: (info) 1525382611579 addons.xpi WARN Invalid XPI: Error: Cannot find id for addon c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\temp\\generated-extension.xpi (resource://gre/modules/addons/XPIInstall.jsm:1772:19) JS Stack trace: loadManifest@XPIInstall.jsm:1772:19
I have really no idea what this is about, I've just fixed all the errors I ran into with the help of Tim via IRC. Surely one review will do ;-)
Flags: needinfo?(richard.marti)
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(aswan)
Attachment #8973055 - Flags: review?(ntim.bugs)
Attachment #8973055 - Flags: review?(kmaglione+bmo)
Attachment #8973055 - Flags: review?(aswan)
Comment on attachment 8973055 [details] [diff] [review] 1458682-test_webextension_theme.patch Review of attachment 8973055 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the theme bits in test_webextension_theme.js. I'll leave the rest to :aswan or :kmag since they know this code better than I do.
Attachment #8973055 - Flags: review?(ntim.bugs) → review+
Dropped the hunk in ExtensionCommon.jsm since it wasn't necessary. So one review should do.
Assignee: nobody → jorgk
Attachment #8973055 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8973055 - Flags: review?(kmaglione+bmo)
Attachment #8973055 - Flags: review?(aswan)
Attachment #8973057 - Flags: review?(ntim.bugs)
Component: General → WebExtensions: Themes
Product: Thunderbird → Toolkit
Attachment #8973057 - Flags: review?(kmaglione+bmo)
Attachment #8973057 - Flags: review?(aswan)
Comment on attachment 8973057 [details] [diff] [review] 1458682-test_webextension_theme.patch Review of attachment 8973057 [details] [diff] [review]: ----------------------------------------------------------------- The color thing is worth fixing, but it isn't fatal. I'm confused though why you need to add an id here, it shouldn't be necessary.
Comment on attachment 8973057 [details] [diff] [review] 1458682-test_webextension_theme.patch Review of attachment 8973057 [details] [diff] [review]: ----------------------------------------------------------------- Talked to :Fallen on IRC, Thunderbird is built with ADDON_SIGNING=0 so the logic for extracting the id from a synthetic certificate in xpcshell doesn't kick in. I'm not sure what a good long-term strategy is for Thunderbird but this is fine for now.
Attachment #8973057 - Flags: review?(aswan) → review+
Comment on attachment 8973057 [details] [diff] [review] 1458682-test_webextension_theme.patch Thanks.
Attachment #8973057 - Flags: review?(ntim.bugs)
Attachment #8973057 - Flags: review?(kmaglione+bmo)
Changed the reviewer in the commit message. Carrying forward Andrew's r+.
Attachment #8973057 - Attachment is obsolete: true
Attachment #8973074 - Flags: review+
Keywords: checkin-needed
Comment on attachment 8973074 [details] [diff] [review] 1458682-test_webextension_theme.patch Wrong patch :-(
Attachment #8973074 - Attachment is obsolete: true
Take two: Changed the reviewer in the commit message. Carrying forward Andrew's r+.
Attachment #8973075 - Flags: review+
Pushed by shindli@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb912ad63b04 modify test_webextension_theme.js so it runs in Thunderbird as well. r=aswan
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Blocks: 1468379
Product: Toolkit → WebExtensions
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: