Bug 225202

Summary: REGRESSION: [iOS] 6 media/modern-media-controls/tracks-support/ tests timing out
Product: WebKit Reporter: Aditya Keerthi <akeerthi>
Component: MediaAssignee: Aditya Keerthi <akeerthi>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, hi, jer.noble, philipj, sergio, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: iPhone / iPad   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Aditya Keerthi 2021-04-29 12:12:17 PDT
...
Comment 1 Aditya Keerthi 2021-04-29 12:12:30 PDT
<rdar://problem/77041631>
Comment 2 Aditya Keerthi 2021-04-29 12:14:05 PDT
Created attachment 427367 [details]
Patch
Comment 3 Devin Rousso 2021-04-29 12:24:18 PDT
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;
```
Comment 4 Aditya Keerthi 2021-04-29 12:53:47 PDT
Created attachment 427370 [details]
Patch for landing
Comment 5 Aditya Keerthi 2021-04-29 12:56:14 PDT
(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`.
Comment 6 EWS 2021-04-29 16:44:37 PDT
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].