Bug 154616 - Add a way to reset the eventTrigger in ControlsTest in LayoutTests/media/controls/controls-test-helpers.js
Summary: Add a way to reset the eventTrigger in ControlsTest in LayoutTests/media/cont...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ada Chan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-23 16:44 PST by Ada Chan
Modified: 2016-04-01 11:15 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.39 KB, patch)
2016-02-23 16:48 PST, Ada Chan
no flags Details | Formatted Diff | Diff
Patch (1.54 KB, patch)
2016-03-29 16:57 PDT, Ada Chan
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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