Closed Bug 851381 Opened 8 years ago Closed 8 years ago

Make the Scratchpad evaluate asynchronously

Categories

(DevTools Graveyard :: Scratchpad, defect)

21 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: bbenvie, Assigned: bbenvie)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 10 obsolete files)

41.88 KB, patch
Details | Diff | Splinter Review
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.
initial stab
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
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.
Status: NEW → ASSIGNED
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).
(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.
Attached patch combined async scratchpad patch (obsolete) — Splinter Review
Combines the previous 7 patches. Contains changes to make the scratchpad asynchronous as well as fixes for all the tests.
Attachment #725206 - Attachment is obsolete: true
Attachment #729182 - Attachment is obsolete: true
Attachment #729898 - Attachment is obsolete: true
Attachment #730285 - Attachment is obsolete: true
Attachment #730374 - Attachment is obsolete: true
Attachment #730424 - Attachment is obsolete: true
Attachment #730933 - Attachment is obsolete: true
Attachment #730975 - Flags: review?(fayearthur)
Same as last patch minus an accidentally included unrelated patch.
Attachment #730975 - Attachment is obsolete: true
Attachment #730975 - Flags: review?(fayearthur)
Attachment #732002 - Flags: review?(fayearthur)
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'.
Attachment #732002 - Flags: review?(fayearthur) → review+
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.
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
Attachment #732159 - Flags: review?(fayearthur)
Attachment #732159 - Flags: review?(fayearthur) → review+
This patch simply combines the last two patches that have been r+'d and is the final resolution for this bug.
Attachment #732002 - Attachment is obsolete: true
Attachment #732159 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/b467a96d1e3e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.