1245780 - In about:debugging tests, consider using ContentTask.spawn() instead of loading frameScripts via data URI

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P3)

Description

[Jan Keromnes [:janx]]

Small code improvement suggested by :pbro.

Relevant tests:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_service_workers_timeout.js#55
https://dxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_service_workers.js#48

Comment 1

[Jan Keromnes [:janx]]

Patrick, I think you've been doing similar changes to other tests.

Could you please describe the benefits of using `ContentTask.spawn()` over loading frameScripts via data URI? And maybe share a few examples in which you did that?

Comment 2

[Patrick Brosset <:pbro>]

We used to (in still do in some test directories) load frame-scripts in various head.js files. Like, when a new tab is being created in `addTab` (or similar), we load the frame script in that new browser window [0]. Which means that we have various frame script files [1] in various test directories, as well as a common one somewhere [2]. And that also means that we have various versions of helper functions in these head.js that are used to send and receive messages to these frame scripts [3].

We still do this in a number of places, but tend to remove them and replace them with `ContentTask.spawn` wherever we can.
Having a common frame script [2] is good because it helps with putting code in common and avoiding duplication, but the overhead of loading this everywhere [1] and having those helpers to communicate [3] is large. Using `ContentTask.spawn` is a much simpler and more standalone way of executing code in the content process.

Now of course, if all our test directories' head.js files would load the shared-head.js file [4] then we could centralize all of this code there and write it once, and have all of our tests sending messages to our common frame script only.

Also, I hadn't come across using loading framescripts as data URI too much before. I guess it's pretty similar to using `ContentTask.spawn` then, with the added complexity that you actually need to load the script.

Anyway, that's some of the background/rationale behind all this.
Using ContentTask is short, sweet, and simple. And is built-in to the test harness. So wherever we load custom frame scripts for things we use only once, I would suggest not loading it and just using ContentTask instead because that means being able to remove a bunch of code.

[0] https://dxr.mozilla.org/mozilla-central/source/devtools/client/jsonview/test/head.js#41-43
[1] https://dxr.mozilla.org/mozilla-central/search?q=path%3Adoc_frame_script&redirect=false&case=false
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/frame-script-utils.js
[3] https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/test/head.js#114-164
[4] https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js

Comment 3

[Jan Keromnes [:janx]]

Benoit, would you be interested in investigating this bug?

Comment 4

[Benoit Chabod [:bigben]]

Sure! I will take a look at Patrick's examples and see what I can do for about:debugging.

Comment 5

[Benoit Chabod [:bigben]]

Attachment: 1st patch - use ContentTask to load frame scripts in tests

Here's a patch for about:debugging tests.
I have replaced every usage of loadFrameScript() with ContentTask.spawn() promise logic, and this enabled me to remove unnecessary system messages (when possible).
Side correction: service worker unregistration tests now fail upon error, instead of timing out.

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8754742 [details] [diff] [review]
1st patch - use ContentTask to load frame scripts in tests

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

Very nice code simplification!

Your patch looks correct, but Patrick or Alex would be better suited to review this change.

Please update the patch description accordingly ("r="), and please also submit it to treeherder in order to verify that all about:debugging tests still pass.

