Closed Bug 1043114 Opened 6 years ago Closed 5 years ago

B2G NetworkService: unify add/removeHostRoute and add/removeHostRouteWithResolve

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S2 (15aug)

People

(Reporter: jessica, Assigned: jessica)

References

Details

(Whiteboard: [p=3])

Attachments

(5 files, 12 obsolete files)

3.63 KB, patch
jessica
: review+
Details | Diff | Splinter Review
9.24 KB, patch
jessica
: review+
Details | Diff | Splinter Review
4.08 KB, patch
edgar
: review+
Details | Diff | Splinter Review
3.52 KB, patch
jessica
: review+
Details | Diff | Splinter Review
21.21 KB, patch
jessica
: review+
Details | Diff | Splinter Review
NetworkService's addHostRoute()/addHostRouteWithResolve() and addHostRouteWithResolve()/removeHostRouteWithResolve() are doing the same thing, just with different parameters. We should use a more generic one, such as:

void addHostRoute(in DOMString interfaceName, in jsval gateways, in jsval hosts);

so that non-network interfaces can use this API as well.

I think it'd be also nice to have something like this in NetworkManager:

void addHostRoute(in long networkType, in jsval hosts);

so that outer modules won't need to take care of the network details.

All suggestions are welcome!
Hi, could you help take a look to see if this new API is appropriate for you? Thanks.
Assignee: nobody → jjong
Attachment #8461416 - Flags: feedback?(vchang)
Attachment #8461416 - Flags: feedback?(echen)
Comment on attachment 8461416 [details] [diff] [review]
Part 1: unify add(remove)HostRoute and add(remove)HostRouteWithResolve (idl).

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

::: dom/system/gonk/nsINetworkService.idl
@@ +262,5 @@
> +   * @param hosts
> +   *        Array of host addresses we want to add route for.
> +   *
> +  void addHostRoute(in DOMString interfaceName, in jsval gateways,
> +                    in jsval hosts);

Why we need to have multiple gateways?
Should this api only add routes to a specific gateway?

@@ +276,5 @@
> +   * @param hosts
> +   *        Array of host addresses we want to remove route for.
> +   *
> +  void removeHostRoute(in DOMString interfaceName, in jsval gateways,
> +                       in jsval hosts);

