[webkit-reviews] review granted: [Bug 225202] REGRESSION: [iOS] 6 media/modern-media-controls/tracks-support/ tests timing out : [Attachment 427367] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Aditya Keerthi
<akeerthi at apple.com>'s request for review:
Bug 225202: REGRESSION: [iOS] 6 media/modern-media-controls/tracks-support/
tests timing out
https://bugs.webkit.org/show_bug.cgi?id=225202

Attachment 427367: Patch

https://bugs.webkit.org/attachment.cgi?id=427367&action=review




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 427367
  --> https://bugs.webkit.org/attachment.cgi?id=427367
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427367&action=review

r=me

> LayoutTests/ChangeLog:14
> +	   of the <video> element was increased. However, on iOS, this led to
the
> +	   tracks button being positioned outside the viewport. Consequently,
any

LOL!!!	I did not think of this at all!  Great catch!

> LayoutTests/ChangeLog:30
> +	   which lead to getTracksContextMenu() getting called before the tap
> +	   actually completed. UIScriptController clears all callbacks after
one

Oh wow that is a *subtle* detail of UIScriptController :(

Why was it made like this?  I would think that one kind of callback wouldn't
affect any other kind of callback?  Personally I prefer the approach of
establishing listeners/callbacks for actions before the action occurs so that
there's no chance of it being missed (I also think it reads nicer).

> LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:49
> +function pressOnElementAsync(element)

NIT: I feel like "Async" isn't the right name for this since `pressOnElement`
also takes a `continuation` callback.  Maybe `promisifiedPressOnElement`, or
even just having `pressOnElement` return a `Promise` if it's not provided a
`continuation`?
```
    let promise = null;
    if (typeof continuation !== "function") {
	promise = new Promise((resolve, reject) => {
	    continuation = resolve;
	});
    }
    ...
    return promise || true;
```