Implement mediasession set positon state #24885

shnmorimoto · 2019-11-27T15:06:07Z

Bonus points if you want to tweak the existing UI by adding a progress bar, and the info about the current position and total duration.

I haven't implemented this yet.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #24808 (GitHub issue number if applicable)

There are tests for these changes OR

highfive · 2019-11-27T15:06:14Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/htmlmediaelement.rs, components/script/dom/mediasession.rs, components/script/dom/webidls/MediaSession.webidl
@KiChjang: components/script/dom/htmlmediaelement.rs, components/script/dom/mediasession.rs, components/script/dom/webidls/MediaSession.webidl

components/script/dom/mediasession.rs

shnmorimoto · 2019-11-27T15:21:44Z

#24808 (comment)

Extend the existing HostTrait trait with a new on_media_session_set_position_state or such method. And implement that method for the C and JNI APIs

For the Android part, extend the existing Callbacks and Client interfaces with the new onMediaSessionSetPositionState callback. And implement it here and here.

To be honest, I don't have enough understanding of these behaviors.
Is there any way to test that these are implemented correctly?

jdm · 2019-11-27T15:22:08Z

r? @ferjm

ferjm

This is a great start, but we need to make some changes.

In addition to the comments I added, we should send MediaSessionEvent::SetPositionState events when the media player notifies about position updates.

components/embedder_traits/lib.rs

ports/libsimpleservo/api/src/lib.rs

ports/libsimpleservo/capi/src/lib.rs

components/script/dom/mediasession.rs

ports/libsimpleservo/api/src/lib.rs

ports/libsimpleservo/jniapi/src/lib.rs

components/script/dom/htmlmediaelement.rs

ferjm · 2019-11-28T10:30:48Z

To be honest, I don't have enough understanding of these behaviors.
Is there any way to test that these are implemented correctly?

You can test on Android. The instructions to build for this platform are on the README and the wiki.

You can try modifying this example.

shnmorimoto · 2019-11-29T16:47:40Z

@ferjm Thank you for a great review.

In addition to the comments I added, we should send MediaSessionEvent::SetPositionState events when the media player notifies about position updates.

I added the code for MediaSessionEvent::SetPositionState.

shnmorimoto · 2019-11-29T16:52:43Z

You can test on Android. The instructions to build for this platform are on the README and the wiki.

Thanks! I don't have Android right now, so I'll get it and try it.

shnmorimoto · 2019-11-29T16:59:40Z

When I change the code under the ports(ex. ports/libsimpleservo/jniapi/src/lib.rs ), it seems that it is not build even if I run ./mach build --dev .

Is this my misunderstanding?

jdm · 2019-11-29T18:38:38Z

