Closed Bug 1167238 Opened 9 years ago Closed 6 years ago

Convert sanitize.js to a module

Categories

(Firefox :: General, defect, P1)

Product:
Firefox
Component:
General
Type:
defect
Points:
3

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- verified

People

(Reporter: mak, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [storage-v2])

Attachments

(6 files, 3 obsolete files)

Bug 1167238 - Part 1 - Remove Sanitizer.jsm shim.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1167238 - Part 2 - Convert sanitize.js to a module.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1167238 - Part 3 - Clean up usage of sanitize.js to properly use Sanitizer.jsm.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1167238 - Part 4 - Move sanitize tests into a topical directory and clean up sanitize.js usage.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1167238 - Part 5 - Clean up sanitize.js usage in remaining tests.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1167238 - Part 6 - Clean up sanitize.js usage in utils.py.
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
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.
Flags: qe-verify+
Flags: firefox-backlog+
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.
Depends on: 1089695
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)
I would love to work on this bug, can someone please assign it to me? Thanks!
Flags: needinfo?(mak77)
Flags: needinfo?(mak77)
Sorry Marco I didn't mean to clear the flag I still want to be assigned.
Flags: needinfo?(mak77)
Flags: needinfo?(mak77)
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.
(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!
This is a dupe of bug 397719, but I guess we can dupe forward.
cool, we can use that bug and patch for reference once we can proceed.
I would like to work on this bug.
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)
Assignee: nobody → mrjohnsonalex
Depends on: 1211849
Status: NEW → ASSIGNED
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...
Attachment #8709778 - Flags: review?(mak77)
Marco, I've been busy with other stuff.  I think I'm going to unassign this one so someone else can pick it up.
Assignee: me → nobody
Status: ASSIGNED → NEW
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.
Whiteboard: [good first bug][lang=js] → [diamond][lang=js]
oh well, I'll just take this as a side project.
Assignee: nobody → mak77
Mentor: mak77
Status: NEW → ASSIGNED
Whiteboard: [diamond][lang=js]
Attachment #8709778 - Attachment is obsolete: true
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/
Attachment #8749949 - Attachment description: MozReview Request: Bug 1167238 - Remove current Sanitize.jsm. → Bug 1167238 - Remove current Sanitize.jsm.
Attachment #8749950 - Attachment description: MozReview Request: Bug 1167238 - Convert sanitize.js to a module. → Bug 1167238 - Convert sanitize.js to a module.
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/
Whiteboard: [storage-v2][triage]
Blocks: 1422365
dropping off my plate, too much stuff.
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Would be great to have this, I'll take a stab at it.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [storage-v2][triage] → [storage-v2]
Attachment #8749949 - Attachment is obsolete: true
Attachment #8749950 - Attachment is obsolete: true
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 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.
Attachment #8943639 - Flags: review?(hskupin) → review+
Comment on attachment 8943634 [details]
Bug 1167238 - Part 1 - Remove Sanitizer.jsm shim.

https://reviewboard.mozilla.org/r/214004/#review220812
Attachment #8943634 - Flags: review?(mak77) → review+
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.
Attachment #8943635 - Flags: review?(mak77)
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.
Attachment #8943636 - Flags: review?(mak77)
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?
Attachment #8943637 - Flags: review?(mak77)
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?
Attachment #8943638 - Flags: review?(mak77) → review+
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.
(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.
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 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 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 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 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]
Is this "landable" in case, or still a WIP?
(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 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?
Attachment #8943635 - Flags: review?(mak77) → review+
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
Attachment #8943636 - Flags: review?(mak77) → review+
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
Attachment #8943637 - Flags: review?(mak77) → review+
(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.
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!
Pushed by jhofmann@mozilla.com:
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
Depends on: 1437486
This bug was verified during Site Storage UI Redesign feature testing.
I will mark as Verified fixed.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: