Closed Bug 458454 Opened 16 years ago Closed 12 years ago

Ability to customize vanilla Thunderbird with a group of settings (through distribution.ini)

Categories

(Thunderbird :: Build Config, defect)

defect
Not set
normal

Tracking

(thunderbird10 fixed)

RESOLVED FIXED
Thunderbird 11.0
Tracking Status
thunderbird10 --- fixed

People

(Reporter: mkmelin, Assigned: donna.oberes)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 6 obsolete files)

This bug is basically about porting bug 392501 to thunderbird.

It adds the ability for distros to customize some prefs by including a directory "distribution" in the application directory, and putting a distribution.ini file in that dir.

As we don't have bookmarks to customize, the benefit isn't as visible as with firefox, but it's still useful to be able to set some prefs, like release notes for the distro.

I imagine we could leverage this, and add more customization in the future.
Attached patch proposed fixSplinter Review
This should fix it (and move the about dialog scripts to a scripts file). It's almost completely copy-paste from firefox + removal of things we won't need, like bookmarks customization.

What's a bit unclear to me is the /modules vs /components usage. Also when to use .js and when to use .jsm. And why is has firefox both a distribution.js and a Distribution.jsm

http://mxr.mozilla.org/mozilla-central/source/browser/modules/Distribution.jsm
http://mxr.mozilla.org/mozilla-central/source/browser/components/distribution.js
We should attempt to share the js module if possible.

The "jsm" was not added by me, and we don't even use it.  It should be removed from the tree.
We'd gladly use it if it were in /toolkit...
I'll file a bug to get rid of the Distribution.jsm.
Well, as a start, you could patch the existing distribution.js to work for you, and copy it verbatim to thunderbird.

Then it would be a pretty small step to just move it to toolkit.
I'd say this was more build config than general.

I think also we need to find a better place than msgMail3PaneWindow.js - we're currently not guaranteed to start that window up all the time, so that could lead to inconsistent results.
Assignee: mkmelin+mozilla → nobody
Component: General → Build Config
QA Contact: general → build-config
Yeah, ideally that place should be an nsMailGlue file, like there is an nsBrowserGlue and nsSuiteGlue.
Assignee: nobody → mkmelin+mozilla
Having looked at it some more i'm not as sure it's such a great benefit to have it in toolkit as we're likely to want to customize different things. 

The parts i ripped out - nav-bookmarks-service, annotation-service and livemark-service do not make much sense outside the browser. (Possibly livemarks though.)
Blocks: TB2SM
Magnus: I'm assuming that you're not actively working on this at the moment. If you are, please let us know as I have some interested in this.

