Bug 161451

<rdar://problem/28100779>

Created attachment 287542 [details] Patch

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));

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.

Created attachment 287551 [details] Patch

Comment on attachment 287551 [details] Patch Regressions: Unexpected text-only failures (1) inspector/unit-tests/object.html [ Failure ]

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

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

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

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

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

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

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.

Created attachment 287575 [details] Patch

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.

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.

Created attachment 287607 [details] Patch

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.

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.

Comment on attachment 287607 [details] Patch Clearing flags on attachment: 287607 Committed r205319: <http://trac.webkit.org/changeset/205319>

All reviewed patches have been landed. Closing bug.