[B2G] move getNeighboringCellIds and getCellInfoList to nsIMobileConnectionService

RESOLVED FIXED in 2.1 S7 (24Oct)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: hsinyi, Assigned: jessica)

Tracking

unspecified
2.1 S7 (24Oct)
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

5 years ago
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!
I'll take care of this. :)
Assignee: nobody → jjong
Reporter

Updated

5 years ago
Posted 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)
Reporter

Comment 3

5 years ago
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
Posted 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)
Posted patch Part 2: implementation, v1. (obsolete) — Splinter Review
Attachment #8507762 - Flags: review?(htsai)
Posted patch Part 3: test changes, v1. (obsolete) — Splinter Review
Attachment #8507763 - Flags: review?(htsai)
Reporter

Updated

5 years ago
Attachment #8507759 - Flags: review?(htsai) → review+
Reporter

Comment 7

5 years ago
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+
Reporter

Comment 8

5 years ago
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
Reporter

Comment 10

5 years ago
(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 :(
Reporter

Comment 11

5 years ago
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: 5 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.