Closed Bug 1062912 Opened 6 years ago Closed 6 years ago

[B2G] move getNeighboringCellIds and getCellInfoList to nsIMobileConnectionService

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(feature-b2g:2.2+, tracking-b2g:backlog)

RESOLVED FIXED
2.1 S7 (24Oct)
feature-b2g 2.2+
tracking-b2g backlog

People

(Reporter: hsinyi, Assigned: jessica)

References

Details

Attachments

(3 files, 4 obsolete files)

This is a follow up to https://bugzilla.mozilla.org/show_bug.cgi?id=1010356#c5

As bug 843452 landed, we could move these two functions to a properer place!
Depends on: 1063304
I'll take care of this. :)
Assignee: nobody → jjong
Attached patch Part 1: idl changes, v1. (obsolete) — Splinter Review
We are moving cell info related APIs to nsIMobileConnection. Since the callbacks use jsval, we are replacing them in this bug as well.

For nsICellInfoListCallback, there can be four types of callbacks and I've added a callback function for each type, therefore callers will need to implement these four callbacks. I am not sure if there is a better way.

Hsinyi, may I have your feedback? Thanks.
Attachment #8504582 - Flags: feedback?(htsai)
Comment on attachment 8504582 [details] [diff] [review]
Part 1: idl changes, v1.

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

Looks good.
Attachment #8504582 - Flags: feedback?(htsai) → feedback+
Blocks: 1032865
Attached patch Part 1: idl changes, v2. (obsolete) — Splinter Review
Hsinyi, per our offline discussion, for notifyGetCellInfoList() we can return an array of base class, nsICellInfo, and callers will do query interface based on the type.

May I have your review? Thanks.
Attachment #8504582 - Attachment is obsolete: true
Attachment #8507759 - Flags: review?(htsai)
Attached patch Part 2: implementation, v1. (obsolete) — Splinter Review
Attachment #8507762 - Flags: review?(htsai)
Attached patch Part 3: test changes, v1. (obsolete) — Splinter Review
Attachment #8507763 - Flags: review?(htsai)
Attachment #8507759 - Flags: review?(htsai) → review+
Comment on attachment 8507762 [details] [diff] [review]
Part 2: implementation, v1.

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

Looks good. r=me with comments addressed!

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +171,5 @@
> +  signalStrength: UNKNOWN_RSSI
> +};
> +
> +function CellInfo() {}
> +CellInfo.prototype = {

Have a comment here: // nsICellInfo

@@ +181,5 @@
> +
> +function GsmCellInfo() {}
> +GsmCellInfo.prototype = {
> +  __proto__: CellInfo.prototype,
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIGsmCellInfo]),

Please also have Ci.nsICellInfo in the generateQI list.

@@ +202,5 @@
> +
> +function WcdmaCellInfo() {}
> +WcdmaCellInfo.prototype = {
> +  __proto__: CellInfo.prototype,
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWcdmaCellInfo]),

ditto.

@@ +224,5 @@
> +
> +function LteCellInfo() {}
> +LteCellInfo.prototype = {
> +  __proto__: CellInfo.prototype,
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsILteCellInfo]),

ditto.

@@ +250,5 @@
> +
> +function CdmaCellInfo() {}
> +CdmaCellInfo.prototype = {
> +  __proto__: CellInfo.prototype,
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsICdmaCellInfo]),

ditto.
Attachment #8507762 - Flags: review?(htsai) → review+
Comment on attachment 8507763 [details] [diff] [review]
Part 3: test changes, v1.

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

::: dom/mobileconnection/tests/marionette/test_mobile_neighboring_cell_ids.js
@@ +4,5 @@
> +MARIONETTE_TIMEOUT = 30000;
> +// This test must run in chrome context.
> +MARIONETTE_CONTEXT = "chrome";
> +
> +let Promise = Cu.import("resource://gre/modules/Promise.jsm").Promise;

We should stop using Promise.jsm over SpecialPowers in marionette tests. Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=1065185 to modify it. Thank you.

@@ +35,5 @@
> +    ok(false, "getNeighboringCellIds should not success");
> +  }, function reject(aError) {
> +    is(aError, "RequestNotSupported", "failed to getNeighboringCellIds");
> +  }).then(finish);
> +

