1167238 - Convert sanitize.js to a module

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Marco Bonardo [:mak]]

We can convert sanitize.js to a module by basically moving Sanitizer to browser/modules/Sanitizer.jsm and doing some modifications to internal properties (like .ignoreTimespan, .range could become arguments of sanitize() method)

We can keep a sanitize.js shim for now, for compatibility, once we have converted internal consumers it could start warning deprecation.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Please don't do this until I have landed bug 1089695. The code was written months ago, I'd like to be able to land it someday.

Comment 2

[Marco Bonardo [:mak]]

no worries, this might take a while, fwiw I think 99% of the code will stay the same (so ideally should just be a manual edit of the filepath in the diff)

Comment 3

[Tyler Steiman]

I would love to work on this bug, can someone please assign it to me? Thanks!

Comment 4

[Tyler Steiman]

Sorry Marco I didn't mean to clear the flag I still want to be assigned.

Comment 5

[Marco Bonardo [:mak]]

would you mind working on bug 1167229 before?
This bug still needs the dependency bug 1089695 to be reviewed and land, that should happen soon. But in the meanwhile you can work on other dependencies.

Comment 6

[Tyler Steiman]

(In reply to Marco Bonardo [::mak] from comment #5)
> would you mind working on bug 1167229 before?
> This bug still needs the dependency bug 1089695 to be reviewed and land,
> that should happen soon. But in the meanwhile you can work on other
> dependencies.

Not a problem at all, I assigned 1167229 to myself. Sorry for the unneeded flags above!

Comment 7

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

This is a dupe of bug 397719, but I guess we can dupe forward.

Comment 8

[Marco Bonardo [:mak]]

cool, we can use that bug and patch for reference once we can proceed.

Comment 9

[Alex Johnson (:alex_johnson)]

I would like to work on this bug.

Comment 10

[Marco Bonardo [:mak]]

Awesome, this is sort of actionable. I'd still like bug 1211849 to land before though, cause it will remove some code. but you can start figuring out what needs to be done. If bug 1211849 ends up being abandoned, we'll first work on landing that.

Yoric added a first "mock" module in browser/modules/Sanitizer.jsm, it is currently just a wrapper.
to avoid losing mercurial history, I'd suggest to remove the existing mock module, and hg mv browser/base/content/sanitize.js browser/modules/Sanitizer.jsm
then you can start building patches on top of that status.

I'd like for Sanitizer to stop being instantiatable, and rather create a state object that can be passed around. Most of the stuff can be "hidden" in the module scope. The interface can be largely simplified:

This queries various uses around, you can check which apis are invoked from the outside and thus should be exposed. Any internal properties set from the outside should be convertable to arguments for sanitize().
http://mxr.mozilla.org/mozilla-central/search?string=Sanitizer.&find=browser%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

For any questions, feel free to reach me here (use needinfo flag) or on irc (#fx-team)

Comment 11

[Alex Johnson (:alex_johnson)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=445e8d2a30f7

Comment 12

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1167238 - Moved sanitize.js to Sanitizer.jsm module. r?mak

Review commit: https://reviewboard.mozilla.org/r/29153/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29153/

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8709778 [details]
MozReview Request: Bug 1167238 - Moved sanitize.js to Sanitizer.jsm module. r?mak

https://reviewboard.mozilla.org/r/29153/#review28495

Ok, this is touching the right files... but I actually would like to change the semantics of Sanitizer.

The first thing to do is to revert changes and properly use "hg mv" to replace Sanitizer.jsm with sanitize.js. If you just copy over code manually you lose all the mercurial blame information. You really need to use hg mv.

Then, Sanitizer.jsm as you implemented it, just by moving stuff from sanitize.js, is still a constructor that needs to be instantiated. This way we lose the main advantage of using a module, that is the fact we can reuse a single instance everywhere and the consumer doesn't have to instantiate and setup an object every time.
I'd like Sanitizer.jsm to expose a single static this.Sanitizer = {} object with these methods: onStartup(), sanitize(), onShutdown(), showUI, getClearRange. This will be our external API.
Currently Sanitizer is instantiatable cause every instance tracks various options, that are set directly on it: .prefDomain, .range, .ignoreTimespan. These should become properties of an input object to sanitize(). So Sanitizer.sanitize({ prefDomain, range, ignoreTimespan }).
Internall You could rename the old instantiatable Sanitizer to SanitizerInternal and reuse it from the new APIs to issue every specific sanitization. In future it's likely we can just pass around a state object but for now reusing it internally should allow to reduce regressions.

This requires a little bit of refactoring, but if we start from a clean blame status it should not be too complex to follow.

::: browser/base/content/test/general/browser_purgehistory_clears_sh.js:43
(Diff revision 1)
> +                                  "resource:///modules/Sanitizer.jsm");

should be moved at the beginning of the test and indented better

::: browser/base/content/test/newtab/browser_newtab_bug722273.js:8
(Diff revision 1)
> -Cc["@mozilla.org/moz/jssubscript-loader;1"]
> +                                  "resource:///modules/Sanitizer.jsm");

it is already included by head.js

::: browser/base/content/test/plugins/browser_clearplugindata.js:5
(Diff revision 1)
> -// Test clearing plugin data using sanitize.js.
> +// Test clearing plugin data using Sanitize.jsm.

typo: Sanitizer.jsm

::: browser/components/nsBrowserGlue.js:221
(Diff revision 1)
>        return sanitizerScope.Sanitizer;

Are you sure this works?
You are returning a constructor from the module and not instantiating it...

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_cache.js:12
(Diff revision 1)
> -
> +    "resource:///modules/Sanitizer.jsm");

