Last Comment Bug 553113 - Update checks for lightweight themes
: Update checks for lightweight themes
Status: VERIFIED FIXED
[rewrite]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9.3a5
Assigned To: Dave Townsend [:mossop]
:
Mentors:
Depends on:
Blocks: 461973
  Show dependency treegraph
 
Reported: 2010-03-17 17:18 PDT by Dave Townsend [:mossop]
Modified: 2010-05-20 08:13 PDT (History)
4 users (show)
dtownsend: in‑testsuite+
dtownsend: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Perform update check for the current lightweight theme (6.18 KB, patch)
2010-03-29 17:21 PDT, Dave Townsend [:mossop]
robert.strong.bugs: review+
Details | Diff | Review
patch rev 2 (7.34 KB, patch)
2010-04-06 10:21 PDT, Dave Townsend [:mossop]
dtownsend: review+
Details | Diff | Review

Description Dave Townsend [:mossop] 2010-03-17 17:18:17 PDT

    
Comment 1 Dave Townsend [:mossop] 2010-03-26 10:50:38 PDT
http://hg.mozilla.org/projects/addonsmgr/rev/adfa6cfa9d81
Comment 2 Dave Townsend [:mossop] 2010-03-29 17:21:29 PDT
Created attachment 435765 [details] [diff] [review]
Perform update check for the current lightweight theme

Adds update checking for the current lightweight theme to match what happens on trunk. May want to switch this to using the new API properly in the future.
Comment 3 Robert Strong [:rstrong] (use needinfo to contact me) 2010-04-05 15:43:16 PDT
Comment on attachment 435765 [details] [diff] [review]
Perform update check for the current lightweight theme

>diff --git a/toolkit/mozapps/extensions/LightweightThemeManager.jsm b/toolkit/mozapps/extensions/LightweightThemeManager.jsm
>--- a/toolkit/mozapps/extensions/LightweightThemeManager.jsm
>+++ b/toolkit/mozapps/extensions/LightweightThemeManager.jsm
>@@ -129,26 +129,26 @@ var LightweightThemeManager = {
>                                      JSON.stringify(aData));
> 
>     if (aData) {
>-      let theme = this.getUsedTheme(aData.id);
>-      // TODO detect if it is an install and act accordingly
>-      if (!theme) {
>+      var theme = this.getUsedTheme(aData.id);
>+      var isInstall = !theme || theme.version != aData.version;
Why the switch from let to var? Looks like both of these could be let

>+      if (isInstall) {
>+        var oldWrapper = theme ? new AddonWrapper(theme) : null;
>         var wrapper = new AddonWrapper(aData);
>         AddonManagerPrivate.callInstallListeners("onExternalInstall", null,
>-                                                 wrapper, null, false);
>+                                                 wrapper, oldWrapper, false);
>         AddonManagerPrivate.callAddonListeners("onInstalling", wrapper, false);
>       }
>+
Comment 4 Dave Townsend [:mossop] 2010-04-06 10:21:00 PDT
Created attachment 437333 [details] [diff] [review]
patch rev 2

Comments addressed
Comment 5 Dave Townsend [:mossop] 2010-05-11 10:17:59 PDT
http://hg.mozilla.org/mozilla-central/rev/5fea6417c8f2
Comment 6 Henrik Skupin (:whimboo) 2010-05-18 07:24:33 PDT
Does it mean updates can not be checked manually via the UI or is the context menu entry just disabled for now? If it's the latter case I would file a new bug.
Comment 7 Dave Townsend [:mossop] 2010-05-20 07:47:32 PDT
(In reply to comment #6)
> Does it mean updates can not be checked manually via the UI or is the context
> menu entry just disabled for now? If it's the latter case I would file a new
> bug.

We're not going to offer users the ability to check for updates to personas.
Comment 8 Henrik Skupin (:whimboo) 2010-05-20 08:13:55 PDT
Verified fixed based on check-in and passing automated tests.

Note You need to log in before you can comment on or make changes to this bug.