1477708 - Make theme loading part of the Theme constructor in ext-theme.js

Status: RESOLVED FIXED

(WebExtensions :: Themes, defect)

Description

[Tim Nguyen :ntim]

With bug 1347207, I'll start doing some async stuff in the constructor before actually loading the theme, so this will make the work easier for me.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1477708 - Make theme loading part of the Theme constructor in ext-theme.js.

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

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8994200 [details]
Bug 1477708 - Make theme loading part of the Theme constructor in ext-theme.js.

https://reviewboard.mozilla.org/r/258796/#review265696


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/parent/ext-theme.js:334
(Diff revision 1)
>        return;
>      }
>  
> -    defaultTheme = new Theme(extension);
> -    defaultTheme.load(manifest.theme);
> +    defaultTheme = new Theme({
> +      extension,
> +      details: manifest.theme

Error: Missing trailing comma. [eslint: comma-dangle]

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8994200 [details]
Bug 1477708 - Make theme loading part of the Theme constructor in ext-theme.js.

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

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8994200 [details]
Bug 1477708 - Make theme loading part of the Theme constructor in ext-theme.js.

https://reviewboard.mozilla.org/r/258796/#review265826

Comment 5

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 325aedb56a74c03067889368d56b1c005fbe3b8b -d 69500fda8331: rebasing 473992:325aedb56a74 "Bug 1477708 - Make theme loading part of the Theme constructor in ext-theme.js. r=jaws" (tip)
merging toolkit/components/extensions/parent/ext-theme.js
warning: conflicts while merging toolkit/components/extensions/parent/ext-theme.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8994200 [details]
Bug 1477708 - Make theme loading part of the Theme constructor in ext-theme.js.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ccf3971a466
Make theme loading part of the Theme constructor in ext-theme.js. r=jaws

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2ccf3971a466

Comment 9

[Vlad Jiman]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!