Bug 154616

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 / TestsAssignee: Ada Chan <adachan>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, eric.carlson, lforschler
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch dino: review+

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 1 Ada Chan 2016-02-23 16:48:36 PST
Created attachment 272065 [details]
Patch
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 4 Ada Chan 2016-03-29 16:57:42 PDT
Created attachment 275153 [details]
Patch
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.
Comment 7 Ada Chan 2016-04-01 11:15:30 PDT
Committed:
http://trac.webkit.org/changeset/198949