Ditto.
Attachment #8461416 - Flags: feedback?(echen)
(In reply to Edgar Chen [:edgar][:echen] from comment #2)
> Comment on attachment 8461416 [details] [diff] [review]
> Part 1: unify add(remove)HostRoute and add(remove)HostRouteWithResolve (idl).
> 
> Review of attachment 8461416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/nsINetworkService.idl
> @@ +262,5 @@
> > +   * @param hosts
> > +   *        Array of host addresses we want to add route for.
> > +   *
> > +  void addHostRoute(in DOMString interfaceName, in jsval gateways,
> > +                    in jsval hosts);
> 
> Why we need to have multiple gateways?
> Should this api only add routes to a specific gateway?

Oh, this is to handle multiple gateways on a network interface, e.g. IPv4/IPv6 dual stack. Currently, NetworkUtils takes an array of gateways and will select one based on network protocol [1]. Note that users can still call this API with a single item(gateway) array.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkUtils.cpp#1370

> 
> @@ +276,5 @@
> > +   * @param hosts
> > +   *        Array of host addresses we want to remove route for.
> > +   *
> > +  void removeHostRoute(in DOMString interfaceName, in jsval gateways,
> > +                       in jsval hosts);
> 
> Ditto.
Per offline discussion with Edgar and Bevis, we would like to change NetworkService's API as follow:

  jsval addHostRoute(in DOMString interfaceName, in jsval gateway, in DOMString host);

This API accepts single gateway, single host and returns a promise to indicate success (resolve) or failure (reject). The caller is responsible of selecting the gateway beforehand and resolve the host if needed.

For the convenience of outer modules, we will provide the following API in NetworkManager:

  jsval addHostRoute(in long networkType, in long serviceId, in jsval host);
(In reply to Jessica Jong [:jjong] [:jessica] from comment #4)
> Per offline discussion with Edgar and Bevis, we would like to change
> NetworkService's API as follow:
> 
>   jsval addHostRoute(in DOMString interfaceName, in jsval gateway, in
> DOMString host);

Sorry, it should be: 

  jsval addHostRoute(in DOMString interfaceName, in DOMString gateway, in DOMString host);

> 
> This API accepts single gateway, single host and returns a promise to
> indicate success (resolve) or failure (reject). The caller is responsible of
> selecting the gateway beforehand and resolve the host if needed.
> 
> For the convenience of outer modules, we will provide the following API in
> NetworkManager:
> 
>   jsval addHostRoute(in long networkType, in long serviceId, in jsval host);
As discussed offline, since we are going to return the result of the action, it'd be better to add/remove one host at a time.
Attachment #8461416 - Attachment is obsolete: true
Attachment #8461416 - Flags: feedback?(vchang)
Attachment #8464481 - Flags: feedback?(vchang)
Attachment #8464481 - Flags: feedback?(echen)
Comment on attachment 8464481 [details] [diff] [review]
Part 1: unify add/removeHostRoute and add/removeHostRouteWithResolve (idl), v2.

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

Looks good, thank you.

::: dom/system/gonk/nsINetworkService.idl
@@ +260,5 @@
> +   *        Gateway ip for the output of the host route.
> +   * @param host
> +   *        Host ip we want to add route for.
> +   *
> +   * @return A deferred promise.

Please help to add some comments about the resolves and rejects stuff.

@@ +275,5 @@
> +   *        Gateway ip for the output of the host route.
> +   * @param host
> +   *        Host ip we want to remove route for.
> +   *
> +   * @return A deferred promise.

Ditto
Attachment #8464481 - Flags: feedback?(echen) → feedback+
Summary: B2G NetworkService: unify addHostRoute/addHostRouteWithResolve and removeHostRoute/removeHostRouteWithResolve → B2G NetworkService: unify add/removeHostRoute and add/removeHostRouteWithResolve
Attachment #8464481 - Flags: feedback?(vchang) → feedback+
Changes since v2:
- refine comment on the returned promise.
Attachment #8464481 - Attachment is obsolete: true
Attachment #8465232 - Flags: review?(echen)
Note that the flow remains the same, still not making use of the returned promise.
Attachment #8464562 - Attachment is obsolete: true
Attachment #8465233 - Flags: review?(echen)
Blocks: 1038531
Blocks: 939026
Comment on attachment 8465233 [details] [diff] [review]
Part 2: unify add/removeHostRoute and add/removeHostRouteWithResolve (NetworkManager), v1.

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

Please see my below comments. Thank you.

::: dom/system/gonk/NetworkManager.js
@@ +479,5 @@
> +        promises.push(gNetworkService.addHostRoute(network.name, gateway, host));
> +      }
> +    }
> +
> +    return Promise.all(promises);

Do we import Promise.jsm first?

@@ +490,5 @@
> +    for (let i = 0; i < hosts.length; i++) {
> +      let host = hosts[i];
> +      let gateway = this.selectGateway(gateways, host);
> +      if (gateway && host) {
> +        gNetworkService.removeHostRoute(network.name, gateway, host);

I guess you miss Promise stuff.

@@ +502,5 @@
> +      if (gateway.match(this.REGEXP_IPV4) && host.match(this.REGEXP_IPV4) ||
> +          gateway.indexOf(":") != -1 && host.indexOf(":") != -1) {
> +        return gateway;
> +      }
> +    }

return null;

@@ +526,5 @@
>  
>        let mmsHosts = this.resolveHostname([hostToResolve]);
>        if (mmsHosts.length == 0) {
>          debug("No valid hostnames can be added. Stop adding host route.");
>          return;

I think we should return a resolved/rejected promise here.

@@ +543,1 @@
>      }

Ditto

@@ +563,5 @@
>  
>        let mmsHosts = this.resolveHostname([hostToResolve]);
>        if (mmsHosts.length == 0) {
>          debug("No valid hostnames can be removed. Stop removing host route.");
>          return;

Ditto

@@ +580,1 @@
>      }

Ditto
Attachment #8465233 - Flags: review?(echen)
Comment on attachment 8465232 [details] [diff] [review]
Part 1: unify add/removeHostRoute and add/removeHostRouteWithResolve (idl), v3.

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

Thank you.
Attachment #8465232 - Flags: review?(echen) → review+
(In reply to Edgar Chen [:edgar][:echen] from comment #15)
> Comment on attachment 8465233 [details] [diff] [review]
> Part 2: unify add/removeHostRoute and add/removeHostRouteWithResolve
> (NetworkManager), v1.
> 
> Review of attachment 8465233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my below comments. Thank you.

Thanks for the review!

> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +479,5 @@
> > +        promises.push(gNetworkService.addHostRoute(network.name, gateway, host));
> > +      }
> > +    }
> > +
> > +    return Promise.all(promises);
> 
> Do we import Promise.jsm first?

