1330337 - Theming API - implement WebExtension framework

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect)

Comment 1

[Matthew Wein [:mattw]]

Attachment: Bug 1330337 - Add basic theme framework hidden behind pref

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

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8825916 [details]
Bug 1330337 - Add basic theme framework hidden behind pref

https://reviewboard.mozilla.org/r/103982/#review104888

Looks good to me, but can you also add a test for this? (for example, test flipping the pref)

Comment 3

[Matthew Wein [:mattw]]

Comment on attachment 8825916 [details]
Bug 1330337 - Add basic theme framework hidden behind pref

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

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8825916 [details]
Bug 1330337 - Add basic theme framework hidden behind pref

https://reviewboard.mozilla.org/r/103982/#review105236

::: browser/components/extensions/test/browser/browser_ext_themes.js:14
(Diff revision 2)
> +  });
> +
> +  SimpleTest.waitForExplicitFinish();
> +  let waitForConsole = new Promise(resolve => {
> +    SimpleTest.monitorConsole(resolve, [{
> +      message: /Themes are not enabled/,

Neat! I am curious: Why didn't you use `extension.emit("test-theme-enabled")` instead? If it works, please use that so we can keep it indefinitely. I don't want to spam our browser console with theming API messages indefinitely.

Comment 5

[Matthew Wein [:mattw]]

> Neat! I am curious: Why didn't you use
> `extension.emit("test-theme-enabled")` instead? If it works, please use that
> so we can keep it indefinitely. I don't want to spam our browser console
> with theming API messages indefinitely.

I tried this and it didn't work so I thought we didn't support it, but I just found out we have a way to send messages to tests using the event name "test-message". The second argument is then the event name you listen for. I'll update the patch to use this before adding checkin-needed :)

Comment 6

[Matthew Wein [:mattw]]

Comment on attachment 8825916 [details]
Bug 1330337 - Add basic theme framework hidden behind pref

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b0cc40ebbbf
Add basic theme framework hidden behind pref r=mikedeboer

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3b0cc40ebbbf

Comment 9

[Vasilica Tamas, QA [:vtamas]]

Is manual testing necessary here? If yes, could you please provide some reliable steps and expected results in order to verify this bug?

I tried to load an extension that contains a manifest.json as it is described in User Story but I was unable to install it because of the following errors: http://pastebin.com/Lp4hGems

Am I missing something? Maybe some prefs?

Comment 10

[Matthew Wein [:mattw]]

Since no support for any of the global properties - colors, images, etc - was added in this bug, I don't think it's necessary for us to have any manual testing. I think it will be very helpful in later bugs though!

Comment 11

[Vasilica Tamas, QA [:vtamas]]

(In reply to Matthew Wein [:mattw] from comment #10)
> Since no support for any of the global properties - colors, images, etc -
> was added in this bug, I don't think it's necessary for us to have any
> manual testing. I think it will be very helpful in later bugs though!

Thanks Matthew for your reply. In this case I am making this bug as qe-verify -
For those bugs which you consider that need our attention, please assign me a needinfo (or as qa contact) and we will gladly test them.

Comment 12

[Matthew Wein [:mattw]]

Thanks Vasilica, will do :)