Bug 149032

Created attachment 260913 [details] Patch

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 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? =)

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

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

Created attachment 261103 [details] Patch

Comment on attachment 261103 [details] Patch This type of change usually doesn't require a review

Comment on attachment 261103 [details] Patch Clearing flags on attachment: 261103 Committed r189684: <http://trac.webkit.org/changeset/189684>

All reviewed patches have been landed. Closing bug.