Closed Bug 1201778 Opened 5 years ago Closed 2 years ago

[GPS-SUPL NI] Support for displaying notification message to user

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: alchen, Assigned: alchen)

References

Details

Attachments

(6 files, 13 obsolete files)

5.04 KB, patch
Details | Diff | Splinter Review
92.08 KB, image/jpeg
Details
104.16 KB, image/jpeg
Details
93.61 KB, image/jpeg
Details
13.09 KB, patch
Details | Diff | Splinter Review
23.67 KB, patch
Details | Diff | Splinter Review
This bug is mainly about this feature : displaying notification message to user.


From bug 853703 comment 28,
"requestor_id" and "text" are received from the modem and needs to be displayed to the user in the notification message. Both the above fields are encoded and need to be decoded as per the "GpsNiEncodingType".

/** Represents an NI request */
typedef struct {

    ...

    /**
     * Requestor ID
     */
    char            requestor_id[GPS_NI_SHORT_STRING_MAXLEN];

    /**
     * Notification message. It can also be used to store client_id in some cases
     */
    char            text[GPS_NI_LONG_STRING_MAXLEN];

    /**
     * Client name decoding scheme
     */
    GpsNiEncodingType requestor_id_encoding;

    /**
     * Client name decoding scheme
     */
    GpsNiEncodingType text_encoding;

    ...

} GpsNiNotification;
Depends on: 853703
For this bug, I will divide the work into two parts.
1. Partial support for displaying notification message to user.(UTF8 and UCS2)
    ETA : 10/5
2. Support GSM_DEFAULT encoding.
    ETA : 10/19
As discussed in mail, I will also integrate the following in this patch.
1. Change the way to know the scenario of this event : Use flag as a mask instead of enum.
2. Add GPS_NI_PRIVACT_OVERRIDE scenario.
Hi Bhavna,
we would like to know how to show "notification->text" and "notification->requestor_id" to user.

For example,
1. [requestor_id] text
2. requestor_id : text

Could you tell us your expectation?
Flags: needinfo?(sbhavna)
Hi Alphan,

On Android it seems this format string is used

<string name="gpsNotifTicker" msgid="5622683912616496172">"Location request from <xliff:g id="NAME">%s</xliff:g>"</string>
<string name="gpsNotifTitle" msgid="5446858717157416839">"Location request"</string>
<string name="gpsNotifMessage" msgid="1374718023224000702">"Requested by <xliff:g id="NAME">%1$s</xliff:g> (<xliff:g id="SERVICE">%2$s</xliff:g>)"</string>

And the call is made in this way from java >>
String message = String.format(context.getString(R.string.gpsNotifMessage),
               decodeString(notif.requestorId, mIsHexInput, notif.requestorIdEncoding),
               decodeString(notif.text, mIsHexInput, notif.textEncoding));

I am not aware of any carrier requirements for it to be in some specific format.
Flags: needinfo?(sbhavna)
Group: qualcomm-confidential
Alphan,

Regarding the email from Hema, i have a few quick questions

1. How are you'll going to handle the bitmask case i.e. if both GPS_NI_NEED_VERIFY and GPS_NI_NEED_NOTIFY is set in the flags ? Will you'll show both the dialog and the notification ?
2. And for GPS_NI_PRIVACY_OVERRIDE the default response has to be ACCEPT. As i know there is no need to notify or verify with the user.

