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. 
<rdar://problem/27574303>
Created attachment 285672 [details] Patch
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.
(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!
Committed: https://trac.webkit.org/changeset/204304
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.
(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.