397719 - Make the sanitizer a module

Status: RESOLVED DUPLICATE of bug 1167238

(Firefox :: General, defect)

Description

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: CVS Copies

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).

Comment 1

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

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.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v2 + tests

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.

Comment 4

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v2.1

This disables the second run of login manager tests until bug 400238 is fixed.

Comment 5

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Updated patch

Comment 6

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Real updated patch

oops.

Comment 7

[Robert Kaiser]

Note that bug 398478 needs to be landed to Sanitize.jsm when this lands one time.

Comment 8

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Unbitrot and add some tests

Comment 9

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Perf win + massive and diverse test coverage == blocking nomination.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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!

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Who am I to argue with Gavin?

Comment 12

[Robert Kaiser]

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.

Comment 13

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Updated patch

Comment 15

[Robert Kaiser]

Wouldn't this still be a good idea, even after those whole restructurings of the sanitize functionality?

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

bug 1167238 has some wip, so duping there.