|Summary:||Add a way to reset the eventTrigger in ControlsTest in LayoutTests/media/controls/controls-test-helpers.js|
|Product:||WebKit||Reporter:||Ada Chan <adachan>|
|Component:||Tools / Tests||Assignee:||Ada Chan <adachan>|
|Severity:||Normal||CC:||dino, eric.carlson, lforschler|
|Version:||WebKit Nightly Build|
Description Ada Chan 2016-02-23 16:44:26 PST
Add a way to reset the eventTrigger in ControlsTest in LayoutTests/media/controls/controls-test-helpers.js We can introduce a method in ControlsTest called listenFor(eventTrigger), that sets the ControlsTest's eventTrigger to the new value and set the media element to be a listener to that event. This makes it easier to write controls tests that need to listen to a new event after changing some state in the media element.
Comment 2 Darin Adler 2016-02-23 21:54:43 PST
Comment on attachment 272065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272065&action=review > LayoutTests/media/controls/controls-test-helpers.js:31 > + this.eventTrigger = eventTrigger; > + if (!this.media) > + return null; > + > + this.media.addEventListener(this.eventTrigger, this, false); > + return this; Don’t we need to removeEventListener on the old value of this.eventTrigger?
Comment 3 Ada Chan 2016-02-24 10:17:17 PST
(In reply to comment #2) > Comment on attachment 272065 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272065&action=review > > > LayoutTests/media/controls/controls-test-helpers.js:31 > > + this.eventTrigger = eventTrigger; > > + if (!this.media) > > + return null; > > + > > + this.media.addEventListener(this.eventTrigger, this, false); > > + return this; > > Don’t we need to removeEventListener on the old value of this.eventTrigger? That's a good question. I think you are right. Thinking about this a little more, instead of introducing a listenFor() function, maybe I should just implement the setter of eventTrigger to do the right thing. I'll talk more with Dean about this.
Comment 5 Ada Chan 2016-03-29 17:04:35 PDT
Uploaded a patch that renames the method and also call removeEventListener on the old event trigger.
Comment 6 Dean Jackson 2016-03-29 17:34:27 PDT
Comment on attachment 275153 [details] Patch Very nice! One of the disadvantages of this chaining().style().of().coding() that I used here is that it makes it harder to use getters and setters. In some ways this method could have been tester.eventTrigger = foo, but I think what you did is good.