(In reply to Magnus Melin from comment #6)
> Yeah, ideally that place should be an nsMailGlue file, like there is an
> nsBrowserGlue and nsSuiteGlue.

We now have mailGlue.js

(In reply to Magnus Melin from comment #7)
> Having looked at it some more i'm not as sure it's such a great benefit to
> have it in toolkit as we're likely to want to customize different things. 

I think initially we should keep it separate and we can always work out what to unify later if it is likely to be significant benefit.
Assignee: mkmelin+mozilla → nobody
Status: ASSIGNED → NEW
Attached file Proposed patch (obsolete) —
Comment on attachment 570087 [details]
Proposed patch

sid0 wanted to see this. It isn't passing the test-cookies.js test in mozmill.
Attachment #570087 - Flags: feedback?(sagarwal)
(In reply to daoberes from comment #12)
> Comment on attachment 570087 [details]
> Proposed patch
> 
> sid0 wanted to see this. It isn't passing the test-cookies.js test in
> mozmill.

SOLUTION: Setting cookies-related prefs in distribution.ini breaks this test. Take out this line from the sample distribution.ini (network.cookie.lifetimePolicy=1), and the test passes.
That's strange. daoberes or mconley: do you guys have any idea why it's happening? If you don't, we need to figure this out.
(In reply to daoberes from comment #13)
> (In reply to daoberes from comment #12)
> > Comment on attachment 570087 [details]
> > Proposed patch
> > 
> > sid0 wanted to see this. It isn't passing the test-cookies.js test in
> > mozmill.
> 
> SOLUTION: Setting cookies-related prefs in distribution.ini breaks this
> test. Take out this line from the sample distribution.ini
> (network.cookie.lifetimePolicy=1), and the test passes.

test-cookies.js assumes the default for the lifetimePolicy - this is 0 which is "accept normally". A value of 1 is "ask before accepting". Therefore I'd expect the test to break. I guess we could change the test to enforce the default though.
Attached patch Patch (obsolete) — Splinter Review
Attachment #570087 - Attachment is obsolete: true
Attachment #572006 - Flags: review?(sagarwal)
Attachment #570087 - Flags: feedback?(sagarwal)
Comment on attachment 572006 [details] [diff] [review]
Patch

Review of attachment 572006 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! I haven't run with it yet because I'd like to see a bunch of changes made and/or questions answered first.

::: mail/base/modules/Makefile.in
@@ +54,5 @@
>    MsgHdrSyntheticView.js \
>    sessionStoreManager.js \
>    mailMigrator.js \
>    mailInstrumentation.js \
> +  distribution.js

There needs to be a \ after the file name.

::: mail/base/modules/distribution.js
@@ +18,5 @@
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Dan Mills <thunder@mozilla.com>
> + *   Marco Bonardo <mak77@bonardo.net>

Please go ahead and add your name to this :)

@@ +34,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +EXPORTED_SYMBOLS = [ "TBDistCustomizer" ];

No spaces inside the brackets.

@@ +44,5 @@
> +
> +const DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC =
> +  "distribution-customization-complete";
> +
> +function TBDistCustomizer() {

What is the value in making this a prototype? Why not simply make this an object?

@@ +45,5 @@
> +const DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC =
> +  "distribution-customization-complete";
> +
> +function TBDistCustomizer() {
> +

Please remove this blank line.

@@ +47,5 @@
> +
> +function TBDistCustomizer() {
> +
> +  let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
> +               getService(Ci.nsIProperties);

Please use Services.dirsvc. See <https://mxr.mozilla.org/mozilla-central/source/storage/test/unit/vacuumParticipant.js#11> for an example.

@@ +50,5 @@
> +  let dirSvc = Cc["@mozilla.org/file/directory_service;1"].
> +               getService(Ci.nsIProperties);
> +  /* Get a property with the given name "XCUrPRocD", return a Ci.nsIFile object */
> +  /* "XCurProcD" is the symbolic name for file or directory location NS_XPCOM_CURRENT_PROCESS_DIR, 
> +   * may be in objdir/mozilla/dist/bin/ */

There's no need to mention all this. Your code is descriptive enough, and this is misleading because in builds we distribute to users it is not in objdir/mozilla/dist/bin.

@@ +66,5 @@
> +  // Create an ini parser instance from a local file
> +  get _ini() {
> +    let ini = Cc["@mozilla.org/xpcom/ini-parser-factory;1"].
> +              getService(Ci.nsIINIParserFactory).
> +              createINIParser(this._iniFile);

By mail/news house rules, this statement needs to be formatted as

let ini = Cc["@mozilla.org/xpcom/ini-parser-factory;1"]
            .getService(Ci.nsIINIParserFactory)
            .createINIParser(this._iniFile);

This goes for any definitions you've made.

@@ +69,5 @@
> +              getService(Ci.nsIINIParserFactory).
> +              createINIParser(this._iniFile);
> +    this.__defineGetter__("_ini", function() ini);
> +    return this._ini;
> +  },

We have XPCOMUtils.defineLazyGetter now. Please use it on TBDistCustomizer.prototype (TBDistCustomizer if you decide to drop the idea of making it a prototype) instead of handrolling your own. See <https://mxr.mozilla.org/comm-central/source/mozilla/browser/components/wintaskbar/WindowsPreviewPerTab.jsm#390> for an example.

@@ +81,5 @@
> +      locale = "en-US";
> +    }
> +    this.__defineGetter__("_locale", function() locale);
> +    return this._locale;
> +  },

Same here.

@@ +90,5 @@
> +    let svc = Cc["@mozilla.org/preferences-service;1"].
> +              getService(Ci.nsIPrefService);
> +    this.__defineGetter__("_prefSvc", function() svc);
> +    return this._prefSvc;
> +  },

I'd ask you to use Services.prefs instead, but apparently you need to QI this to nsIObserver. So I'd suggest that you
- add an XPCOMUtils.defineLazyServiceGetter here which takes the pref service and QIs it to nsIObserver, and call this _prefSvcObserver or something similar.
- use Services.prefs wherever you're using the pref service as nsIPrefService or nsIPrefBranch.

@@ +97,5 @@
> +  get _prefs() {
> +    let branch = this._prefSvc.getBranch(null);
> +    this.__defineGetter__("_prefs", function() branch);
> +    return this._prefs;
> +  },

You don't need this. Services.prefs implements both nsIPrefService and nsIPrefBranch.

@@ +101,5 @@
> +  },
> +
> +  _customizationsApplied: false,
> +  applyCustomizations: function DIST_applyCustomizations() {
> +

- I'm not really clear what the separation difference is between applyPrefDefaults and applyCustomizations. What happens first, what happens later, and why are the two called at different times?
- Please remove this blank line.

@@ +104,5 @@
> +  applyCustomizations: function DIST_applyCustomizations() {
> +
> +    this._customizationsApplied = true;
> +    if (!this._iniFile)
> +      return this._checkCustomizationComplete();

this._checkCustomizationComplete doesn't return anything, so all the return this.checkCustomizationComplete() lines seem wrong.

@@ +119,5 @@
> +    this._prefDefaultsApplied = true;
> +    if (!this._iniFile)
> +      return this._checkCustomizationComplete();
> +
> +	// Grab the sections of the ini file

A tab sneaked in here.

@@ +126,5 @@
> +    // The global section, and several of its fields, is required
> +    if (!sections["Global"])
> +      return this._checkCustomizationComplete();
> +
> +	// Get the keys in the "Global" section  of the ini file

And another tab.

@@ +131,5 @@
> +    let globalPrefs = enumToObject(this._ini.getKeys("Global"));
> +    if (!(globalPrefs["id"] && globalPrefs["version"] && globalPrefs["about"]))
> +      return this._checkCustomizationComplete();
> +
> +	// Get the entire preferences tree (defaults is an instance of nsIPrefBranch)

Yet another tab.

@@ +134,5 @@
> +
> +	// Get the entire preferences tree (defaults is an instance of nsIPrefBranch)
> +    let defaults = this._prefSvc.getDefaultBranch(null);
> +
> +	// Set the following user prefs

One more tab.

@@ +143,5 @@
> +      createInstance(Ci.nsISupportsString);
> +    if (globalPrefs["about." + this._locale]) {
> +      partnerAbout.data = this._ini.getString("Global", "about." + this._locale);
> +    } else {
> +      partnerAbout.data = this._ini.getString("Global", "about");

What happens if the string isn't present? Does it throw an exception or return null?

@@ +146,5 @@
> +    } else {
> +      partnerAbout.data = this._ini.getString("Global", "about");
> +    }
> +    defaults.setComplexValue("distribution.about",
> +                             Ci.nsISupportsString, partnerAbout);

Should you be setting the pref if it's null?

@@ +149,5 @@
> +    defaults.setComplexValue("distribution.about",
> +                             Ci.nsISupportsString, partnerAbout);
> +
> +    if (sections["Preferences"]) {
> +	  // Grab the keys in the "Preferences" section, and iterate

Tab

@@ +152,5 @@
> +    if (sections["Preferences"]) {
> +	  // Grab the keys in the "Preferences" section, and iterate
> +      for (let key in enumerate(this._ini.getKeys("Preferences"))) {
> +        try {
> +		  // Get the string value of the key 

Tab

@@ +154,5 @@
> +      for (let key in enumerate(this._ini.getKeys("Preferences"))) {
> +        try {
> +		  // Get the string value of the key 
> +          let value = eval(this._ini.getString("Preferences", key));
> +		  // After determining what type it is, set the pref

Tab

@@ +167,5 @@
> +            defaults.setCharPref(key, value);
> +            break;
> +          case "undefined":
> +            defaults.setCharPref(key, value);
> +            break;

When and why will typeof value be undefined?

@@ +169,5 @@
> +          case "undefined":
> +            defaults.setCharPref(key, value);
> +            break;
> +          }
> +        } catch (e) { /* ignore bad prefs and move on */ }

I think we should at least log the error in the error console. (Components.utils.reportError.)

@@ +173,5 @@
> +        } catch (e) { /* ignore bad prefs and move on */ }
> +      }
> +    }
> +
> +	// Set the prefs in the other sections 

Tab

@@ +189,5 @@
> +          let value = eval(this._ini.getString("LocalizablePreferences", key));
> +          value = value.replace("%LOCALE%", this._locale, "g");
> +          localizedStr.data = "data:text/plain," + key + "=" + value;
> +          defaults.setComplexValue(key, Ci.nsIPrefLocalizedString, localizedStr);
> +        } catch (e) { /* ignore bad prefs and move on */ }

Again, we should log the error.

@@ +199,5 @@
> +        try {
> +          let value = eval(this._ini.getString("LocalizablePreferences-" + this._locale, key));
> +          localizedStr.data = "data:text/plain," + key + "=" + value;
> +          defaults.setComplexValue(key, Ci.nsIPrefLocalizedString, localizedStr);
> +        } catch (e) { /* ignore bad prefs and move on */ }

And here too.

@@ +210,5 @@
> +    let prefDefaultsApplied = this._prefDefaultsApplied || !this._iniFile;
> +
> +    if (this._customizationsApplied && prefDefaultsApplied) {
> +      let os = Cc["@mozilla.org/observer-service;1"].
> +               getService(Ci.nsIObserverService);

Services.obs.

::: mail/base/test/unit/test_mailGlue_distribution.js
@@ +1,1 @@
> +function run_test ()

Please remove the space before the (

@@ +13,5 @@
> +
> +    // Create a descendant of iniFile
> +    iniFile.append("distribution.ini");
> +    if (iniFile.exists()) {
> +        iniFile.remove(false);

I don't see the value in this. If the ini file exists before the test starts then it seems to me to be a bug, so we should fail accordingly.

@@ +14,5 @@
> +    // Create a descendant of iniFile
> +    iniFile.append("distribution.ini");
> +    if (iniFile.exists()) {
> +        iniFile.remove(false);
> +        print("********** distribution.ini already exists **********\n");

Are these print commands useful? If they aren't then please consider getting rid of them.

@@ +16,5 @@
> +    if (iniFile.exists()) {
> +        iniFile.remove(false);
> +        print("********** distribution.ini already exists **********\n");
> +    }
> +    print ("*********** distroDir: " + distroDir.path + "\n");

No space before the (.

@@ +18,5 @@
> +        print("********** distribution.ini already exists **********\n");
> +    }
> +    print ("*********** distroDir: " + distroDir.path + "\n");
> +
> +    //Copy gTestDir

Space after the //

@@ +19,5 @@
> +    }
> +    print ("*********** distroDir: " + distroDir.path + "\n");
> +
> +    //Copy gTestDir
> +    let gTestDir = Services.dirsvc.get("CurWorkD", Ci.nsIFile)

Semicolon at the end.

@@ +27,5 @@
> +    // Construct descendant file
> +    testDistributionFile.append("distribution.ini");
> +    // Copy to distroDir
> +    testDistributionFile.copyTo(distroDir, "distribution.ini");
> +    do_check_true(testDistributionFile.exists());

This isn't actually testing anything, merely copying the file over. You need to see that preferences get set, etc.

::: mail/components/mailGlue.js
@@ +91,5 @@
>      }
>    },
>  
> +  _onAppDefaults: function()
> +  {

- Please name the function MailGlue__onAppDefaults (see a few lines below for another example.)
- Please move the { into the function's line, following the style of the code around it.

@@ +102,3 @@
>    _onProfileStartup: function MailGlue__onProfileStartup() {
> +	var distro = new TBDistCustomizer();
> +	distro.applyCustomizations(); // --> Probably don't have to call this

??

@@ +102,5 @@
>    _onProfileStartup: function MailGlue__onProfileStartup() {
> +	var distro = new TBDistCustomizer();
> +	distro.applyCustomizations(); // --> Probably don't have to call this
> +
> +  	// check if we're in safe mode

Tab

@@ +107,4 @@
>      if (Services.appinfo.inSafeMode) {
>        Services.ww.openWindow(null, "chrome://messenger/content/safeMode.xul", 
>                               "_blank", "chrome,centerscreen,modal,resizable=no", null);
> +

Please remove this extra line.
Attachment #572006 - Flags: review?(sagarwal) → review-
Attachment #572006 - Attachment is obsolete: true
Attachment #575494 - Flags: review?(sagarwal)
Comment on attachment 575494 [details] [diff] [review]
Resubmition of patch from 2011-11-04

Review of attachment 575494 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there, just a few more changes.

::: mail/base/modules/distribution.js
@@ +35,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +EXPORTED_SYMBOLS = ["TBDistCustomizer", "enumToObject", "enumerate"];

Why do you need to export enumToObject and enumerate?

@@ +39,5 @@
> +EXPORTED_SYMBOLS = ["TBDistCustomizer", "enumToObject", "enumerate"];
> +
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;
> +const Cr = Components.results;

I don't think you use Cr anywhere.

@@ +48,5 @@
> +
> +const DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC =
> +  "distribution-customization-complete";
> +
> +var TBDistCustomizer = {}

Semi-colon at the end please.

@@ +50,5 @@
> +  "distribution-customization-complete";
> +
> +var TBDistCustomizer = {}
> +
> +XPCOMUtils.defineLazyGetter(TBDistCustomizer, "_checkIni",

- _checkIni isn't a good name for this. Let's call it _findIni instead.

- I think that instead of setting _iniFile you should call it inside the _ini getter, returning the file if it's found and null if it isn't. Then _ini returns the parsed ini if it's found and null if it isn't. It makes things much simpler for callers.

- This should be a method inside TBDistCustomizer, not a getter.

@@ +60,5 @@
> +        this._iniFile = iniFile;
> +});
> +
> +XPCOMUtils.defineLazyGetter(TBDistCustomizer, "_ini", 
> +  function() {

Please name this TBDistCustomizer_get__ini and the others similarly.

@@ +79,5 @@
> +    }
> +    return locale;
> +});
> +
> +XPCOMUtils.defineLazyGetter(TBDistCustomizer, "_applyPrefDefaults",

- This gets called from outside so it shouldn't be prefixed with a _.
- This should be a method too.

@@ +85,5 @@
> +    this._prefDefaultsApplied = true;
> +    if (!this._iniFile)
> +      return;
> +
> +	// Grab the sections of the ini file

Stray tab here.

@@ +94,5 @@
> +    if (!sections["Global"]) {
> +      return;
> +    }
> +
> +	// Get the keys in the "Global" section  of the ini file

Another stray tab.

@@ +100,5 @@
> +    if (!(globalPrefs["id"] && globalPrefs["version"] && globalPrefs["about"])) {
> +      return;
> +    }
> +
> +	// Get the entire preferences tree (defaults is an instance of nsIPrefBranch)

Another tab. There are few more in this file, please clean them up and replace them with spaces.

::: mail/base/test/unit/test_mailGlue_distribution.js
@@ +1,4 @@
> +Components.utils.import("resource:///modules/distribution.js");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +function run_test()

Please fix the indentation for this file to two spaces.

@@ +2,5 @@
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +function run_test()
> +{
> +    

Spare blank line.

@@ +16,5 @@
> +    let iniFile = distroDir.clone();
> +
> +    // Create a descendant of iniFile
> +    iniFile.append("distribution.ini");
> +    if (iniFile.exists()){

Space before the {

@@ +17,5 @@
> +
> +    // Create a descendant of iniFile
> +    iniFile.append("distribution.ini");
> +    if (iniFile.exists()){
> +        do_throw("***** distribution.ini already exists in objdir/mozilla/dist/bin/distribution *****\n");

No need for the decorative asterisks.

@@ +20,5 @@
> +    if (iniFile.exists()){
> +        do_throw("***** distribution.ini already exists in objdir/mozilla/dist/bin/distribution *****\n");
> +    } 
> +    else {
> +

Another blank line.

@@ +31,5 @@
> +        // Copy to distroDir
> +        testDistributionFile.copyTo(distroDir, "distribution.ini");
> +        do_check_true(testDistributionFile.exists());
> +
> +        // Now check that prefs were set

This goes below setting the prefs.

@@ +39,5 @@
> +        TBDistCustomizer._applyPrefDefaults;
> +
> +        let testIni = Cc["@mozilla.org/xpcom/ini-parser-factory;1"]
> +                  .getService(Ci.nsIINIParserFactory)
> +                  .createINIParser(testDistributionFile);

Please align the dots under the [. Please fix this for the other XPCOM getService/createInstance calls too.

@@ +47,5 @@
> +            var iniValue = testIni.getString("Global", "id");
> +            var pref = Services.prefs.getCharPref("distribution.id");
> +            do_check_eq(iniValue, pref);
> +
> +            

Extra blank line with trailing whitespace.

@@ +60,5 @@
> +            iniValue = testIni.getString("Global", "about-" + TBDistCustomizer._locale);
> +            pref = Services.prefs.getCharPref("distribution.about." + TBDistCustomizer._locale);
> +            do_check_eq(iniValue, pref);
> +        } catch (e) {
> +            Components.utils.reportError(e);

No, let the exception be unhandled so that the test fails. Do this for all the other sections too.

@@ +90,5 @@
> +        }
> +
> +        // Test the LocalizablePreferences-[locale] section
> +        // Add any prefs found in it to the overrides array
> +        var overrides = new Array();

let overrides = [];

@@ +108,5 @@
> +        // Any prefs here that aren't found in overrides are not overriden 
> +        //   by LocalizablePrefs-[locale] and should be tested
> +        s = "LocalizablePreferences"; 
> +        for (let key in enumerate(testIni.getKeys(s))) {
> +            if (!contains(overrides, key)) {

overrides.indexOf(key) != -1?

@@ +119,5 @@
> +                    Components.utils.reportError(e);
> +                }
> +            }
> +        }
> +        distribution_ini_cleanup();

No, use do_register_cleanup instead. The general idea should be:

function run_test() {
  [check if INI exists]
  do_register_cleanup(function () { [delete ini] });
  [the actual test]
}

::: mail/components/mailGlue.js
@@ +86,5 @@
>      }
>    },
>  
>    _onProfileStartup: function MailGlue__onProfileStartup() {
> +	TBDistCustomizer._checkIni;

Stray tabs in this file.
Attachment #575494 - Flags: review?(sagarwal) → review-
I did away with the enumerate function in distribution.js and just used the hasMore() and getNext() functions of the nsIUTF8StringEnumerator interface.
Attachment #575494 - Attachment is obsolete: true
Attachment #579321 - Flags: review?(sagarwal)
Comment on attachment 579321 [details] [diff] [review]
Resubmition of patch from 2011-11-18

Review of attachment 579321 [details] [diff] [review]:
-----------------------------------------------------------------

We're almost there, but there are still several issues with your patch, some of which you haven't addressed from earlier reviews. If you go through and address all of them, I think the next patch should get r+.

::: mail/base/modules/distribution.js
@@ +35,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +EXPORTED_SYMBOLS = ["TBDistCustomizer", "enumToObject"];

You're still exporting enumToObject.

@@ +48,5 @@
> +const DISTRIBUTION_CUSTOMIZATION_COMPLETE_TOPIC =
> +  "distribution-customization-complete";
> +
> +var TBDistCustomizer = {
> +  find_ini: function() {

- We use camelCase in JavaScript. Please name this _findIni. You might also consider inlining the whole function into the ini getter you've written below, but I'm going to leave that choice to you.

- You still haven't named the function. Please name the function TBDistCustomizer__findIni. Naming functions gives us better stack traces in exceptions.

@@ +53,5 @@
> +    let iniFile = Services.dirsvc.get("XCurProcD", Ci.nsIFile);
> +    iniFile.append("distribution");
> +    iniFile.append("distribution.ini");
> +    if (iniFile.exists())
> +      return iniFile;

and if iniFile doesn't exist? Return null in that case.

@@ +56,5 @@
> +    if (iniFile.exists())
> +      return iniFile;
> +  },
> +
> +  applyPrefDefaults: function() {

Please name this TBDistCustomizer_applyPrefDefaults.

@@ +164,5 @@
> +    return true;
> +  }
> +};
> +
> +XPCOMUtils.defineLazyGetter(TBDistCustomizer, "_get_ini", 

No, this should still be called _ini. The *function* should be named TBDistCustomizer__get_ini. Like so:

XPCOMUtils.defineLazyGetter(TBDistCustomizer, "_ini",
  function TBDistCustomizer_get__ini() {
    let ini = ...
  });

@@ +168,5 @@
> +XPCOMUtils.defineLazyGetter(TBDistCustomizer, "_get_ini", 
> +  function() {
> +    let ini = Cc["@mozilla.org/xpcom/ini-parser-factory;1"]
> +                .getService(Ci.nsIINIParserFactory)
> +                .createINIParser(TBDistCustomizer.find_ini());

You aren't properly handling the case where the find ini function returns null.

@@ +172,5 @@
> +                .createINIParser(TBDistCustomizer.find_ini());
> +    return ini;
> +});
> +
> +XPCOMUtils.defineLazyGetter(TBDistCustomizer, "_get_locale",

See above. This should be named "_locale".

@@ +186,5 @@
> +});
> +
> +function enumToObject(UTF8Enumerator) {
> +  let ret = {};
> +  while (UTF8Enumerator.hasMore()){

Space before the {.

@@ +192,5 @@
> +  }
> +  return ret;
> +}
> +
> +

Unnecessary trailing blank lines.

::: mail/base/test/unit/distribution.ini
@@ +56,5 @@
> +app.releaseNotesURL="http://example.com/relnotes/"
> +mailnews.start_page.welcome_url="http://example.com/firstrun/"
> +
> +
> +

Extra newlines.

::: mail/base/test/unit/test_mailGlue_distribution.js
@@ +15,5 @@
> +  let iniFile = distroDir.clone();
> +
> +  // Create a descendant of iniFile
> +  iniFile.append("distribution.ini");
> +  // It's a bug if distribution.ini alreday exists

Please fix the typo with "already" here.

@@ +17,5 @@
> +  // Create a descendant of iniFile
> +  iniFile.append("distribution.ini");
> +  // It's a bug if distribution.ini alreday exists
> +  if (iniFile.exists()) {
> +    do_throw("distribution.ini already exists in objdir/mozilla/dist/bin/distribution.\n");

No need for the \n.

@@ +21,5 @@
> +    do_throw("distribution.ini already exists in objdir/mozilla/dist/bin/distribution.\n");
> +  }
> +
> +  // Copy gTestDir
> +  let gTestDir = Services.dirsvc.get("CurWorkD", Ci.nsIFile)

- Semicolon after the line please, and this should be called testDir since it isn't a global.
- You aren't copying the directory here, so the comment makes no sense. I'd suggest simply removing it, since the code is descriptive enough.

@@ +31,5 @@
> +  testDistributionFile.copyTo(distroDir, "distribution.ini");
> +  do_check_true(testDistributionFile.exists());
> +
> +  // Set the prefs
> +  TBDistCustomizer.find_ini();

This function call has no side effects, so it is completely unnecessary. Please get rid of it.

@@ +50,5 @@
> +  do_check_eq(iniValue, pref);
> +
> +  var aboutLocale;
> +  try {
> +    aboutLocale = testIni.getString("Global", "about." + TBDistCustomizer._get_locale);

No, please don't use TBDistCustomizer._get_locale like that. You shouldn't normally have any sort of need to access private properties of an object from outside it. If you do think you need to, chances are there's a mistake in your assumptions. In this case: what locale is this test going to be run as? Your distribution file assumes en-US anyway, so you should probably set the general.useragent.locale pref to en-US at the start for your test and hardcode en-US in the rest of it.

@@ +77,5 @@
> +        do_check_eq(value, Services.prefs.getIntPref(key));
> +        break;
> +    case "string":
> +        var pref = Services.prefs.getCharPref(key);
> +        do_check_eq(value, pref);

Why the asymmetry here compared to the other calls? You could just write do_check_eq(value, Services.prefs.getCharPref(key)). Any asymmetry of this sort draws attention while someone else is reading the code, and it's really unnecessary here.

@@ +88,5 @@
> +  
> +  // Test the LocalizablePreferences-[locale] section
> +  // Add any prefs found in it to the overrides array
> +  let overrides = [];
> +  s = "LocalizablePreferences-" + TBDistCustomizer._get_locale;

See the comment about this above.

@@ +113,5 @@
> +      do_check_eq(value, Services.prefs.getCharPref(key));
> +    }
> +  }
> +
> +  do_register_cleanup(function(){

No, you register a cleanup function at the point where you're sure you want to run it. In our case it's immediately after the first ini file existence check. Doing it right at the end is pointless.

@@ +124,5 @@
> +  });
> +  do_test_finished();
> +}
> +
> +

Extra trailing newlines.

::: mail/components/mailGlue.js
@@ +86,5 @@
>      }
>    },
>  
>    _onProfileStartup: function MailGlue__onProfileStartup() {
> +    TBDistCustomizer.find_ini();

This function call now has no side effects and so is unnecessary.

@@ +89,5 @@
>    _onProfileStartup: function MailGlue__onProfileStartup() {
> +    TBDistCustomizer.find_ini();
> +    TBDistCustomizer.applyPrefDefaults();
> +
> +  	// check if we're in safe mode

You still have a stray tab here.
Attachment #579321 - Flags: review?(sagarwal) → review-
Assignee: nobody → donna.oberes
Status: NEW → ASSIGNED
Attachment #582077 - Flags: review?(sagarwal)
Please ignore the patch I sent earlier. This is the correct one.
Attachment #579321 - Attachment is obsolete: true
Attachment #582077 - Attachment is obsolete: true
Attachment #582084 - Flags: review?(sagarwal)
Attachment #582077 - Flags: review?(sagarwal)
Attached patch ResubmissionSplinter Review
Attachment #582084 - Attachment is obsolete: true
Attachment #582351 - Flags: review?(sagarwal)
Attachment #582084 - Flags: review?(sagarwal)
Comment on attachment 582351 [details] [diff] [review]
Resubmission

To facilitate enterprise deployment, we'd like this patch to be fasttracked into Aurora now so that it can make it into the proposed Extended Support Release of Thunderbird based off TB10
Attachment #582351 - Flags: approval-comm-aurora?
Attachment #582351 - Flags: review?(sagarwal)
Checked in (after getting rid of the Makefile.in.rej changes).

https://hg.mozilla.org/comm-central/rev/d33123c68cf2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 11.0
Attachment #582927 - Flags: approval-comm-aurora+
Attachment #582351 - Flags: approval-comm-aurora?
This should probably be documented on the wiki and the documentation linked into the relnotes.
Keywords: relnote
(In reply to Ludovic Hirlimann [:Usul] from comment #29)
> This should probably be documented on the wiki and the documentation linked
> into the relnotes.

I don't think we need a relnote for this. Documentation would be fine (although I would expect to just copy from FF, but I can't see any documentation for FF at the moment).
Keywords: relnote
Depends on: 734550
You need to log in before you can comment on or make changes to this bug.