Closed Bug 1024579 Opened 7 years ago Closed 7 years ago

Create a sync method to get mcc-mnc and ip, and use it on _getNetworkState

Categories

(Core :: DOM: Push Notifications, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: willyaranda, Assigned: willyaranda)

References

Details

Attachments

(1 file, 2 obsolete files)

This will be used also by bug 894879.
(Better format for the patch)
Attachment #8439923 - Attachment is obsolete: true
Attachment #8439923 - Flags: review?(nsm.nikhil)
Attachment #8439927 - Flags: review?(nsm.nikhil)
Comment on attachment 8439927 [details] [diff] [review]
Create getNetworkInformation and make getNetworkStatus use it

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

r=me with comments addressed.

::: dom/push/src/PushService.jsm
@@ +1501,5 @@
>      this._beginWSSetup();
>    },
>  
>    /**
> +   * Returns information about MCC-MNC and the IP of the current connection

Period at the end.

@@ +1551,5 @@
> +
> +  /**
> +   * Get mobile network information to decide if the client is capable of being
> +   * woken up by UDP (which currently just means having an mcc and mnc along
> +   * with an IP).

Please update the comment to reflect the optional netid.

@@ +1558,5 @@
> +    debug("getNetworkState()");
> +
> +    if (typeof callback !== 'function') {
> +      throw new Error("No callback method. Aborting push agent !");
> +    }

Nit: Newline after this.

@@ +1576,5 @@
> +      callback({
> +        mcc: 0,
> +        mnc: 0,
> +        ip: undefined
> +      });

Just call with networkInfo?
Attachment #8439927 - Flags: review?(nsm.nikhil) → review+
Hi, could you provide a try link for this checkin ?
(In reply to Carsten Book [:Tomcat] from comment #5)
> Hi, could you provide a try link for this checkin ?

There is no try because there are no tests in push code, but I let Nikhil to properly answer here :)
Flags: needinfo?(nsm.nikhil)
(In reply to Guillermo López :willyaranda from comment #6)
> (In reply to Carsten Book [:Tomcat] from comment #5)
> > Hi, could you provide a try link for this checkin ?
> 
> There is no try because there are no tests in push code, but I let Nikhil to
> properly answer here :)

Agreed.
Flags: needinfo?(nsm.nikhil)
(In reply to Carsten Book [:Tomcat] from comment #5)
> Hi, could you provide a try link for this checkin ?

Due to comments #6 & #7, try is not needed here.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f60e633f247

(In reply to Guillermo López :willyaranda from comment #6)
> There is no try because there are no tests in push code

That's mildly disturbing. But what could possibly go wrong? :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0f60e633f247
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment on attachment 8440439 [details] [diff] [review]
Create getNetworkInformation and make getNetworkStatus use it, v3

Approval Request Comment
[Feature/regressing bug #]: Needed by https://bugzilla.mozilla.org/show_bug.cgi?id=894879
[User impact if declined]: Bug 894879 cannot land on aurora
[Describe test coverage new/current, TBPL]: - 
[Risks and why]: -
[String/UUID change made/needed]: -
Attachment #8440439 - Flags: approval-mozilla-aurora?
Attachment #8440439 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8440439 [details] [diff] [review]
Create getNetworkInformation and make getNetworkStatus use it, v3

This is missing a risk assessment in the approval process. We need that information before making an approval request.
Attachment #8440439 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Guillermo, could you provide the risk assessment? Thanks
Flags: needinfo?(willyaranda)
[Risks and why]: Small risk. This only splits a method into two parts: one sync and other async (previous method was fully async).

The sync method are being used by this new async part, and also by bug 894879.
Flags: needinfo?(willyaranda)
Comment on attachment 8440439 [details] [diff] [review]
Create getNetworkInformation and make getNetworkStatus use it, v3

Merge day is passed. Taking it for 32 beta.
Attachment #8440439 - Flags: approval-mozilla-beta+
Attachment #8440439 - Flags: approval-mozilla-aurora?
Attachment #8440439 - Flags: approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.