Bug 161451 – Web Inspector: add Object.awaitEvent which is like singleFireEventListener but returns a promise

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

This will make a bunch of tests easier to read.

[Radar WebKit Bug Importer]

<rdar://problem/28100779>

[Devin Rousso]

Created attachment 287542 [details]
Patch

[Matt Baker]

Comment on attachment 287542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287542&action=review

> Source/WebInspectorUI/UserInterface/Base/Object.js:105
> +                resolve(...arguments);

This can be written with an arrow:
this.singleFireEventListener(eventType, () => resolve(...arguments));

[Blaze Burg]

Comment on attachment 287542 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287542&action=review

>> Source/WebInspectorUI/UserInterface/Base/Object.js:105
>> +                resolve(...arguments);
> 
> This can be written with an arrow:
> this.singleFireEventListener(eventType, () => resolve(...arguments));

Resolve can only be passed one argument, the others are ignored. Please add a named argument 'event' and pass it.

[Devin Rousso]

Created attachment 287551 [details]
Patch

[Blaze Burg]

Comment on attachment 287551 [details]
Patch

Regressions: Unexpected text-only failures (1)
  inspector/unit-tests/object.html [ Failure ]

[Build Bot]

Comment on attachment 287551 [details]
Patch

Attachment 287551 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1982987

New failing tests:
inspector/unit-tests/object.html

[Build Bot]

Created attachment 287556 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287551 [details]
Patch

Attachment 287551 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1982977

New failing tests:
inspector/unit-tests/object.html

[Build Bot]

Created attachment 287558 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 287551 [details]
Patch

Attachment 287551 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1982990

New failing tests:
inspector/unit-tests/object.html

[Build Bot]

Created attachment 287562 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Comment on attachment 287551 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287551&action=review

> LayoutTests/inspector/unit-tests/object.html:38
> +            object.awaitEvent(eventName).then(() => dispatchCount++);

In a Synchronous test suite this will not work, because any Promise's then microtask is guaranteed to run asynchronously. I'm not sure how you had gotten this to work locally. Either way, switching to an Async test suite (and updating all the tests to appropriately resolve/reject may do better.

Maybe you could even have a synchronous suite runTestCasesAndNOTFinish() then run async tests right after.

[Devin Rousso]

Created attachment 287575 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 287575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287575&action=review

r- only because I want to see a new test! Sorry for the back and forth on this =/

> LayoutTests/inspector/unit-tests/object.html:51
> +        name: "Single Fire Promise",
> +        description: "awaitEvent should only trigger once",
> +        test(resolve, reject) {
> +            const eventName = "test";
> +            let dispatchCount = 0;
> +
> +            let object = new WebInspector.Object;
> +
> +            object.awaitEvent(eventName).then(() => {
> +                dispatchCount++;
> +
> +                InspectorTest.log("Dispatch count: " + dispatchCount);
> +            });
> +
> +            object.dispatchEventToListeners(eventName);
> +            object.dispatchEventToListeners(eventName);
> +
> +            resolve();
>          }
>      });

Err, resolve doesn't this call resolve before the awaitEvent's then block fires?

There should probably be a separate event that ends up resolving.

For example: (Assumes dispatchEvent is some microtask that happens before the Promise.resolve's)

    object.dispatchEventToListeners(eventName);
    object.dispatchEventToListeners(eventName);
    Promise.resolve().then(resolve, reject);

Or: (assumes the timeout fires after all event listeners have fired)

    object.dispatchEventToListeners(eventName);
    object.dispatchEventToListeners(eventName);
    setTimeout(() => { resolve() }, 10);

Or: (also best, this ensures the original is not dispatched more than once, and resolves the test after handling the second event)

    object.awaitEvent(eventName).then(() => {
        dispatchCount++;
        InspectorTest.expectThat(dispatchCount === 1, "Await event handler should be dispatched just once.");
        InspectorTest.log("Dispatch count: " + dispatchCount);
        object.awaitEvent(eventName).then(() => {
            resolve();
        });
    });

    object.dispatchEventToListeners(eventName);
    object.dispatchEventToListeners(eventName);

I prefer the last version, since it verifies both that the event is actually dispatched twice and that the original awaitEvent was dispatched just once.

[Joseph Pecoraro]

Comment on attachment 287575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287575&action=review

> LayoutTests/inspector/unit-tests/object.html:32
> +        name: "Single Fire Promise",

And for Unit tests I've been giving the name the name of the method they test. This could be WebInspector.Object.prototype.awaitEvent. The one above could be dispatchEventToListeners.

[Devin Rousso]

Created attachment 287607 [details]
Patch

[Blaze Burg]

Comment on attachment 287607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287607&action=review

r=me

> LayoutTests/inspector/unit-tests/object.html:48
> +                    resolve();

You should add the dispatchCount === 1 assertion down here too, to check that the listener didn't fire a second time even though its a single fire event listener.

[Joseph Pecoraro]

Comment on attachment 287607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=287607&action=review

>> LayoutTests/inspector/unit-tests/object.html:48
>> +                    resolve();
> 
> You should add the dispatchCount === 1 assertion down here too, to check that the listener didn't fire a second time even though its a single fire event listener.

Would the assertion down here matter? The only place dispatchCount changes is up above. If it changed again it would fail up above. Shrug.

[WebKit Commit Bot]

Comment on attachment 287607 [details]
Patch

Clearing flags on attachment: 287607

Committed r205319: <http://trac.webkit.org/changeset/205319>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.