::: devtools/client/aboutdebugging/test/head.js
@@ +273,5 @@
>   * has successfully registered itself.
>   * @param {Tab} tab
>   */
>  function waitForServiceWorkerRegistered(tab) {
> +  return ContentTask.spawn(tab.linkedBrowser, {}, function () {

Nit: It looks like sometimes this function is a generator, sometimes not. Is it because you don't use yield here?

Comment 7

[Benoit Chabod [:bigben]]

Attachment: 1st patch - use ContentTask to load frame scripts in tests, r=pbro f=janx

Thanks!
Here's the same patch with updated reviewer.
Indeed, I chose to use a function generator if I call yield, and a simple function otherwise.
Patrick, I'm looking forward to your advice :)

Comment 8

[Jan Keromnes [:janx]]

(In reply to Benoit Chabod [:bigben] from comment #7)
> Indeed, I chose to use a function generator if I call yield, and a simple
> function otherwise.

While this makes sense for your patch, please keep in mind that in the future, other developers might try to add `yield` to these functions, or copy/paste your code as boilerplate and then use `yield`, and might get confused why it doesn't work.

It's a small detail, but in cases like these it might be nicer for future contributors to have generators even without yield.

Comment 9

[Benoit Chabod [:bigben]]

(In reply to Jan Keromnes [:janx] from comment #8)
> While this makes sense for your patch, please keep in mind that in the
> future, other developers might try to add `yield` to these functions, or
> copy/paste your code as boilerplate and then use `yield`, and might get
> confused why it doesn't work.
> 
> It's a small detail, but in cases like these it might be nicer for future
> contributors to have generators even without yield.

You're right, I will change this along with Patrick's potential remarks.
Here's the job to prove that about:debugging tests still pass :

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

Comment 10

[Benoit Chabod [:bigben]]

Please don't mind the previous comment, a problem with taskcluster prevented the job from starting. Here's another link to prove that about:debugging tests still pass :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=989655a172a9

The gecko-decision opt error is still due to Bug 1273673, I could rebase and launch the job again but I believe this is sufficient to show that tests still pass.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8754791 [details] [diff] [review]
1st patch - use ContentTask to load frame scripts in tests, r=pbro f=janx

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

Thanks for the cleanup. This looks good. I just have a couple of comments.

::: devtools/client/aboutdebugging/test/browser_service_workers.js
@@ +41,5 @@
>  
> +  yield unregisterServiceWorker(swTab).then(() => {
> +    ok(true, "Service worker registration unregistered");
> +  }).catch(function (e) {
> +    ok(false, "SW not unregistered; " + e);

I think you should use try/catch in tasks to catch rejected promises:

try {
  yield unregisterServiceWorker(swTab);
  ok(true, "Service worker registration unregistered");
} catch (e) {
  ok(false, "SW not unregistered; " + e);
}

At least, this was how rejected promises were reported in tasks last time I checked: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm#Exception_handling

But, the test will fail in case of a rejection too, so I don't think you need to do anything else than:

yield unregisterServiceWorker(swTab);

You could test this easily by changing unregisterServiceWorker in head.js, making it return a rejected promise, and seeing how the test behaves.

::: devtools/client/aboutdebugging/test/head.js
@@ +270,5 @@
>  
>  /**
>   * Returns a promise that will resolve after the service worker in the page
>   * has successfully registered itself.
>   * @param {Tab} tab

Since you're changing this function, it's a good opportunity to correct the jsdoc for it. In particular it misses:
@return {Promise} Resolves when the service worker is registered.

@@ +278,3 @@
>      // Retrieve the `sw` promise created in the html page.
>      let { sw } = content.wrappedJSObject;
> +    return sw;

I think it would be more obvious to readers that we're waiting until |sw| resolves if it was written as a generator that yields on it. |sw| doesn't need to be returned here anyway, right?

function* () {
  let { sw } = content.wrappedJSObject;
  yield sw;
}

@@ +283,5 @@
>  
>  /**
>   * Asks the service worker within the test page to unregister, and returns a
>   * promise that will resolve when it has successfully unregistered itself.
>   * @param {Tab} tab

Same comment about adding jsdoc @return here.

Comment 12

[Benoit Chabod [:bigben]]

Attachment: 2nd patch - use ContentTask to load frame scripts in tests, r=pbro f=janx

Thanks for the review Patrick!
I've addressed most of your comments, but I believe a try/catch mechanism is still needed to call unregisterServiceWorker.
If I just do:

yield unregisterServiceWorker(tab);

The test won't pass, you're right, but it will log "Uncaught exception : etc" which is bad in my opinion.
It's safer to catch the exception and to describe what caused the test failure with precision.
What do you think?

Comment 13

[Benoit Chabod [:bigben]]

Attachment: Final patch - use ContentTask to load frame scripts in tests, r=pbro f=janx

Oops, I forgot to run ESLint on my patch.
Here's the final attachment version, changes with the 2nd patch are insignificant.

Comment 14

[Benoit Chabod [:bigben]]

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7f76d5af0837

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f76d5af0837