Closed Bug 397719 Opened 16 years ago Closed 7 years ago

Make the sanitizer a module

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1167238

People

(Reporter: rflint, Unassigned)

References

Details

(Keywords: perf)

Attachments

(1 file, 7 obsolete files)

Attached file CVS Copies (obsolete) —
Right now we're using the subscript loader to import the sanitizer on startup - we should make it a module so that it utilizes fastload.

As a first step, this copy script moves sanitize.js and distribution.js into a new browser/modules/ directory and renames them to follow the convention we've used on previous modules (<symbol>.jsm).
Attachment #282489 - Flags: review?(gavin.sharp)
Attachment #282489 - Flags: review?(gavin.sharp) → review+
Attached patch Patch (obsolete) — Splinter Review
This patch hooks up the new modules/ directory, sets up the sanitizer as a module and preprocesses out the license block and removes some unused constants in Distribution.jsm.
Attachment #283534 - Flags: review?(gavin.sharp)
Comment on attachment 283534 [details] [diff] [review]
Patch

>Index: Makefile.in

>-DIRS = base components locales themes fuel app
>+DIRS = base components locales themes fuel app modules

Leave "app" last, otherwise you'll run into strange problems on Mac (e.g. bug 380877).

>Index: modules/Sanitizer.jsm

>     history: {
>+      clear: function() {

>+        // Clear last URL of the Open Web Location dialog
>+        var prefs = Cc["@mozilla.org/preferences-service;1"].
>+                    getService(Ci.nsIPrefBranch2);
>+        prefs.clearUserPref("general.open_location.last_url");

I guess this was before the patch for bug 398894, but it looks like reed landed those in the new file already.

>+function sanitize() {
>+  var psvc = Cc["@mozilla.org/preferences-service;1"].
>+             getService(Ci.nsIPrefService);
>+  var branch = psvc.getBranch(itemBranch);
>+  var errors = null;
>+  for (var itemName in items) {

Error: items is not defined
Source File: file:///I:/moz/mozilla/ff-opt/dist/bin/modules/Sanitizer.jsm
Line: 186

The rest looks good, but needs further testing I think.
Attachment #283534 - Flags: review?(gavin.sharp) → review-
Attached patch Patch v2 + tests (obsolete) — Splinter Review
This adds some unit tests which currently all pass except for the second round of password manager ones due to some issue dolske's helping me take a look at.
Attachment #283534 - Attachment is obsolete: true
Attachment #285160 - Flags: review?(gavin.sharp)
Depends on: 400238
Attached patch Patch v2.1 (obsolete) — Splinter Review
This disables the second run of login manager tests until bug 400238 is fixed.
Attachment #285160 - Attachment is obsolete: true
Attachment #290770 - Flags: review?(gavin.sharp)
Attachment #285160 - Flags: review?(gavin.sharp)
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #290770 - Attachment is obsolete: true
Attachment #295005 - Flags: review?(gavin.sharp)
Attachment #290770 - Flags: review?(gavin.sharp)
Attached patch Real updated patch (obsolete) — Splinter Review
oops.
Attachment #295005 - Attachment is obsolete: true
Attachment #295006 - Flags: review?(gavin.sharp)
Attachment #295005 - Flags: review?(gavin.sharp)
Note that bug 398478 needs to be landed to Sanitize.jsm when this lands one time.
Blocks: 416233
Attached patch Unbitrot and add some tests (obsolete) — Splinter Review
Attachment #295006 - Attachment is obsolete: true
Attachment #302062 - Flags: review?(mconnor)
Attachment #295006 - Flags: review?(gavin.sharp)
Perf win + massive and diverse test coverage == blocking nomination.
Flags: blocking-firefox3?
Priority: -- → P2
Target Milestone: --- → Firefox 3 beta4
Flags: blocking-firefox3? → blocking-firefox3+
I don't think this needs to block, in the "blocking" sense, but I will review the patch. Moves+refactoring+random cleanup means hard to actually review, though!
Flags: blocking-firefox3+ → blocking-firefox3?
Who am I to argue with Gavin?
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Attachment #302062 - Flags: review?(mconnor) → review?(gavin.sharp)
bug 416233 just added sanitize support for SeaMonkey, based on the work from here and using the .jsm - we had a lot of review comments on all that, maybe some of those also apply to this work on the Firefox side and can improve stuff here.
Attached patch Updated patchSplinter Review
Attachment #282489 - Attachment is obsolete: true
Attachment #302062 - Attachment is obsolete: true
Attachment #342061 - Flags: review?(mconnor)
Attachment #302062 - Flags: review?(gavin.sharp)
Target Milestone: Firefox 3 beta4 → Firefox 3.1b2
No longer depends on: 426836
Priority: P2 → --
Target Milestone: Firefox 3.1b2 → ---
Attachment #342061 - Flags: review?(mconnor)
Assignee: rflint → nobody
Status: ASSIGNED → NEW
Wouldn't this still be a good idea, even after those whole restructurings of the sanitize functionality?
bug 1167238 has some wip, so duping there.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.