Closed Bug 392501 Opened 12 years ago Closed 12 years ago

Ability to customize vanilla Firefox with a group of settings

Categories

(Firefox :: General, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 alpha8

People

(Reporter: hello, Assigned: hello)

References

Details

Attachments

(2 files, 7 obsolete files)

This is a tracking bug for item DIST-002a in the Firefox 3 PRD.
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Depends on: 392771
Attached patch distribution.ini parser (wip) (obsolete) — Splinter Review
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.
Sample distribution.ini that I'm working with.

Place into appdir/distribution/ to try out the parser patch.
Depends on: 392967
Attached patch distribution.ini parser (wip) (obsolete) — Splinter Review
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'.
Attachment #277458 - Attachment is obsolete: true
Attachment #277640 - Attachment is patch: true
Attachment #277640 - Attachment mime type: application/octet-stream → text/plain
Attached patch distribution.ini parser (wip) (obsolete) — Splinter Review
Latest iteration, requires patch from bug 392967 for prefs to load.

todo:
- bookmarks
- call loadEngines (bug 392771)
- cleanup/refactoring
Attachment #277640 - Attachment is obsolete: true
Attached patch distribution.ini parser (wip) (obsolete) — Splinter Review
* 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
Attachment #278551 - Attachment is obsolete: true
Attached patch distribution.ini parser v1 (obsolete) — Splinter Review
Ok, I've added support for bookmarks/separators/folders.  This is (I think) feature-complete now.
Attachment #278847 - Attachment is obsolete: true
Attachment #278892 - Flags: review?(benjamin)
Attached patch distribution.ini parser v1.1 (obsolete) — Splinter Review
Improve the way we force prefs to be reloaded - now we only force default prefs to be reloaded.
Attachment #278892 - Attachment is obsolete: true
Attachment #279041 - Flags: review?(benjamin)
Attachment #278892 - Flags: review?(benjamin)
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
Attachment #279041 - Flags: review?(benjamin) → review-
Attached patch distribution.ini parser v1.2 (obsolete) — Splinter Review
Attachment #279793 - Flags: review?(benjamin)
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.
Attachment #279041 - Attachment is obsolete: true
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.
Attachment #279793 - Flags: review?(benjamin) → review+
> 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.
Attachment #279793 - Attachment is obsolete: true
Attachment #280419 - Flags: approval1.9?
Comment on attachment 280419 [details] [diff] [review]
distribution.ini parser v1.3

a=me, please land ASAP
Attachment #280419 - Flags: approval1.9? → approval1.9+
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
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 395862
You need to log in before you can comment on or make changes to this bug.