839778 - Use OS.File in about:crashes

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 1

[Drew Willcoxon :adw]

Comment on attachment 712131 [details] [diff] [review]
Patch

Review of attachment 712131 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is OK.

::: toolkit/crashreporter/content/crashes.js
@@ +117,5 @@
> +        return result;
> +      }, function onError() {});
> +    } catch (e) {
> +      if (!(e instanceof OS.File.Error && e.becauseNoSuchFile)) {
> +        throw e;

This pattern of calling DirectoryIterator in a try-catch to get a promise is repeated five times in the patch.  Couldn't you factor it out into a function that returns the promise?

@@ +136,3 @@
>  
> +    if (submittedPromise) {
> +      let submittedEntries = yield submittedPromise;

I'd prefer to keep the bodies of Task functions as small as possible.  The first yield is here, so couldn't the Task be spawned here, at the conditional?  Same question for the other Task in the patch.

Comment 2

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

What's the status of this bug?

Comment 3

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

De-assigning

Comment 4

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

Marco, do you want to pick up this bug again?

Comment 5

[Marco Castelluccio [:marco]]

Ok, I'll rewrite the patch.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Patch v2

Rewritten it from scratch, I've avoided the three promises and just do things sequentially.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8402977 [details] [diff] [review]
Patch v2

Review of attachment 8402977 [details] [diff] [review]:
-----------------------------------------------------------------

just some drive-by comments (sorry I was curious about the patch)

::: toolkit/crashreporter/content/crashes.js
@@ +6,5 @@
>  
>  var reportURL;
>  
> +Cu.import("resource://gre/modules/CrashReports.jsm");
> +Cu.import("resource://gre/modules/CrashSubmit.jsm");

why isn't CrashSubmit.jsm lazyModuleGetter-ed? it has only one use and doesn't look like it's always invoked.

@@ +127,5 @@
>      body.appendChild(row);
>    }
>  }
>  
> +clearReports = function() {

missing a let (though what's the point of not doing function name() here, doesn't look like it's breaking strict)

fwiw you may do:
let clearReports = Task.async(function* () {
  ...
});

though then the caller should handle the resolution, since you can't put a .then here. But that may make a cleaner API

@@ +130,5 @@
>  
> +clearReports = function() {
> +  Task.spawn(function*() {
> +    var bundles = Cc["@mozilla.org/intl/stringbundle;1"].
> +                  getService(Ci.nsIStringBundleService);

Services.strings

@@ +133,5 @@
> +    var bundles = Cc["@mozilla.org/intl/stringbundle;1"].
> +                  getService(Ci.nsIStringBundleService);
> +    var bundle = bundles.createBundle("chrome://global/locale/crashes.properties");
> +    var prompts = Cc["@mozilla.org/embedcomp/prompt-service;1"].
> +                  getService(Ci.nsIPromptService);

Services.prompt

@@ +142,2 @@
>  
> +    let iterator = new OS.File.DirectoryIterator(CrashReports.submittedDir.path);

mixing let and var without a reason, just change other vars inside this function to let?

@@ +144,5 @@
> +    try {
> +      for (let promiseEntry in iterator) {
> +        let entry = yield promiseEntry;
> +
> +        let leaf = entry.name;

nit: filename? :)

@@ +145,5 @@
> +      for (let promiseEntry in iterator) {
> +        let entry = yield promiseEntry;
> +
> +        let leaf = entry.name;
> +        if (leaf.substr(0, 3) == "bp-" &&

leaf.startsWith("bp-")

@@ +146,5 @@
> +        let entry = yield promiseEntry;
> +
> +        let leaf = entry.name;
> +        if (leaf.substr(0, 3) == "bp-" &&
> +            leaf.substr(-4) == ".txt") {

leaf.endsWith(".txt")

@@ +168,5 @@
> +          date = stat.lastModificationDate;
> +        }
> +
> +        let leaf = entry.name;
> +        if (leaf.substr(0, 11) == "InstallTime" &&

startsWith

@@ +181,3 @@
>      }
> +
> +    iterator = new OS.File.DirectoryIterator(CrashReports.pendingDir.path);

nit: may create a cleanupFolder(path, filter) helper and reuse it these 3 times. would save some boilerplate

Comment 8

[Marco Castelluccio [:marco]]

(In reply to Marco Bonardo [:mak] from comment #7)
> ::: toolkit/crashreporter/content/crashes.js
> @@ +6,5 @@
> >  
> >  var reportURL;
> >  
> > +Cu.import("resource://gre/modules/CrashReports.jsm");
> > +Cu.import("resource://gre/modules/CrashSubmit.jsm");
> 
> why isn't CrashSubmit.jsm lazyModuleGetter-ed? it has only one use and
> doesn't look like it's always invoked.

I wanted to build a self-contained patch, so I've just "translated" the nsIFile parts to OS.File (this applies to the other comments too).
I can refactor a bit the code if you want.

> 
> @@ +127,5 @@
> >      body.appendChild(row);
> >    }
> >  }
> >  
> > +clearReports = function() {
> 
> missing a let (though what's the point of not doing function name() here,
> doesn't look like it's breaking strict)

At the beginning I was using Task.async, I went back to Task.spawn because of the |then|, but eventually I forgot to modify the line back to "function clearReports()" :D

> fwiw you may do:
> let clearReports = Task.async(function* () {
>   ...
> });
> 
> though then the caller should handle the resolution, since you can't put a
> .then here. But that may make a cleaner API

Good idea.

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8402977 [details] [diff] [review]
Patch v2

Review of attachment 8402977 [details] [diff] [review]:
-----------------------------------------------------------------

Rubber-stamping Marco's review. :-)  r+ with comments addressed, nits at your discretion.

::: toolkit/crashreporter/content/crashes.js
@@ +6,5 @@
>  
>  var reportURL;
>  
> +Cu.import("resource://gre/modules/CrashReports.jsm");
> +Cu.import("resource://gre/modules/CrashSubmit.jsm");

I agree with Marco about lazifying CrashSubmit.  It's only used when you click a submit link.  Task and osfile should be lazified, too, since they're only used in clearReports, which is only called when you click the clear button.

@@ +170,5 @@
> +
> +        let leaf = entry.name;
> +        if (leaf.substr(0, 11) == "InstallTime" &&
> +            date < oneYearAgo &&
> +            leaf != "InstallTime" + buildID) {

Nit: Some refactoring, but it looks like you could avoid getting the date altogether if `leaf != "InstallTime" + buildID` is false.

Comment 10

[Drew Willcoxon :adw]

Of course if you go with Task.async() in the global scope, no need to lazify Task.

Thanks for picking this back up.

Comment 11

[Marco Castelluccio [:marco]]

(In reply to Drew Willcoxon :adw from comment #9)
> Comment on attachment 8402977 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8402977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Rubber-stamping Marco's review. :-)  r+ with comments addressed, nits at
> your discretion.
> 
> ::: toolkit/crashreporter/content/crashes.js
> @@ +6,5 @@
> >  
> >  var reportURL;
> >  
> > +Cu.import("resource://gre/modules/CrashReports.jsm");
> > +Cu.import("resource://gre/modules/CrashSubmit.jsm");
> 
> I agree with Marco about lazifying CrashSubmit.  It's only used when you
> click a submit link.  Task and osfile should be lazified, too, since they're
> only used in clearReports, which is only called when you click the clear
> button.

I think we usually avoid to lazy load Task.jsm and osfile.jsm because they're assumed to be loaded right at startup.

Comment 12

[Marco Castelluccio [:marco]]

Attachment: Patch v3

I did a small refactoring and added a test to ensure no regressions happen.

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8404113 [details] [diff] [review]
Patch v3

Review of attachment 8404113 [details] [diff] [review]:
-----------------------------------------------------------------

I learned some things from this patch.  Thanks also for the test.

::: toolkit/crashreporter/content/crashes.js
@@ +157,5 @@
> +
> +  yield cleanupFolder(CrashReports.submittedDir.path, function*(aEntry) {
> +    if (aEntry.name.startsWith("bp-") &&
> +        aEntry.name.endsWith(".txt")) {
> +      return true;

return aEntry.name.startsWith("bp-") &&
       aEntry.name.endsWith(".txt");

::: toolkit/crashreporter/test/browser/browser_clearReports.js
@@ +1,1 @@
> +function clickClearReports(tab, cb) {

This file needs a license header.

@@ +17,5 @@
> +    for (let mutation of mutations) {
> +      if (mutation.type == "attributes" &&
> +          mutation.attributeName == "style") {
> +        is(style.display, "none", "Clear reports button hidden");
> +        cb();

observer.disconnect();

@@ +34,5 @@
> +
> +var promptShown = false;
> +
> +let oldPrompt = Services.prompt;
> +Services.prompt = {

Add a registerCleanupFunction right here for restoring oldPrompt instead of waiting until the end of test().

@@ +85,5 @@
> +  report4.lastModifiedTime = Date.now() - 63172000000;
> +
> +  let tab = gBrowser.selectedTab = gBrowser.addTab("about:blank");
> +  let browser = gBrowser.getBrowserForTab(tab);
> +  let onLoad = function() {

function onLoad() {

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Patch v3

Carrying r+.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/597f5b9dba63

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/597f5b9dba63

Comment 17

[Gregory Szorc [:gps]]

A follow-up to this bug is bug 946645. We already established a JSM for interacting with crash files. Ideally about:crashes would be rewritten to use that API.