please reindent

::: browser/modules/Sanitizer.jsm:6
(Diff revision 1)
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

looks like you copied the contents of sanitize.js into Sanitizer.jsm.
Unfortunately this destroys all blame information.

What you should do instead is delete Sanitizer.jsm, hg mv sanitize.js to Sanitizer.jsm, then edit the new Sanitizer.jsm

Or maybe it's a bug in the way mozReview shows this? Let me know.

::: testing/puppeteer/firefox/firefox_puppeteer/api/utils.py:38
(Diff revision 1)
> -        more: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js
> +        more: https://dxr.mozilla.org/mozilla-central/source/browser/modules/Sanitizer.jsm

I don't know anything about this thing, we'll need someone else to look at it once ready.

::: toolkit/components/thumbnails/test/browser_thumbnails_storage.js:10
(Diff revision 1)
>    return tmp.Sanitizer;

Are you sure this works?
You are returning a constructor from the module and not instantiating it...

Comment 14

[Alex Johnson (:alex_johnson)]

Marco, I've been busy with other stuff.  I think I'm going to unassign this one so someone else can pick it up.

Comment 15

[Marco Bonardo [:mak]]

I'm sorry to hear that. On the other side I think I'm converting this to a [diamond] bug since it's not trivial, especially for a newcomer. We have various requirements with blame history and some of the code in sanitize is not yet crystal clear.
Moreover lately we changed a bunch of code in sanitize, bitrotting the patch badly (but I think we're done for a while).

I may even take this, if I find some spare time.

Comment 16

[Marco Bonardo [:mak]]

oh well, I'll just take this as a side project.

Comment 18

[Marco Bonardo [:mak]]

Attachment: Bug 1167238 - Remove current Sanitize.jsm.

Review commit: https://reviewboard.mozilla.org/r/51261/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51261/

Comment 19

[Marco Bonardo [:mak]]

Attachment: Bug 1167238 - Convert sanitize.js to a module.

Review commit: https://reviewboard.mozilla.org/r/51263/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51263/

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8749949 [details]
Bug 1167238 - Remove current Sanitize.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51261/diff/1-2/

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8749950 [details]
Bug 1167238 - Convert sanitize.js to a module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51263/diff/1-2/

Comment 22

[Marco Bonardo [:mak]]

dropping off my plate, too much stuff.

Comment 23

[Johann Hofmann [:johannh]]

Would be great to have this, I'll take a stab at it.

Comment 24

[Johann Hofmann [:johannh]]

Attachment: Bug 1167238 - Part 1 - Remove Sanitizer.jsm shim.

Review commit: https://reviewboard.mozilla.org/r/214004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214004/

Comment 25

[Johann Hofmann [:johannh]]

Attachment: Bug 1167238 - Part 2 - Convert sanitize.js to a module.

Review commit: https://reviewboard.mozilla.org/r/214006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214006/

Comment 26

[Johann Hofmann [:johannh]]

Attachment: Bug 1167238 - Part 3 - Clean up usage of sanitize.js to properly use Sanitizer.jsm.

This replaces all non-test usage of sanitize.js or legacy Sanitizer.jsm
to use the new Sanitizer.jsm module which does not hold internal state
and instead receives all configuration through function arguments (or by reading prefs).

Review commit: https://reviewboard.mozilla.org/r/214008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214008/

Comment 27

[Johann Hofmann [:johannh]]

Attachment: Bug 1167238 - Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage.

We're changing the way sanitize.js/Sanitizer.jsm works and need to adjust a lot of tests to that.
I'm using this opportunity to also move the sanitization tests into their
own topical directory and out of b/b/c/test/general.

Review commit: https://reviewboard.mozilla.org/r/214010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214010/

Comment 28

[Johann Hofmann [:johannh]]

Attachment: Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

This cleans up all tests that were not moved into the sanitize directory
as part of the previous commit, but still use sanitize.js

Review commit: https://reviewboard.mozilla.org/r/214012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214012/

Comment 29

[Johann Hofmann [:johannh]]

Attachment: Bug 1167238 - Part 6 - Clean up sanitize.js usage in utils.py.

We're turning Sanitizer.jsm into a proper module and are simplifying
its API surface, and need to adjust all consumers.

Review commit: https://reviewboard.mozilla.org/r/214014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214014/

Comment 30

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c15ca9b2b6de24c6031e708f55c29270c883e67e

Comment 31

[Johann Hofmann [:johannh]]

So, this is still a little WIP (I haven't even run try on it), but feel free to take a first look at it. I feel like I'm at a point where I can't improve it any further apart from fixing failing tests, so review felt more appropriate than feedback. :)

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8943639 [details]
Bug 1167238 - Part 6 - Clean up sanitize.js usage in utils.py.

https://reviewboard.mozilla.org/r/214014/#review219764

The changes look fine to me. r=me in case try jobs are passing.

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/utils.py
(Diff revision 1)
> -                history: data_type.history || false,
> -                offlineApps: data_type.offlineApps || false,
> -                passwords: data_type.passwords || false,
> -                sessions: data_type.sessions || false,
> -                siteSettings: data_type.siteSettings || false
> -              };

Ah, nice! Thanks for the clean-up.

Comment 33

[Johann Hofmann [:johannh]]

Comment on attachment 8943637 [details]
Bug 1167238 - Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214010/diff/1-2/

Comment 34

[Johann Hofmann [:johannh]]

Comment on attachment 8943638 [details]
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214012/diff/1-2/

Comment 35

[Johann Hofmann [:johannh]]

Comment on attachment 8943639 [details]
Bug 1167238 - Part 6 - Clean up sanitize.js usage in utils.py.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214014/diff/1-2/

Comment 36

[Marco Bonardo [:mak]]

Comment on attachment 8943634 [details]
Bug 1167238 - Part 1 - Remove Sanitizer.jsm shim.

https://reviewboard.mozilla.org/r/214004/#review220812

Comment 37

[Marco Bonardo [:mak]]

Comment on attachment 8943635 [details]
Bug 1167238 - Part 2 - Convert sanitize.js to a module.

https://reviewboard.mozilla.org/r/214006/#review220814

looks good so far, will review the "final" version once you're ready

::: browser/modules/Sanitizer.jsm:80
(Diff revision 1)
> +
> +  _prefs: null,
> +  get prefs() {
> +    return Sanitizer._prefs ? Sanitizer._prefs
> +      : Sanitizer._prefs = Services.prefs.getBranch(Sanitizer.PREF_DOMAIN);
> +  },

This doesn't need to be part of Sanitizer, depending on the number of prefs, we could use defineLazyPreferenceGetter (if we care just about a few prefs) or a lazy Prefs Map in the global to track multiple prefs.
Currently it's only used for timespan, so either a DLPG or just read the pref every time. We can go fancy at a later time.

::: browser/modules/Sanitizer.jsm:88
(Diff revision 1)
> +   * Shows a sanitization dialog to the user.
> +   *
> +   * @param [optional] aParentWindow the window to use as
> +   *                   parent for the created dialog.
> +   */
> +  showUI(aParentWindow) {

when moving code, we could likely lose the XPCOM coding style with "a" prefixes and move to plain modern js style.

::: browser/modules/Sanitizer.jsm:103
(Diff revision 1)
> +
> +  /**
> +   * Performs startup tasks:
> +   *  - Checks if sanitization was interrupted during last shutdown.
> +   *  - Registers sanitize-on-shutdown.
> +   *

empty line

::: browser/modules/Sanitizer.jsm:107
(Diff revision 1)
> +   *  - Registers sanitize-on-shutdown.
> +   *
> +   */
> +  async onStartup() {
> +    // Check if we were interrupted during the last shutdown sanitization.
> +    let shutownSanitizationWasInterrupted =

there is a global typo here "shutownSanitizationWasInterrupted" (instead of shutdown). not a bug because it's copy-pasted everywhere...

::: browser/modules/Sanitizer.jsm:122
(Diff revision 1)
> +    }
> +
> +    // Make sure that we are triggered during shutdown.
> +    let shutdownClient = Cc["@mozilla.org/browser/nav-history-service;1"]
> +                           .getService(Ci.nsPIPlacesDatabase)
> +                           .shutdownClient

PlacesUtils.history.shutdownClient.jsclient will do.

Comment 38

[Marco Bonardo [:mak]]

Comment on attachment 8943636 [details]
Bug 1167238 - Part 3 - Clean up usage of sanitize.js to properly use Sanitizer.jsm.

https://reviewboard.mozilla.org/r/214008/#review220816

looks good, will review final version.

::: browser/components/nsBrowserGlue.js:249
(Diff revision 1)
> -      Services.scriptloader.loadSubScript("chrome://browser/content/sanitize.js", sanitizerScope);
> -      return sanitizerScope.Sanitizer;
> -    });
> -
>    XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts", "resource://gre/modules/FxAccounts.jsm");
> +  XPCOMUtils.defineLazyModuleGetter(this, "Sanitizer", "resource:///modules/Sanitizer.jsm");

Why not putting this in the global with the other modules, and then just refer to it as Sanitizer?

::: browser/components/nsBrowserGlue.js:2374
(Diff revision 1)
>    // public nsIBrowserGlue members
>    // ------------------------------
>  
>    sanitize: function BG_sanitize(aParentWindow) {
> -    this._sanitizer.sanitize(aParentWindow);
> +    this.Sanitizer.showUI(aParentWindow);
>    },

Note that the only reason for sanitize() in glue is that we didn't have a Sanitizer module. Indeed bug 1167237. We don't have to fix it here, the separate bug will be fine. Just pointing it out.

Comment 39

[Marco Bonardo [:mak]]

Comment on attachment 8943637 [details]
Bug 1167238 - Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage.

https://reviewboard.mozilla.org/r/214010/#review220818

::: browser/base/content/moz.build:68
(Diff revision 2)
>  
>  with Files("test/referrer/**"):
>      BUG_COMPONENT = ("Core", "Document Navigation")
>  
> +with Files("test/sanitize/**"):
> +    BUG_COMPONENT = ("Firefox", "Preferences")

This is interesting, in the sense the sanitizer and clear recent history never had a component and ended up either in general, in Bookmarks & History (imo wrong since that's only about bookmarks & history views) and Private Browsing (considered wrong as well by Ehsan who was the PB owner).

This seems to put it into Preferences. Imo it deserves a brief discussion in firefox-dev, if Jared (prefs owner I think) is fine with this, I'm ok with it. Otherwise, we could even evaluate to create its own component in Firefox or re-target Private Browsing to Private Browsing and Sanitization.
I'm fine either way, but I'd like this to have some kind of agreement by the team.

::: browser/base/content/test/sanitize/browser_sanitize-timespans.js:17
(Diff revision 2)
> -
>  var FormHistory = (Components.utils.import("resource://gre/modules/FormHistory.jsm", {})).FormHistory;
>  var Downloads = (Components.utils.import("resource://gre/modules/Downloads.jsm", {})).Downloads;
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +                                  "resource://gre/modules/PlacesUtils.jsm");

