B2G Bluetooth: StartDiscovery/StopDiscovery

RESOLVED DUPLICATE of bug 740744

Status

()

RESOLVED DUPLICATE of bug 740744
7 years ago
7 years ago

People

(Reporter: echou, Assigned: echou)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
One of major functions of Bluetooth is Device Discovering. We need to implement function 'StartDiscovery'/'StopDiscovery' of nsIDOMBluetoothAdapter to let user do/cancel device discovering routine.
(Assignee)

Updated

7 years ago
Blocks: 727618
(Assignee)

Updated

7 years ago
Blocks: 740744
(Assignee)

Updated

7 years ago
Assignee: nobody → echou
(Assignee)

Updated

7 years ago
Depends on: 730942
(Assignee)

Comment 1

7 years ago
Created attachment 614333 [details] [diff] [review]
WIP: StartDiscovery/StopDiscovery

I added two functions dbus_func_args_timeout_valist() and dbus_func_args() to RawDBusConnection.cpp, may be used in BluetoothAdapter many times for DBus communications. However, those should be implemented in Bug 730942. That's why this issue depends on that one. So we should focus on StartDiscovery() and StopDiscovery().

While discovering, DeviceFound events would be created whenever a device has been discovered. For more information, please check Bug 740744.
Attachment #614333 - Flags: feedback?(kyle)
Comment on attachment 614333 [details] [diff] [review]
WIP: StartDiscovery/StopDiscovery

We should actually throw this in their own file as free functions, since they'll be under the android license. I'll do this over in 730942 real quick.
Attachment #614333 - Flags: feedback?(kyle) → feedback-
(Assignee)

Updated

7 years ago
Summary: Implement 'StartDiscovery' and 'StopDiscovery' functions of Bluetooth → B2G Bluetooth: StartDiscovery/StopDiscovery
(Assignee)

Comment 3

7 years ago
Created attachment 626393 [details] [diff] [review]
WIP v2: StartDiscovery/StopDiscovery

WIP. Functions called in StartDiscovery and StopDiscovery are not actually implemented in current code base, so implemetation will still be modified, but interface won't.
Attachment #614333 - Attachment is obsolete: true
(Assignee)

Comment 4

7 years ago
Created attachment 626395 [details] [diff] [review]
WIP v2: StartDiscovery/StopDiscovery

WIP. Functions called in StartDiscovery and StopDiscovery are not actually implemented in current code base, so implemetation will still be modified, but interface won't.
Attachment #626393 - Attachment is obsolete: true
(Assignee)

Comment 5

7 years ago
Created attachment 627093 [details] [diff] [review]
WIP v3: needs feedback

1. BluetoothService will handle DBus related functions. BluetoothAdapter & BluetoothDevice should not call those functions directly, or it will be hard to read. Furthermore, there will be several functions used by both BluetoothAdapter & BluetoothDevice, so we place them to BluetoothService.

2. We use Android apache license because in BluetoothService because most functions will be modified from android_server_BluetoothService.cpp
Attachment #626395 - Attachment is obsolete: true
Attachment #627093 - Flags: feedback?(kyle)
Comment on attachment 627093 [details] [diff] [review]
WIP v3: needs feedback

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

Service probably isn't the best name for this file. We expect anything named "Service" in gecko to be a sort of Singleton accessible to the universe for some certain function (i.e. SettingsService, etc...). Maybe Utils? 

Also, why does Start/StopDiscovery need to be there versus being methods of BluetoothAdapter? The only thing that can discover is an adapter, so there's no reason that I can tell to put those outside of the class? I can certainly see some of the other DBus-ish functions going in here, but Discovery seems pretty Adapter specific?
Attachment #627093 - Flags: feedback?(kyle)
(Assignee)

Comment 7

7 years ago
I know what a service is. I'll name it BluetoothUtils. :)

Although Start/Stop Discovery will only be called in BluetoothAdapter, the reason I put them outside of BluetoothAdapter is to make code more clean. You will not see dbus_func_args_async , dbus_set_error_from_message or dbus_message_iter_init_append is called in BluetoothAdapter or BluetoothDevice, sure we can just call them if you think that would be better.
Wow, yeah, I totally flaked on that "maybe we want to do this on other platforms someday" thing that I used to talk about so much, heh. Been totally focused on just getting this done for B2G. So I guess moving the implementations out to platform specific files may be a good idea. Just need to rethink how I'm going to deal with that elsewhere. Honestly, if it does take to long, we can throw them in the Bluetooth* classes to start and then refactor later, since we are just shipping this on B2G for the foreseeable future.
(Assignee)

Comment 9

7 years ago
So, I'll make the implementation stay at BluetoothUtils, because that's what I've done in my branch now. Make sense? If so, I'm going to send the patch again.
(Assignee)

Comment 10

7 years ago
Created attachment 627146 [details] [diff] [review]
WIP v4: StartDiscovery/StopDiscovery in BluetoothUtils

Move to BluetoothUtils.cpp, needs more feedback.
Attachment #627146 - Flags: feedback?(kyle)
Comment on attachment 627146 [details] [diff] [review]
WIP v4: StartDiscovery/StopDiscovery in BluetoothUtils

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

Ok, this all looks good. I should change my mName to mPath to match your usage. Also, I know I already said we should change this once, but... If we're going to keep all of the DBusish stuff in BluetoothUtils (which brings up a really interesting question of what we want to do with the RawDBusConnection base class I had...), we should probably rename it to BluetoothDBusUtils. That way we can also have BluetoothOSXUtils/BluetoothWindowsUtils when the time comes.
Attachment #627146 - Flags: feedback?(kyle) → feedback+
(Assignee)

Comment 12

7 years ago
Actually, I'm not quite sure how to use RawDBusConnection, should I inherit it, or should I put a member object in BluetoothAdapter and use it as a utility class? I don't even know what functions should be in RawDBusConnection and what should be in BluetoothUtils or the other files.

And, we can rename it since functions in this file are mostly composed of DBus-ish stuff.
I originally saw it as an inheritance sort of thing, to give classes the ability to have their own DBusConnection, and probably where some of the more general functions would go (dbus_func_* from android, etc...). However, I wasn't really thinking much about the whole cross platform issue when I came up with that, so... I suppose things could continue to go in Utils for the moment. I don't really know if the idea of exposing DBusThread's dbus connection as the only one will work, because of the thread conflicts we'll have between the IOThread Event Loop and other things trying to make calls.
This got wrapped into the snowball that is bug 740744.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 740744
You need to log in before you can comment on or make changes to this bug.