remove the empty lines.
Attachment #8507763 - Flags: review?(htsai)
Thank you hsinyi for the review.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> Comment on attachment 8507763 [details] [diff] [review]
> Part 3: test changes, v1.
> 
> Review of attachment 8507763 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/tests/marionette/test_mobile_neighboring_cell_ids.js
> @@ +4,5 @@
> > +MARIONETTE_TIMEOUT = 30000;
> > +// This test must run in chrome context.
> > +MARIONETTE_CONTEXT = "chrome";
> > +
> > +let Promise = Cu.import("resource://gre/modules/Promise.jsm").Promise;
> 
> We should stop using Promise.jsm over SpecialPowers in marionette tests.
> Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=1065185 to
> modify it. Thank you.

Sorry, I am still not very familiar with this. From my observations, this test is running in chrome context, so we are not using SpecialPowers, so do we still need to change it? From https://bugzilla.mozilla.org/show_bug.cgi?id=1065185, tests running in chrome context were not modified, like [1]. Please let me know if I am wrong or misunderstanding anything. Thanks!

> 
> @@ +35,5 @@
> > +    ok(false, "getNeighboringCellIds should not success");
> > +  }, function reject(aError) {
> > +    is(aError, "RequestNotSupported", "failed to getNeighboringCellIds");
> > +  }).then(finish);
> > +
> 
> remove the empty lines.

Will do.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/tests/marionette/head.js#12
(In reply to Jessica Jong [:jjong] [:jessica] from comment #9)
> Thank you hsinyi for the review.
> 
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> > Comment on attachment 8507763 [details] [diff] [review]
> > Part 3: test changes, v1.
> > 
> > Review of attachment 8507763 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/mobileconnection/tests/marionette/test_mobile_neighboring_cell_ids.js
> > @@ +4,5 @@
> > > +MARIONETTE_TIMEOUT = 30000;
> > > +// This test must run in chrome context.
> > > +MARIONETTE_CONTEXT = "chrome";
> > > +
> > > +let Promise = Cu.import("resource://gre/modules/Promise.jsm").Promise;
> > 
> > We should stop using Promise.jsm over SpecialPowers in marionette tests.
> > Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=1065185 to
> > modify it. Thank you.
> 
> Sorry, I am still not very familiar with this. From my observations, this
> test is running in chrome context, so we are not using SpecialPowers, so do
> we still need to change it? From
> https://bugzilla.mozilla.org/show_bug.cgi?id=1065185, tests running in
> chrome context were not modified, like [1]. Please let me know if I am wrong
> or misunderstanding anything. Thanks!
> 

You are totally right!!! Sorry for the mistake :(
Comment on attachment 8507763 [details] [diff] [review]
Part 3: test changes, v1.

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

yay
Attachment #8507763 - Flags: review+
Attachment #8507759 - Attachment is obsolete: true
Attachment #8509299 - Flags: review+
Address review comments in comment 7.
Attachment #8507762 - Attachment is obsolete: true
Attachment #8509302 - Flags: review+
Remove empty lines at end of file.
Attachment #8507763 - Attachment is obsolete: true
Attachment #8509303 - Flags: review+
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #10)
> (In reply to Jessica Jong [:jjong] [:jessica] from comment #9)
> > Thank you hsinyi for the review.
> > 
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> > > Comment on attachment 8507763 [details] [diff] [review]
> > > Part 3: test changes, v1.
> > > 
> > > Review of attachment 8507763 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/mobileconnection/tests/marionette/test_mobile_neighboring_cell_ids.js
> > > @@ +4,5 @@
> > > > +MARIONETTE_TIMEOUT = 30000;
> > > > +// This test must run in chrome context.
> > > > +MARIONETTE_CONTEXT = "chrome";
> > > > +
> > > > +let Promise = Cu.import("resource://gre/modules/Promise.jsm").Promise;
> > > 
> > > We should stop using Promise.jsm over SpecialPowers in marionette tests.
> > > Please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=1065185 to
> > > modify it. Thank you.
> > 
> > Sorry, I am still not very familiar with this. From my observations, this
> > test is running in chrome context, so we are not using SpecialPowers, so do
> > we still need to change it? From
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1065185, tests running in
> > chrome context were not modified, like [1]. Please let me know if I am wrong
> > or misunderstanding anything. Thanks!
> > 
> 
> You are totally right!!! Sorry for the mistake :(

No problem! and thanks for the review! ;)
https://hg.mozilla.org/mozilla-central/rev/2ab6850fd8ba
https://hg.mozilla.org/mozilla-central/rev/d201452415aa
https://hg.mozilla.org/mozilla-central/rev/ca208be2edfc
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
blocking-b2g: --- → backlog
feature-b2g: --- → 2.2+
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.