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)
WebExtensions
Themes
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)
286.87 KB,
text/plain
|
Details | |
1.31 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•7 years ago
|
||
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
Comment 2•7 years ago
|
||
I see nothing we need to port. And default_locale is optional, so it should work without it too.
Flags: needinfo?(richard.marti)
Assignee | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
Sorry, event "chrome-document-global-created" creates the LightweightThemeConsumer.
Assignee | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 years ago
|
||
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
Assignee | ||
Comment 7•7 years ago
|
||
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 8•7 years ago
|
||
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+
Assignee | ||
Comment 9•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Component: General → WebExtensions: Themes
Product: Thunderbird → Toolkit
Assignee | ||
Updated•7 years ago
|
Attachment #8973057 -
Flags: review?(kmaglione+bmo)
Attachment #8973057 -
Flags: review?(aswan)
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
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+
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
Changed the reviewer in the commit message. Carrying forward Andrew's r+.
Attachment #8973057 -
Attachment is obsolete: true
Attachment #8973074 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 14•7 years ago
|
||
Comment on attachment 8973074 [details] [diff] [review]
1458682-test_webextension_theme.patch
Wrong patch :-(
Attachment #8973074 -
Attachment is obsolete: true
Assignee | ||
Comment 15•7 years ago
|
||
Take two:
Changed the reviewer in the commit message. Carrying forward Andrew's r+.
Attachment #8973075 -
Flags: review+
Comment 16•7 years ago
|
||
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
Comment 17•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•7 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•