Closed Bug 910974 Opened 7 years ago Closed 7 years ago

[B2G] [Bluetooth] [AVRCP] Dispatch events when remote device queries current play status

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gyeh, Assigned: gyeh)

Details

Attachments

(2 files, 4 obsolete files)

Currently, when remote device queries current play status, we reply with our cached information in BluetoothA2dpManager.

However, when integrating with Music App, I found that it would be better to ask for real play status from Music App again. It's easier to sync up the play status.
Whenever receiving this event, Music App should update current play status by calling our API |sendMediaPlayStatus()|.

What do you think, dkuo?
Flags: needinfo?(dkuo)
Eric, could you take a look at this patch? I'll send another review request to mrbkap after your review. Thanks.
Assignee: nobody → gyeh
Attachment #797613 - Flags: review?(echou)
Attached patch Patch 2(v1): Cleanup (obsolete) — Splinter Review
Shawn found that class |ConnectBluetoothSocketRunnable| isn't used anymore. :)
Attachment #797621 - Flags: review?(echou)
Comment on attachment 797621 [details] [diff] [review]
Patch 2(v1): Cleanup

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

Thanks for cleaning this up.
Attachment #797621 - Flags: review?(echou) → review+
Comment on attachment 797613 [details] [diff] [review]
Patch 1(v1): Dispatch events when remote device queries current play status

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

Hi Gina, I think your BluetoothDBuService.cpp is not the right one since you removed SendPlayStatusTask which still being used. Please check it out. Thanks.

::: dom/webidl/BluetoothAdapter.webidl
@@ +68,5 @@
>             attribute EventHandler   onscostatuschanged;
>  
> +  // Fired when remote devices query current media play status
> +  [SetterThrows]
> +  attribute EventHandler   onrequestmediaplaystatus;

nit: please make sure the indentation is the same as other 'attribute' declarations.
Attachment #797613 - Flags: review?(echou)
Patch updated.
Attachment #797613 - Attachment is obsolete: true
Attachment #798352 - Flags: review?(echou)
Attached patch Patch 2: Cleanup, r=echou (obsolete) — Splinter Review
Attachment #797621 - Attachment is obsolete: true
(In reply to Eric Chou [:ericchou] [:echou] from comment #5)
> Comment on attachment 797613 [details] [diff] [review]
> Patch 1(v1): Dispatch events when remote device queries current play status
> 
> Review of attachment 797613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Gina, I think your BluetoothDBuService.cpp is not the right one since you
> removed SendPlayStatusTask which still being used. Please check it out.
> Thanks.
> 
> ::: dom/webidl/BluetoothAdapter.webidl
> @@ +68,5 @@
> >             attribute EventHandler   onscostatuschanged;
> >  
> > +  // Fired when remote devices query current media play status
> > +  [SetterThrows]
> > +  attribute EventHandler   onrequestmediaplaystatus;
> 
> nit: please make sure the indentation is the same as other 'attribute'
> declarations.

This should be correct. Actually, I'm know why the other attributes align like this way. I've already aligned them correctly in the patch of bug 906305.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #8)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #5)
> > Comment on attachment 797613 [details] [diff] [review]
> > Patch 1(v1): Dispatch events when remote device queries current play status
> > 
> > Review of attachment 797613 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hi Gina, I think your BluetoothDBuService.cpp is not the right one since you
> > removed SendPlayStatusTask which still being used. Please check it out.
> > Thanks.
> > 
> > ::: dom/webidl/BluetoothAdapter.webidl
> > @@ +68,5 @@
> > >             attribute EventHandler   onscostatuschanged;
> > >  
> > > +  // Fired when remote devices query current media play status
> > > +  [SetterThrows]
> > > +  attribute EventHandler   onrequestmediaplaystatus;
> > 
> > nit: please make sure the indentation is the same as other 'attribute'
> > declarations.
> 
> This should be correct. Actually, I'm know why the other attributes align
> like this way. I've already aligned them correctly in the patch of bug
> 906305.

Yeah, I understand that you don't like this manner. In fact, I had raised the same question in the review of that patch, and in the end I accepted it. Please refer to bug 888595 comment 48, 50, 51 and 53.
This is weird for me :|

However, I'll follow the style in the final patch....
Comment on attachment 798352 [details] [diff] [review]
Patch 1(v2): Dispatch events when remote device queries current play status

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

Looks good, and please fix the alignment nit before merging.

Thanks.
Gina, as we discussed offline last week, the bluetooth adapter will receive the "requestmediaplaystatus" event when the remote device queries current play status, then music app should call adapter.sendMediaPlayStatus, so let's go for this design, thanks.
Flags: needinfo?(dkuo)
Comment on attachment 798352 [details] [diff] [review]
Patch 1(v2): Dispatch events when remote device queries current play status

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

Obviously I forgot r+ ;)
Attachment #798352 - Flags: review?(echou) → review+
(In reply to Dominic Kuo [:dkuo] from comment #12)
> Gina, as we discussed offline last week, the bluetooth adapter will receive
> the "requestmediaplaystatus" event when the remote device queries current
> play status, then music app should call adapter.sendMediaPlayStatus, so
> let's go for this design, thanks.

Yep! This is exactly what I'm thinking! :D
Comment on attachment 798352 [details] [diff] [review]
Patch 1(v2): Dispatch events when remote device queries current play status

Hi Blake,

Could you take a look at this patch? I added a event handler in BluetoothAdapter.webidl.
Attachment #798352 - Flags: superreview?(mrbkap)
Attachment #798352 - Flags: superreview?(mrbkap) → superreview+
Thanks, Blake.
Attachment #798353 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.