The build command only builds one of the ports at a time. For libsimpleservo/jniapi, you need to pass the --android argument to ./mach build (https://github.com/servo/servo/wiki/Building-for-Android).

...t/android/apk/servoapp/src/main/java/org/mozilla/servo/MainActivity.java

...t/android/apk/servoapp/src/main/java/org/mozilla/servo/MediaSession.java

ferjm

Looks good. Thank you!

r=me with the last minor comments addressed.

ferjm · 2019-12-02T14:48:58Z

components/script/dom/mediasession.rs

+        // If the duration is not present or its value is null, throw a TypeError.
+        if state.duration.is_none() {
+            return Err(Error::Type(
+                "duration is not preset or its value is null".to_owned(),


typo: present

Oops. This is also typo..., Sorry.
done.

ferjm · 2019-12-02T14:48:58Z

components/script/dom/mediasession.rs

+            }
+        }
+
+        // If the position is not present or its value is null, set it to zero.


Remove this comment, please.

Thanks! done.


        implement MediaSession.setPositionState method


        updated tests for mediasession


        extended MediaSessionEvent with SetPositionState


        fix typo

shnmorimoto · 2019-12-03T13:28:19Z

In my local, build is success.
Should I rebase and put commits in one?

jdm · 2019-12-03T13:49:16Z

./mach build --libsimpleservo fails with this error:

error[E0609]: no field `on_media_session_set_position_state` on type `CHostCallbacks`
   --> ports\libsimpleservo\capi\src\lib.rs:741:17
    |
741 |         (self.0.on_media_session_set_position_state)(duration, position, playback_rate);
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unknown field
    |
    = note: available fields are: `flush`, `make_current`, `on_alert`, `on_load_started`, `on_load_ended` ... and 11 others
error: aborting due to previous error

shnmorimoto · 2019-12-03T13:52:06Z

@jdm Thanks! I'll check.

shnmorimoto · 2019-12-03T14:10:19Z

I fixed an error when run ./mach build --libsimpleservo.

> ./mach build --libsimpleservo                                                                                                                        implement_mediasession_set_positon_state
   Compiling simpleservo_capi v0.0.1 (/Users/shn/Work/servo/ports/libsimpleservo/capi)
   Completed simpleservo_capi v0.0.1 custom-build (run) in 1.2s
   Completed simpleservo_capi v0.0.1 in 195.3s
    Finished dev [unoptimized + debuginfo] target(s) in 3m 21s
Build Completed in 0:03:22


        extended HostTrait with on_media_session_set_position_state


        extended android jni with onMediaSessionSetPositionState

jdm · 2019-12-03T14:22:58Z

@bors-servo r=ferjm

bors-servo · 2019-12-03T14:22:59Z

📌 Commit 41ff93e has been approved by ferjm

bors-servo · 2019-12-03T15:23:02Z

⌛ Testing commit 41ff93e with merge 4ba6bfa...


        Auto merge of #24885 - shnmorimoto:implement_mediasession_set_positon…

…_state, r=ferjm Implement mediasession set positon state  fix #24808 > Bonus points if you want to tweak the existing UI by adding a progress bar, and the info about the current position and total duration. I haven't implemented this yet. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24808 (GitHub issue number if applicable)  - [x] There are tests for these changes OR

bors-servo · 2019-12-03T16:08:01Z

💔 Test failed - status-taskcluster

jdm · 2019-12-03T17:13:03Z

@bors-servo retry

#25046

bors-servo · 2019-12-03T17:13:09Z

⌛ Testing commit 41ff93e with merge f31a88d...


        Auto merge of #24885 - shnmorimoto:implement_mediasession_set_positon…

…_state, r=ferjm Implement mediasession set positon state  fix #24808 > Bonus points if you want to tweak the existing UI by adding a progress bar, and the info about the current position and total duration. I haven't implemented this yet. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24808 (GitHub issue number if applicable)  - [x] There are tests for these changes OR

bors-servo · 2019-12-03T18:11:18Z

☀️ Test successful - status-taskcluster
Approved by: ferjm
Pushing f31a88d to master...

highfive assigned asajeffrey Nov 27, 2019

highfive added the S-awaiting-review label Nov 27, 2019

shnmorimoto reviewed Nov 27, 2019

View changes

components/script/dom/mediasession.rs Show resolved Hide resolved

highfive assigned ferjm and unassigned asajeffrey Nov 27, 2019

ferjm requested changes Nov 28, 2019

View changes

ferjm added S-needs-code-changes and removed S-awaiting-review labels Nov 28, 2019

shnmorimoto force-pushed the shnmorimoto:implement_mediasession_set_positon_state branch from 769df62 to 187c90d Nov 29, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 29, 2019

shnmorimoto reviewed Dec 1, 2019

View changes

...t/android/apk/servoapp/src/main/java/org/mozilla/servo/MainActivity.java Show resolved Hide resolved

shnmorimoto force-pushed the shnmorimoto:implement_mediasession_set_positon_state branch from 187c90d to 17286de Dec 2, 2019

shnmorimoto reviewed Dec 2, 2019

View changes

...t/android/apk/servoapp/src/main/java/org/mozilla/servo/MediaSession.java Show resolved Hide resolved

ferjm approved these changes Dec 2, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 2, 2019

shnmorimoto added 4 commits Nov 27, 2019

implement MediaSession.setPositionState method

274d4d2

updated tests for mediasession

3e83b0b

extended MediaSessionEvent with SetPositionState

8bdee36

fix typo

e268193

shnmorimoto force-pushed the shnmorimoto:implement_mediasession_set_positon_state branch from 17286de to 29d6a35 Dec 2, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 3, 2019

shnmorimoto force-pushed the shnmorimoto:implement_mediasession_set_positon_state branch from 29d6a35 to 70b76ca Dec 3, 2019

highfive added S-awaiting-review and removed S-tests-failed labels Dec 3, 2019

shnmorimoto added 2 commits Nov 27, 2019

extended HostTrait with on_media_session_set_position_state

4d215c9

extended android jni with onMediaSessionSetPositionState

Loading status checks…

41ff93e

shnmorimoto force-pushed the shnmorimoto:implement_mediasession_set_positon_state branch from 70b76ca to 41ff93e Dec 3, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 3, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 3, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 3, 2019

bors-servo merged commit 41ff93e into servo:master Dec 3, 2019
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 3, 2019

bors-servo mentioned this pull request Dec 3, 2019

Basic Media Session UI for Hololens #24927

Merged

3 of 3 tasks complete

servo / servo

Implement mediasession set positon state #24885

Implement mediasession set positon state #24885

shnmorimoto commented Nov 27, 2019 •

edited

highfive commented Nov 27, 2019

shnmorimoto commented Nov 27, 2019

jdm commented Nov 27, 2019

ferjm left a comment

ferjm commented Nov 28, 2019 •

edited

shnmorimoto commented Nov 29, 2019 •

edited

shnmorimoto commented Nov 29, 2019 •

edited

shnmorimoto commented Nov 29, 2019

jdm commented Nov 29, 2019

ferjm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shnmorimoto commented Dec 3, 2019 •

edited

jdm commented Dec 3, 2019

shnmorimoto commented Dec 3, 2019

shnmorimoto commented Dec 3, 2019

jdm commented Dec 3, 2019

bors-servo commented Dec 3, 2019

bors-servo commented Dec 3, 2019

bors-servo commented Dec 3, 2019

jdm commented Dec 3, 2019

bors-servo commented Dec 3, 2019

bors-servo commented Dec 3, 2019

servo / servo

Join GitHub today

Implement mediasession set positon state #24885

Implement mediasession set positon state #24885

Conversation

shnmorimoto commented Nov 27, 2019 • edited

highfive commented Nov 27, 2019

shnmorimoto commented Nov 27, 2019

jdm commented Nov 27, 2019

ferjm left a comment

ferjm commented Nov 28, 2019 • edited

shnmorimoto commented Nov 29, 2019 • edited

shnmorimoto commented Nov 29, 2019 • edited

shnmorimoto commented Nov 29, 2019

jdm commented Nov 29, 2019

ferjm left a comment

This comment has been minimized.

ferjm Dec 2, 2019

This comment has been minimized.

shnmorimoto Dec 2, 2019

This comment has been minimized.

ferjm Dec 2, 2019

This comment has been minimized.

shnmorimoto Dec 2, 2019

shnmorimoto commented Dec 3, 2019 • edited

jdm commented Dec 3, 2019

shnmorimoto commented Dec 3, 2019

shnmorimoto commented Dec 3, 2019

jdm commented Dec 3, 2019

bors-servo commented Dec 3, 2019

bors-servo commented Dec 3, 2019

bors-servo commented Dec 3, 2019

jdm commented Dec 3, 2019

bors-servo commented Dec 3, 2019

bors-servo commented Dec 3, 2019

shnmorimoto commented Nov 27, 2019 •

edited

ferjm commented Nov 28, 2019 •

edited

shnmorimoto commented Nov 29, 2019 •

edited

shnmorimoto commented Nov 29, 2019 •

edited

shnmorimoto commented Dec 3, 2019 •

edited