Closed Bug 511108 Opened 11 years ago Closed 11 years ago

Need a service that manages recently used lightweight themes

Categories

(Toolkit :: Add-ons Manager, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: verified1.9.2)

Attachments

(1 file, 6 obsolete files)

Not sure where this should live. Maybe toolkit/mozapps/extensions/?
Summary: Need a service that manages most popular personas (from AMO) and recently used ones → Need a service that manages recently used Personas
Assignee: nobody → dao
Blocks: 511107
Attached patch wip (obsolete) — Splinter Review
Summary: Need a service that manages recently used Personas → Need a service that manages recently used lightweight themes
Status: NEW → ASSIGNED
Blocks: 511771
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #395697 - Attachment is obsolete: true
Attachment #396333 - Flags: review?(dtownsend)
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #396333 - Attachment is obsolete: true
Attachment #396755 - Flags: review?(dtownsend)
Attachment #396333 - Flags: review?(dtownsend)
Comment on attachment 396755 [details] [diff] [review]
patch v2

This looks pretty good, I had a few questions about the API design you've chosen here that I'd like to hear some response to before I do a full review.

>+var LightweightThemeManager = {
>+  get usedThemes () {
>+    try {
>+      return JSON.parse(gPrefs.getCharPref("usedThemes"));
>+    } catch (e) {
>+      return [];
>+    }
>+  },
>+  set usedThemes (val) {
>+    if (val.length > MAX_USED_THEMES_COUNT)
>+      val.length = MAX_USED_THEMES_COUNT;
>+
>+    gPrefs.setCharPref("usedThemes", JSON.stringify(val));
>+    return val;
>+  },

I don't think it makes sense to have a public setter for usedThemes. Users of this API shouldn't be setting the full list directly, and it looks like the setter is only used once. What I think we do need is a method to remove a theme from the list.

>+  get isThemeSelected () {
>+    try {
>+      return gPrefs.getBoolPref("isThemeSelected");
>+    } catch (e) {
>+      return false;
>+    }
>+  },
>+  set isThemeSelected (val) {
>+    gPrefs.setBoolPref("isThemeSelected", val);
>+    return val;
>+  },

I'm not entirely sure that isThemeSelected is worth having as a public property. currentTheme can just expose this by returning null, as it does already. Is there a reasong for isThemeSelected that I'm missing?

>+    gObservers.notifyObservers(null, "lightweight-theme-selected", null);

I'm wondering whether the observer service is the best choice for notification here. Perhaps having an add/removeListener function on LightweightThemeManager might work better as the listener could be passed the json for the theme directly in the notification. Was there a reason why the observer service is a better choice?
Attachment #396755 - Flags: review?(dtownsend) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #396755 - Attachment is obsolete: true
Attachment #396877 - Flags: review?(dtownsend)
(In reply to comment #4)
> Was there a reason why the observer service is a better choice?

Yeah, I'd like to avoid having to keep track of the listeners...
The observer service does this for free.

I just realized that both JSON.stringify(null) and JSON.parse(null) are null, making dealing with the string slightly more convenient.
Attached patch patch v4 (obsolete) — Splinter Review
Added a selectUsedTheme method to select a theme by id. The Add-ons manager will probably want to use this.
Attachment #396877 - Attachment is obsolete: true
Attachment #397031 - Flags: review?(dtownsend)
Attachment #396877 - Flags: review?(dtownsend)
Comment on attachment 397031 [details] [diff] [review]
patch v4

This is pretty much there, just a few bits to change. What are your plans for tests, did you just want to do tests for this module on its own or something more?

>+  set currentTheme (aData) {
>+    if (_previewTimer) {
>+      _previewTimer.cancel();
>+      _previewTimer = null;
>+    }
>+
>+    if (aData) {
>+      let usedThemes = _usedThemesExceptX(aData.id);
>+      usedThemes.unshift(aData);
>+      _updateUsedThemes(usedThemes);
>+
>+      _prefs.setBoolPref("isThemeSelected", true);
>+    } else {
>+      _prefs.setBoolPref("isThemeSelected", false);
>+    }
>+
>+    _notifyWindows(this.currentTheme);

It's cheaper just to pass aData rather than this.currentTheme.

>+  selectUsedTheme: function (aId) {
>+    var usedThemes = this.usedThemes;
>+    var i = _getIndexById(aId, usedThemes);
>+    if (i >= 0)
>+      this.currentTheme = usedThemes[i];
>+  },

It's actually going to be more useful to just have a getUsedTheme method to retrieve a theme using an ID. I can then just set currentTheme if I actually need to change the selection.

>+function _usedThemesExceptX(aId) {
>+  var usedThemes = LightweightThemeManager.usedThemes;
>+  var i = _getIndexById(aId, usedThemes);
>+  if (i >= 0)
>+    usedThemes.splice(i, 1);
>+  return usedThemes;
>+}

This name seems wrong, how about _usedThemesExceptID? I think it's probably cleaner to just use usedThemes.filter rather than finding the item and splicing it out.

>+var _previewTimer;
>+var _previewTimerCallback = {
>+  notify: function (aTimer) {
>+    if (aTimer == _previewTimer)
>+      LightweightThemeManager.resetPreview();
>+  }
>+}

aTimer is guaranteed to be _previewTimer since we aren't reusing this elsewhere so I don't think there is a need for the comparison.

>+__defineGetter__("_prefs", function () {
>+  delete this._prefs;
>+  return this._prefs =
>+         Cc["@mozilla.org/preferences-service;1"]
>+           .getService(Ci.nsIPrefBranch2)
>+           .QueryInterface(Ci.nsIPrefService)
>+           .getBranch("lightweightThemes.");
>+});

You just need .getService(Ci.nsIPrefService).getBranch(lightweightThemes.");
Attachment #397031 - Flags: review?(dtownsend) → review-
Attached patch patch v5 (obsolete) — Splinter Review
addressed comments and added some tests
Attachment #397031 - Attachment is obsolete: true
Attachment #397361 - Flags: review?(dtownsend)
Comment on attachment 397361 [details] [diff] [review]
patch v5

Looks good
Attachment #397361 - Flags: review?(dtownsend) → review+
Attached patch patch v5.1Splinter Review
forgot about using .filter instead of .splice
Attachment #397361 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/14cf3892b66f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #397382 - Flags: approval1.9.2?
Flags: in-testsuite+
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Attachment #397382 - Flags: approval1.9.2?
Comment on attachment 397382 [details] [diff] [review]
patch v5.1

>+ * The Initial Developer of the Original Code is
>+ * Dao Gottwald <dao@mozilla.com>.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.

This is completely wrong. As an employee of MoCo, the initial developer should always be MoFo or MoCo (pretty sure it's MoFo, but MoCo is used in most places). This needs to be fixed.
I'm not an employee.
(In reply to comment #14)
> (From update of attachment 397382 [details] [diff] [review])
> >+ * The Initial Developer of the Original Code is
> >+ * Dao Gottwald <dao@mozilla.com>.
> >+ * Portions created by the Initial Developer are Copyright (C) 2009
> >+ * the Initial Developer. All Rights Reserved.
> 
> This is completely wrong. As an employee of MoCo, the initial developer should
> always be MoFo or MoCo (pretty sure it's MoFo, but MoCo is used in most
> places). This needs to be fixed.

Do we have some docs that explain what we should be putting in the license
block and when, it looks like I have been making the same mistake for quite
some time so it'd be useful to have something to refer to in the future.
(In reply to comment #15)
> I'm not an employee.

You did this work as part of you duties towards the Mozilla Corporation, I assume. By "employee", I was meaning "you get paid to work on this", which includes contractors/interns.

(In reply to comment #16)
> Do we have some docs that explain what we should be putting in the license
> block and when, it looks like I have been making the same mistake for quite
> some time so it'd be useful to have something to refer to in the future.

Not sure if we have any MoCo-specific docs on this type of thing, but 
http://www.mozilla.org/MPL/boilerplate-1.1/ does mention "The "Initial Developer" is the copyright holder, which may be your employer rather than you. In this case, add yourself as a Contributor."

Gerv, do you know of any internal documentation on this? I know both you and I have mentioned it in a number of bugs, but I don't know off the top of my head if there's actually something somewhere explaining how this works for people employed by MoCo/MoFo.
(CCing harvey for confirmation.)

In fact, for contractors and interns, I believe it depends on the terms of your contract and whether it defines your output as "work for hire". But I would expect standard MoCo contracts to include that language.

The copyright on code written by MoCo/MoFo employees or people fitting the above rests with the Mozilla Foundation, and they should be listed as the Initial Developer.

Strictly speaking, the Contributors: line is also for people with a copyright interest, but in practice in the past we've said that MoFo/MoCo people can put their names there instead of as ID.

I don't know if this is documented anywhere. Where would be good?

Gerv
Verified fix awhile back.  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091202 Minefield/3.7a1pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b5pre) Gecko/20091202 Namoroka/3.6b5pre
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.