1062912 - [B2G] move getNeighboringCellIds and getCellInfoList to nsIMobileConnectionService

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 1

[Jessica Jong [:jessica] (inactive)]

I'll take care of this. :)

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes, v1.

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.

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8504582 [details] [diff] [review]
Part 1: idl changes, v1.

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

Looks good.

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes, v2.

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.

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: implementation, v1.

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: test changes, v1.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 9

[Jessica Jong [:jessica] (inactive)]

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

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8507763 [details] [diff] [review]
Part 3: test changes, v1.

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

yay

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 1: idl changes. r=hsinyi

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 2: implementation. r=hsinyi

Address review comments in comment 7.

Comment 14

[Jessica Jong [:jessica] (inactive)]

Attachment: [final] Part 3: test changes. r=hsinyi

Remove empty lines at end of file.

Comment 15

[Jessica Jong [:jessica] (inactive)]

(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! ;)

Comment 16

[Jessica Jong [:jessica] (inactive)]

try looks good!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ce1e79c81be4

Comment 17

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/2ab6850fd8ba
https://hg.mozilla.org/integration/b2g-inbound/rev/d201452415aa
https://hg.mozilla.org/integration/b2g-inbound/rev/ca208be2edfc

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ab6850fd8ba
https://hg.mozilla.org/mozilla-central/rev/d201452415aa
https://hg.mozilla.org/mozilla-central/rev/ca208be2edfc