If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Developer Tools: about:debugging
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: janx, Assigned: bigben, Mentored)

Tracking

unspecified
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
Mentor: janx@linux.com
Priority: -- → P3
(Reporter)

Comment 1

2 years ago
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?
Flags: needinfo?(pbrosset)
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
Flags: needinfo?(pbrosset)
(Reporter)

Comment 3

a year ago
Benoit, would you be interested in investigating this bug?
Flags: needinfo?(bchabod)
(Assignee)

Comment 4

a year ago
Sure! I will take a look at Patrick's examples and see what I can do for about:debugging.
Flags: needinfo?(bchabod)
(Assignee)

Updated

a year ago
Assignee: nobody → bchabod
(Assignee)

Comment 5

a year ago
Created attachment 8754742 [details] [diff] [review]
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.
Attachment #8754742 - Flags: review?(janx)
(Reporter)

Comment 6

a year ago
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?
Attachment #8754742 - Flags: review?(pbrosset)
Attachment #8754742 - Flags: review?(janx)
Attachment #8754742 - Flags: feedback+
(Assignee)

Comment 7

a year ago
Created attachment 8754791 [details] [diff] [review]
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 :)
Attachment #8754791 - Flags: review?(pbrosset)
Attachment #8754791 - Flags: feedback+
(Assignee)

Updated

a year ago
Attachment #8754742 - Attachment is obsolete: true
Attachment #8754742 - Flags: review?(pbrosset)
(Reporter)

Comment 8

a year ago
(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.
(Assignee)

Comment 9

a year ago
(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
(Assignee)

Comment 10

a year ago
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 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.
Attachment #8754791 - Flags: review?(pbrosset)
(Assignee)

Comment 12

a year ago
Created attachment 8755480 [details] [diff] [review]
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?
Attachment #8755480 - Flags: review?(pbrosset)
Attachment #8755480 - Flags: feedback+
(Assignee)

Updated

a year ago
Attachment #8754791 - Attachment is obsolete: true
Attachment #8755480 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 13

a year ago
Created attachment 8755755 [details] [diff] [review]
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.
Attachment #8755755 - Flags: review+
Attachment #8755755 - Flags: feedback+
(Assignee)

Updated

a year ago
Attachment #8755480 - Attachment is obsolete: true
(Assignee)

Comment 14

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecc4179546f4
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 15

a year ago
https://hg.mozilla.org/integration/fx-team/rev/7f76d5af0837
Keywords: checkin-needed

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7f76d5af0837
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.