160707 2016-08-09 13:41:59 -0700 [macOS Sierra] Fix flaky test: media/controls/picture-in-picture.html 2016-08-26 13:20:10 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 adachan adachan lforschler oldest_to_newest 1218824 0 adachan 2016-08-09 13:41:59 -0700 media/controls/picture-in-picture.html is flaky on macOS Sierra. @@ -21,13 +21,13 @@ Test for the pip placeholder visibility in pip mode PASS: Should be in pip mode -PASS: Inline placeholder should be visible at this point +FAIL: Inline placeholder should be visible at this point Expected to not contain "hidden". Actual: "hidden picture-in-picture" Test for the pip placeholder visibility in pip mode without a 'controls' attribute PASS: Should still be in pip mode PASS: No controls attribute -PASS: Inline placeholder should still be visible +FAIL: Inline placeholder should still be visible Expected to not contain "hidden". Actual: "hidden picture-in-picture" Testing finished.  1218826 1 adachan 2016-08-09 13:42:21 -0700 <rdar://problem/27574303> 1218832 2 285672 adachan 2016-08-09 13:52:47 -0700 Created attachment 285672 Patch 1218849 3 285672 eric.carlson 2016-08-09 14:34:22 -0700 Comment on attachment 285672 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285672&action=review > LayoutTests/media/controls/picture-in-picture.html:75 > + if (stateForPlaceholder.className.indexOf("hidden") >= 0) { > + // Use 33 to match PlaceholderPollingDelay in mediaControlsApple.js. > + setTimeout(pollPIPPlaceholderVisibilityChange, 33); > + return; > + } Nit: in case something goes wrong I think it would be better to fail and log an error message after some maximum number of retrys, rather than trying until the test times out. 1218857 4 adachan 2016-08-09 14:54:29 -0700 (In reply to comment #3) > Comment on attachment 285672 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285672&action=review > > > LayoutTests/media/controls/picture-in-picture.html:75 > > + if (stateForPlaceholder.className.indexOf("hidden") >= 0) { > > + // Use 33 to match PlaceholderPollingDelay in mediaControlsApple.js. > > + setTimeout(pollPIPPlaceholderVisibilityChange, 33); > > + return; > > + } > > Nit: in case something goes wrong I think it would be better to fail and log > an error message after some maximum number of retrys, rather than trying > until the test times out. OK, I'll set a maximum number retrys to 10. Thanks for the review! 1218859 5 adachan 2016-08-09 14:57:11 -0700 Committed: https://trac.webkit.org/changeset/204304 1218961 6 285672 ap 2016-08-09 20:44:05 -0700 Comment on attachment 285672 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285672&action=review >>> LayoutTests/media/controls/picture-in-picture.html:75 >>> + } >> >> Nit: in case something goes wrong I think it would be better to fail and log an error message after some maximum number of retrys, rather than trying until the test times out. > > OK, I'll set a maximum number retrys to 10. > > Thanks for the review! 10 retries is just 330 ms. I suggest waiting ~10 seconds before failing. 1223653 7 adachan 2016-08-26 13:20:10 -0700 (In reply to comment #6) > Comment on attachment 285672 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=285672&action=review > > >>> LayoutTests/media/controls/picture-in-picture.html:75 > >>> + } > >> > >> Nit: in case something goes wrong I think it would be better to fail and log an error message after some maximum number of retrys, rather than trying until the test times out. > > > > OK, I'll set a maximum number retrys to 10. > > > > Thanks for the review! > > 10 retries is just 330 ms. I suggest waiting ~10 seconds before failing. The placeholder is not guaranteed to be visible right when the presentation mode changes but it should be visible basically very soon after. If it takes in the order of seconds to become visible, then that's a bug. 285672 2016-08-09 13:52:47 -0700 2016-08-09 14:34:22 -0700 Patch bug-160707-20160809135142.patch text/plain 3289 adachan U3VidmVyc2lvbiBSZXZpc2lvbjogMjA0Mjg0CmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFu Z2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggOGViYmFiZjIyMmJiNzZiY2UyZGNl MzkyZDFkYmViMDA4OWU4M2VkYy4uMDBjOTFjYzljN2UyOGJjMGQxMWIyZjVkZDE3NjJiMmQ5NGEx MWMzMyAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3Rz L0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDE2LTA4LTA5ICBBZGEgQ2hhbiAgPGFkYWNo YW5AYXBwbGUuY29tPgorCisgICAgICAgIFttYWNPUyBTaWVycmFdIEZpeCBmbGFreSB0ZXN0OiBt ZWRpYS9jb250cm9scy9waWN0dXJlLWluLXBpY3R1cmUuaHRtbAorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTYwNzA3CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyB0ZXN0IGJlY2FtZSBmbGFreSBhZnRl ciByMjAxNDc0IHdoZW4gd2Ugc3RhcnRlZCB0byBkZWxheSBzaG93aW5nCisgICAgICAgIHRoZSBp bmxpbmUgcGxhY2Vob2xkZXIgdW50aWwgd2UgYXJlIHN1cmUgdGhlIHZpZGVvIGxheWVyIGhhcyBi ZWVuIG1vdmVkCisgICAgICAgIGludG8gdGhlIHZpZGVvIGZ1bGxzY3JlZW4gbGF5ZXIuIFRoaXMg bWVhbnMgd2UgY2FuJ3QgZ3VhcmFudGVlIHRoYXQgdGhlCisgICAgICAgIHBsYWNlaG9sZGVyIGlz IHZpc2libGUgcmlnaHQgYXdheSBhZnRlciB0aGUgdmlkZW8ncyBwcmVzZW50YXRpb24gbW9kZQor ICAgICAgICBjaGFuZ2VzIHRvICJwaWN0dXJlLWluLXBpY3R1cmUiLgorCisgICAgICAgIFRvIGZp eCB0aGlzLCB3ZSdsbCB1cGRhdGUgdGhlIHRlc3Qgc28gdGhhdCB3ZSdsbCB3YWl0IHVudGlsIHRo ZSBwbGFjZWhvbGRlcgorICAgICAgICBiZWNvbWVzIHZpc2libGUgYmVmb3JlIHRlc3RpbmcgaXRz IHZpc2liaWxpdHkgd2l0aG91dCB0aGUgImNvbnRyb2xzIiBhdHRyaWJ1dGUuCisKKyAgICAgICAg KiBtZWRpYS9jb250cm9scy9waWN0dXJlLWluLXBpY3R1cmUuaHRtbDoKKyAgICAgICAgKiBwbGF0 Zm9ybS9tYWMtd2syL1Rlc3RFeHBlY3RhdGlvbnM6CisKIDIwMTYtMDgtMDggIFJ5YW4gSGFkZGFk ICA8cnlhbmhhZGRhZEBhcHBsZS5jb20+CiAKICAgICAgICAgVXBkYXRlIHRlc3QgZXhwZWN0YXRp b25zIGZvciByZGFyOi8vcHJvYmxlbS8yNzcxMTA0OC4KZGlmZiAtLWdpdCBhL0xheW91dFRlc3Rz L21lZGlhL2NvbnRyb2xzL3BpY3R1cmUtaW4tcGljdHVyZS5odG1sIGIvTGF5b3V0VGVzdHMvbWVk aWEvY29udHJvbHMvcGljdHVyZS1pbi1waWN0dXJlLmh0bWwKaW5kZXggZWY4Yzc2MzlhNDk2YWI1 NTE3NDBmYjk2NTdiMDNmMzE2YzY5Y2E0Yi4uMGNkOTBmZGE1NGZhYTE3MWQ3ODQzOTM0OThiMjgy N2E5MjJkMTNlYiAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvbWVkaWEvY29udHJvbHMvcGljdHVy ZS1pbi1waWN0dXJlLmh0bWwKKysrIGIvTGF5b3V0VGVzdHMvbWVkaWEvY29udHJvbHMvcGljdHVy ZS1pbi1waWN0dXJlLmh0bWwKQEAgLTYyLDcgKzYyLDE4IEBACiAgICAgICAgICAgICAgICAgICAg IC52YWx1ZSh0ZXN0ZXIubWVkaWEud2Via2l0UHJlc2VudGF0aW9uTW9kZSkKICAgICAgICAgICAg ICAgICAgICAgLmlzRXF1YWxUbygicGljdHVyZS1pbi1waWN0dXJlIik7CiAKKyAgICAgICAgICAg ICAgICBwb2xsUElQUGxhY2Vob2xkZXJWaXNpYmlsaXR5Q2hhbmdlKCk7CisgICAgICAgICAgICB9 CisKKyAgICAgICAgICAgIGZ1bmN0aW9uIHBvbGxQSVBQbGFjZWhvbGRlclZpc2liaWxpdHlDaGFu Z2UoKQorICAgICAgICAgICAgewogICAgICAgICAgICAgICAgIGNvbnN0IHN0YXRlRm9yUGxhY2Vo b2xkZXIgPSB0ZXN0ZXIuc3RhdGVGb3JDb250cm9sc0VsZW1lbnQoIklubGluZSBwbGF5YmFjayBw bGFjZWhvbGRlciIsIHRydWUpOworICAgICAgICAgICAgICAgIGlmIChzdGF0ZUZvclBsYWNlaG9s ZGVyLmNsYXNzTmFtZS5pbmRleE9mKCJoaWRkZW4iKSA+PSAwKSB7CisgICAgICAgICAgICAgICAg ICAgIC8vIFVzZSAzMyB0byBtYXRjaCBQbGFjZWhvbGRlclBvbGxpbmdEZWxheSBpbiBtZWRpYUNv bnRyb2xzQXBwbGUuanMuCisgICAgICAgICAgICAgICAgICAgIHNldFRpbWVvdXQocG9sbFBJUFBs YWNlaG9sZGVyVmlzaWJpbGl0eUNoYW5nZSwgMzMpOworICAgICAgICAgICAgICAgICAgICByZXR1 cm47CisgICAgICAgICAgICAgICAgfQorCiAgICAgICAgICAgICAgICAgdGVzdGVyLnRlc3QoIklu bGluZSBwbGFjZWhvbGRlciBzaG91bGQgYmUgdmlzaWJsZSBhdCB0aGlzIHBvaW50IikKICAgICAg ICAgICAgICAgICAgICAgLnZhbHVlKHN0YXRlRm9yUGxhY2Vob2xkZXIuY2xhc3NOYW1lKQogICAg ICAgICAgICAgICAgICAgICAuZG9lc05vdENvbnRhaW4oImhpZGRlbiIpOwpkaWZmIC0tZ2l0IGEv TGF5b3V0VGVzdHMvcGxhdGZvcm0vbWFjLXdrMi9UZXN0RXhwZWN0YXRpb25zIGIvTGF5b3V0VGVz dHMvcGxhdGZvcm0vbWFjLXdrMi9UZXN0RXhwZWN0YXRpb25zCmluZGV4IGUxZGVhZWJlYjAyYTJm NjMzZmM3NGYzODNjOTA3NmY0YjE1M2I3MGIuLjgyMWVkYjI2NWRkMThjMGQ3MDBlZTFkNzdlZTA5 MmQ4MDgyMzU1NmUgMTAwNjQ0Ci0tLSBhL0xheW91dFRlc3RzL3BsYXRmb3JtL21hYy13azIvVGVz dEV4cGVjdGF0aW9ucworKysgYi9MYXlvdXRUZXN0cy9wbGF0Zm9ybS9tYWMtd2syL1Rlc3RFeHBl Y3RhdGlvbnMKQEAgLTQ3Miw3ICs0NzIsNyBAQCB3ZWJraXQub3JnL2IvMTU4NjM5IFsgUmVsZWFz ZSBZb3NlbWl0ZSBdIGltcG9ydGVkL2JsaW5rL3N0b3JhZ2UvaW5kZXhlZGRiL2Jsb2ItZAogCiAj IFBpUCB0ZXN0cyBhcmUgb25seSBlbmFibGVkIGZvciBTaWVycmEgV2ViS2l0Mi4KICMgcmRhcjov L3Byb2JsZW0vMjc1NzQzMDMKLVsgU2llcnJhKyBdIG1lZGlhL2NvbnRyb2xzL3BpY3R1cmUtaW4t cGljdHVyZS5odG1sIFsgUGFzcyBGYWlsdXJlIF0KK1sgU2llcnJhKyBdIG1lZGlhL2NvbnRyb2xz L3BpY3R1cmUtaW4tcGljdHVyZS5odG1sIFsgUGFzcyBdCiBbIFNpZXJyYSsgXSBtZWRpYS9lbGVt ZW50LWNvbnRhaW5pbmctcGlwLXZpZGVvLWdvaW5nLWludG8tZnVsbHNjcmVlbi5odG1sIFsgUGFz cyBdCiBbIFNpZXJyYSsgXSBtZWRpYS9mdWxsc2NyZWVuLWFwaS1lbmFibGVkLW1lZGlhLXdpdGgt cHJlc2VudGF0aW9uLW1vZGUuaHRtbCBbIFBhc3MgXQogWyBTaWVycmErIF0gbWVkaWEvZnVsbHNj cmVlbi12aWRlby1nb2luZy1pbnRvLXBpcC5odG1sIFsgUGFzcyBdCg==