Bug 225202 – REGRESSION: [iOS] 6 media/modern-media-controls/tracks-support/ tests timing out

• Component: Media
• Status: RESOLVED FIXED

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/77041631>

[Aditya Keerthi]

Created attachment 427367 [details]
Patch

[Devin Rousso]

Comment on attachment 427367 [details]
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;
```

[Aditya Keerthi]

Created attachment 427370 [details]
Patch for landing

[Aditya Keerthi]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 427367 [details]
> 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).

The logic to clear callbacks on `uiScriptComplete` was added in https://trac.webkit.org/changeset/192039/webkit, to prevent callbacks from sticking around in later tests. 

> > 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;
> ```

Thanks for code snippet! I've made `pressOnElement` return a `Promise` if it's not provided a `continuation`.

[EWS]

Committed r276822 (237177@main): <https://commits.webkit.org/237177@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427370 [details].