I suppose a head with some defineLazyModuleGetters would do for the most commonly used modules.

::: browser/base/content/test/sanitize/browser_sanitize-timespans.js:37
(Diff revision 2)
> + * @param aExpectedValue The expected value.
> + * @return {Promise}
> + * @resolves When the check has been added successfully.
> + * @rejects JavaScript exception.
> + */
> +function promiseIsURIVisited(aURI, aExpectedValue) {

This should use PlacesUtils.history.hasVisits (see bug 1370881)

::: browser/base/content/test/sanitize/browser_sanitizeDialog.js:32
(Diff revision 2)
>  XPCOMUtils.defineLazyModuleGetter(this, "Timer",
>                                    "resource://gre/modules/Timer.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
>                                    "resource://testing-common/PlacesTestUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
> +                                  "resource://gre/modules/PlacesUtils.jsm");

ditto

::: browser/base/content/test/sanitize/browser_sanitizeDialog.js:54
(Diff revision 2)
> +    function callbackDone() {
> +      if (++callbackCount == aURIs.length)
> +        resolve();
> +    }
> +    aURIs.forEach(function(aURI) {
> +      PlacesUtils.asyncHistory.isURIVisited(aURI, function(uri, isVisited) {

ditto

::: browser/base/content/test/sanitize/head.js:2
(Diff revision 2)
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +const {Sanitizer} = Components.utils.import("resource:///modules/Sanitizer.jsm", {});

Just defineLazyModuleGetters the most common ones, included Sanitizer.jsm?

Comment 40

[Marco Bonardo [:mak]]

Comment on attachment 8943638 [details]
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

https://reviewboard.mozilla.org/r/214012/#review220820

::: browser/base/content/test/newtab/head.js:22
(Diff revision 2)
>  
>  var tmp = {};
>  Cu.import("resource://gre/modules/NewTabUtils.jsm", tmp);
>  Cu.import("resource:///modules/DirectoryLinksProvider.jsm", tmp);
>  Cu.import("resource://testing-common/PlacesTestUtils.jsm", tmp);
> -Services.scriptloader.loadSubScript("chrome://browser/content/sanitize.js", tmp);
> +Cu.import("resource:///modules/Sanitizer.jsm", tmp);

definelazymodulegetters these?

Comment 41

[Marco Bonardo [:mak]]

Johann, this looks quite good, and I can't wait for it to land. We are early in the cycle and may be a good time to try to squash this into the tree. Thank you very much for bringing this on.

Comment 42

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #37)
> ::: browser/modules/Sanitizer.jsm:80
> (Diff revision 1)
> > +
> > +  _prefs: null,
> > +  get prefs() {
> > +    return Sanitizer._prefs ? Sanitizer._prefs
> > +      : Sanitizer._prefs = Services.prefs.getBranch(Sanitizer.PREF_DOMAIN);
> > +  },
> 
> This doesn't need to be part of Sanitizer, depending on the number of prefs,
> we could use defineLazyPreferenceGetter (if we care just about a few prefs)
> or a lazy Prefs Map in the global to track multiple prefs.
> Currently it's only used for timespan, so either a DLPG or just read the
> pref every time. We can go fancy at a later time.

If we fetch the prefs more than once, lazy pref getters would be preferable, yes.

Comment 43

[Ted Campbell [:tcampbell]]

We can probably stop using |this.XX| for top-levels. A simple |var EXPORTED_SYMBOLS = ...| and |function Sanitize()...| works fine and is preferred going forward in JSMs. If we ever get to ES6 modules, the global |this| will not be available anyways (and the EXPORTED_SYMBOLS stuff would simply be an 'export' keyword.

Comment 44

[Johann Hofmann [:johannh]]

Comment on attachment 8943634 [details]
Bug 1167238 - Part 1 - Remove Sanitizer.jsm shim.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214004/diff/1-2/

Comment 45

[Johann Hofmann [:johannh]]

Comment on attachment 8943635 [details]
Bug 1167238 - Part 2 - Convert sanitize.js to a module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214006/diff/1-2/

Comment 46

[Johann Hofmann [:johannh]]

Comment on attachment 8943636 [details]
Bug 1167238 - Part 3 - Clean up usage of sanitize.js to properly use Sanitizer.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214008/diff/1-2/

Comment 47

[Johann Hofmann [:johannh]]

Comment on attachment 8943637 [details]
Bug 1167238 - Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214010/diff/2-3/

Comment 48

[Johann Hofmann [:johannh]]

Comment on attachment 8943638 [details]
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214012/diff/2-3/

Comment 49

[Johann Hofmann [:johannh]]

Comment on attachment 8943639 [details]
Bug 1167238 - Part 6 - Clean up sanitize.js usage in utils.py.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214014/diff/2-3/

Comment 50

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf24cc2f5ff7301ed8b8c8543f1dbe64977f0605

Comment 51

[Johann Hofmann [:johannh]]

Comment on attachment 8943635 [details]
Bug 1167238 - Part 2 - Convert sanitize.js to a module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214006/diff/2-3/

Comment 52

[Johann Hofmann [:johannh]]

Comment on attachment 8943636 [details]
Bug 1167238 - Part 3 - Clean up usage of sanitize.js to properly use Sanitizer.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214008/diff/2-3/

Comment 53

[Johann Hofmann [:johannh]]

Comment on attachment 8943637 [details]
Bug 1167238 - Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214010/diff/3-4/

Comment 54

[Johann Hofmann [:johannh]]

Comment on attachment 8943638 [details]
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214012/diff/3-4/

Comment 55

[Johann Hofmann [:johannh]]

Comment on attachment 8943639 [details]
Bug 1167238 - Part 6 - Clean up sanitize.js usage in utils.py.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214014/diff/3-4/

Comment 56

[Code Review Bot [:reviewbot]]

Comment on attachment 8943638 [details]
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

https://reviewboard.mozilla.org/r/214012/#review222586


Static analysis found 4 defects in this patch.
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/plugins/browser_clearplugindata.js:9
(Diff revision 3)
>  
> -// Test clearing plugin data using sanitize.js.
> +// Test clearing plugin data using Sanitizer.jsm.
>  const testURL1 = gTestRoot + "browser_clearplugindata.html";
>  const testURL2 = gTestRoot + "browser_clearplugindata_noage.html";
>  
> -var tempScope = {};
> +const {Sanitizer} = Components.utils.import("resource:///modules/Sanitizer.jsm", {});

Error: Please use ChromeUtils.import instead of Cu.import [eslint: mozilla/use-chromeutils-import]

::: browser/components/originattributes/test/browser/browser_sanitize.js:11
(Diff revision 3)
>  
>  let {LoadContextInfo} = ChromeUtils.import("resource://gre/modules/LoadContextInfo.jsm", {});
>  
>  const TEST_DOMAIN = "http://example.net/";
>  
> -let tempScope = {};
> +const {Sanitizer} = Components.utils.import("resource:///modules/Sanitizer.jsm", {});

Error: Please use ChromeUtils.import instead of Cu.import [eslint: mozilla/use-chromeutils-import]

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_cache.js:13
(Diff revision 3)
>  
>  var {LoadContextInfo} = ChromeUtils.import("resource://gre/modules/LoadContextInfo.jsm", null);
>  
>  var tmp = {};
>  
> -Services.scriptloader.loadSubScript("chrome://browser/content/sanitize.js", tmp);
> +const {Sanitizer} = Components.utils.import("resource:///modules/Sanitizer.jsm", {});

Error: Please use ChromeUtils.import instead of Cu.import [eslint: mozilla/use-chromeutils-import]

::: toolkit/components/thumbnails/test/browser_thumbnails_storage.js:7
(Diff revision 3)
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  const URL = "http://mochi.test:8888/";
>  const URL_COPY = URL + "#copy";
>  
> -XPCOMUtils.defineLazyGetter(this, "Sanitizer", function() {
> +const {Sanitizer} = Components.utils.import("resource:///modules/Sanitizer.jsm", {});

Error: Please use ChromeUtils.import instead of Cu.import [eslint: mozilla/use-chromeutils-import]

Comment 57

[Johann Hofmann [:johannh]]

Comment on attachment 8943635 [details]
Bug 1167238 - Part 2 - Convert sanitize.js to a module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214006/diff/3-4/

Comment 58

[Johann Hofmann [:johannh]]

Comment on attachment 8943636 [details]
Bug 1167238 - Part 3 - Clean up usage of sanitize.js to properly use Sanitizer.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214008/diff/3-4/

Comment 59

[Johann Hofmann [:johannh]]

Comment on attachment 8943637 [details]
Bug 1167238 - Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214010/diff/4-5/

Comment 60

[Johann Hofmann [:johannh]]

Comment on attachment 8943638 [details]
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214012/diff/4-5/

Comment 61

[Johann Hofmann [:johannh]]

Comment on attachment 8943639 [details]
Bug 1167238 - Part 6 - Clean up sanitize.js usage in utils.py.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214014/diff/4-5/

Comment 62

[Johann Hofmann [:johannh]]

Comment on attachment 8943637 [details]
Bug 1167238 - Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage.

https://reviewboard.mozilla.org/r/214010/#review220818

> This is interesting, in the sense the sanitizer and clear recent history never had a component and ended up either in general, in Bookmarks & History (imo wrong since that's only about bookmarks & history views) and Private Browsing (considered wrong as well by Ehsan who was the PB owner).
> 
> This seems to put it into Preferences. Imo it deserves a brief discussion in firefox-dev, if Jared (prefs owner I think) is fine with this, I'm ok with it. Otherwise, we could even evaluate to create its own component in Firefox or re-target Private Browsing to Private Browsing and Sanitization.
> I'm fine either way, but I'd like this to have some kind of agreement by the team.

Yeah, I talked to Jared and he wasn't enthusiastic about putting it in Preferences, we're considering just adding a new component. Maybe we should chat about this tomorrow.

Comment 63

[Code Review Bot [:reviewbot]]

Comment on attachment 8943638 [details]
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

https://reviewboard.mozilla.org/r/214012/#review222600


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/thumbnails/test/browser_thumbnails_storage.js:7
(Diff revision 5)
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  const URL = "http://mochi.test:8888/";
>  const URL_COPY = URL + "#copy";
>  
> -XPCOMUtils.defineLazyGetter(this, "Sanitizer", function() {
> +const {Sanitizer} = Components.utils.import("resource:///modules/Sanitizer.jsm", {});

Error: Please use ChromeUtils.import instead of Cu.import [eslint: mozilla/use-chromeutils-import]

Comment 64

[Code Review Bot [:reviewbot]]

Comment on attachment 8943638 [details]
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

https://reviewboard.mozilla.org/r/214012/#review222606


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/thumbnails/test/browser_thumbnails_storage.js:7
(Diff revision 4)
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  const URL = "http://mochi.test:8888/";
>  const URL_COPY = URL + "#copy";
>  
> -XPCOMUtils.defineLazyGetter(this, "Sanitizer", function() {
> +const {Sanitizer} = Components.utils.import("resource:///modules/Sanitizer.jsm", {});

Error: Please use ChromeUtils.import instead of Cu.import [eslint: mozilla/use-chromeutils-import]

Comment 65

[Johann Hofmann [:johannh]]

Comment on attachment 8943638 [details]
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214012/diff/5-6/

Comment 66

[Johann Hofmann [:johannh]]

Comment on attachment 8943639 [details]
Bug 1167238 - Part 6 - Clean up sanitize.js usage in utils.py.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214014/diff/5-6/

Comment 67

[Marco Bonardo [:mak]]

Is this "landable" in case, or still a WIP?

Comment 68

[Marco Bonardo [:mak]]

(In reply to Johann Hofmann [:johannh] from comment #62)
> Yeah, I talked to Jared and he wasn't enthusiastic about putting it in
> Preferences, we're considering just adding a new component. Maybe we should
> chat about this tomorrow.

Having a component for Data Sanitization, also including the Clear Recent History Dialog and ForgetAboutSite.jsm bugs would probably be great. The less bugs we pile in General, the better is.

Since you were discussing about moving Sanitize to toolkit, I must note Forget About Site is a toolkit component and it's related. I suspect people tend to fix Sanitize but may forget to port the same fixes to ForgetaboutSite.jsm Having both together could help.
The current owner is Ehsan, but I doubt he's interested in growing that component with all the duties he already has for DOM and JS. If we'd find a new owner we could probably coalesce things a bit there (maybe rename the component to Data Sanitization or Forget About Site and Sanitize).
The Clear Recent History dialog bugs imo could still be part of Firefox Preferences, being in browser.

Comment 69

[Marco Bonardo [:mak]]

Comment on attachment 8943635 [details]
Bug 1167238 - Part 2 - Convert sanitize.js to a module.

https://reviewboard.mozilla.org/r/214006/#review223228

::: browser/modules/Sanitizer.jsm:8
(Diff revision 4)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -/* import-globals-from sanitizeDialog.js */
> +var EXPORTED_SYMBOLS = ["Sanitizer"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

These should be no more necessary after bug 767640.

::: browser/modules/Sanitizer.jsm:147
(Diff revision 4)
> -  prefDomain: "",
> +  /**
> +   * Returns a 2 element array representing the start and end times,
> +   * in the uSec-since-epoch format that PRTime likes. If we should
> +   * clear everything, this function returns null.
> +   *
> +   * @param [optional] a timespan to convert to start and end time.

nit: it's missing the param name

::: browser/modules/Sanitizer.jsm:776
(Diff revision 4)
> -    default:
> -      throw "Invalid time span for clear private data: " + ts;
>    }
> -  return [startDate, endDate];
> +
> +  // Ensure open windows get cleared first, if they're in our list, so that they don't stick
> +  // around in the recently closed windows list, and so we can cancel the whole thing

nit: since you're moving this comment, could you please wrap it to ~80 chars?

Comment 70

[Marco Bonardo [:mak]]

Comment on attachment 8943636 [details]
Bug 1167238 - Part 3 - Clean up usage of sanitize.js to properly use Sanitizer.jsm.

https://reviewboard.mozilla.org/r/214008/#review223282

Comment 71

[Marco Bonardo [:mak]]

Comment on attachment 8943637 [details]
Bug 1167238 - Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage.

https://reviewboard.mozilla.org/r/214010/#review223284

Comment 72

[Johann Hofmann [:johannh]]

(In reply to Marco Bonardo [::mak] from comment #68)
> (In reply to Johann Hofmann [:johannh] from comment #62)
> > Yeah, I talked to Jared and he wasn't enthusiastic about putting it in
> > Preferences, we're considering just adding a new component. Maybe we should
> > chat about this tomorrow.
> 
> Having a component for Data Sanitization, also including the Clear Recent
> History Dialog and ForgetAboutSite.jsm bugs would probably be great. The
> less bugs we pile in General, the better is.
> 
> Since you were discussing about moving Sanitize to toolkit, I must note
> Forget About Site is a toolkit component and it's related. I suspect people
> tend to fix Sanitize but may forget to port the same fixes to
> ForgetaboutSite.jsm Having both together could help.

Yeah, I agree, that's what I'd like to do in bug 1422365 (and probably dependent bugs).

> The current owner is Ehsan, but I doubt he's interested in growing that
> component with all the duties he already has for DOM and JS. If we'd find a
> new owner we could probably coalesce things a bit there (maybe rename the
> component to Data Sanitization or Forget About Site and Sanitize).
> The Clear Recent History dialog bugs imo could still be part of Firefox
> Preferences, being in browser.

Ok, I can file a bug for a "Data Sanitization" component. I don't think it really matters whether we put it in Firefox or Toolkit at this point.

Comment 73

[Johann Hofmann [:johannh]]

For reference I filed bug 1435528 but we decided to land the tests with the Firefox::General component while that bug is being sorted out. I'll rebase and land this now!

Comment 74

[Johann Hofmann [:johannh]]

Comment on attachment 8943634 [details]
Bug 1167238 - Part 1 - Remove Sanitizer.jsm shim.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214004/diff/2-3/

Comment 75

[Johann Hofmann [:johannh]]

Comment on attachment 8943635 [details]
Bug 1167238 - Part 2 - Convert sanitize.js to a module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214006/diff/4-5/

Comment 76

[Johann Hofmann [:johannh]]

Comment on attachment 8943636 [details]
Bug 1167238 - Part 3 - Clean up usage of sanitize.js to properly use Sanitizer.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214008/diff/4-5/

Comment 77

[Johann Hofmann [:johannh]]

Comment on attachment 8943637 [details]
Bug 1167238 - Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214010/diff/5-6/

Comment 78

[Johann Hofmann [:johannh]]

Comment on attachment 8943638 [details]
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214012/diff/6-7/

Comment 79

[Johann Hofmann [:johannh]]

Comment on attachment 8943639 [details]
Bug 1167238 - Part 6 - Clean up sanitize.js usage in utils.py.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214014/diff/6-7/

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d001895822cd
Part 1 - Remove Sanitizer.jsm shim. r=mak
https://hg.mozilla.org/integration/autoland/rev/38091dd39b83
Part 2 - Convert sanitize.js to a module. r=mak
https://hg.mozilla.org/integration/autoland/rev/d81e94b05091
Part 3 - Clean up usage of sanitize.js to properly use Sanitizer.jsm. r=mak
https://hg.mozilla.org/integration/autoland/rev/092f32e30ed9
Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage. r=mak
https://hg.mozilla.org/integration/autoland/rev/45425d68c1dd
Part 5 - Clean up sanitize.js usage in remaining tests. r=mak
https://hg.mozilla.org/integration/autoland/rev/93903673b0eb
Part 6 - Clean up sanitize.js usage in utils.py. r=whimboo

Comment 81

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d001895822cd
https://hg.mozilla.org/mozilla-central/rev/38091dd39b83
https://hg.mozilla.org/mozilla-central/rev/d81e94b05091
https://hg.mozilla.org/mozilla-central/rev/092f32e30ed9
https://hg.mozilla.org/mozilla-central/rev/45425d68c1dd
https://hg.mozilla.org/mozilla-central/rev/93903673b0eb

Comment 82

[roxleitan]

This bug was verified during Site Storage UI Redesign feature testing.
I will mark as Verified fixed.