Closed Bug 952465 Opened 6 years ago Closed 6 years ago

[Session Restore] Replace Components.utils.reportError with Console.jsm

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: Yoric, Assigned: lpy)

Details

(Whiteboard: [Async][mentor=Yoric][lang=js][qa-])

Attachments

(1 file, 1 obsolete file)

Session Restore uses Components.utils.reportError (often abbreviated as Cu.reportError) to report errors. We should rather use the more complete Console.jsm.

The code that needs to be patched lives in:
http://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/
Assignee: nobody → pylaurent1314
Attached patch bug952465.patch (obsolete) — Splinter Review
replace reportError with console.error
Attachment #8350876 - Flags: review?(dteller)
Comment on attachment 8350876 [details] [diff] [review]
bug952465.patch

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

Looks good, thanks.
r=me with the changes below, if it passes tests

::: browser/components/sessionstore/src/SessionFile.jsm
@@ +35,5 @@
>  Cu.import("resource://gre/modules/osfile/_PromiseWorker.jsm", this);
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.import("resource://gre/modules/AsyncShutdown.jsm");
>  
> +let console = Cu.import("resource://gre/modules/devtools/Console.jsm", {}).console;

Let's make it lazy:
 XPCOMUtils.defineLazyModuleGetter(this, "console", "resource://...");

This will ensure that we load the Console module only when we actually need it, which will make startup a little faster. Also, could you move this a bit lower, close to the other calls to |XPCOMUtils.defineLazyModuleGetter|?

Same thing for the other files.

@@ +103,5 @@
>    captureErrors: function captureErrors(promise) {
>      return promise.then(
>        null,
>        function onError(reason) {
> +        console.error("Uncaught asynchronous error: " + reason + " at\n" + reason.stack);

Ideally, this should be
 console.error("Uncaught asynchronous error", reason, "at", reason.stack);

However, we'll completely remove TaskUtils one of these days, so that's not very important.

@@ +181,5 @@
>          this._recordTelemetry(msg.telemetry);
>        } catch (ex) {
>          TelemetryStopwatch.cancel("FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS", refObj);
> +        console.error("Could not write session state file " + this.path
> +                      + ": " + ex);

This will be more precise if you write
  console.error("Could not write session state file", this.path, ex);
Attachment #8350876 - Flags: review?(dteller) → review+
Thank you Yoric :)
Attachment #8350876 - Attachment is obsolete: true
Attachment #8356154 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/c0c89e5d0cf1
Keywords: checkin-needed
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async][mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c0c89e5d0cf1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Async][mentor=Yoric][lang=js][fixed-in-fx-team] → [Async][mentor=Yoric][lang=js]
Target Milestone: --- → Firefox 29
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async][mentor=Yoric][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.