[GTK] Layout Test media/video-volume-slider.html is flaky

• Component: Tools / Tests
• Status: RESOLVED FIXED

[ChangSeok Oh]

SSIA

[ChangSeok Oh]

Created attachment 260913 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 260913 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260913&action=review

Before doing any changes and any landing, I would way for bug 147933 as it will probably change the way events are handled. It might happen that bug goes away by itself without complicating the test. Btw, I checked and pixel tests are passing.

Summing up, apart from the typos and moving around the timeout (not compulsory), LGTM.

> LayoutTests/ChangeLog:9
> +        sometims passes and sometimes not. We can fix this by making volumechange event always

typo with sometims

> LayoutTests/media/video-volume-slider.html:38
> +            setTimeout(function() { video.volume = 0.7; }, 0);

I guess you can move the timeout to the same place it was setting the volume before, because the task should be either way delayed.

[ChangSeok Oh]

Comment on attachment 260913 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260913&action=review

> Before doing any changes and any landing, I would way for bug 147933 as it will probably change the way events are handled. It might happen that bug goes away by itself without complicating the test. Btw, I checked and pixel tests are passing.
Good to know. Thanks for the pointer!

>> LayoutTests/ChangeLog:9
>> +        sometims passes and sometimes not. We can fix this by making volumechange event always
> 
> typo with sometims

Oops.

>> LayoutTests/media/video-volume-slider.html:38
>> +            setTimeout(function() { video.volume = 0.7; }, 0);
> 
> I guess you can move the timeout to the same place it was setting the volume before, because the task should be either way delayed.

Isn't it more natural to understand that (triggering mouse event for volume slider) -> (triggering volumechange event) rather than the inverse? And that order is what we expect to happen from the events. How does it sound? =)

[Xabier Rodríguez Calvar]

(In reply to comment #3)
> >> LayoutTests/media/video-volume-slider.html:38
> >> +            setTimeout(function() { video.volume = 0.7; }, 0);
> > 
> > I guess you can move the timeout to the same place it was setting the volume before, because the task should be either way delayed.
> 
> Isn't it more natural to understand that (triggering mouse event for volume
> slider) -> (triggering volumechange event) rather than the inverse? And that
> order is what we expect to happen from the events. How does it sound? =)

Yeah, it sounds better. Then my question is if we need the timeout, because I guess the point of setting the timeout is delaying setting the volume, but I guess it should work if we just set the volume after setting the volumechange handler.

[ChangSeok Oh]

(In reply to comment #4)
> (In reply to comment #3)
> > >> LayoutTests/media/video-volume-slider.html:38
> > >> +            setTimeout(function() { video.volume = 0.7; }, 0);
> > > 
> > > I guess you can move the timeout to the same place it was setting the volume before, because the task should be either way delayed.
> > 
> > Isn't it more natural to understand that (triggering mouse event for volume
> > slider) -> (triggering volumechange event) rather than the inverse? And that
> > order is what we expect to happen from the events. How does it sound? =)
> 
> Yeah, it sounds better. Then my question is if we need the timeout, because
> I guess the point of setting the timeout is delaying setting the volume, but
> I guess it should work if we just set the volume after setting the
> volumechange handler.

It is odd. After rebasing source, the race issue seems gone. :/
Just removing the test from TestExpectations is enough now.

[ChangSeok Oh]

Created attachment 261103 [details]
Patch

[Philippe Normand]

Comment on attachment 261103 [details]
Patch

This type of change usually doesn't require a review

[WebKit Commit Bot]

Comment on attachment 261103 [details]
Patch

Clearing flags on attachment: 261103

Committed r189684: <http://trac.webkit.org/changeset/189684>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.