Closed
Bug 1193379
Opened 9 years ago
Closed 9 years ago
Followup of bug 1192693: Relocate files under dom/bluetooth/bluetooth2
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
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)
25.65 KB,
patch
|
yrliou
:
review+
|
Details | Diff | Splinter Review |
6.42 KB,
patch
|
yrliou
:
review+
|
Details | Diff | Splinter Review |
15.03 KB,
patch
|
yrliou
:
review+
|
Details | Diff | Splinter Review |
831 bytes,
patch
|
yrliou
:
review+
|
Details | Diff | Splinter Review |
8.71 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Jocelyn & Thomas, any thoughts on comment 1? More options are welcome.
Flags: needinfo?(tzimmermann)
Flags: needinfo?(joliu)
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Ben Tian [:btian] from comment #1)
> Jocelyn & Thomas, any thoughts on comment 1? More options are welcome.
Should be comment 0.
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
(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)?
Comment 5•9 years ago
|
||
(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)
Assignee | ||
Comment 6•9 years ago
|
||
(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.
Comment 7•9 years ago
|
||
(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.
Comment 8•9 years ago
|
||
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.
Assignee | ||
Comment 9•9 years ago
|
||
Comment 7 option seems to be the most acceptable. I'll work toward it on this bug.
Thanks for the input!
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → btian
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8650889 -
Flags: review?(joliu)
Assignee | ||
Comment 11•9 years ago
|
||
Also remove redundant BluetoothRilListener.cpp under bluetooth2.
Attachment #8650890 -
Flags: review?(joliu)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8650892 -
Flags: review?(joliu)
Assignee | ||
Comment 13•9 years ago
|
||
Also revise include file in BluetoothService.h.
Attachment #8650893 -
Flags: review?(joliu)
Assignee | ||
Comment 14•9 years ago
|
||
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)
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8650940 -
Flags: review?(joliu)
Comment 16•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8650890 -
Flags: review?(joliu) → review+
Assignee | ||
Comment 17•9 years ago
|
||
Rename marionette test js files per reviewer's comment.
Attachment #8650939 -
Attachment is obsolete: true
Comment 18•9 years ago
|
||
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+
Comment 19•9 years ago
|
||
(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.
Comment 20•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #8650893 -
Flags: review?(joliu) → review+
Updated•9 years ago
|
Attachment #8650940 -
Flags: review?(joliu) → review+
Assignee | ||
Comment 21•9 years ago
|
||
Assignee | ||
Comment 22•9 years ago
|
||
try with more tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e78d14211ee3
Assignee | ||
Updated•9 years ago
|
Attachment #8650940 -
Attachment description: Patch 5 (v1): Touch clobber for file relocation → [final] Patch 5: Touch clobber for file relocation, r=joliu
Assignee | ||
Updated•9 years ago
|
Attachment #8650893 -
Attachment description: Patch 4 (v1): Create dom/bluetooth/common/webapi folder → [final] Patch 4: Create dom/bluetooth/common/webapi folder, r=joliu
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Updated•9 years ago
|
Attachment #8650890 -
Attachment description: Patch 2 (v1): Rename bluetooth2 folder to common → [final] Patch 2: Rename bluetooth2 folder to common, r=joliu
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/a619abf20603
https://hg.mozilla.org/integration/b2g-inbound/rev/a07c01c0ae44
https://hg.mozilla.org/integration/b2g-inbound/rev/e85b1e693e3f
https://hg.mozilla.org/integration/b2g-inbound/rev/eaf58bf9b753
https://hg.mozilla.org/integration/b2g-inbound/rev/4a8baa5e1cf4
Keywords: checkin-needed
Assignee | ||
Comment 24•9 years ago
|
||
Resolve fixed since all commits have been landed to m-c.
http://hg.mozilla.org/mozilla-central/rev/a619abf20603
http://hg.mozilla.org/mozilla-central/rev/a07c01c0ae44
http://hg.mozilla.org/mozilla-central/rev/e85b1e693e3f
http://hg.mozilla.org/mozilla-central/rev/eaf58bf9b753
http://hg.mozilla.org/mozilla-central/rev/4a8baa5e1cf4
Comment 25•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•