392501 - Ability to customize vanilla Firefox with a group of settings

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dan Mills [:thunder]]

This is a tracking bug for item DIST-002a in the Firefox 3 PRD.

Comment 1

[Dan Mills [:thunder]]

Attachment: distribution.ini parser (wip)

First pass at the distribution.ini parser.

Loads [Preferences] and [LocalizablePreferences], but currently hooks into the startup process rather late, so it stomps on extension defaults.

Goes through searchplugins/, but doesn't actually load them yet (bug 392771).

Bookmarks, other UI mods, AUS work not done yet.

Comment 2

[Dan Mills [:thunder]]

Attachment: Sample distribution.ini (wip)

Sample distribution.ini that I'm working with.

Place into appdir/distribution/ to try out the parser patch.

Comment 3

[Dan Mills [:thunder]]

Attachment: distribution.ini parser (wip)

Updated to support localized prefs.
Supports adding a string to the about box.  The 'name' setting in the Global section is gone, and replaced with 'about'.

Comment 4

[Dan Mills [:thunder]]

Attachment: distribution.ini parser (wip)

Latest iteration, requires patch from bug 392967 for prefs to load.

todo:
- bookmarks
- call loadEngines (bug 392771)
- cleanup/refactoring

Comment 5

[Dan Mills [:thunder]]

Attachment: distribution.ini parser (wip)

* Adds skeleton bookmarks parsing
* Load search engines (depends on patch on bug 392771)
* Do a little refactoring to avoid code duplication

todo:
* finish hooking up bookmarks, make sure they only get added once

Comment 6

[Dan Mills [:thunder]]

Attachment: distribution.ini parser v1

Ok, I've added support for bookmarks/separators/folders.  This is (I think) feature-complete now.

Comment 7

[Dan Mills [:thunder]]

Attachment: distribution.ini parser v1.1

Improve the way we force prefs to be reloaded - now we only force default prefs to be reloaded.

Comment 8

[Benjamin Smedberg]

Comment on attachment 279041 [details] [diff] [review]
distribution.ini parser v1.1

>+++ b/browser/components/distribution.js	Wed Aug 29 18:11:05 2007 -0700

>+ * The Initial Developer of the Original Code is Mozilla Corporation

Actually all copyrights are assigned to MoFo, not MoCo.

>+function DistributionCustomizer() {
>+  this._distroDir = this._dirSvc.get("XREExeF", Ci.nsIFile).parent;

XREExeF is a poor choice. Please use XCurProcD instead (which represents the application directory). See http://mxr.mozilla.org/mozilla/source/xpcom/io/nsDirectoryServiceDefs.h#75

As a general nit, I think it would be preferable to use "let" instead of "var" throughout new code, but do as you see fit. Did you run this with strict warnings enabled? I think there were at least a few cases where you should get "redeclared var" warnings.

>+    for (var i = 0; i < keys.length; i++) {
>+      if (keys[i].match(/^item\.(\d+)\.(\w+)\.?(\w*)/)) {
>+        var iid = RegExp.$1;
>+        var iprop = RegExp.$2;
>+        var ilocale = RegExp.$3;

I think the use of RegExp globals is hard to understand. Can you switch it around to use regexp.exec, like so?

let m = /^item\.(\d+)\.(\w+)\.?(\w*)/.exec(keys[i]);
if (m) {
  let [iid, iprop, ilocale] = m;
  ...
}
else {
  dump("Key did not match: " + keys[i] + "\n");
}

>+      switch (items[iid]["type"]) {
>+      case "default":
>+        continue;
>+        break;

Something's not right here.

>+  applyCustomizations: function DIST_applyCustomizations() {
>+    if (!this._iniExists)
>+      return;
>+
>+    // nsPrefService loads very early.  Reload prefs so we can set
>+    // distribution defaults during the prefservice:after-app-defaults
>+    // notification (see applyPrefDefaults below)
>+    this._prefSvc.resetPrefs();
>+    this._prefSvc.resetUserPrefs();
>+    this._prefSvc.readUserPrefs(null);

I thought you weren't going to do this, per bug 392967 comment 11 and 13.

>diff -r 5591ea443aee browser/components/nsBrowserGlue.js

Comment 9

[Dan Mills [:thunder]]

Attachment: distribution.ini parser v1.2

Comment 10

[Dan Mills [:thunder]]

Comment on attachment 279041 [details] [diff] [review]
distribution.ini parser v1.1

v1.2 removes the search engine code - working on a patch to load those by changing NS_APP_SEARCH_DIR_LIST instead.

Comment 11

[Benjamin Smedberg]

Comment on attachment 279793 [details] [diff] [review]
distribution.ini parser v1.2

>+function contains(array, thing) {
>+  for (let i = 0; i < array.length; i++)
>+    if (array[i] == thing)
>+      return true;
>+  return false;
>+}

I'm not sure why you need this function. Doesn't array.indexOf(thing) do basically the same thing, and I bet it's better-optimized. 

As discussed on IRC, the install-once bookmarks changes need to be keyed by the distro ID, so that if you install Firefox+A and then Firefox+B, you get the +B bookmarks customizations added in. This can be done in a followup bug.

Comment 12

[Dan Mills [:thunder]]

Attachment: distribution.ini parser v1.3

> I'm not sure why you need this function. Doesn't array.indexOf(thing) do
> basically the same thing, and I bet it's better-optimized. 

Totally right.  I'm using array.indexOf(thing) >= 0 now.

> As discussed on IRC, the install-once bookmarks changes need to be keyed by the
> distro ID, so that if you install Firefox+A and then Firefox+B, you get the +B
> bookmarks customizations added in. This can be done in a followup bug.

I'd rather do it right away... this patch does that.

Also, it fixes a bug getting the match data from the regexp that I introduced in v1.2 of the patch.  Bookmarks now work correctly again.

I'm assuming I can carry over the r+, and requesting approval to get this into m8.

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 280419 [details] [diff] [review]
distribution.ini parser v1.3

a=me, please land ASAP

Comment 14

[Dan Mills [:thunder]]

Checking in base/content/aboutDialog.css;
/cvsroot/mozilla/browser/base/content/aboutDialog.css,v  <--  aboutDialog.css
new revision: 1.7; previous revision: 1.6
done
Checking in base/content/aboutDialog.js;
/cvsroot/mozilla/browser/base/content/aboutDialog.js,v  <--  aboutDialog.js
new revision: 1.9; previous revision: 1.8
done
Checking in base/content/aboutDialog.xul;
/cvsroot/mozilla/browser/base/content/aboutDialog.xul,v  <--  aboutDialog.xul
new revision: 1.29; previous revision: 1.28
done
Checking in components/Makefile.in;
/cvsroot/mozilla/browser/components/Makefile.in,v  <--  Makefile.in
new revision: 1.61; previous revision: 1.60
done
RCS file: /cvsroot/mozilla/browser/components/distribution.js,v
done
Checking in components/distribution.js;
/cvsroot/mozilla/browser/components/distribution.js,v  <--  distribution.js
initial revision: 1.1
done
Checking in components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.35; previous revision: 1.34
done