914076 - [B2G] [Bluetooth] Notification is missed after pausing the music

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Comment 1

[Gina Yeh [:gyeh] [:ginayeh]]

Since the event wasn't correctly sent, the remote device like Carkit would fail to show correct play status.

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Send play status change event after pausing music

Hi Eric, please take a look at this patch. Thanks.

Comment 3

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 801537 [details] [diff] [review]
Patch 1(v1): Send play status change event after pausing music

Review of attachment 801537 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me with nits picked.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +3189,5 @@
> +  }
> +
> +  if (eventId != ControlEventId::EVENT_UNKNOWN) {
> +    UpdateNotification(eventId, data);
> +  }

We could make this part simpler:

uint32_t tempPlayStatus = playStatus;

if (playStatus != a2dp->GetPlayStatus()) {
  UpdateNotification(ControlEventId::EVENT_PLAYBACK_STATUS_CHANGED, tempPlayStatus);
} else if (aPosition != a2dp->GetPosition()) {
  UpdateNotification(ControlEventId::EVENT_PLAYBACK_POS_CHANGED, aPosition);
}

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #3)
> Comment on attachment 801537 [details] [diff] [review]
> Patch 1(v1): Send play status change event after pausing music
> 
> Review of attachment 801537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. r=me with nits picked.
> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +3189,5 @@
> > +  }
> > +
> > +  if (eventId != ControlEventId::EVENT_UNKNOWN) {
> > +    UpdateNotification(eventId, data);
> > +  }
> 
> We could make this part simpler:
> 
> uint32_t tempPlayStatus = playStatus;
> 
> if (playStatus != a2dp->GetPlayStatus()) {
>   UpdateNotification(ControlEventId::EVENT_PLAYBACK_STATUS_CHANGED,
> tempPlayStatus);
> } else if (aPosition != a2dp->GetPosition()) {
>   UpdateNotification(ControlEventId::EVENT_PLAYBACK_POS_CHANGED, aPosition);
> }

Sounds good. Thanks!

Comment 5

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Send play status change event after pausing music, r=echou

nit addressed.

Comment 6

[Gina Yeh [:gyeh] [:ginayeh]]

try: 

https://tbpl.mozilla.org/?tree=Try&rev=d6a3854dd34b
https://tbpl.mozilla.org/?tree=Try&rev=c25ec449c9ce

Comment 7

[Gina Yeh [:gyeh] [:ginayeh]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d5802977fc32

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d5802977fc32