851381 - Make the Scratchpad evaluate asynchronously

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[Brandon Benvie [:benvie]]

In order for the Scratchpad work over the debugger protocol it will have to be asynchronously. I am breaking this part of the task into its own bug as it is standalone and touches a number of things in the scratchpad.

Comment 1

[Brandon Benvie [:benvie]]

Attachment: make the scratchpad evaluate code asynchronously

initial stab

Comment 2

[Brandon Benvie [:benvie]]

Attachment: Further refinemen of async scratchpad and fixes for most tests

Most of the remaining test failures indicate some few overarching issue that is causing the test runner to become out of sync, causing scratchpad windows to interfere with each other.

bug690552_display_outputs_errors.js
message display output - Got "Hello World!", expected "Hello World!"
Test timed out

bug740948_reload_and_run.js
Test timed out

bug756681_display_non_error_exceptions.js
uncaught exception - ReferenceError: scratchpad is not defined at chrome://mochitests/content/browser/browser/devtools/scratchpad/test/browser_scratchpad_bug756681_display_non_error_exceptions.js:122
Test timed out

bug_644413_modeline.js
 - Got 2, expected 1

bug_661762_wrong_window_focus.js
Test timed out
Found a devtools:scratchpad after previous test timed out

bug_679467_falsy.js
Test timed out

contexts.js
content.foobarBug636725 has been set - Got undefined, expected aloha
no window.foobarBug636725
window.foobarBug636725 has been set - Got aloha, expected aloha2
Test timed out

execute_print.js
Test timed out
leaked window property: foobarBug636725

inspect.js
Test timed out

tab_switch.js
content.foosbug653108 has been set - Got undefined, expected aloha
no window.foosbug653108
Test timed out
leaked window property: foosbug653108

Comment 3

[Brandon Benvie [:benvie]]

Attachment: change promises to only use resolve, iterate on tests

I realized that changing from the original pattern where the result of execution of `[code, error, result]` to using `resolve([code, result])` and `reject(error)` was causing some subtle issues with the tests. This patch changes to using `resolve([code, error, result])` and updates the patches to work with that pattern.

This patch also changes more of the tests to use the `runTestsAsync` where essentially uses a loop of deferreds to execute tests described in a more declarative manner using object literals. This helps prevent to rightward drift of nested promises.

Comment 4

[Brandon Benvie [:benvie]]

Attachment: fix off-by-one bug and get failing tests down to 4 timeouts

This patch fixes a bug where content and chrome scratchpad contexts were switched; this bug was responsible for breaking numerous tests. This patch also includes further test fixes and refinement and the remaining failures are all due to timeouts:

browser_scratchpad_bug_661762_wrong_window_focus.js | Test timed out
browser_scratchpad_bug_661762_wrong_window_focus.js | Found a devtools:scratchpad after previous test timed out
browser_scratchpad_tab_switch.js | Test timed out
browser_scratchpad_contexts.js | Test timed out
browser_scratchpad_execute_print.js | Test timed out

In this patch those four tests are disabled temporarily to allow for faster debugging of them individually. I believe the timeouts are due to errors being caught and, due to the async nature, not being identified by the test harness (as from the eventual timeout).

Comment 5

[Brandon Benvie [:benvie]]

Attachment: fix handling of promises that caused errors and make all tests pass

Comment 6

[Brandon Benvie [:benvie]]

Attachment: add runAsyncCallbackTests to some tests to reduce callbackitis

Comment 7

[Brandon Benvie [:benvie]]

(In reply to Brandon Benvie [:bbenvie] from comment #3)
> I realized that changing from the original pattern where the result of
> execution of `[code, error, result]` to using `resolve([code, result])` and
> `reject(error)` was causing some subtle issues with the tests. This patch
> changes to using `resolve([code, error, result])`

My reasoning for this is that the rejection state for executing code in the scratchpad should not come from user code resulting in errors, but from failures at the debugger client level.

Comment 8

[Brandon Benvie [:benvie]]

Attachment: change to using arrow functions in functions impacted by this bug

Comment 9

[Brandon Benvie [:benvie]]

Attachment: combined async scratchpad patch

Combines the previous 7 patches. Contains changes to make the scratchpad asynchronous as well as fixes for all the tests.

Comment 10

[Brandon Benvie [:benvie]]

Attachment: remove accidentally included bit from last patch

Same as last patch minus an accidentally included unrelated patch.

Comment 11

[Heather Arthur [:harth]]

Comment on attachment 732002 [details] [diff] [review]
remove accidentally included bit from last patch

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

This looks great. Just a few comment requests:

::: browser/devtools/scratchpad/scratchpad.js
@@ +366,5 @@
>    evalForContext: function SP_evaluateForContext(aString)
>    {
> +    let deferred = Promise.defer();
> +
> +    setTimeout(() => {

why the setTimeout? Wondering, and also comment about it.

@@ +386,5 @@
>  
>    /**
>     * Execute the selected text (if any) or the entire editor content in the
>     * current context.
>     * @return mixed

mixed -> Promise

@@ +402,2 @@
>     */
>    run: function SP_run()

This didn't have a @return comment before, but it'd be nice to add one now. Same for inspect, display, reloadAndRun.

::: browser/devtools/scratchpad/test/browser_scratchpad_bug740948_reload_and_run.js
@@ +68,4 @@
>  
>    ok(browser.contentWindow.document.body.innerHTML !== "Modified text",
>        "Before reloading, HTML is intact.");
> +  

Looks like you're adding some whitespace here.

::: browser/devtools/scratchpad/test/head.js
@@ +119,5 @@
>  }
>  
>  registerCleanupFunction(cleanup);
> +
> +function runAsyncTests(scratchpad, tests)

Add a comment about what this is doing, and descriptions of the properties of an object in 'tests'.

Comment 12

[Brandon Benvie [:benvie]]

Thanks! I'll address all your comments.

The setTimeout is there as a temporary way to make it asynchronous. It will be replaced by code that evals over the DebuggerClient and I wanted to assure it worked correctly under real asynchrony. I suppose it's not necessary, since the promise will still resolve as desired, but I wanted to make sure there wasn't some accidental subtle dependency on the promise being resolved synchronously (I've heard this may be a problem in some other parts of devtools/addon sdk). I'll note the reason in a comment.

Comment 13

[Brandon Benvie [:benvie]]

Attachment: add comments and minor styling fixes

The patch is almost entirely about adding comments, with a couple other small changes.

* Note explaining why setTimeout is used
* Add @return type to execute, run, inspect, display, and reloadAndRun
* Remove added line in reloadAndRun test
* Add comments detailing @params for runAsyncTests and runAsyncCallbackTests
* Rename params to use "aParam" naming convention for runAsyncTests and runAsyncCallbackTests
* Move cleanup to the end of head.js

Comment 14

[Brandon Benvie [:benvie]]

Attachment: make the scratchpad evaluate code asynchronously

This patch simply combines the last two patches that have been r+'d and is the final resolution for this bug.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 16

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/b467a96d1e3e