Closed Bug 1358399 Opened 4 years ago Closed 4 years ago

Refactor SetupEME() in eme.js

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(7 files)

http://searchfox.org/mozilla-central/rev/7aa21f3b531ddee90a353215bd86e97d6974e25b/dom/media/test/eme.js#288

Some things we want to improve:

1. The function is huge and hard to reuse without modification.
   The behavior is controlled by the flags or callbacks contained in the |params| parameter and we often find ourselves having to add new flags/callbacks to customize the behavior of the function. Also |params| is confusing and we have to search through the code to find out how each flag/callback changes the behavior of SetupEME().

2. The error handling is poor.
   bail() is called to finish the whole test case whenever an error is encountered. Instead, we want to propagate the error to the client code which can finish the test and continue to run other tests to catch as many errors as possible in one test run.

3. The code is inherently racy.
   The 'onlyLoadFirstFragments' flag is added by bug 1152151 to mitigate the issue where tests are run after being finished. An ad hoc value 2 is chosen for the flag which is kinda confusing.

How to improve:
1. Split a large function into small ones.
   Small functions are more likely to be reused without modification.
2. Change all functions to be promise-based.
   The error can be propagated by rejecting the promise with an error which will be handled by the client code.
3. Since all functions are promise-based, it is easy to control the order of events by using .then() or Promise.all() to erase the race.
Keywords: meta
Depends on: 1358401
Assignee: nobody → jwwang
Keywords: meta
Priority: -- → P3
Attachment #8860816 - Flags: review?(gsquelart)
Attachment #8860817 - Flags: review?(gsquelart)
Attachment #8860818 - Flags: review?(gsquelart)
Attachment #8860819 - Flags: review?(gsquelart)
Attachment #8860820 - Flags: review?(gsquelart)
Attachment #8860821 - Flags: review?(gsquelart)
Attachment #8860822 - Flags: review?(gsquelart)
Comment on attachment 8860816 [details]
Bug 1358399. P1 - move "elem.crossOrigin = test.crossOrigin || false" from SetupEME() to LoadTest() to improve cohesion.

https://reviewboard.mozilla.org/r/132798/#review136590
Attachment #8860816 - Flags: review?(gsquelart) → review+
Comment on attachment 8860817 [details]
Bug 1358399. P2 - split SetupEME() into small functions which will be useful in next patches.

https://reviewboard.mozilla.org/r/132800/#review136592

Thank you very much for cleaning up this old code!
Nits:

