Bug 154616 – Add a way to reset the eventTrigger in ControlsTest in LayoutTests/media/controls/controls-test-helpers.js

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ada Chan]

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.

[Ada Chan]

Created attachment 272065 [details]
Patch

[Darin Adler]

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?

[Ada Chan]

(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.

[Ada Chan]

Created attachment 275153 [details]
Patch

[Ada Chan]

Uploaded a patch that renames the method and also call removeEventListener on the old event trigger.

[Dean Jackson]

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.

[Ada Chan]

Committed:
http://trac.webkit.org/changeset/198949