956678 - [Session Restore] Get rid of TaskUtils

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

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

Now that Promise can report uncaught errors, TaskUtils is not needed anymore. We should just remove it and replace all the calls to TaskUtils.spawn with Task.spawn.

Comment 1

[Johannes Mittendorfer]

As I'm not into Firefox developement:

Is the API of Task exactly the same as TaskUtils, what is about TaskUtils.readBlob() and should the definition of the object also be removed from the code?

Comment 2

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

I realize that I haven't been clear. The objective of this bug is to get rid of TaskUtils in directory browser/components/sessionstore/src. In this directory, there is no TaskUtils.readBlob().

Comment 3

[Johannes Mittendorfer]

Attachment: TaskUtilsToTask.patch

Comment 4

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

Comment on attachment 8356578 [details] [diff] [review]
TaskUtilsToTask.patch

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

Looks good to me, thanks for the patch.

If you haven't done so yet, you should make sure that your hg is configured as per https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F to ensure that the patch matches our conventions.

Also:
- In your commit message, could you replace "browser/components/sessionstore/src" with just "Session Restore"? This will be more readable.
- Add ";r=yoric" at the end of your commit message, to mark that I was the reviewer.
- I'll run the automated tests.
- Once we are satisfied that everything works, we'll mark this as "checkin-needed" and this code will be added to Firefox.

Comment 5

[Johannes Mittendorfer]

Should I reupload the patch with the suggested changes or should I just pay attention to it next time?

Comment 6

[Johannes Mittendorfer]

Comment on attachment 8356578 [details] [diff] [review]
TaskUtilsToTask.patch

># HG changeset patch
># Parent c0b5c88949d7d51bb92284a93058923be5b0c072
># User Johannes Mittendorfer <[e-mail]>
>Bug 956678 - Replacing TaskUtils.spawn() by Task.spawn() in Session Restore;r=yoric
>diff --git a/browser/components/sessionstore/src/SessionFile.jsm b/browser/components/sessionstore/src/SessionFile.jsm
>--- a/browser/components/sessionstore/src/SessionFile.jsm
>+++ b/browser/components/sessionstore/src/SessionFile.jsm
>@@ -85,49 +85,16 @@ this.SessionFile = {
>   }
> };
> 
> Object.freeze(SessionFile);
> 
> /**
>  * Utilities for dealing with promises and Task.jsm
>  */
>-const TaskUtils = {
>-  /**
>-   * Add logging to a promise.
>-   *
>-   * @param {Promise} promise
>-   * @return {Promise} A promise behaving as |promise|, but with additional
>-   * logging in case of uncaught error.
>-   */
>-  captureErrors: function captureErrors(promise) {
>-    return promise.then(
>-      null,
>-      function onError(reason) {
>-        Cu.reportError("Uncaught asynchronous error: " + reason + " at\n" + reason.stack);
>-        throw reason;
>-      }
>-    );
>-  },
>-  /**
>-   * Spawn a new Task from a generator.
>-   *
>-   * This function behaves as |Task.spawn|, with the exception that it
>-   * adds logging in case of uncaught error. For more information, see
>-   * the documentation of |Task.jsm|.
>-   *
>-   * @param {generator} gen Some generator.
>-   * @return {Promise} A promise built from |gen|, with the same semantics
>-   * as |Task.spawn(gen)|.
>-   */
>-  spawn: function spawn(gen) {
>-    return this.captureErrors(Task.spawn(gen));
>-  }
>-};
>-
> let SessionFileInternal = {
>   /**
>    * The path to sessionstore.js
>    */
>   path: OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.js"),
> 
>   /**
>    * The path to sessionstore.bak
>@@ -161,17 +128,17 @@ let SessionFileInternal = {
> 
>     let isFinalWrite = false;
>     if (Services.startup.shuttingDown) {
>       // If shutdown has started, we will want to stop receiving
>       // write instructions.
>       isFinalWrite = this._isClosed = true;
>     }
> 
>-    return this._latestWrite = TaskUtils.spawn(function task() {
>+    return this._latestWrite = Task.spawn(function task() {
>       TelemetryStopwatch.start("FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS", refObj);
> 
>       try {
>         let promise = SessionWorker.post("write", [aData]);
>         // At this point, we measure how long we stop the main thread
>         TelemetryStopwatch.finish("FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS", refObj);
> 
>         // Now wait for the result and record how long the write took

Comment 7

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

If would be better if you could reupload the patch. No need to ask me for another review, though.
Usually, we try to add a version number when we upload the patch (not in the commit message, just in Bugzilla), so this would be your v2.

Comment 8

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

I believe that there was a manipulation error: you seem to have copied and pasted the patch instead of reuploading it.

Comment 9

[Johannes Mittendorfer]

Attachment: TaskUtilsToTaskV2.patch

New Version of the patch with the suggested changes

Comment 10

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

Test results will appear here: https://tbpl.mozilla.org/?tree=Try&rev=6a9c7e96329a

Comment 11

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

Looks good, let's land this.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/77d90521f46f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77d90521f46f