I just want to confirm the expected behavior.
(In reply to Bhavna Sharma from comment #5)
> Alphan,
> 
> Regarding the email from Hema, i have a few quick questions
> 
> 1. How are you'll going to handle the bitmask case i.e. if both
> GPS_NI_NEED_VERIFY and GPS_NI_NEED_NOTIFY is set in the flags ? Will you'll
> show both the dialog and the notification ?
Yes, there are notification in status bar and dialog in this case.

> 2. And for GPS_NI_PRIVACY_OVERRIDE the default response has to be ACCEPT. As
> i know there is no need to notify or verify with the user.
> 
Yes, it will just reply accept in this scenario.

> I just want to confirm the expected behavior.
Please review the patch once it is ready.
Thanks.
Hi Alphan

Sorry to bother you again, I am wondering why we cannot have the timeout handling in the UI as well ?
With the current patchsets, the gonkGPSGeolocationProvider setups up a timer handler, and if it expires, it calls back into the chrome with a 'supl-ni-verify-timeout' notification. Instead the UI itself could take the timeout, show the notification for the given time and send back default response if there is no response. Let me know if it works for you.
After completing the support of displaying notification message, gecko can pass multiple information to gaia. So you can add extra functionality or pass more thing to gaia by the same way. As a result, we will finish it first.



(In reply to Bhavna Sharma from comment #7)
> Hi Alphan
> 
> Sorry to bother you again, I am wondering why we cannot have the timeout
> handling in the UI as well ?
We can do that. However, currently we choose another way.
> With the current patchsets, the gonkGPSGeolocationProvider setups up a timer
> handler, and if it expires, it calls back into the chrome with a
> 'supl-ni-verify-timeout' notification. Instead the UI itself could take the
> timeout, show the notification for the given time and send back default
> response if there is no response. Let me know if it works for you.
Is this a hard-requirement? If yes, please tell us the reason.
Besides, is there any other requirement like this?
Could you list them for us at a time?
Hi Harly,

In this bug, we're going to address the additional information, "requestor_id" and "text", that are given from the cell tower. 

According to comment 4, the message on Android is currently displayed like below:

    Title: Location request
    Message: Requested by <requestor_id> (<text>)

And our design is like bug 853703 comment 32.

About the way to append this information to our current design, it would be much helpful if you can give us some advice. (Note that the fields, "requestor_id" and "text", may be omitted.)

Thanks a lot.
Flags: needinfo?(hhsu)
(In reply to Alphan Chen [:alchen] from comment #8)
> After completing the support of displaying notification message, gecko can
> pass multiple information to gaia. So you can add extra functionality or
> pass more thing to gaia by the same way. As a result, we will finish it
> first.
> 
> 
> 
> (In reply to Bhavna Sharma from comment #7)
> > Hi Alphan
> > 
> > Sorry to bother you again, I am wondering why we cannot have the timeout
> > handling in the UI as well ?
> We can do that. However, currently we choose another way.
> > With the current patchsets, the gonkGPSGeolocationProvider setups up a timer
> > handler, and if it expires, it calls back into the chrome with a
> > 'supl-ni-verify-timeout' notification. Instead the UI itself could take the
> > timeout, show the notification for the given time and send back default
> > response if there is no response. Let me know if it works for you.
> Is this a hard-requirement? If yes, please tell us the reason.
> Besides, is there any other requirement like this?
> Could you list them for us at a time?

There is no hard requirement.
Attached patch (WIP) phase 1 (obsolete) — Splinter Review
This patch includes phase 1 and comment 6.

Phase 1. Partial support for displaying notification message to user.(UTF8 and UCS2)
Assignee: nobody → alchen
Status: NEW → ASSIGNED
(In reply to Bhavna Sharma from comment #5)
> 1. How are you'll going to handle the bitmask case i.e. if both
> GPS_NI_NEED_VERIFY and GPS_NI_NEED_NOTIFY is set in the flags ? Will you'll
> show both the dialog and the notification ?

I just heard that GPS_NI_NEED_VERIFY always comes with GPS_NI_NEED_NOTIFY. If so, it should be unnecessary to amend our current design. I suggest to simply ignore the GPS_NI_NEED_NOTIFY flag and treat it as a single GPS_NI_NEED_VERIFY case.

In Gaia, for now, the verification event will always trigger a notification as well when device is idle (say, screen is locked or turned off). If we try to invoke both events at the same time, there will be two notifications on the lock screen and utility tray. Users may be confused.
(In reply to Luke Chang [:lchang] from comment #12)
> (In reply to Bhavna Sharma from comment #5)
> > 1. How are you'll going to handle the bitmask case i.e. if both
> > GPS_NI_NEED_VERIFY and GPS_NI_NEED_NOTIFY is set in the flags ? Will you'll
> > show both the dialog and the notification ?
> 
> I just heard that GPS_NI_NEED_VERIFY always comes with GPS_NI_NEED_NOTIFY.
> If so, it should be unnecessary to amend our current design. I suggest to
> simply ignore the GPS_NI_NEED_NOTIFY flag and treat it as a single
> GPS_NI_NEED_VERIFY case.
> 
> In Gaia, for now, the verification event will always trigger a notification
> as well when device is idle (say, screen is locked or turned off). If we try
> to invoke both events at the same time, there will be two notifications on
> the lock screen and utility tray. Users may be confused.

Yes I would agree to that
Comment on attachment 8670674 [details] [diff] [review]
(WIP) phase 1

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +354,4 @@
>    };
>  
> +  bool needNotify = (flags & GPS_NI_NEED_NOTIFY) != 0;
> +  if (needNotify) {

I think this should be for needVerify only, there is no need to setup timer for needNotify.
Comment on attachment 8670674 [details] [diff] [review]
(WIP) phase 1

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

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +237,5 @@
> +{
> +  nsString wide_str;
> +  if (encodingType == GPS_ENC_SUPL_UTF8) {
> +    nsCString str(message);
> +    str.ReplaceSubstring("\\", "\\\\");

Why do you need the ReplaceSubstring call, it seems to be an obsolete API, not accessible from proprietary code.
Hi Alphan,

I guess there should be a new gaia patch as well right to show the notification as given from network.
Please share that as well.
Hi, below is the modify text for NI with the additional info:

NI Notify:
Dialog title: 
Location Request

Dialog content: 
Your location is being transmitted to the operator.
Message: <requestor_id> (<text>

NI Verify:
Dialog title: 
Location Request

Dialog content: 
The operator is requesting your location. Do you want to provide your location info to the operator?
Message: <requestor_id> (<text>
Flags: needinfo?(hhsu)
(In reply to Bhavna Sharma from comment #14)
> Comment on attachment 8670674 [details] [diff] [review]
> (WIP) phase 1
> ::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
> @@ +354,4 @@
> >    };
> >  
> > +  bool needNotify = (flags & GPS_NI_NEED_NOTIFY) != 0;
> > +  if (needNotify) {
> 
> I think this should be for needVerify only, there is no need to setup timer
> for needNotify.
Yes, will update a new patch.


> > +{
> > +  nsString wide_str;
> > +  if (encodingType == GPS_ENC_SUPL_UTF8) {
> > +    nsCString str(message);
> > +    str.ReplaceSubstring("\\", "\\\\");

> Why do you need the ReplaceSubstring call, it seems to be an obsolete API, not accessible from 
> proprietary code.
What do you mean? It is a method of nsString.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Glue_classes/nsString#ReplaceSubstring

> I guess there should be a new gaia patch as well right to show the notification as given from network.
> Please share that as well.
ni Luke for gaia patch.
Flags: needinfo?(lchang)
Attached patch (WIP-1008) phase 1 (obsolete) — Splinter Review
Update the patch of phase 1.
Attachment #8670674 - Attachment is obsolete: true
Attached patch WIP patch - Gaia part (obsolete) — Splinter Review
Here is the Gaia patch. Thanks.
Flags: needinfo?(lchang)
Hi Alphan,

The requester-id and text string received in GpsNiNotification structure is a hexadecimal representation of the actual string. For example if requester-id is "Verizon" it will be received as "566572697a6f6e"
Hence before calling DecodeString it has to converted into its ASCII representation, else its hexadecimal format will get printed on the UI notification. This is what is happening now for both requester-id and client name.

Here is where the ASCII is converted to hex string when filling into the GpsNiNotification structure.
https://www.codeaurora.org/cgit/quic/la/platform/vendor/qcom-opensource/location/tree/loc_api/loc_api_v02/LocApiV02.cpp#n2252
Regarding your comment 18 on ReplaceSubstring, the issue is only API's in nsStringAPI.h are available to us i.e. gecko/xpcom/glue/nsStringAPI.h which does not have ReplaceSubstring. You might be able to use it in your implementation of GonkGPSGeolocationProvider, but we will not be able to use it in our proprietary code.
(In reply to Bhavna Sharma from comment #21)
> Hi Alphan,
> 
> The requester-id and text string received in GpsNiNotification structure is
> a hexadecimal representation of the actual string. For example if
> requester-id is "Verizon" it will be received as "566572697a6f6e"
> Hence before calling DecodeString it has to converted into its ASCII
> representation, else its hexadecimal format will get printed on the UI
> notification. This is what is happening now for both requester-id and client
> name.
> 
> Here is where the ASCII is converted to hex string when filling into the
> GpsNiNotification structure.
> https://www.codeaurora.org/cgit/quic/la/platform/vendor/qcom-opensource/
> location/tree/loc_api/loc_api_v02/LocApiV02.cpp#n2252

Could you show me the three results of verizon?

If the client name is "verizon" and the decode format is
1. UTF8,
2. UTF16,
3. GSM_DEFAULT
What is the actual string in GpsNiNotification structure?
Flags: needinfo?(sbhavna)
For "verizon" actual string got in the GpsNiNotification structure is

UTF8 : 766572697a6f6e
UTF16: 760000006500000072000000690000007A0000006F0000006E000000

I am not familiar with GSM_DEFAULT encoding, but per my understanding whatever be the byte stream it is just converted to hex string with this algorithm
https://www.codeaurora.org/cgit/quic/la/platform/vendor/qcom-opensource/location/tree/loc_api/loc_api_v02/LocApiV02.cpp#n2252
Flags: needinfo?(sbhavna)
My apologies on comment 24. 
In UTF16 the hex representation will be : 76006500720069007A006F006E00
Here is the latest version.

I test the patch with the following test case.
It can work.

1. UTF-8
// verizon
const char* testing_id = "766572697a6f6e";
// Verizon is doing the test
const char* testing_str = "566572697A6F6E20697320646F696E67207468652074657374";


2. UTF-16
// Verizon-UTF16
const char* testing_id = "56006500720069007A006F006E002D0055005400460031003600";
// Verizon-UTF16 is doing the test
const char* testing_str = "56006500720069007A006F006E002D0055005400460031003600200069007300200064006F0069006E006700200074006800650020007400650073007400";

However, I cannot test the GSM_DEFAULT encode.
Need your help.
Attachment #8671132 - Attachment is obsolete: true
Is it possible to receive this code in a way that the same GeolocationUtil functions can be used directly from our proprietary code ?
Hi Alphan,

I would prefer calling into some OS provided API to do the GSM_DEFAULT decoding. Can you please provide it as an API, just like we have string utils etc.
Clubbing the comment 27 & comment 28 (sorry for the half-baked comments) I think just like we have helper classes to convert from utf-8 to utf-16 or vice versa, i.e. NS_ConvertUTF8toUTF16  or NS_ConvertUTF16toUTF8, I need a similar class to decode GSM_DEFAULT format, rather than copying over the DecodeGsmdefaultToString code to proprietary code. IMO these are OS provided API's and should not be duplicated in proprietary implementation.
(In reply to Bhavna Sharma from comment #29)
> Clubbing the comment 27 & comment 28 (sorry for the half-baked comments) I
> think just like we have helper classes to convert from utf-8 to utf-16 or
> vice versa, i.e. NS_ConvertUTF8toUTF16  or NS_ConvertUTF16toUTF8, I need a
> similar class to decode GSM_DEFAULT format, rather than copying over the
> DecodeGsmdefaultToString code to proprietary code. IMO these are OS provided
> API's and should not be duplicated in proprietary implementation.

First of all, we don't expect GonkGPSGeolocationProvider.cpp are totally replaced.

I don't think our reviewer will allow me to add this kind of helper class.
Since GSM_DEFAULT decoding is not for normal usage, we only see this decoding in RIL related code. For urf-8 and utf-16 converting, they are used commonly. That's why we have a helper class.
(In reply to Bhavna Sharma from comment #29)
> IMO these are OS provided
> API's and should not be duplicated in proprietary implementation.

There is another way you can do.
However, as usual I don't think our reviewer will agree to add this kind of code in our codebase.

[Step]
1. Add .cpp and .h in "xpcom/glue/"
   e.g. GeolocationUtil.cpp and GeolocationUtil.h
2. Modify "xpcom/glue/moz.build"
 EXPORTS += [                                                                   
+    'GeolocationUtil.h',                                              
     'MainThreadUtils.h',                                                       
                                                                                                  
@@ -86,16 +87,17 @@ EXPORTS.mozilla.threads +=
 UNIFIED_SOURCES += [                                                           
     'GenericModule.cpp',                                                       
+    'GeolocationUtil.cpp',                                            
     'nsStringAPI.cpp',                                                         
 ]                                                                              
3. Then include this header for your usage.
(In reply to Alphan Chen [:alchen] from comment #31)
> (In reply to Bhavna Sharma from comment #29)
> > IMO these are OS provided
> > API's and should not be duplicated in proprietary implementation.
> 
> 
> [Step]
> 1. Add .cpp and .h in "xpcom/glue/"
>    e.g. GeolocationUtil.cpp and GeolocationUtil.h

More detail...
Something like below...


@GeolocationUtil.h
class GeolocationUtil
{
public:
  static nsString DecodeGsmdefaultToString(const char* message, int offset, int lengthSeptets);
};
Hi Alphan,

GonkGPSGeolocationProvider does get replaced completely in QC's implementation. I have not been able to verify your GSM_DEFAULT decode code, i'll get back on this.

Meanwhile the test team has reported a UI issue, can you please look into it.
If there is a voice call active at the same time when a verification request comes in, the verification dialog gets hidden by the voice call and the user is unable to accept /deny the request. If its just a notification request it is fine, since it just shows in the notification bar, but this behavior is not acceptable for verification requests.
Comment on attachment 8673550 [details] [diff] [review]
(WIP-1014) [GPS-SUPL-NI] Support for displaying notification message and requestor_id to user

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

::: dom/system/gonk/GeolocationUtil.cpp
@@ +18,5 @@
> +    output.AssignLiteral("Failed to decode string.");
> +    return output;
> +  }
> +
> +  const char* languageTableToChar =

The table should be defined as nsString as they are wide string not char. For example:   nsString languageTableToCharW = NS_LITERAL_STRING(
    /* 3GPP TS 23.038 V9.1.1 section 6.2.1 - GSM 7 bit Default Alphabet
     01.....23.....4.....5.....6.....7.....8.....9.....A.B.....C.....D.E.....F.....0.....1 */
    "@\u00a3$\u00a5\  ................ etc");

@@ +41,5 @@
> +      "\u00bfabcdefghijklmno"
> +    // 0123456789AB.....C.....D.....E.....F.....
> +      "pqrstuvwxyz\u00e4\u00f6\u00f1\u00fc\u00e0"
> +  };
> +  const char* shiftTableToChar =

Same as above define table as nsString

@@ +116,5 @@
> +    return wide_str;
> +  }
> +
> +  int byteArrayLen = (hexstring_length / 2);
> +  if (encodingType == GPS_ENC_SUPL_UTF8) {

add encodingType || GPS_ENC_SUPL_GSM_DEFAULT

@@ +155,5 @@
> +    int lengthSeptets = (lengthBytes*8)/7;
> +
> +    if (lengthBytes % 7 == 0) {
> +      char PADDING_CHAR = 0x00;
> +      if(message[lengthBytes-1]>>1 == PADDING_CHAR) {

use byteArray to decide the lengthSeptet not message
These are some initial comments, I have corrected these in my code as well.
Testing it more, will get back if I see any other issues.
(In reply to Bhavna Sharma from comment #33)
> 
> Meanwhile the test team has reported a UI issue, can you please look into it.
> If there is a voice call active at the same time when a verification request
> comes in, the verification dialog gets hidden by the voice call and the user
> is unable to accept /deny the request. If its just a notification request it
> is fine, since it just shows in the notification bar, but this behavior is
> not acceptable for verification requests.

Hi Luke,
please help on this part.
Thanks.
Flags: needinfo?(lchang)
Update the patch according comment 34.
Attachment #8673550 - Attachment is obsolete: true
I've updated my patch according to comment 36. For now, the verification event triggers a notification instead during a phone call and will hide the callscreen when you tap it.
Attachment #8672450 - Attachment is obsolete: true
Flags: needinfo?(lchang)
Thanks Luke, I have given your gaia patchset to test team to verify. Will let you know the results.

Alphan,
Coming back to my question on ReplaceSubstring. As I understand you'll need the ReplaceSubstring to account for any comma in the message itself, as the gecko breaks the notification-id, requestor-id and text based on the comma. Is that correct ?
As I mentioned before we do not have access to the ReplaceSubstring function in the proprietary code, and it is unnecessarily expensive iteration to insert \\ in the string. Can we not just pass an array of string to the gecko module i.e. data[0] = notification-id, data[1] = requestor-id and data[2] = text and so on, rather than inserting comma's and then doing a replace call.
(In reply to Bhavna Sharma from comment #39)
> Thanks Luke, I have given your gaia patchset to test team to verify. Will
> let you know the results.
> 
> Alphan,
> Coming back to my question on ReplaceSubstring. As I understand you'll need
> the ReplaceSubstring to account for any comma in the message itself, as the
> gecko breaks the notification-id, requestor-id and text based on the comma.
> Is that correct ?

Here is current design.
Gecko use a chrome event to send message to gaia.

I.
obs->NotifyObservers(nullptr, SUPL_NI_TYPE, wide_str.BeginReading());

II.
@b2g/chrome/content/shell.js
     shell.sendChromeEvent({
       type: 'supl-notification',
       data: aData
     });

III.
The message is composed by "notification_id, notification_text, notification_requestor_id".

> As I mentioned before we do not have access to the ReplaceSubstring function
> in the proprietary code, and it is unnecessarily expensive iteration to
> insert \\ in the string. Can we not just pass an array of string to the
> gecko module i.e. data[0] = notification-id, data[1] = requestor-id and
> data[2] = text and so on, rather than inserting comma's and then doing a
> replace call.

In current design, gaia cannot read the array of string.
If we can know a token that would not exist in notification_text or notification_requestor_id, we can use the token without doing ReplaceSubstring.

How about using "\," as the separate token?
(e.g. notification_id \, notification_text \, notification_requestor_id)
Flags: needinfo?(sbhavna)
Alphan, on second thoughts I think its fine the way it is now. I have implemented the functions to do the replace in my code. I think you need to do the same for GSM_DEFAULT case as well in your code.
Flags: needinfo?(sbhavna)
Luke,

During the voice call, we noticed that after the user accepts/denies the location request message, the screen comes back to home screen and the on-going voice call is not visible on the notification bar.
This might be little confusing to the user. I have attached the screen-shots in the bug.
(In reply to Bhavna Sharma from comment #45)
> During the voice call, we noticed that after the user accepts/denies the
> location request message, the screen comes back to home screen and the
> on-going voice call is not visible on the notification bar.
> This might be little confusing to the user. I have attached the screen-shots
> in the bug.

It has nothing to do with the SUPL dialog. By design, an attention window (the callscreen in this case) will dismiss its notification toast automatically and only show in the utility tray after you hide it for a while. You can check the same behavior by clicking HOME button during a phone call.

And it's also by design that we won't resume an hidden attention window unless user clicks the notification in the utility tray.
Update the patch for the lack of replacing in GSM_DEFAULT case.
Attachment #8675580 - Attachment is obsolete: true
Thanks Luke, I guess its fine in that case.

Alphan,

Different carriers have different requirements for location request triggered by SUPL when location mode is on/off. There is a GPS_LOCK setting in /system/etc/gps.conf which can have the following values

# Below bit mask configures how GPS functionalities
# should be locked when user turns off GPS on Settings
# Set bit 0x1 if MO GPS functionalities are to be locked
# Set bit 0x2 if NI GPS functionalities are to be locked
# default - non is locked for backward compatibility
#GPS_LOCK = 0

As of today the default value is 0 which means even if Location mode is OFF, SUPL location request should still be handled. But some carriers may want to set this to 0x2, which means even SUPL requested location must be turned off if location mode is OFF. What is Mozilla's plan on delivering this config for different carriers ?
(In reply to Bhavna Sharma from comment #48)
> Alphan,
> 
> Different carriers have different requirements for location request
> triggered by SUPL when location mode is on/off. There is a GPS_LOCK setting
> in /system/etc/gps.conf which can have the following values
> 
> # Below bit mask configures how GPS functionalities
> # should be locked when user turns off GPS on Settings
> # Set bit 0x1 if MO GPS functionalities are to be locked
> # Set bit 0x2 if NI GPS functionalities are to be locked
> # default - non is locked for backward compatibility
> #GPS_LOCK = 0
> 
> As of today the default value is 0 which means even if Location mode is OFF,
> SUPL location request should still be handled. But some carriers may want to
> set this to 0x2, which means even SUPL requested location must be turned off
> if location mode is OFF. What is Mozilla's plan on delivering this config
> for different carriers ?

Bhavna,
Is gps.conf common in every vendor? Or it is just for QCT solution.
Is there any api we can use to get this preference?
(In reply to Bhavna Sharma from comment #48)
> Thanks Luke, I guess its fine in that case.
> 
> Alphan,
> 
> Different carriers have different requirements for location request
> triggered by SUPL when location mode is on/off. There is a GPS_LOCK setting
> in /system/etc/gps.conf which can have the following values
> 
> 
> As of today the default value is 0 which means even if Location mode is OFF,
> SUPL location request should still be handled. But some carriers may want to
> set this to 0x2, which means even SUPL requested location must be turned off
> if location mode is OFF. What is Mozilla's plan on delivering this config
> for different carriers ?

Since you said that it is a carrier requirement, the value may be different when capturing a different carrier network.
When do you set the value of "GPS_LOCK"? Every-time we capture a network?
Moreover, I tried to build L of AOSP by myself. (I can get root permission)
http://developer.sonymobile.com/knowledge-base/open-source/open-devices/aosp-build-instructions/how-to-build-aosp-lollipop-for-unlocked-xperia-devices/

However, the radio functionality is abnormal at that time.
So there is no output at that try.
GPS_LOCK is to be set everytime geolocation is turned off/on. But before we get into implementing it, I want to know if this is required for this SBA release. Can you'll help check with TCL or I am not sure who has to take the call ?
(In reply to Bhavna Sharma from comment #52)
> GPS_LOCK is to be set everytime geolocation is turned off/on. But before we
> get into implementing it, I want to know if this is required for this SBA
> release. Can you'll help check with TCL or I am not sure who has to take the
> call ?

Hi Bhavna,
I prefer to open another bug for "GPS_LOCK".
Hi Bhavna:

Before reaching to TCL, is it worth clarifying more about "GPS_LOCK"? For example isn't it something OEM can customize per different operator requirement? And is this part of SUPL 2.0 requirement?... (I'm not expert of this area but seems it has enough work to have a separate bug then this one?)

Back to your question, I'm assuming the SBA is for TCL, and there is official QCT-TCL channel in place (should be TCL *US team* as my understanding), which might be your preferred way to check? I have contact w/ TCL *China team* and can link you together (eg. add him in this bugzilla), please feel free to let me know if it works for you better.


(In reply to Bhavna Sharma from comment #52)
> GPS_LOCK is to be set everytime geolocation is turned off/on. But before we
> get into implementing it, I want to know if this is required for this SBA
> release. Can you'll help check with TCL or I am not sure who has to take the
> call ?
Flags: needinfo?(sbhavna)
Hi Wesly,

As per QC PM, GPS_LOCK configuration is very important, as it hits the privacy of the user. 
You are right , this is not a SUPL 2.0 requirement but more of a carrier requirement to not allow locations for SUPL if location mode itself is OFF. 

I am fine with you'll having a separate bug on this, but IMO Mozilla should understand the issues that OEM's will face with having to maintain different images for every carrier for each GPS_LOCK config, and  come up with a way to load the GPS config dynamically based on SIM card.

I will reach out to my PM regarding communicating this to TCL.
Flags: needinfo?(sbhavna)
Alphan / Wesley,

So actually there are 2 aspects of this
1.The need for GPS_LOCK itself i.e. to pass the required GPS_LOCK to modem when location mode is turned OFF.
2.The need for loading the gps config dynamically i.e. 
loading the GPS_LOCK based on the SIM card (i.e. every carrier may have their own GPS_LOCK setting)
Say if SIM card is changed, and each carrier had their own GPS_LOCK config, then a hard coded GPS_LOCK set in gps.conf will not work.

While 1> is simpler where we just need to pass the required GPS_LOCK to modem when location is turned OFF and then reset the lock when location mode is turned back ON.
<<Pasting from email>>
The way it is handled in Android is to call loc_cleanup (https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/gps/tree/loc_api/libloc_api_50001/loc.cpp?h=LA.BR.1.2.6_rb1.2#n351) when Geolocation is turned OFF and call loc_init (https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/gps/tree/loc_api/libloc_api_50001/loc.cpp?h=LA.BR.1.2.6_rb1.2#n316) again when Geolocation is turned back ON. The loc_init resets the GPS_LOCK and loc_cleanup sets the required GPS_LOCK.

But for 2> , this is needed so that OEM's do not have to care about setting a different GPS_LOCK as per carrier requirement. This support is needed from OS layer. 

Please open separate bugs for each, so that each can be discussed separately.
I've opened two bugs for these two aspects.

> 1.The need for GPS_LOCK itself i.e. to pass the required GPS_LOCK to modem
> when location mode is turned OFF.
Bug 1220022.
> 2.The need for loading the gps config dynamically i.e. 
> loading the GPS_LOCK based on the SIM card (i.e. every carrier may have
> their own GPS_LOCK setting)
Bug 1220024.
What is the status of current SUPL NI implementation?
Flags: needinfo?(sbhavna)
Thanks Alphan.

The state of the current NI implementation looks good so far.

Although there is one Verizon related test case, that our team is trying to find if it is mandated by VzW. Since this release does not have VoLTE support, there is a E911 scenario over CDMA which happens in the modem itself. But with this when the GPS engine is started in the modem to get the location, it seems it is mandated by the Vzw carrier that when the GPS engine is started, it is required that the HLOS shows a location tracking icon is shown in the notification bar.

Our test team is still trying to find out if this test is mandated. I will get back as soon as I have this information. If it is mandated, i think we will need a way to let Gecko/Gaia know that a location tracking icon be shown.
Flags: needinfo?(sbhavna)
Hi Alphan,

For UCS2 decoding we also have to take care of the BOM(Byte Order Mark)
On our test setups the encoded strings are coming in with the BOM (FEFF)

i.e. if the input string is : R_LN_abcd%&~
Encoded string appears as :  FEFF0052005F004C004E005F006100620063006400250026007E
where FEFF is the BOM.
Also, regarding Comment 59, our test team is still trying to check with Vzw if that test is mandate.
I would recommend you open a separate bug for this requirement and atleast estimate the work required.
i.e. the work required to show a location tracking icon when a GPS engine is started / stopped.

Please note this session is started in the GNSS engine itself, hence the nsGeolocationService has no clue of it.
(In reply to Bhavna Sharma from comment #60)
> Hi Alphan,
> 
> For UCS2 decoding we also have to take care of the BOM(Byte Order Mark)
> On our test setups the encoded strings are coming in with the BOM (FEFF)
> 
> i.e. if the input string is : R_LN_abcd%&~
> Encoded string appears as : 
> FEFF0052005F004C004E005F006100620063006400250026007E
> where FEFF is the BOM.

There is no "BOM" handling in Android implementation.
Could you make sure about this part?
I assume we should do the same thing as Android.
http://osxr.org/android/source/frameworks/base/telephony/java/com/android/internal/telephony/GsmAlphabet.java#0426
On Android it seems the Java String API's itself take care of the BOM.

String decoded = "";
decoded = new String(input, "UTF-16");, where the input string is the hex encoded string.
Alphan,

Is there a way to show / hide the location tracking icon from GonkGPSGeolocationProvider ?
(In reply to Bhavna Sharma from comment #64)
> Alphan,
> 
> Is there a way to show / hide the location tracking icon from
> GonkGPSGeolocationProvider ?

We should be able to show the location tracking icon when using location now. (e.g MAP)
The icon should be combined with nsGeolocationService.

Do we need to show the icon when there is SUPL NI?
Alphan,

The location icon is not for SUPL NI. But as part of the Verizon carrier compliance test , we run a E911 scenario over CDMA. During this GPS engine is turned ON in the modem, and the pass condition of the test case is to see that the location tracking icon is visible to device user. Once GPS engine is turned OFF, the location icon should disappear.

This is different from the legacy behavior where nsGeolocationService always knows about when location tracking is started and stopped. The event to indicate that tracking has started or stopped is this callback from GPS HAL: (StatusCallback) https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/tree/dom/system/gonk/GonkGPSGeolocationProvider.cpp?h=mozilla/v2.2#n129, where the GPSStatus indicates the state of GPS engine.
Once GonkGPSGeolocationProvider knows this, it needs a way to tell Gecko/Gaia to show / hide the location icon.

You can open a separate bug on this if you wish. 

Please note I am still waiting on our test team to confirm if this test is mandated by Vzw carrier, but their recommendation is to start scoping out the work so that we do not loose time, in case we do need it.
Please note in the above test case nsGeolocationService will have no clue that the GPS engine has been started / stopped in the modem.
(In reply to Bhavna Sharma from comment #66)
> Alphan,
> 
> This is different from the legacy behavior where nsGeolocationService always
> knows about when location tracking is started and stopped. The event to
> indicate that tracking has started or stopped is this callback from GPS HAL:
> (StatusCallback)
> https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/tree/dom/system/
> gonk/GonkGPSGeolocationProvider.cpp?h=mozilla/v2.2#n129, where the GPSStatus
> indicates the state of GPS engine.
> Once GonkGPSGeolocationProvider knows this, it needs a way to tell
> Gecko/Gaia to show / hide the location icon.
> 
> You can open a separate bug on this if you wish. 
> 
Bug 1222899 is for this purpose.
Update the patch with the handling of BOM(Byte Order Mark)
Attachment #8677339 - Attachment is obsolete: true
Comment on attachment 8684834 [details] [diff] [review]
(1109) [GPS-SUPL-NI] Support for displaying notification message and requestor_id to user

Hi Kanru,
I would like to ask you to review this patch.
This patch is mainly for "displaying notification message and requestor_id to user", which is a following work of Bug 853703 (SUPL NI).
In that bug, we support SUPL NI without this ability(which is required for SUPL 2.0)

There are two main efforts of this patch.
1. As Bug 853703, we use the same chrome event to communicate with gaia. The data of this chrome event here is composed by "notification_id, notification_text, notification_requestor_id". Gaia will display the information by specific format.
2. Since there are three encoding types(UTF8, USC2, GSM_DEFAULT) of SUPL NI string, we need to decode the string by ourself. The functions in "GeolocationUtils.cpp" are for this purpose.
Attachment #8684834 - Flags: review?(kchen)
Comment on attachment 8684834 [details] [diff] [review]
(1109) [GPS-SUPL-NI] Support for displaying notification message and requestor_id to user

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

First of all, this patch does not apply to mozilla-central. Overall looks fine. I need a closer look at the decode part. I wonder if we could reuse the decode support from our RIL code. If that is too much effort I can live with having the decode part in GeolocationUtil.cpp but we have to write gtests for that.
Attachment #8684834 - Flags: review?(kchen)
Per discussed to Bhavna, reset the bug from confidential to open.
Group: qualcomm-confidential
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #71)
> Comment on attachment 8684834 [details] [diff] [review]
> (1109) [GPS-SUPL-NI] Support for displaying notification message and
> requestor_id to user
> 
> Review of attachment 8684834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> First of all, this patch does not apply to mozilla-central. Overall looks
> fine. I need a closer look at the decode part. I wonder if we could reuse
> the decode support from our RIL code. If that is too much effort I can live
> with having the decode part in GeolocationUtil.cpp but we have to write
> gtests for that.

ni Edgar for reuse the decode support from our RIL code.
Flags: needinfo?(echen)
(In reply to Alphan Chen [:alchen] from comment #73)
> ni Edgar for reuse the decode support from our RIL code.

The decoder of moz RIL lives in ChromeWorker and implemented by javascript, it seem not easy to reuse them on Geolocation. Besides, QCT replaces our RIL code, reusing our decoder won't work on QCT RIL. :(
Flags: needinfo?(echen)
Hi Kanru,
I update the patch and add gtest for GSMDefault decoding.
Please have a review.
Thank you.
Attachment #8684834 - Attachment is obsolete: true
Attachment #8702814 - Flags: review?(kchen)
Update the patch again.

Here is the gtest result.

Note: Google Test filter = GSMDefaultDecode.*
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from GSMDefaultDecode
[ RUN      ] GSMDefaultDecode.DecodeAlphabet
[       OK ] GSMDefaultDecode.DecodeAlphabet (0 ms)
[ RUN      ] GSMDefaultDecode.DecodeNullptr
[       OK ] GSMDefaultDecode.DecodeNullptr (0 ms)
[----------] 2 tests from GSMDefaultDecode (1 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (1 ms total)
[  PASSED  ] 2 tests.
Finished running GTest tests.
nsStringStats
 => mAllocCount:           3752
 => mReallocCount:          466
 => mFreeCount:            3752
 => mShareCount:           3747
 => mAdoptCount:             46
 => mAdoptFreeCount:         46
 => Process ID: 23639, Thread ID: 140010894792576
Attachment #8702814 - Attachment is obsolete: true
Attachment #8702814 - Flags: review?(kchen)
Attachment #8702816 - Flags: review?(kchen)
Alphan, can you also merge this change to v2.2r ?
Flags: needinfo?(alchen)
Will qct still pick v2.2r for your BSP?
I am planning to phase in these patches for SUPL NI into m-c.
Flags: needinfo?(alchen)
Yes we will need this for 2.2r
Comment on attachment 8702816 [details] [diff] [review]
1230-[GPS-SUPL NI] Support for displaying NI message to user-v2

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

I'm not particularly fond of the way UCS2 strings are decoded but it's OK in our code base. Please fix the other nits and request review again.

::: dom/moz.build
@@ +160,5 @@
>  
>  TEST_DIRS += [
>      'tests',
>      'imptests',
> +    'system/gonk/tests/gtest'

Move this to dom/system/moz.build

::: dom/system/gonk/GeolocationUtil.cpp
@@ +5,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "GeolocationUtil.h"
> +#include <cmath>
> +#include "nsContentUtils.h"

Sort the includes alphabetically and the <cmath> one should be on its own line.

@@ +29,5 @@
>  }
>  
> +
> +bool
> +DecodeGsmdefaultToString(UniquePtr<char[]> &message, nsString &wide_s)

arguments name should be aMessage and aWideStr respectively.

@@ +108,5 @@
> +
> +    const char GSM_EXTENDED_ESCAPE = 0x1B;
> +    if(prevCharWasEscape) {
> +      if (gsmVal == GSM_EXTENDED_ESCAPE) {
> +        output += 0x20; // 0X20 is space

Use MOZ_UTF16(' ') or AppendLiteral

@@ +111,5 @@
> +      if (gsmVal == GSM_EXTENDED_ESCAPE) {
> +        output += 0x20; // 0X20 is space
> +      } else {
> +        char16_t c = shiftTableToChar.CharAt(gsmVal);
> +        if (c == 0x20) { // 0X20 is space

MOZ_UTF16(' ')

@@ +130,5 @@
> +  return true;
> +}
> +
> +nsString
> +DecodeNIString(const char* message, GpsNiEncodingType encodingType)

aMessage and aEncodingType

@@ +138,5 @@
> +  int starthexIndex = 0, byteIndex = 0;
> +
> +  // convert to byte string first
> +  int hexstring_length = strlen(message);
> +  // Note: hexstring_length cannot be an ODD number.

lower case odd.

@@ +148,5 @@
> +  }
> +
> +  int byteArrayLen = (hexstring_length / 2);
> +  if (encodingType == GPS_ENC_SUPL_UTF8 || encodingType == GPS_ENC_SUPL_GSM_DEFAULT) {
> +    byteArrayLen += sizeof(char);

I guess this is reserving the string terminator. Since you know you want 1 why not just write 1 here?

@@ +150,5 @@
> +  int byteArrayLen = (hexstring_length / 2);
> +  if (encodingType == GPS_ENC_SUPL_UTF8 || encodingType == GPS_ENC_SUPL_GSM_DEFAULT) {
> +    byteArrayLen += sizeof(char);
> +  } else if (encodingType == GPS_ENC_SUPL_UCS2) {
> +    byteArrayLen += sizeof(char16_t);

Same here. The program fails if sizeof(char16_t) is not 2 but you really want 2 here.

Also add a comment to explain the purpose.

@@ +170,5 @@
> +    nsContentUtils::LogMessageToConsole("DecodeNIString - Failed to allocate string.");
> +    wide_str.AssignLiteral("Failed to decode string.");
> +    return wide_str;
> +  }
> +  memset(byteArray.get(), 0, byteArrayLen);

MakeUnique is infallible so we don't need to check the result here. MakeUnique also takes care of the zeroing.

@@ +176,5 @@
> +  // In this loop we have to read 4 bytes at a time so as to take care of
> +  // endianess while converting hex string to byte array.
> +  // If the hexstring_length is not divisble by 4, we still take care of
> +  // remaining bytes outside the for loop.
> +  if (hexstring_length > 2) {

We can bailout earlier if hexstring_length <= 2

@@ +199,5 @@
> +  if (encodingType == GPS_ENC_SUPL_UTF8) {
> +    nsCString str(byteArray.get());
> +    wide_str.Append(NS_ConvertUTF8toUTF16(str));
> +  } else if (encodingType == GPS_ENC_SUPL_UCS2) {
> +    wide_str.Append((const char16_t*)byteArray.get());

Ideally we should not cast the array directly to const char16_t* but it seems ok because mozilla/Char16.h asserts that char16_t is 2 bytes.

@@ +210,5 @@
> +    nsContentUtils::LogMessageToConsole("DecodeNIString - UnknownEncoding.");
> +    wide_str.AssignLiteral("Failed to decode string");
> +  }
> +  wide_str.ReplaceSubstring(MOZ_UTF16("\\"), MOZ_UTF16("\\\\"));
> +  wide_str.ReplaceSubstring(MOZ_UTF16(","), MOZ_UTF16("\\,"));

Why is the result escaped here?

::: dom/system/gonk/GeolocationUtil.h
@@ +6,5 @@
>  
>  #ifndef GEOLOCATIONUTIL_H
>  #define GEOLOCATIONUTIL_H
> +#include "nsPrintfCString.h"
> +#include "mozilla/UniquePtr.h"

One newline between include guards and the first #include

@@ +9,5 @@
> +#include "nsPrintfCString.h"
> +#include "mozilla/UniquePtr.h"
> +
> +using namespace mozilla;
> +using mozilla::UniquePtr;

Don't use |using| in headers

@@ +24,3 @@
>  
>  double CalculateDeltaInMeter(double aLat, double aLon, double aLastLat, double aLastLon);
> +bool DecodeGsmdefaultToString(UniquePtr<char[]> &byteArray, nsString &result);

DecodeGSMDefaultToString or DecodeGsmDefaultToString

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +361,5 @@
>        RefPtr<GonkGPSGeolocationProvider> provider = GonkGPSGeolocationProvider::GetSingleton();
>        provider->SetNiResponse(id, GPS_NI_RESPONSE_NORESP);
>      }
> +    nsContentUtils::LogMessageToConsole(
> +      "SendChromeEvent Failed(id:%d, flags:%x)", id, flags);

Prefix the message with "geo "

@@ +366,5 @@
>      return false;
>    }
>  
> +  if (flags == GPS_NI_NEED_TIMEOUT) {
> +    obs->NotifyObservers(nullptr, SUPL_NI_VERIFY_TIMEOUT, wide_str.BeginReading());

I think BeginReading() does not always return a pointer. Use &wide_str (&msg) directly.

@@ +382,5 @@
> +  if (needNotify && needVerify) {
> +    obs->NotifyObservers(nullptr, SUPL_NI_VERIFY, wide_str.BeginReading());
> +  } else if (needNotify) {
> +    obs->NotifyObservers(nullptr, SUPL_NI_NOTIFY, wide_str.BeginReading());
> +  }

Is !needNotify && needVerify a possible condition?

@@ +386,5 @@
> +  }
> +
> +  if ((needNotify && !needVerify) ||
> +      (!needNotify && !needVerify) ||
> +      privacyOverride) {

Simplify this condition please.

@@ +420,5 @@
> +      // The message is composed by "notification_id, notification_text,
> +      // notification_requestor_id"
> +      // If the encoding of text and requestor_id is not supported, the message
> +      // would be "id, ,".
> +      nsString wide_str;

Call this |msg| or |message|

@@ +422,5 @@
> +      // If the encoding of text and requestor_id is not supported, the message
> +      // would be "id, ,".
> +      nsString wide_str;
> +      wide_str.AppendInt(id);
> +      wide_str += MOZ_UTF16(",");

use AppendLiteral for consistency.

@@ +425,5 @@
> +      wide_str.AppendInt(id);
> +      wide_str += MOZ_UTF16(",");
> +      GpsNiEncodingType encodingType = mNotification->text_encoding;
> +      if (encodingType == GPS_ENC_SUPL_UTF8 || encodingType == GPS_ENC_SUPL_UCS2
> +          || encodingType == GPS_ENC_SUPL_GSM_DEFAULT) {

Put all conditions on separate lines.

Maybe log some message when seeing unsupported encoding type

@@ +431,2 @@
>        }
> +      wide_str += MOZ_UTF16(",");

ditto.

@@ +431,5 @@
>        }
> +      wide_str += MOZ_UTF16(",");
> +      encodingType = mNotification->requestor_id_encoding;
> +      if (encodingType == GPS_ENC_SUPL_UTF8 || encodingType == GPS_ENC_SUPL_UCS2
> +          || encodingType == GPS_ENC_SUPL_GSM_DEFAULT) {

ditto.

@@ +461,5 @@
>            }
>          private:
>            int mId;
>            int mDefaultResp;
> +          nsString mStr;

Call this mMsg

@@ +466,4 @@
>        };
>  
> +      bool needVerify = (flags & GPS_NI_NEED_VERIFY) != 0;
> +      if (needVerify) {

unindent if

::: dom/system/gonk/GonkGPSGeolocationProvider.h
@@ +83,5 @@
>    void ShutdownGPS();
>    void InjectLocation(double latitude, double longitude, float accuracy);
>    void RequestSettingValue(const char* aKey);
>    void SetNiResponse(int id, int response);
> +  bool SendChromeEvent(int id, GpsNiNotifyFlags flags, nsAString& wide_str);

s/wide_str/msg/

::: dom/system/gonk/tests/gtest/TestGSMDefaultDecode.cpp
@@ +10,5 @@
> +
> +using namespace mozilla;
> +using mozilla::UniquePtr;
> +
> +TEST(GSMDefaultDecode, DecodeAlphabet) {

Please also test two other encoding format and unknown encoding format. If escaping is required, also test that.

@@ +17,5 @@
> +                                     "\x49\xE5\x92\xD9\x74\x3E\xA1",
> +                                     "\x51\xE9\x94\x5A\xB5\x5E\xB1",
> +                                     "\x41\xE1\x90\x58\x34\x1E\x91"
> +                                       "\x49\xE5\x92\xD9\x74\x3E\xA1"
> +                                         "\x51\xE9\x94\x5A\xB5\x5E\xB1\x59\x2D"};

nit: indent.

::: dom/system/gonk/tests/gtest/moz.build
@@ +7,5 @@
> +UNIFIED_SOURCES += [
> +    'TestGSMDefaultDecode.cpp',
> +]
> +
> +include('/ipc/chromium/chromium-config.mozbuild')

We don't use any ipc code so probably don't need this line.

@@ +19,5 @@
> +]
> +
> +FINAL_LIBRARY = 'xul-gtest'
> +
> +CXXFLAGS += CONFIG['MOZ_CAIRO_CFLAGS']

We don't need this line, right?
Attachment #8702816 - Flags: review?(kchen) → review-
Here is the latest one.
Add three more tests for two other decoding formats and unknown format.
Attachment #8702816 - Attachment is obsolete: true
Attachment #8705509 - Flags: review?(kchen)
Comment on attachment 8702816 [details] [diff] [review]
1230-[GPS-SUPL NI] Support for displaying NI message to user-v2

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

::: dom/system/gonk/GeolocationUtil.cpp
@@ +176,5 @@
> +  // In this loop we have to read 4 bytes at a time so as to take care of
> +  // endianess while converting hex string to byte array.
> +  // If the hexstring_length is not divisble by 4, we still take care of
> +  // remaining bytes outside the for loop.
> +  if (hexstring_length > 2) {

This condition will be covered in the first checking.

if ((hexstring_length == 0) || (hexstring_length % 2 != 0)) {
    ...
    return wide_str;
}
Update the patch again.
Attachment #8705509 - Attachment is obsolete: true
Attachment #8705509 - Flags: review?(kchen)
Comment on attachment 8705581 [details] [diff] [review]
0108-[GPS-SUPL NI] Support for displaying NI message to user v2

Hi Kanru,
please have a review, thanks.
Attachment #8705581 - Flags: review?(kchen)
Comment on attachment 8705509 [details] [diff] [review]
0108-[GPS-SUPL NI] Support for displaying NI message to user

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

::: dom/system/gonk/GeolocationUtil.h
@@ +24,2 @@
>  double CalculateDeltaInMeter(double aLat, double aLon, double aLastLat, double aLastLon);
> +bool DecodeGsmDefaultToString(UniquePtr<char[]> &byteArray, nsString &result);

nit: aByteArray and aWideStr to match your implementation.

@@ +24,3 @@
>  double CalculateDeltaInMeter(double aLat, double aLon, double aLastLat, double aLastLon);
> +bool DecodeGsmDefaultToString(UniquePtr<char[]> &byteArray, nsString &result);
> +nsString DecodeNIString(const char* message, GpsNiEncodingType aEncodingType);

nit: aMessage
Attachment #8705509 - Attachment is obsolete: false
Attachment #8705581 - Flags: review?(kchen)
Update the patch to fix nits mentioned in comment 87.
Attachment #8705509 - Attachment is obsolete: true
Attachment #8705581 - Attachment is obsolete: true
Attachment #8706704 - Flags: review?(kchen)
Comment on attachment 8706704 [details] [diff] [review]
0112-[GPS-SUPL NI] Support for displaying NI message to user v2

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

r=me with the string type fixed

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +351,5 @@
>    mGpsNiInterface->respond(id, response);
>  }
>  
>  bool
> +GonkGPSGeolocationProvider::SendChromeEvent(int id, GpsNiNotifyFlags flags, nsAString& message)

nsString& message.

I didn't notice you are using the data in JavaScript directly. By using nsString you avoid one conversion.

@@ +366,5 @@
>      return false;
>    }
>  
> +  if (flags == GPS_NI_NEED_TIMEOUT) {
> +    obs->NotifyObservers(nullptr, SUPL_NI_VERIFY_TIMEOUT, (char16_t*) &message);

message.get()

@@ +379,5 @@
> +  // Verify behavior in gaia include notify and dialog in current
> +  // implementation.
> +  // So the logic here is different from Android
> +  if (needNotify && needVerify) {
> +    obs->NotifyObservers(nullptr, SUPL_NI_VERIFY, (char16_t*) &message);

ditto

@@ +381,5 @@
> +  // So the logic here is different from Android
> +  if (needNotify && needVerify) {
> +    obs->NotifyObservers(nullptr, SUPL_NI_VERIFY, (char16_t*) &message);
> +  } else if (needNotify) {
> +    obs->NotifyObservers(nullptr, SUPL_NI_NOTIFY, (char16_t*) &message);

ditto
Attachment #8706704 - Flags: review?(kchen) → review+
Before phasing the patch of displaying NI message to user, we need to phase in Bug 853703 to m-c first.
Attachment #8708263 - Attachment description: [m-c Gecko part] Bug 853703-Support SUPL NI feature. r=jaliu → [m-c Gecko part.1] Bug 853703-Support SUPL NI feature. r=jaliu
Attachment #8708266 - Attachment description: [m-c Gecko part] Bug 1201778 Support for displaying NI message to user. r=kanru → [m-c Gecko part.2] Bug 1201778 Support for displaying NI message to user. r=kanru
(In reply to Bhavna Sharma from comment #81)
> Yes we will need this for 2.2r
I will upload this patch for 2.2r later.
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #97)
> backed out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=3784547&repo=b2g-
> inbound

I'm running a new patch for fixing this build break on windows.
Flags: needinfo?(alchen)
I updated the patch again to fix the build break on Windows machine.

Here is the try server result. It looks fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b29f97ca0327
Attachment #8708266 - Attachment is obsolete: true
Keywords: checkin-needed
dropping the checkin needed keyword here, alphan with the new tier 3 change for b2g and the drop of sheriff support for tier 3 we do not do checkin needed anymore for b2g-inbound - if this needs to land on mozilla-central let me know
Flags: needinfo?(alchen)
Keywords: checkin-needed
I will double confirm about how to deal with this patch.
Flags: needinfo?(alchen)
Firefox OS is not being worked on
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.