Yes, we should. It's weird there wasn't any error though.

> 
> @@ +490,5 @@
> > +    for (let i = 0; i < hosts.length; i++) {
> > +      let host = hosts[i];
> > +      let gateway = this.selectGateway(gateways, host);
> > +      if (gateway && host) {
> > +        gNetworkService.removeHostRoute(network.name, gateway, host);
> 
> I guess you miss Promise stuff.

Yes, will address this in the next version.

> 
> @@ +502,5 @@
> > +      if (gateway.match(this.REGEXP_IPV4) && host.match(this.REGEXP_IPV4) ||
> > +          gateway.indexOf(":") != -1 && host.indexOf(":") != -1) {
> > +        return gateway;
> > +      }
> > +    }
> 
> return null;

Will do.

> 
> @@ +526,5 @@
> >  
> >        let mmsHosts = this.resolveHostname([hostToResolve]);
> >        if (mmsHosts.length == 0) {
> >          debug("No valid hostnames can be added. Stop adding host route.");
> >          return;
> 
> I think we should return a resolved/rejected promise here.

Yes, will address this in the next version.

> 
> @@ +543,1 @@
> >      }
> 
> Ditto

Will do.

> 
> @@ +563,5 @@
> >  
> >        let mmsHosts = this.resolveHostname([hostToResolve]);
> >        if (mmsHosts.length == 0) {
> >          debug("No valid hostnames can be removed. Stop removing host route.");
> >          return;
> 
> Ditto

Will do.

> 
> @@ +580,1 @@
> >      }
> 
> Ditto

Will do.
Edgar, may I have your review again? Thanks.

Changes since v1 (address review comment 15):
- import Promise.jsm
- turn removeHostRoutes() into promise
- return resolved/rejected promise in setExtraHostRoute() and removeExtraHostRoute()
Attachment #8465233 - Attachment is obsolete: true
Attachment #8466891 - Flags: review?(echen)
Blocks: 992772
Comment on attachment 8465236 [details] [diff] [review]
Part 4: unify add/removeHostRoute and add/removeHostRouteWithResolve (NetworkUtils), v1.

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

Thanks for your work.
Please see my below comments.

::: dom/system/gonk/NetworkUtils.cpp
@@ +1135,5 @@
>      NetworkResultOptions result;
> +    result.mError = ret == SUCCESS ? false : true;
> +    result.mResultCode = ret;
> +    if (ret != SUCCESS) {
> +      result.mReason = NS_ConvertUTF8toUTF16(strerror(abs(ret)));

Why we need abs()?

@@ +1283,5 @@
>  
>  /**
>   * Set default route and DNS servers for given network interface.
>   */
>  bool NetworkUtils::setDefaultRouteAndDNS(NetworkParams& aOptions)

The declaration says the return type is bool, but we returns an int actually?

@@ +1320,5 @@
>      property_get(key, gateway, "");
>  
>      int type = getIpType(gateway);
>      if (type != AF_INET && type != AF_INET6) {
> +      return FAILURE;

How about returning an error number which is defined in errno.h here?
EINVAL seems a good one for this case.

@@ +1337,5 @@
>  
>  /**
>   * Remove default route for given network interface.
>   */
>  bool NetworkUtils::removeDefaultRoute(NetworkParams& aOptions)

Ditto, here and elsewhere

@@ +1345,5 @@
>      NS_ConvertUTF16toUTF8 autoGateway(aOptions.mGateways[i]);
>  
>      int type = getIpType(autoGateway.get());
>      if (type != AF_INET && type != AF_INET6) {
> +      return FAILURE;

Ditto, here and elsewhere
Attachment #8465236 - Flags: review?(echen)
Comment on attachment 8465235 [details] [diff] [review]
Part 3: unify add/removeHostRoute and add/removeHostRouteWithResolve (NetworkService), v1.

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

Please see my below comments, thank you.

::: dom/system/gonk/NetworkService.js
@@ +328,2 @@
>      let options = {
> +      cmd: action === "add" ? "addHostRoute" : "removeHostRoute",

How about just using boolean here?
and s/action/isAdd/

@@ +328,5 @@
>      let options = {
> +      cmd: action === "add" ? "addHostRoute" : "removeHostRoute",
> +      ifname: interfaceName,
> +      gateway: gateway,
> +      ip: host

For "addHostRoute" and "remoteHostRoute", we no longer use |hostnames| and |gateways|, could you help to revise the comments in NetworkOptions.webidl. And if no one use |hostnames| any more, maybe we could consider to remove it in this bug. Thank you.
Attachment #8465235 - Flags: review?(echen)
Comment on attachment 8466891 [details] [diff] [review]
Part 2: unify add/removeHostRoute and add/removeHostRouteWithResolve (NetworkManager), v2.

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

r=me with below comments addressed. Thank you.

::: dom/system/gonk/NetworkManager.js
@@ +534,5 @@
> +    }
> +
> +    let mmsHosts = this.resolveHostname([hostToResolve]);
> +    if (mmsHosts.length == 0) {
> +      let errMsg = "No valid hostnames can be removed. Stop removing host route."

s/removed/added/
s/removing/adding/
Attachment #8466891 - Flags: review?(echen) → review+
(In reply to Edgar Chen [:edgar][:echen] from comment #19)
> Comment on attachment 8465236 [details] [diff] [review]
> Part 4: unify add/removeHostRoute and add/removeHostRouteWithResolve
> (NetworkUtils), v1.
> 
> Review of attachment 8465236 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for your work.
> Please see my below comments.

Thanks for the review. :)

> 
> ::: dom/system/gonk/NetworkUtils.cpp
> @@ +1135,5 @@
> >      NetworkResultOptions result;
> > +    result.mError = ret == SUCCESS ? false : true;
> > +    result.mResultCode = ret;
> > +    if (ret != SUCCESS) {
> > +      result.mReason = NS_ConvertUTF8toUTF16(strerror(abs(ret)));
> 
> Why we need abs()?

The number returned by ifc_utils functions are sometimes negative, so we use abs() to ensure that the number passed to strerror() is positive.

> 
> @@ +1283,5 @@
> >  
> >  /**
> >   * Set default route and DNS servers for given network interface.
> >   */
> >  bool NetworkUtils::setDefaultRouteAndDNS(NetworkParams& aOptions)
> 
> The declaration says the return type is bool, but we returns an int actually?

Oh yes, should change all the return type to int32_t.

> 
> @@ +1320,5 @@
> >      property_get(key, gateway, "");
> >  
> >      int type = getIpType(gateway);
> >      if (type != AF_INET && type != AF_INET6) {
> > +      return FAILURE;
> 
> How about returning an error number which is defined in errno.h here?
> EINVAL seems a good one for this case.

Sounds good. Will do it in the next version.

> 
> @@ +1337,5 @@
> >  
> >  /**
> >   * Remove default route for given network interface.
> >   */
> >  bool NetworkUtils::removeDefaultRoute(NetworkParams& aOptions)
> 
> Ditto, here and elsewhere

Will do.

> 
> @@ +1345,5 @@
> >      NS_ConvertUTF16toUTF8 autoGateway(aOptions.mGateways[i]);
> >  
> >      int type = getIpType(autoGateway.get());
> >      if (type != AF_INET && type != AF_INET6) {
> > +      return FAILURE;
> 
> Ditto, here and elsewhere

Will do.
(In reply to Edgar Chen [:edgar][:echen] from comment #20)
> Comment on attachment 8465235 [details] [diff] [review]
> Part 3: unify add/removeHostRoute and add/removeHostRouteWithResolve
> (NetworkService), v1.
> 
> Review of attachment 8465235 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my below comments, thank you.
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +328,2 @@
> >      let options = {
> > +      cmd: action === "add" ? "addHostRoute" : "removeHostRoute",
> 
> How about just using boolean here?
> and s/action/isAdd/

Sounds good, will use 'doAdd' instead if it's okay with you.

> 
> @@ +328,5 @@
> >      let options = {
> > +      cmd: action === "add" ? "addHostRoute" : "removeHostRoute",
> > +      ifname: interfaceName,
> > +      gateway: gateway,
> > +      ip: host
> 
> For "addHostRoute" and "remoteHostRoute", we no longer use |hostnames| and
> |gateways|, could you help to revise the comments in NetworkOptions.webidl.
> And if no one use |hostnames| any more, maybe we could consider to remove it
> in this bug. Thank you.

Will address this in the next version. Thanks!
(In reply to Jessica Jong [:jjong] [:jessica] from comment #23)
> (In reply to Edgar Chen [:edgar][:echen] from comment #20)
> > Comment on attachment 8465235 [details] [diff] [review]
> > Part 3: unify add/removeHostRoute and add/removeHostRouteWithResolve
> > (NetworkService), v1.
> > 
> > Review of attachment 8465235 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please see my below comments, thank you.
> > 
> > ::: dom/system/gonk/NetworkService.js
> > @@ +328,2 @@
> > >      let options = {
> > > +      cmd: action === "add" ? "addHostRoute" : "removeHostRoute",
> > 
> > How about just using boolean here?
> > and s/action/isAdd/
> 
> Sounds good, will use 'doAdd' instead if it's okay with you.

Go for it. :)
Add f=vchang, r=edgar.
Attachment #8465232 - Attachment is obsolete: true
Attachment #8468324 - Flags: review+
Add r=edgar and revise debug message (comment 21).
Attachment #8466891 - Attachment is obsolete: true
Attachment #8468325 - Flags: review+
Edgar, may I have your review again?

Changes since v1:
- use boolean parameter to indicate adding or removing host route.
Attachment #8465235 - Attachment is obsolete: true
Attachment #8468330 - Flags: review?(echen)
Attachment #8468330 - Attachment description: Part 3: unify add/removeHostRoute and add/removeHostRouteWithResolve (NetworkService), v2. → Part 3.1: unify add/removeHostRoute and add/removeHostRouteWithResolve (NetworkService), v2.
Remove unused network parameter 'hostnames' and revise comments.
Attachment #8468331 - Flags: review?(echen)
Edgar, may I have your review again? Thanks.

Changes since v1:
- change function return type to int32_t
- return a defined error number on failure cases
Attachment #8465236 - Attachment is obsolete: true
Attachment #8468334 - Flags: review?(echen)
Comment on attachment 8468330 [details] [diff] [review]
Part 3.1: unify add/removeHostRoute and add/removeHostRouteWithResolve (NetworkService), v2.

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

Looks good to me, thank you.
Attachment #8468330 - Flags: review?(echen) → review+
Comment on attachment 8468331 [details] [diff] [review]
Part 3.2: remove unused NetworkParams and refine comments, v1.

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

Thank you. :)
This patch touches webidl interface, please remember to invite DOM peer for the review.
Attachment #8468331 - Flags: review?(echen) → review+
Comment on attachment 8468334 [details] [diff] [review]
Part 4: unify add/removeHostRoute and add/removeHostRouteWithResolve (NetworkUtils), v2.

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

Thank you. :)

::: dom/system/gonk/NetworkUtils.cpp
@@ +78,5 @@
>  
>  static const uint32_t BUF_SIZE = 1024;
>  
> +static const int32_t SUCCESS = 0;
> +static const int32_t FAILURE = 1;

We no longer need this const, please remove it.

@@ +1092,5 @@
>      NetworkResultOptions result;
> +    result.mError = ret == SUCCESS ? false : true;
> +    result.mResultCode = ret;
> +    if (ret != SUCCESS) {
> +      result.mReason = NS_ConvertUTF8toUTF16(strerror(abs(ret)));

Please add some comments about why we need abs(). Thank you.

::: dom/system/gonk/NetworkUtils.h
@@ +111,5 @@
>      COPY_OPT_FIELD(mPrefixLength, 0)
>      COPY_OPT_STRING_FIELD(mOldIfname, EmptyString())
>      COPY_OPT_STRING_FIELD(mMode, EmptyString())
>      COPY_OPT_FIELD(mReport, false)
> +    COPY_OPT_FIELD(mIsAsync, false)

Hmm, Having a isAsync attribute in NetworkOptions seems a little bit wried to me.
IMHO, NetworkService should not need to worry about the underlying operation is async (netd) or sync (libnetutils). This should be handled inside NetworkUtils. We can do this in a follow-up.
Attachment #8468334 - Flags: review?(echen) → review+
Thanks for the review, Edgar!

(In reply to Edgar Chen [:edgar][:echen] from comment #32)
> Comment on attachment 8468334 [details] [diff] [review]
> Part 4: unify add/removeHostRoute and add/removeHostRouteWithResolve
> (NetworkUtils), v2.
> 
> Review of attachment 8468334 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you. :)
> 
> ::: dom/system/gonk/NetworkUtils.cpp
> @@ +78,5 @@
> >  
> >  static const uint32_t BUF_SIZE = 1024;
> >  
> > +static const int32_t SUCCESS = 0;
> > +static const int32_t FAILURE = 1;
> 
> We no longer need this const, please remove it.

Will do.

> 
> @@ +1092,5 @@
> >      NetworkResultOptions result;
> > +    result.mError = ret == SUCCESS ? false : true;
> > +    result.mResultCode = ret;
> > +    if (ret != SUCCESS) {
> > +      result.mReason = NS_ConvertUTF8toUTF16(strerror(abs(ret)));
> 
> Please add some comments about why we need abs(). Thank you.

Will do.

> 
> ::: dom/system/gonk/NetworkUtils.h
> @@ +111,5 @@
> >      COPY_OPT_FIELD(mPrefixLength, 0)
> >      COPY_OPT_STRING_FIELD(mOldIfname, EmptyString())
> >      COPY_OPT_STRING_FIELD(mMode, EmptyString())
> >      COPY_OPT_FIELD(mReport, false)
> > +    COPY_OPT_FIELD(mIsAsync, false)
> 
> Hmm, Having a isAsync attribute in NetworkOptions seems a little bit wried
> to me.
> IMHO, NetworkService should not need to worry about the underlying operation
> is async (netd) or sync (libnetutils). This should be handled inside
> NetworkUtils. We can do this in a follow-up.

Totally agree! The work in Bug 1038531 is trying to separate native functions and netd functions, so after that, I think we can get rid of the 'isAsync' flag.
Comment on attachment 8468331 [details] [diff] [review]
Part 3.2: remove unused NetworkParams and refine comments, v1.

Hi Olli, we are just cleaning up some unused parameters/comments. May I have your review? Thanks.
Attachment #8468331 - Flags: review+ → review?(bugs)
Attachment #8468331 - Flags: review?(bugs) → review+
Add r=edgar,smaug.
Attachment #8468331 - Attachment is obsolete: true
Attachment #8469917 - Flags: review+
Changes since v2: 
- Address review comment 32:
  a. remove unused const FAILURE
  b. add comment for why we need abs()
Attachment #8468334 - Attachment is obsolete: true
Attachment #8469922 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=f68cc306172a

try green!
Keywords: checkin-needed
Whiteboard: [p=3]
Target Milestone: --- → 2.1 S2 (15aug)
[Blocking Requested - why for this release]:
1. In comment 86 of Bug 1031656(https://bugzilla.mozilla.org/show_bug.cgi?id=1031656#c86),
   We saw bad UX of our product of receiving MMS in the Free Mobile.
2. To Fix Bug 1031656, bug 1032097 & its dependency bug 1043114 is required.
3. ZTE device was sold in v1.3, so a solution for v1.3 has been requested for their MR release.
4. Set 1.4? to see the necessity of uplifting fix in 1.4 branch.

Note: patch for 1.3v, 1.4v, 2.0v has to been created accordingly instead of landing the fix in m-c directly.
blocking-b2g: --- → 1.4?
only Free@French hit this case, to reduce the risk, cancel 1.4?
blocking-b2g: 1.4? → ---
You need to log in before you can comment on or make changes to this bug.