Closed Bug 1193379 Opened 4 years ago Closed 4 years ago

Followup of bug 1192693: Relocate files under dom/bluetooth/bluetooth2

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-master fixed)

RESOLVED FIXED
Tracking Status
b2g-master --- fixed

People

(Reporter: ben.tian, Assigned: ben.tian)

References

Details

Attachments

(5 files, 2 obsolete files)

Per bug 1192693 comment 9, we should discuss how to relocate files under dom/bluetooth/bluetooth2.

I have 3 options now:
1) Move to dom/bluetooth:
   The legacy location. Cons - many files under dom/bluetooth, looks messy.
2) Move to dom/bluetooth/common:
   Move common files used by both backends under this folder. Pros - more organized.
3) Leave them under dom/bluetooth/bluetooth2:
   Pros - in case we'll re-design connection & profile related API, we can create another folder to isolate new implementation as bluetooth2.
Jocelyn & Thomas, any thoughts on comment 1? More options are welcome.
Flags: needinfo?(tzimmermann)
Flags: needinfo?(joliu)
(In reply to Ben Tian [:btian] from comment #1)
> Jocelyn & Thomas, any thoughts on comment 1? More options are welcome.

Should be comment 0.
I'd not go for option 1 for the reason you mention. How about renaming 'bluetooth2' to something like 'webapi' or 'dom'?
Flags: needinfo?(tzimmermann)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> I'd not go for option 1 for the reason you mention. How about renaming
> 'bluetooth2' to something like 'webapi' or 'dom'?

I prefer 'webapi' to 'dom' since 'dom' is ambiguous with that in 'dom/bluetooth'. Then should we move out files NOT for webapi (BluetoothService/RilListener/ReplyRunnable/ProfileController)?
(In reply to Ben Tian [:btian] from comment #4)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> > I'd not go for option 1 for the reason you mention. How about renaming
> > 'bluetooth2' to something like 'webapi' or 'dom'?
> 
> I prefer 'webapi' to 'dom' since 'dom' is ambiguous with that in
> 'dom/bluetooth'. Then should we move out files NOT for webapi
> (BluetoothService/RilListener/ReplyRunnable/ProfileController)?

Agreed 'dom' is ambiguous here.
I think we should move out those files and ipc, test folders.

3): 'bluetooth2' looks like a strange folder to me and may confuse people who are not familiar with our module.
Flags: needinfo?(joliu)
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #5)
> > I prefer 'webapi' to 'dom' since 'dom' is ambiguous with that in
> > 'dom/bluetooth'. Then should we move out files NOT for webapi
> > (BluetoothService/RilListener/ReplyRunnable/ProfileController)?
> 
> Agreed 'dom' is ambiguous here.
> I think we should move out those files and ipc, test folders.

Another option is to have dom/bluetooth/common and dom/bluetooth/common/webapi for the two types of files above.
(In reply to Ben Tian [:btian] from comment #6)
> (In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #5)
> > > I prefer 'webapi' to 'dom' since 'dom' is ambiguous with that in
> > > 'dom/bluetooth'. Then should we move out files NOT for webapi
> > > (BluetoothService/RilListener/ReplyRunnable/ProfileController)?
> > 
> > Agreed 'dom' is ambiguous here.
> > I think we should move out those files and ipc, test folders.
> 
> Another option is to have dom/bluetooth/common and
> dom/bluetooth/common/webapi for the two types of files above.

IMHO, this option looks better!
But for ipc, test folders, how about we put them under dom/bluetooth?
My thought is to only see folders (common, bluedroid, bluez, ipc, tests) and moz.build under dom/bluetooth.
I don't have a strong opinion; I'd just like to see 'bluetooth2' be gone. That name only makes sense in the historical context in which it was created.
Comment 7 option seems to be the most acceptable. I'll work toward it on this bug.

Thanks for the input!
Assignee: nobody → btian
Depends on: 1180556
Also remove redundant BluetoothRilListener.cpp under bluetooth2.
Attachment #8650890 - Flags: review?(joliu)
Also revise include file in BluetoothService.h.
Attachment #8650893 - Flags: review?(joliu)
Revise testing/marionette/client/marionette/tests/webapi-tests.ini accordingly.
Attachment #8650889 - Attachment is obsolete: true
Attachment #8650889 - Flags: review?(joliu)
Attachment #8650939 - Flags: review?(joliu)
Comment on attachment 8650939 [details] [diff] [review]
Patch 1 (v2): Move ipc and tests folders out from dom/bluetooth/bluetooth2

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

Overall looks good to me.

Could we also revise the file name for our marionette tests?
(ex: s/test_dom_BluetoothAdapter_discovery_API2.js/test_dom_BluetoothAdapter_discovery.js)
Also, revise the file list in dom/bluetooth/tests/marionette/manifest.ini.
Attachment #8650939 - Flags: review?(joliu) → review+
Attachment #8650890 - Flags: review?(joliu) → review+
Rename marionette test js files per reviewer's comment.
Attachment #8650939 - Attachment is obsolete: true
Comment on attachment 8650892 [details] [diff] [review]
[final] Patch 3: Move backend-neutral files into dom/bluetooth/common, r=joliu

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

::: dom/bluetooth/moz.build
@@ +7,5 @@
>  if CONFIG['MOZ_B2G_BT']:
>  
>      #
>      # Generic code
>      #

Please remove these three comment lines.
Attachment #8650892 - Flags: review?(joliu) → review+
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #18)
> Comment on attachment 8650892 [details] [diff] [review]
> Patch 3 (v1): Move backend-neutral files into dom/bluetooth/common
> 
> Review of attachment 8650892 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/moz.build
> @@ +7,5 @@
> >  if CONFIG['MOZ_B2G_BT']:
> >  
> >      #
> >      # Generic code
> >      #
> 
> Please remove these three comment lines.

The generic code follow below; in contrast to the backend code, which is specified elsewhere in the file.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #19)
> (In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #18)
> > Comment on attachment 8650892 [details] [diff] [review]
> > Patch 3 (v1): Move backend-neutral files into dom/bluetooth/common
> > 
> > Review of attachment 8650892 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/moz.build
> > @@ +7,5 @@
> > >  if CONFIG['MOZ_B2G_BT']:
> > >  
> > >      #
> > >      # Generic code
> > >      #
> > 
> > Please remove these three comment lines.
> 
> The generic code follow below; in contrast to the backend code, which is
> specified elsewhere in the file.

You were right.
Thanks for the comment. :)

Ben,
Sorry for my misunderstanding, please keep them, thanks.
Attachment #8650893 - Flags: review?(joliu) → review+
Attachment #8650940 - Flags: review?(joliu) → review+
Attachment #8650940 - Attachment description: Patch 5 (v1): Touch clobber for file relocation → [final] Patch 5: Touch clobber for file relocation, r=joliu
Attachment #8650893 - Attachment description: Patch 4 (v1): Create dom/bluetooth/common/webapi folder → [final] Patch 4: Create dom/bluetooth/common/webapi folder, r=joliu
Attachment #8650892 - Attachment description: Patch 3 (v1): Move backend-neutral files into dom/bluetooth/common → [final] Patch 3: Move backend-neutral files into dom/bluetooth/common, r=joliu
Attachment #8650890 - Attachment description: Patch 2 (v1): Rename bluetooth2 folder to common → [final] Patch 2: Rename bluetooth2 folder to common, r=joliu
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.