149032 2015-09-09 23:40:18 -0700 [GTK] Layout Test media/video-volume-slider.html is flaky 2015-09-21 01:00:35 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build PC Linux RESOLVED FIXED P2 Normal --- 145639 147933 149396 1 changseok changseok calvaris cgarcia commit-queue gustavo pnormand oldest_to_newest 1124854 0 changseok 2015-09-09 23:40:18 -0700 SSIA 1124855 1 260913 changseok 2015-09-09 23:49:28 -0700 Created attachment 260913 Patch 1124866 2 260913 calvaris 2015-09-10 01:41:38 -0700 Comment on attachment 260913 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. 1124871 3 260913 changseok 2015-09-10 02:53:08 -0700 Comment on attachment 260913 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? =) 1124872 4 calvaris 2015-09-10 03:07:54 -0700 (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. 1125546 5 changseok 2015-09-13 22:37:22 -0700 (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. 1125547 6 261103 changseok 2015-09-13 22:38:34 -0700 Created attachment 261103 Patch 1125553 7 261103 pnormand 2015-09-14 00:01:08 -0700 Comment on attachment 261103 Patch This type of change usually doesn't require a review 1125559 8 261103 commit-queue 2015-09-14 00:47:55 -0700 Comment on attachment 261103 Patch Clearing flags on attachment: 261103 Committed r189684: <http://trac.webkit.org/changeset/189684> 1125560 9 commit-queue 2015-09-14 00:47:59 -0700 All reviewed patches have been landed. Closing bug. 260913 2015-09-09 23:49:28 -0700 2015-09-13 22:38:28 -0700 Patch bug-149032-20150910154921.patch text/plain 2554 changseok U3VidmVyc2lvbiBSZXZpc2lvbjogMTg5NDY0CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFu Z2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggZjUwZGRjNGM0N2YwZjg0YzMwZTk4 MDhiM2M0ZjNkNGI0N2IxOTlmZC4uMjdkMDcwZTE4OTI4ZWU2ZWM4NmQzNTliOGJjODQxY2UzY2Vj MmM1ZSAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3Rz L0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDE1LTA5LTA5ICBDaGFuZ1Nlb2sgT2ggIDxj aGFuZ3Nlb2sub2hAY29sbGFib3JhLmNvbT4KKworICAgICAgICBbR1RLXSBMYXlvdXQgVGVzdCBt ZWRpYS92aWRlby12b2x1bWUtc2xpZGVyLmh0bWwgaXMgZmxha3kKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0OTAzMgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZXJlIGlzIGEgcmFjZSBpc3N1ZSBiZXR3 ZWVuIG1vdmluZyBtb3VzZSBldmVudCBhbmQgdm9sdW1lY2hhZ2UuIFNvIHZpZGVvLXZvbHVtZS1z bGlkZXIuaHRtbAorICAgICAgICBzb21ldGltcyBwYXNzZXMgYW5kIHNvbWV0aW1lcyBub3QuIFdl IGNhbiBmaXggdGhpcyBieSBtYWtpbmcgdm9sdW1lY2hhbmdlIGV2ZW50IGFsd2F5cworICAgICAg ICBoYXBwZW4gYWZ0ZXIgYSBtb3VzZW1vdmUgZXZlbnQgYnkgdXNpbmcgc2V0VGltZW91dC4KKwor ICAgICAgICAqIG1lZGlhL3ZpZGVvLXZvbHVtZS1zbGlkZXIuaHRtbDoKKyAgICAgICAgKiBwbGF0 Zm9ybS9ndGsvVGVzdEV4cGVjdGF0aW9uczogdmlkZW8tdm9sdW1lLXNsaWRlci5odG1sIHJlbW92 ZWQuCisKIDIwMTUtMDktMDYgIEd5dXlvdW5nIEtpbSAgPGd5dXlvdW5nLmtpbUB3ZWJraXQub3Jn PgogCiAgICAgICAgIFVucmV2aWV3ZWQsIEVGTCBnYXJkZW5pbmcuIE1hbnkgdGVzdHMgaGF2ZSBi ZWVuIGZhaWxlZCBzaW5jZSByMTg4NjkyLgpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvbWVkaWEv dmlkZW8tdm9sdW1lLXNsaWRlci5odG1sIGIvTGF5b3V0VGVzdHMvbWVkaWEvdmlkZW8tdm9sdW1l LXNsaWRlci5odG1sCmluZGV4IDgzNWNhOWVmNzM1MjczYmNkYmM0ZTgwYjkxZDZkNDJhYmUwODky ZWMuLmViNDE1YmY5NWY1N2I5Mzg5ZDYzYTc5Y2Q4MDhiNDY5ZGNjMWI4N2UgMTAwNjQ0Ci0tLSBh L0xheW91dFRlc3RzL21lZGlhL3ZpZGVvLXZvbHVtZS1zbGlkZXIuaHRtbAorKysgYi9MYXlvdXRU ZXN0cy9tZWRpYS92aWRlby12b2x1bWUtc2xpZGVyLmh0bWwKQEAgLTE5LDggKzE5LDYgQEAKIAog ICAgICAgICBmdW5jdGlvbiB0ZXN0KCkKICAgICAgICAgewotICAgICAgICAgICAgdmlkZW8udm9s dW1lID0gMC43OwotCiAgICAgICAgICAgICBpZiAod2luZG93LmV2ZW50U2VuZGVyKSB7CiAgICAg ICAgICAgICAgICAgdmFyIG11dGVDb29yZHM7CiAgICAgICAgICAgICAgICAgdHJ5IHsKQEAgLTM2 LDYgKzM0LDggQEAKICAgICAgICAgICAgICAgICAgaWYgKHdpbmRvdy50ZXN0UnVubmVyKQogICAg ICAgICAgICAgICAgICAgICAgdGVzdFJ1bm5lci5ub3RpZnlEb25lKCk7CiAgICAgICAgICAgICB9 KTsKKworICAgICAgICAgICAgc2V0VGltZW91dChmdW5jdGlvbigpIHsgdmlkZW8udm9sdW1lID0g MC43OyB9LCAwKTsKICAgICAgICAgfQogICAgIDwvc2NyaXB0PgogPC9oZWFkPgpkaWZmIC0tZ2l0 IGEvTGF5b3V0VGVzdHMvcGxhdGZvcm0vZ3RrL1Rlc3RFeHBlY3RhdGlvbnMgYi9MYXlvdXRUZXN0 cy9wbGF0Zm9ybS9ndGsvVGVzdEV4cGVjdGF0aW9ucwppbmRleCA1NjI3YWFiOTgwYjExYWE0YzEy NzU1ODk3NzhiNjE1NzQ2MTcxZjZhLi43NWIyNDE4YzA2YjBlY2EzY2YwNWZkYzEwOGFhMjdmZTBk NmEwMTJiIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9wbGF0Zm9ybS9ndGsvVGVzdEV4cGVjdGF0 aW9ucworKysgYi9MYXlvdXRUZXN0cy9wbGF0Zm9ybS9ndGsvVGVzdEV4cGVjdGF0aW9ucwpAQCAt NTQ5LDcgKzU0OSw2IEBAIHdlYmtpdC5vcmcvYi8xMTgxMjYgY3NzMy9mbGV4Ym94L2ltYWdlLXBl cmNlbnQtbWF4LWhlaWdodC5odG1sIFsgSW1hZ2VPbmx5RmFpbHVyCiB3ZWJraXQub3JnL2IvMTQy MjY4IGVkaXRpbmcvcGFzdGVib2FyZC9jb3B5LXBhc3RlLWF0dGFjaG1lbnQuaHRtbCBbIEZhaWx1 cmUgXQogd2Via2l0Lm9yZy9iLzE0MjI2OSBmYXN0L2F0dGFjaG1lbnQvYXR0YWNobWVudC1kb20u aHRtbCBbIEZhaWx1cmUgXQogd2Via2l0Lm9yZy9iLzE0MjI3MCBmYXN0L2NhbnZhcy9jYW52YXMt ZWxsaXBzZS16ZXJvLWxpbmV0by5odG1sIFsgRmFpbHVyZSBdCi13ZWJraXQub3JnL2IvMTQyMTQy IG1lZGlhL3ZpZGVvLXZvbHVtZS1zbGlkZXIuaHRtbCBbIEZhaWx1cmUgXQogCiB3ZWJraXQub3Jn L2IvMTI3MTA5IG1lZGlhL3ZpZGVvLW9iamVjdC1maXQuaHRtbCBbIEltYWdlT25seUZhaWx1cmUg XQogd2Via2l0Lm9yZy9iLzE0MjI4MyBmYXN0L3JlZ2lvbnMvb3ZlcmZsb3cvb3ZlcmZsb3ctZmly c3QtYW5kLWxhc3QtcmVnaW9ucy5odG1sIFsgSW1hZ2VPbmx5RmFpbHVyZSBdCg== 261103 2015-09-13 22:38:34 -0700 2015-09-14 00:47:55 -0700 Patch bug-149032-20150914143824.patch text/plain 1513 changseok U3VidmVyc2lvbiBSZXZpc2lvbjogMTg5NTY2CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFu Z2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggZjQyNTQ0MjI0MmYwZWQ3ZDY2MzRj MDFiZjNlZDBhMmM5YWJkYmI1Yy4uMDQ5OTM0OTVmMjUwZjJlNjZiNmZiNWQ1MjkxZTRiMzRlOWQ1 NzRkMiAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3Rz L0NoYW5nZUxvZwpAQCAtMSwzICsxLDEyIEBACisyMDE1LTA5LTEzICBDaGFuZ1Nlb2sgT2ggIDxj aGFuZ3Nlb2sub2hAY29sbGFib3JhLmNvbT4KKworICAgICAgICBbR1RLXSBMYXlvdXQgVGVzdCBt ZWRpYS92aWRlby12b2x1bWUtc2xpZGVyLmh0bWwgaXMgZmxha3kKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0OTAzMgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogcGxhdGZvcm0vZ3RrL1Rlc3RFeHBlY3Rh dGlvbnM6IG1lZGlhL3ZpZGVvLXZvbHVtZS1zbGlkZXIuaHRtbCBpcyByZW1vdmVkLgorCiAyMDE1 LTA5LTEwICBDaGFuZ1Nlb2sgT2ggIDxjaGFuZ3Nlb2sub2hAY29sbGFib3JhLmNvbT4KIAogICAg ICAgICBbR1RLXSBWb2x1bWUgYmFyIGlzIGJyb2tlbgpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMv cGxhdGZvcm0vZ3RrL1Rlc3RFeHBlY3RhdGlvbnMgYi9MYXlvdXRUZXN0cy9wbGF0Zm9ybS9ndGsv VGVzdEV4cGVjdGF0aW9ucwppbmRleCBiYWI3ZWY0MTIyNTZmNjczMTM0ZjYyNzEzYmMzY2U4YmQz MjRlM2VmLi4xNzM0YWQ2NWEyNTZiNmVjZWMxYmY4ZDUyZGMyNTZiNDkyZDBkMWNjIDEwMDY0NAot LS0gYS9MYXlvdXRUZXN0cy9wbGF0Zm9ybS9ndGsvVGVzdEV4cGVjdGF0aW9ucworKysgYi9MYXlv dXRUZXN0cy9wbGF0Zm9ybS9ndGsvVGVzdEV4cGVjdGF0aW9ucwpAQCAtNTUyLDcgKzU1Miw2IEBA IHdlYmtpdC5vcmcvYi8xMTgxMjYgY3NzMy9mbGV4Ym94L2ltYWdlLXBlcmNlbnQtbWF4LWhlaWdo dC5odG1sIFsgSW1hZ2VPbmx5RmFpbHVyCiB3ZWJraXQub3JnL2IvMTQyMjY4IGVkaXRpbmcvcGFz dGVib2FyZC9jb3B5LXBhc3RlLWF0dGFjaG1lbnQuaHRtbCBbIEZhaWx1cmUgXQogd2Via2l0Lm9y Zy9iLzE0MjI2OSBmYXN0L2F0dGFjaG1lbnQvYXR0YWNobWVudC1kb20uaHRtbCBbIEZhaWx1cmUg XQogd2Via2l0Lm9yZy9iLzE0MjI3MCBmYXN0L2NhbnZhcy9jYW52YXMtZWxsaXBzZS16ZXJvLWxp bmV0by5odG1sIFsgRmFpbHVyZSBdCi13ZWJraXQub3JnL2IvMTQyMTQyIG1lZGlhL3ZpZGVvLXZv bHVtZS1zbGlkZXIuaHRtbCBbIEZhaWx1cmUgXQogCiB3ZWJraXQub3JnL2IvMTI3MTA5IG1lZGlh L3ZpZGVvLW9iamVjdC1maXQuaHRtbCBbIEltYWdlT25seUZhaWx1cmUgXQogd2Via2l0Lm9yZy9i LzE0MjI4MyBmYXN0L3JlZ2lvbnMvb3ZlcmZsb3cvb3ZlcmZsb3ctZmlyc3QtYW5kLWxhc3QtcmVn aW9ucy5odG1sIFsgSW1hZ2VPbmx5RmFpbHVyZSBdCg==