Update 'Contributors' section automatically in about:nightly. Fixes #58. #85

Closed
wants to merge 10 commits into
from

Projects

None yet

3 participants

@xabolcs

Hi!

This pull request contains:

  • fix for automatically filling Contributors
  • little styling fix for Songbird (it lacks of chrome://global/skin/about.css)
  • some fix for Nightly Tester Tools links
xabolcs added some commits Jun 12, 2012
@whimboo whimboo and 1 other commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Nightly Tester Tools.
+ *
+ * The Initial Developer of the Original Code is
+ * Dave Townsend <dtownsend@oxymoronical.com>.
+ *
+ * Portions created by the Initial Developer are Copyright (C) 2007
@whimboo
whimboo Jun 14, 2012

This credit goes to you. Also please update the year. I hope that we can have a patch for the MPL2 license soon. Would you be interested in doing that?

@xabolcs
xabolcs Jun 15, 2012

whimboo commented on commit

I hope that we can have a patch for the MPL2 license soon. Would you be interested in doing that?

Sure!
I'd like to provide a file list with three element in the MPL2 issue (#39) before creating the commit:

  • files with new boilerplate
  • files with changed boilerplate
  • files with no boilerplate

Of course all files would came from extension directory.

@whimboo whimboo and 2 others commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+
+const ourAddonID = "{8620c15f-30dc-4dba-a131-7c5d20cf4a29}";
@whimboo
whimboo Jun 14, 2012

That reminds me that we should move away from a guid and use a name like 'nightlytestertools@mozilla.org' for the id. Would you mind filing an issue for it?

Beside that I think we can remove the 'our' prefix of the constant. Further please make it all capital letters.

@xabolcs
xabolcs Jun 14, 2012

whimboo wrote

... we should move away from a guid ...

Hmm. IMHO that will cause a new addon. How do You migrate 100k users?
CCing @jvillalobos to help count our possibilities about this move.

@whimboo
whimboo Jun 14, 2012

No, it will not. With Firefox 4 we included the feature to change the add-on id with the update.rdf. I'm fairly sure AMO supports that.

@jvillalobos
jvillalobos Jun 14, 2012

No, AMO doesn't support changing IDs. It's best that you stick to the one you have.

@whimboo
whimboo Jun 14, 2012

Do you have a bug open for AMO to support changing the pref? If not i would like to file one.

@jvillalobos
jvillalobos Jun 14, 2012

None that I know of. This is too much of an edge case and too much of an inconvenience (messes up blocklisting, stats, etc.) for it to be a priority.

@whimboo
whimboo Jun 18, 2012

Makes sense. In any way I have filed is as Bug 765717.

@whimboo whimboo commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+
+const ourAddonID = "{8620c15f-30dc-4dba-a131-7c5d20cf4a29}";
+
+
+function init() {
+ var has_EM = "@mozilla.org/extensions/manager;1" in Components.classes;
+ if (has_EM) {
+ extensionLoader();
@whimboo
whimboo Jun 14, 2012

we don't need the extra variable here. Just add the check directly to the if condition.

@whimboo
whimboo Jun 14, 2012

Or better directly try to import 'resource://gre/modules/AddonManager.jsm' and put a try/catch around. That way we can kill the other two functions.

@whimboo whimboo and 1 other commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ var has_EM = "@mozilla.org/extensions/manager;1" in Components.classes;
+ if (has_EM) {
+ extensionLoader();
+ } else {
+ addonLoader();
+ }
+}
+
+function fillContributorsCB(addon) {
+ appendToList("about", "about", [ addon.creator ], "label");
+ appendToList("contributors", "contributorsList", addon.contributors, "li");
+}
+
+function addonLoader() {
+ Components.utils.import("resource://gre/modules/AddonManager.jsm");
+ AddonManager.getAddonByID(ourAddonID, function(addon) {
@whimboo
whimboo Jun 14, 2012

nit: function ( and can we start making use of an 'a' prefix for parameters? Given that file is new we could find our coding styles here. What do you think?

@xabolcs
xabolcs Jun 14, 2012

whimboo

... Given that file is new we could find our coding styles here. What do you think?

Sure!
Then please nit all kind of styles here which doesn't fit! Of course You could also comment on #87.

@whimboo
whimboo Jun 14, 2012

I will do it here and you can sync it with issue #87 later. I think that's a good way. Before I do it I will wait for another update.

@whimboo whimboo commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ AddonManager.getAddonByID(ourAddonID, function(addon) {
+ fillContributorsCB(addon);
+ });
+}
+
+function extensionLoader() {
+ function EM_NS(aProperty)
+ {
+ return "http://www.mozilla.org/2004/em-rdf#" + aProperty;
+ }
+
+ var em = Cc['@mozilla.org/extensions/manager;1']
+ .getService(Ci.nsIExtensionManager);
+ var rdfs = Cc["@mozilla.org/rdf/rdf-service;1"]
+ .getService(Ci.nsIRDFService);
+ var ds = em.datasource;
@whimboo
whimboo Jun 14, 2012

That should better align with Cc.

@whimboo whimboo commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ function EM_NS(aProperty)
+ {
+ return "http://www.mozilla.org/2004/em-rdf#" + aProperty;
+ }
+
+ var em = Cc['@mozilla.org/extensions/manager;1']
+ .getService(Ci.nsIExtensionManager);
+ var rdfs = Cc["@mozilla.org/rdf/rdf-service;1"]
+ .getService(Ci.nsIRDFService);
+ var ds = em.datasource;
+ var extension = rdfs.GetResource("urn:mozilla:item:" + ourAddonID);
+
+ var addon = {};
+
+ var arc = rdfs.GetResource(EM_NS("homepageURL"));
+ addon.creator = {};
@whimboo
whimboo Jun 14, 2012

please move this up so it doesn't intersect the code flow.

@whimboo whimboo commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ var homepage = ds.GetTarget(extension, arc, true);
+ if (homepage)
+ addon.creator.url = homepage.QueryInterface(Ci.nsIRDFLiteral).Value;
+
+ arc = rdfs.GetResource(EM_NS("creator"));
+ var creator = ds.GetTarget(extension, arc, true);
+ if (creator)
+ addon.creator.name = creator.QueryInterface(Ci.nsIRDFLiteral).Value;
+
+ arc = rdfs.GetResource(EM_NS("contributor"));
+ var contributors = ds.GetTargets(extension, arc, true);
+ if (contributors.hasMoreElements()) {
+ addon.contributors = [];
+ while (contributors.hasMoreElements()) {
+ var contributor = {};
+ contributor.name = contributors.getNext().QueryInterface(Ci.nsIRDFLiteral).Value;
@whimboo
whimboo Jun 14, 2012

I don't think we need another level here. Contributors only have a name and nothing else. So an array with strings would be sufficient.

@whimboo whimboo and 1 other commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+const Cc = Components.classes;
+const Ci = Components.interfaces;
+
+const ourAddonID = "{8620c15f-30dc-4dba-a131-7c5d20cf4a29}";
+
+
+function init() {
+ var has_EM = "@mozilla.org/extensions/manager;1" in Components.classes;
+ if (has_EM) {
+ extensionLoader();
+ } else {
+ addonLoader();
+ }
+}
+
+function fillContributorsCB(addon) {
@whimboo
whimboo Jun 14, 2012

What is CB? Do we need this?

@xabolcs
xabolcs Jun 14, 2012

CB is for CallBack.
Should I rename to fillContributorsCallback?

@whimboo
whimboo Jun 14, 2012

Just remove CB because it's not always a callback method.

@whimboo whimboo commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+const Ci = Components.interfaces;
+
+const ourAddonID = "{8620c15f-30dc-4dba-a131-7c5d20cf4a29}";
+
+
+function init() {
+ var has_EM = "@mozilla.org/extensions/manager;1" in Components.classes;
+ if (has_EM) {
+ extensionLoader();
+ } else {
+ addonLoader();
+ }
+}
+
+function fillContributorsCB(addon) {
+ appendToList("about", "about", [ addon.creator ], "label");
@whimboo
whimboo Jun 14, 2012

A bit overhead here. I would suggest to directly replace the element and make not use of appendToList.

@whimboo whimboo commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+
+const ourAddonID = "{8620c15f-30dc-4dba-a131-7c5d20cf4a29}";
+
+
+function init() {
+ var has_EM = "@mozilla.org/extensions/manager;1" in Components.classes;
+ if (has_EM) {
+ extensionLoader();
+ } else {
+ addonLoader();
+ }
+}
+
+function fillContributorsCB(addon) {
+ appendToList("about", "about", [ addon.creator ], "label");
+ appendToList("contributors", "contributorsList", addon.contributors, "li");
@whimboo
whimboo Jun 14, 2012

I would also suggest we get rid of appendToList and move its code in here. Any reason we need this as a separate function?

@whimboo whimboo commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ var listElem = document.createElement(aEType);
+ var textContainer = listElem;
+ if (url) {
+ textContainer = document.createElement("a");
+ listElem.appendChild(textContainer);
+ textContainer.href = url;
+ }
+ textContainer.textContent = currentItem.name;
+ node.appendChild(listElem);
+ });
+
+ return aItems.length;
+}
+
+
+window.addEventListener("load", init, false);
@whimboo
whimboo Jun 14, 2012

Please remove the listener in init().

@whimboo whimboo commented on an outdated diff Jun 14, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ var textContainer = listElem;
+ if (url) {
+ textContainer = document.createElement("a");
+ listElem.appendChild(textContainer);
+ textContainer.href = url;
+ }
+ textContainer.textContent = currentItem.name;
+ node.appendChild(listElem);
+ });
+
+ return aItems.length;
+}
+
+
+window.addEventListener("load", init, false);
+
@whimboo
whimboo Jun 14, 2012

nit: please add.

@xabolcs xabolcs Changes (mostly) based on comments:
- noting myself as initial developer, year
- adding others from cvs and hg blame (the EM stuff, appendToList)
- destructuring assignment to set Ci, Cu, ...
- uppercasing ADDONID
- no more has_EM
- real callback of fillContributorsCallback
- move extensionLoader() to ExtensionManager.getAddonByID()
- ... and it now fills just names, no url for creator
- new line at end of file
- removing event listener
- team changes in install.rdf
ba1062d
@whimboo

nit: just separate both words by an underscore.

@whimboo

You should do the if/else the other way around. Once we drop old features we wouldn't have to rewrite most of the stuff. As mentioned use a try/catch here.

@whimboo

Otherwise I like that change. Looks good and we can land it soon.

@whimboo whimboo and 1 other commented on an outdated diff Jun 19, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+
+
+function init() {
+ window.removeEventListener("load", init, false);
+
+ var haveAM;
+ try {
+ Cu.import("resource://gre/modules/AddonManager.jsm");
+ haveAM = true;
+ } catch(e) {
+ haveAM = false;
+ }
+ if (haveAM) {
+ AddonManager.getAddonByID(ADDON_ID, fillContributorsCallback);
+ } else {
+ ExtensionManager.getAddonByID(ADDON_ID, fillContributorsCallback);
@whimboo
whimboo Jun 19, 2012

There is no need for an if/else condition. Please directly put the code in the above try/catch block.

@xabolcs
xabolcs Jun 19, 2012

whimboo commented on commit

There is no need for an if/else condition. Please directly put the code in the above try/catch block.

I desagree about that.
haveAM is almost as safe as have_EM was.
But the recommended try ... catch method is more unsafe. It could hide underlying problems. (At least more than haveAM could.)

@whimboo
whimboo Jun 19, 2012

With this method you will not get any other information why the Cu.import() could have been failed. So the code I'm talking about here should be:

'+ try {

  • Cu.import("resource://gre/modules/AddonManager.jsm");
  • AddonManager.getAddonByID(ADDON_ID, fillContributorsCallback);
  • } catch(e) {
  • ExtensionManager.getAddonByID(ADDON_ID, fillContributorsCallback);
  • }`
@whimboo whimboo and 1 other commented on an outdated diff Jun 19, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ }
+ if (haveAM) {
+ AddonManager.getAddonByID(ADDON_ID, fillContributorsCallback);
+ } else {
+ ExtensionManager.getAddonByID(ADDON_ID, fillContributorsCallback);
+ }
+}
+
+function fillContributorsCallback(aAddon) {
+ appendToList("about", "about", [ aAddon.creator ], "label");
+ appendToList("contributors", "contributorsList", aAddon.contributors, "li");
+}
+
+var ExtensionManager = {
+ getAddonByID: function (aID, aCallback) {
+ if (!aID || typeof aID != "string")
@whimboo
whimboo Jun 19, 2012

Regarding coding style for comparisons... We should also change all of those instances and make use of the triple operator to check for the type too and kill the currently necessary type conversions. That means in this case use !==.

@xabolcs
xabolcs Jun 19, 2012

Please note that typeof (typeof foobar) === "string" and typeof( Array().length) === "number" ! :)
There aren't any type conversion here ... in the whole aboutNightly.js!
So IMHO no changes needed in

  • typeof aID != "string" near L72
  • typeof aCallback != "function" near L76
  • aItems.length == 0 near L117

Of course if You ask for it, I will change them.

@whimboo
whimboo Jul 2, 2012

It's always safe to have them. It makes it more obvious when you run into conversion issues.

@xabolcs

whimboo commented on commit

With this method you will not get any other information why the Cu.import() could have been failed. ...

Yeah, because of that have I used have_EM.
I know what kind of code are You talking about. IMHO it's not the best pattern/practice.
As I said your try ... catch code is more unsafe like haveAM.
Because with Your method You will not get any other informarion why the Cu.import() or even AddonManager.getAddonByID() could have been failed.

Therefore I proposed the have_EM method: simply in check, without any exception to catch.

@whimboo

Ah i see. Sure we can fix that by using:

var manager = null;
try:
  var { AddonManager: manager } = Cu.import("resource://gre/modules/AddonManager.jsm");
catch (e) {
  var manager = ExtensionManager;
}

if (manager) {
  ...
} else {
  throw whatever we want to report.
}
@xabolcs

whimboo commented on commit

Ah i see. Sure we can fix that by using ...

Thanks, this is more acceptable for me. :)

@xabolcs

Added commit d991c7f containing:

  • EM / AM changed to manager as recommended
  • no changes to comparisons - see my comment above!
@whimboo whimboo commented on an outdated diff Jul 2, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
+
+const ADDON_ID = "{8620c15f-30dc-4dba-a131-7c5d20cf4a29}";
+
+
+function init() {
+ window.removeEventListener("load", init, false);
+
+ var manager = null;
+ try {
+ var { AddonManager: manager } = Cu.import("resource://gre/modules/AddonManager.jsm");
@whimboo
whimboo Jul 2, 2012

That would redeclare manager which already exists since line 50. We should simply remove the var here. I would assume that will work.

@whimboo whimboo commented on an outdated diff Jul 2, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
+
+const ADDON_ID = "{8620c15f-30dc-4dba-a131-7c5d20cf4a29}";
+
+
+function init() {
+ window.removeEventListener("load", init, false);
+
+ var manager = null;
+ try {
+ var { AddonManager: manager } = Cu.import("resource://gre/modules/AddonManager.jsm");
+ } catch (e) {
+ var manager = ExtensionManager;
@whimboo
whimboo Jul 2, 2012

Same here. Or remove the declaration in line 50.

@whimboo whimboo commented on an outdated diff Jul 2, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+function init() {
+ window.removeEventListener("load", init, false);
+
+ var manager = null;
+ try {
+ var { AddonManager: manager } = Cu.import("resource://gre/modules/AddonManager.jsm");
+ } catch (e) {
+ var manager = ExtensionManager;
+ }
+
+ if (manager) {
+ manager.getAddonByID(ADDON_ID, fillContributorsCallback);
+ } else {
+ throw Components.Exception("no usable Manager found",
+ Cr.NS_ERROR_NOT_IMPLEMENTED);
+ }
@whimboo
whimboo Jul 2, 2012

Actually to be honest, when would we fail? It should never happen except syntax failures in the ExtensionManager class we would see in either way. I don't see a reason for the else case here.

@xabolcs

@whimboo:
added commits with changes:

  • strict checks: === and !==
  • do not define manager at L50:
  • not checking the existence of manager near L57: else is removed by Your comments, if is removed because I would avoid silent fails if something is wrong near before.
@whimboo whimboo and 1 other commented on an outdated diff Aug 26, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ } catch (e) {
+ var manager = ExtensionManager;
+ }
+
+ manager.getAddonByID(ADDON_ID, fillContributorsCallback);
+}
+
+function fillContributorsCallback(aAddon) {
+ appendToList("about", "about", [ aAddon.creator ], "label");
+ appendToList("contributors", "contributorsList", aAddon.contributors, "li");
+}
+
+var ExtensionManager = {
+ getAddonByID: function (aID, aCallback) {
+ if (!aID || typeof aID !== "string")
+ throw Components.Exception("aID must be a non-empty string",
@whimboo
whimboo Aug 26, 2012

Reading through the docs for Components.Exception it tells me that those should be used for XPCOM components. Here we do not have a XPCOM component implemented in JS so I think we should better throw an Error.

@xabolcs
xabolcs Aug 27, 2012

I decided to use Components.Exception based on Bug 757663 comment 3, to be consistent with AddonManager. For the details see Bug 759642!

@whimboo
whimboo Sep 19, 2012

Chatted with unfocused about it and as he mentioned we can even safely make use of Components.Exception in our js modules. So we are good here.

@whimboo whimboo commented on an outdated diff Aug 26, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+}
+
+function fillContributorsCallback(aAddon) {
+ appendToList("about", "about", [ aAddon.creator ], "label");
+ appendToList("contributors", "contributorsList", aAddon.contributors, "li");
+}
+
+var ExtensionManager = {
+ getAddonByID: function (aID, aCallback) {
+ if (!aID || typeof aID !== "string")
+ throw Components.Exception("aID must be a non-empty string",
+ Cr.NS_ERROR_INVALID_ARG);
+
+ if (typeof aCallback !== "function")
+ throw Components.Exception("aCallback must be a function",
+ Cr.NS_ERROR_INVALID_ARG);
@whimboo
whimboo Aug 26, 2012

Same as above.

@whimboo
whimboo Aug 26, 2012

Also please add brackets for single statements within an if/else/for section.

@whimboo whimboo commented on an outdated diff Aug 26, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ appendToList("about", "about", [ aAddon.creator ], "label");
+ appendToList("contributors", "contributorsList", aAddon.contributors, "li");
+}
+
+var ExtensionManager = {
+ getAddonByID: function (aID, aCallback) {
+ if (!aID || typeof aID !== "string")
+ throw Components.Exception("aID must be a non-empty string",
+ Cr.NS_ERROR_INVALID_ARG);
+
+ if (typeof aCallback !== "function")
+ throw Components.Exception("aCallback must be a function",
+ Cr.NS_ERROR_INVALID_ARG);
+
+ function EM_NS(aProperty)
+ {
@whimboo
whimboo Aug 26, 2012

Opening bracket at the same line as the function definition.

@whimboo whimboo commented on an outdated diff Aug 26, 2012
extension/chrome/content/aboutNightly/aboutNightly.js
+ }
+
+ aCallback(addon);
+ }
+}
+
+function appendToList(aHeaderId, aNodeId, aItems, aEType) {
+ var header = document.getElementById(aHeaderId);
+ var node = document.getElementById(aNodeId);
+
+ if (!aItems || aItems.length === 0) {
+ header.hidden = true;
+ return 0;
+ }
+
+ aItems.forEach(function (currentItem) {
@whimboo
whimboo Aug 26, 2012

You should use the 'a' prefix also for currentItem.

@whimboo whimboo commented on the diff Aug 26, 2012
extension/chrome/skin/aboutNightly/aboutNightly-sb.css
+}
+
+ul {
+ margin: 0;
+ -moz-margin-start: 1.5em;
+ padding: 0;
+ list-style: square;
+}
+
+ul > li {
+ margin-top: .5em;
+}
+
+th, td {
+ padding: 0 5px;
+}
@whimboo
whimboo Aug 26, 2012

I will trust those styles because I cannot test it in the next time.

@xabolcs
xabolcs Aug 27, 2012

It was grabbed from chrome://global/skin/about.css. Interestingly Songbird doesn't have one.

@whimboo

With the remaining items fixed we will have a good template for coding styles and we can get the code checked-in. Thanks in advance for the update.

@whimboo

@xabolcs, would you mind to fix the remaining nits? I kinda would like to get this pushed soon.

@xabolcs

whimboo commented:

@xabolcs, would you mind to fix the remaining nits? I kinda would like to get this pushed soon.

I try my best, but I'm currently busy so not sure if I could provide an update this week.

@whimboo

No worries. Whenever you have time. Thanks!

@xabolcs xabolcs Issue #58 - addressing comments part 5:
- if brackets
- aCurrentItem
4e7181b
@xabolcs

@whimboo, 5th set of commits added.

@whimboo

Just tested and it looks great! I'm going to merge it now.

@whimboo whimboo closed this Sep 26, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment