Bug 149032

Summary: [GTK] Layout Test media/video-volume-slider.html is flaky
Product: WebKit Reporter: ChangSeok Oh <changseok>
Component: Tools / TestsAssignee: ChangSeok Oh <changseok>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, cgarcia, commit-queue, gns, pnormand
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Bug Depends on: 145639, 147933    
Bug Blocks: 149396    
Attachments:
Description Flags
Patch
none
Patch none

Description ChangSeok Oh 2015-09-09 23:40:18 PDT
SSIA
Comment 1 ChangSeok Oh 2015-09-09 23:49:28 PDT
Created attachment 260913 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2015-09-10 01:41:38 PDT
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.
Comment 3 ChangSeok Oh 2015-09-10 02:53:08 PDT
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? =)
Comment 4 Xabier Rodríguez Calvar 2015-09-10 03:07:54 PDT
(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.
Comment 5 ChangSeok Oh 2015-09-13 22:37:22 PDT
(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.
Comment 6 ChangSeok Oh 2015-09-13 22:38:34 PDT
Created attachment 261103 [details]
Patch
Comment 7 Philippe Normand 2015-09-14 00:01:08 PDT
Comment on attachment 261103 [details]
Patch

This type of change usually doesn't require a review
Comment 8 WebKit Commit Bot 2015-09-14 00:47:55 PDT
Comment on attachment 261103 [details]
Patch

Clearing flags on attachment: 261103

Committed r189684: <http://trac.webkit.org/changeset/189684>
Comment 9 WebKit Commit Bot 2015-09-14 00:47:59 PDT
All reviewed patches have been landed.  Closing bug.