::: dom/media/test/eme.js:285
(Diff revision 1)
>    });
>  }
>  
> +/*
> + * Create a new MediaKeys object.
> + * Return a promsie which will be resolved with a new MediaKeys object

'promsie' -> 'promise'

::: dom/media/test/eme.js:285
(Diff revision 1)
> + * Return a promsie which will be resolved with a new MediaKeys object
> + * if succeeded or rejected with a string that describes the failure.

Sorry, this sentence is a bit hard to parse (I blame the English language for allowing such ambiguous grammar!)
I think you can just replace "...object if succeeded or rejected..." with "...object, or will be rejected...".

And same for the following functions.

::: dom/media/test/eme.js:330
(Diff revision 1)
> +  CreateMediaKeys(v, test, token).then(mediaKeys => {
> +    v.setMediaKeys(mediaKeys).then(
> +      p.resolve,
> +      () => p.reject(`${token} Failed to set MediaKeys on <video> element.`)
> +    );
> +  }, p.reject)

No string in this one?

::: dom/media/test/eme.js:337
(Diff revision 1)
> + * Return a promise which will be resolved with the init data when collection
> + * is completed (specified by test.sessionCount).

Note to self -- and you if you agree! ;-)

This will wait forever if we don't get the expected number of sessions; i.e., the whole test will eventually time out, but it won't be clear what went wrong.

I'd be great if we had our own timeouts, so we'd cancel bad tests quicker, and with a better explanation for them.

What do you think?
We could open another bug to tackle this later on.

::: dom/media/test/eme.js:359
(Diff revision 1)
> +  return p.promise;
> +}
> +
> +/*
> + * Generate a license request and update the session.
> + * Return a promsise which will be resolved with the updated session

'promsie' -> 'promise'

::: dom/media/test/eme.js:375
(Diff revision 1)
> +
> +  Log(token, str);
> +  session.generateRequest(ev.initDataType, ev.initData)
> +  .catch(reason => {
> +    // Reject the promise if generateRequest() failed.
> +    // Otherwise it will be resolve in UpdateSessionFunc().

'resolve' -> 'resolved'
Attachment #8860817 - Flags: review?(gsquelart) → review+
Comment on attachment 8860818 [details]
Bug 1358399. P3 - rewrite test_eme_waitingforkey.html using the new helper functions.

https://reviewboard.mozilla.org/r/132802/#review136602
Attachment #8860818 - Flags: review?(gsquelart) → review+
Comment on attachment 8860819 [details]
Bug 1358399. P4 - rewrite test_eme_playback.html using the new helper functions.

https://reviewboard.mozilla.org/r/132804/#review136608
Attachment #8860819 - Flags: review?(gsquelart) → review+
Comment on attachment 8860820 [details]
Bug 1358399. P5 - rewrite test_eme_stream_capture_blocked_case1.html using the new helper functions.

https://reviewboard.mozilla.org/r/132806/#review136610
Attachment #8860820 - Flags: review?(gsquelart) → review+
Comment on attachment 8860821 [details]
Bug 1358399. P6 - rewrite SetupEME() using the new helper functions and fix its callers.

https://reviewboard.mozilla.org/r/132808/#review136614
Attachment #8860821 - Flags: review?(gsquelart) → review+
Comment on attachment 8860822 [details]
Bug 1358399. P7 - remove unused code.

https://reviewboard.mozilla.org/r/132810/#review136616
Attachment #8860822 - Flags: review?(gsquelart) → review+
Comment on attachment 8860817 [details]
Bug 1358399. P2 - split SetupEME() into small functions which will be useful in next patches.

https://reviewboard.mozilla.org/r/132800/#review136592

> No string in this one?

CreateMediaKeys(...).then(foo, p.reject);
The reject value of CreateMediaKeys(...) will be forwarded to p.reject so we don't need to supply an error string.

> Note to self -- and you if you agree! ;-)
> 
> This will wait forever if we don't get the expected number of sessions; i.e., the whole test will eventually time out, but it won't be clear what went wrong.
> 
> I'd be great if we had our own timeouts, so we'd cancel bad tests quicker, and with a better explanation for them.
> 
> What do you think?
> We could open another bug to tackle this later on.

Sure. I will open a new bug to add the feature.
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9caffc5a9cb7
P1 - move "elem.crossOrigin = test.crossOrigin || false" from SetupEME() to LoadTest() to improve cohesion. r=gerald
https://hg.mozilla.org/integration/autoland/rev/443a9233a444
P2 - split SetupEME() into small functions which will be useful in next patches. r=gerald
https://hg.mozilla.org/integration/autoland/rev/b9b458868e4c
P3 - rewrite test_eme_waitingforkey.html using the new helper functions. r=gerald
https://hg.mozilla.org/integration/autoland/rev/c3aa65388e86
P4 - rewrite test_eme_playback.html using the new helper functions. r=gerald
https://hg.mozilla.org/integration/autoland/rev/7f21399ac77c
P5 - rewrite test_eme_stream_capture_blocked_case1.html using the new helper functions. r=gerald
https://hg.mozilla.org/integration/autoland/rev/bae75c8977d6
P6 - rewrite SetupEME() using the new helper functions and fix its callers. r=gerald
https://hg.mozilla.org/integration/autoland/rev/83784052849b
P7 - remove unused code. r=gerald
See Also: → 1359725
You need to log in before you can comment on or make changes to this bug.