Andreas Gal :gal Reporter
	Description • 11 years ago

Asking the user to set the time is really 19th century. NITZ is often not available, but NTP is easy to do. We should implement a service as a jsm that syncs the time whenever the data connection comes up. NTP uses tiny packets. The charge to the user is negligible.

	Andreas Gal :gal Reporter
	Comment 1 • 11 years ago

We want to avoid a race between NITZ (which is likely more precise) and NTP, so if there is NITZ, we might not want to override that time with NTP.

	Gene Lian (I already quit Mozilla)
	Comment 3 • 11 years ago

This is a dupe of Bug 714355. May be we can close that one.

Btw, NITZ was implemented by me so I know how to set the system time and handle the interaction between NITZ and NTP/SNTP. It might be suitable for me to take this bug or to be the reviewer.

Ken, are you going to find someone to take this?

	Andreas Gal :gal Reporter
	Comment 4 • 11 years ago

Gene, cool, definitely feel free to grab it. We should probably fire of NTP every time the network comes up and there is no NITZ. The interaction is definitely interesting. We don't want NITZ and NTP to ping-pong. Gene, I also filed a bug for Rudy to determine local timezone based on GPS coordinates. That bug could come in handy as well. Ideally I really want all of this automated instead of bugging the user during setup or via settings, with settings having the ability to override the automatic mechanism.

	Shao Hang Kao
	Comment 5 • 11 years ago

Hello,

I'm new to firefox os. If this is not so urgent may I take this as start?

	Shao Hang Kao
	Comment 6 • 11 years ago

Also a duplicate of Bug 714355 ?

	Andreas Gal :gal Reporter
	Comment 7 • 11 years ago

Not a trivial first bug but its not urgent so yeah, please grab it. Turns out our chipset vendor updates the time when GPS uses NTP so this would be for other chipsets and we could switch everything over once this works (no time pressure).

	Andreas Gal :gal Reporter
	Comment 9 • 11 years ago

Requirement: Get ntp as fast as possible after boot and refresh every day or so.

	Shao Hang Kao
	Comment 10 • 11 years ago

Hello Gene,

could you help to take a loop at this patch? Also I have some questions:
1. where can we set the settings? (ex. time.nitz.automatic-update.enabled)
2. how to test/simulate wifi on emulator (I don't have devices so I only tested this over the (emulated) cellular network)
3. I feel it's not good to put all these stuff in RadioInterfaceLayer.js, but can't find a proper place for them, could you suggest or do you think it's ok to put these in RIL?

Thanks!

	Gene Lian (I already quit Mozilla)
	Comment 11 • 11 years ago

Excellent, Shao Hang!

(In reply to Shao Hang Kao from comment #10)
> Created attachment 754244 [details] [diff] [review]
> could you help to take a loop at this patch? Also I have some questions:
> 1. where can we set the settings? (ex. time.nitz.automatic-update.enabled)

Settings App -> Date & Time -> Set automatically

Currently, the presence of UI for toggling the "Set automatically" depends on whether "time.nitz.available" is true or not. This setting value can only be determined by Gecko to reflect the dynamic availability of NITZ.

When the "time.nitz.available" is true, then the Gaia end can reveal that UI and let users to enable or disable "Set automatically", thus setting "time.nitz.automatic-update.enabled" to true or false. If it's enabled, the available NITZ data will be immediately applied on the system time.

> 2. how to test/simulate wifi on emulator (I don't have devices so I only
> tested this over the (emulated) cellular network)

We've not yet supported Wifi for emulator. To test NTP/SNTP, indeed, you need a real device for now. Ken has tried to borrow a Inari for you. Please reach me to get that.

> 3. I feel it's not good to put all these stuff in RadioInterfaceLayer.js,
> but can't find a proper place for them, could you suggest or do you think
> it's ok to put these in RIL?

Sure. I'll take a look tomorrow ASAP. I'm busy with MMS stuff now.

	Shao Hang Kao
	Comment 12 • 11 years ago

I see, I think we should share the settings for nitz & sntp. But maybe change it to better names like "time.automatic-update.enabled" & "time.automatic-update.available". For internally we can check if _lastXXXMessage == null to see if nitz or sntp is available.

	Gene Lian (I already quit Mozilla)
	Comment 13 • 11 years ago

Comment on attachment 754244 [details] [diff] [review]
Patch: RadioInterfaceLayer.js - v1.0

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

Hi Shao Hang, I just had a very quick review and hope you can come back with a better structure and nit-free. Thanks!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +55,5 @@
>  const kSysClockChangeObserverTopic       = "system-clock-change";
>  const kTimeNitzAutomaticUpdateEnabled    = "time.nitz.automatic-update.enabled";
>  const kTimeNitzAvailable                 = "time.nitz.available";
> +const kTimeSntpAutomaticUpdateEnabled    = "time.sntp.automatic-update.enabled";
> +const kTimeSntpAvailable                 = "time.sntp.available";

Yes, we definitely share the setting entries. Please do:

const kTimeAutoUpdateEnabled = "time.automatic-update.enabled";
const kTimeTimeAutoUpdateAvailable = "time.automatic-update.available";

This also needs Gaia's cooperation to ensure the compatibilities. Will ask the Gaia folks later.

@@ +325,5 @@
> +  // we need to adjust the system clock time and time zone by sntp.
> +  lock.get(kTimeSntpAutomaticUpdateEnabled, this);
> +
> +  // Set "time.sntp.available" to false when starting up.
> +  this.setSntpAvailable(false);

We have to combine these two functions with the original ones for NITZ and rephrase the comments.

@@ +1814,5 @@
> +   */
> +  setSntpAvailable: function setSntpAvailable(value) {
> +    gSettingsService.createLock().set(kTimeSntpAvailable, value, null,
> +                                      "fromInternalSetting");
> +  },

Ditto. We can just use .setTimeAutoUpdateAvailable(...).

@@ +1827,5 @@
> +
> +      notify: function notify(timer) {
> +        this._ril && this._ril.requestSntpTime(false);
> +      }
> +    };

This is very strange to have a reference to RIL when you're actually in it. I don't think we don't need a function object here.

@@ +1830,5 @@
> +      }
> +    };
> +
> +    if (this._lastNitzMessage) {
> +      debug("NITZ available, forget SNTP")

Nit: please add |;| at the end of each JS statement.

@@ +1834,5 @@
> +      debug("NITZ available, forget SNTP")
> +      return;
> +    }
> +    var oldTimeInMS = Date.now();
> +    var newTimeInMS = oldTimeInMS + message.clockOffset;

Nit: s/var/let for all the Gecko codes.

@@ +1847,5 @@
> +    var daily = new DailySNTP();
> +    daily._ril = this;
> +
> +    this._sntpDailyTimer.initWithCallback(daily, 86400 * 1000,
> +                                Ci.nsITimer.TYPE_ONE_SHOT);

Please refer to the following codes in the MmsService.js

  // Set a timer to clear the buffered MMS requests if the
  // MMS network fails to be connected within a time period.
  this.connectTimer.
    initWithCallback(this.onConnectTimerTimeout.bind(this),
                     TIME_TO_BUFFER_MMS_REQUESTS,
                     Ci.nsITimer.TYPE_ONE_SHOT);

The first |this| means we want to refer to the RIL's member function and the |.bind(this)| means we want to let the codes in .onConnectTimerTimeout() recognize the |this|.

I think you should put the timer callback under the RIL object so that you don't need to have an extra reference to the RIL.

Also, we often align the parameters like below:

this._sntpDailyTimer.initWithCallback(daily, 86400 * 1000,
                                      Ci.nsITimer.TYPE_ONE_SHOT);

@@ +1854,5 @@
> +
> +  /**
> +   * Handle the SNTP message (generated by ourself)
> +   */
> +  handleSntpTime: function handleSntpTime(message) {

This function can be merged.

@@ +1869,5 @@
> +  /**
> +   * Used for retry SNTP requests
> +   */
> +  retrySntpTime: function retrySntpTime() {
> +    function RetrySNTP() {}

You don't need create this redundant object.

@@ +1878,5 @@
> +        this._ril && this._ril.requestSntpTime(true);
> +      }
> +    };
> +
> +    var MAX_RETRY_COUNT = 10;

Please move this kind of definition on the top of file.

@@ +1900,5 @@
> +    retry._ril = this;
> +
> +    this._sntpRetryTimer.initWithCallback(retry, this._sntpRetryTimeOffset * 1000,
> +                                Ci.nsITimer.TYPE_ONE_SHOT);
> +    debug ("SNTP: scheduled SNTP retry after " + this._sntpRetryTimeOffset + " seconds");

Nit: one single line cannot exceed 80 characters. Please fold all the long lines.

@@ +1931,5 @@
> +
> +      return String.fromCharCode.apply(null, new Uint8Array(buffer));
> +    }
> +
> +    function SNTPListener(){}

function SNTPListener() {};

@@ +1935,5 @@
> +    function SNTPListener(){}
> +    SNTPListener.prototype = {
> +      _ril: null,
> +
> +      _success: false,

Nit: don't need a blank line between the member variables.

@@ +1946,5 @@
> +          debug ("SNTP fail");
> +          this._ril && (this._ril._sntpRequesting = false);
> +          this._ril && this._ril.retrySntpTime();
> +        }
> +      },

Maybe you can do:

onStopRequest: function onStopRequest(request, context, status) {
  if (!Components.isSuccessCode(status)) {
    debug ("SNTP fail");
    this._sntpRequesting = false;
    this.retrySntpTime();
  }
}.bind(this),

Actually, I'm not very sure if we can do it this way. Basically, my point is you don't need a redundant reference (._ril) since you've already in it.

Try to use |.bind(this)| or |let self = this;| to make the codes simpler.

@@ +1961,5 @@
> +        try {
> +          var binaryInputStream = Cc["@mozilla.org/binaryinputstream;1"].createInstance(Ci.nsIBinaryInputStream);
> +          binaryInputStream.setInputStream(inputStream);
> +          // we don't need first 24 bytes
> +          for (let i=0; i<6; i++)

s/i=0/i = 0
s/i<6/i < 6

we need blanks around operators.

@@ +1962,5 @@
> +          var binaryInputStream = Cc["@mozilla.org/binaryinputstream;1"].createInstance(Ci.nsIBinaryInputStream);
> +          binaryInputStream.setInputStream(inputStream);
> +          // we don't need first 24 bytes
> +          for (let i=0; i<6; i++)
> +            binaryInputStream.read32();

Please add |{...}| even for a one-line block.

@@ +1975,5 @@
> +          var clockOffset = Math.floor( ((receiveTimeInMS - originateTimeInMS) + (transmitTimeInMS - respondTimeInMS)) / 2 );
> +          debug("SNTP: clock offset: " + clockOffset);
> +          var message = {rilMessageType: "sntpTime",
> +                         clockOffset: clockOffset,
> +                         respondTimeInMS: respondTimeInMS};

let message = {
  rilMessageType: "sntpTime",
  clockOffset: clockOffset,
  respondTimeInMS: respondTimeInMS
};

@@ +2002,5 @@
> +          debug ("SNTP: sent " + bytes_write + " bytes");
> +          /*
> +          var binaryOutputStream = Cc["@mozilla.org/binaryoutputstream;1"].createInstance(Ci.nsIBinaryOutputStream);
> +          binaryOutputStream.setOutputStream(stream);
> +          binaryOutputStream.*/

Please remove the /* ... */ if you're sure the codes are not going to happen.

@@ +2020,5 @@
> +    var NTP_PORT = 123;
> +    var NTP_POOLS = ["0.pool.ntp.org",
> +                     "1.pool.ntp.org",
> +                     "2.pool.ntp.org",
> +                     "3.pool.ntp.org"];

let NTP_POOLS = [
  "0.pool.ntp.org",
  "1.pool.ntp.org",
  "2.pool.ntp.org",
  "3.pool.ntp.org"
];

@@ +2031,5 @@
> +      this._sntpRetryCount = 0;
> +      this._sntpRetryTimeOffset = 0;
> +    }
> +    this._sntpRetryTimer && this._sntpRetryTimer.cancel();
> +    this._sntpDailyTimer && this._sntpDailyTimer.cancel();

What's this? Shouldn't it be:

if (this._sntpRetryTimer) {
  this._sntpRetryTimer.cancel();
}

@@ +2274,5 @@
>    _lastNitzMessage: null,
>  
> +  // Flag to determine whether to use SNTP. It corresponds to the
> +  // 'time.sntp.automatic-update.enabled' setting from the UI.
> +  _sntpAutomaticUpdateEnabled: true,

This one can be merged with NITZ.

@@ +2292,5 @@
> +
> +  // Daily timer of SNTP
> +  _sntpDailyTimer: null,
> +
> +  // Retry time offset (in seconds) of SNTP

We often write the comment in the format of:

//{one-space}{upper-case-letter}...{period}

like:

// Hello world.

	Gene Lian (I already quit Mozilla)
	Comment 14 • 11 years ago

Hi Kaze,

We're planning to support SNTP and I believe lots of codes can be shared with NITZ both in Gecko and Gaia. The first thing we want to refine regards the settings entries. We're hoping to do:

s/time.nitz.automatic-update.enabled/time.automatic-update.enabled
s/time.nitz.available/time.automatic-update.available

I believe from the Gaia's point of view, you don't need to care about whether the time auto-update is coming from NITZ or SNTP. Right? I think we should share the same entries for NITZ and SNTP in the settings.

Does that sound reasonable to you?

	Shao Hang Kao
	Comment 15 • 11 years ago

Gene,
I've revise the codes except the merging for handleNitzTime & handleSntpTime since they're different message and comes from different source. Could you check again if we really should merge these two functions? Thanks!

	Shao Hang Kao
	Comment 17 • 11 years ago

Sorry, fixed some coding styles.

	Fabien Cazenave [:kaze]
	Comment 18 • 11 years ago

(In reply to Andreas Gal :gal from comment #0)
> Asking the user to set the time is really 19th century.

I couldn’t agree more.

(In reply to Gene Lian [:gene] from comment #14)
> I believe from the Gaia's point of view, you don't need to care about
> whether the time auto-update is coming from NITZ or SNTP. Right? I think we
> should share the same entries for NITZ and SNTP in the settings.
> 
> Does that sound reasonable to you?

It worries me a bit because they don’t require the same kind of connectivity.  Besides (if I understood correctly) SNTP only provides GMT time, not local time — hence, the signification of `time.automatic-update.[available|enabled]' would depend on the connectivity:
 • when NITZ is available, the time is always properly updated;
 • when NITZ is not available, the time is only updated when the data connection is active (3G or wifi) and if the timezone has been properly selected.

However, grouping these settings makes sense from a UX point of view.  If the `moztimechange' event provides enough information to know whether this time update depends is “absolute” (NITZ) or “relative” to a user-chosen timezone (NTP), I’m fine with this.

	Fabien Cazenave [:kaze]
	Comment 19 • 11 years ago

To (try to) clarify: the use case I have in mind is when you travel to a different timezone, get out of the plane (hence, no data communication — neither 3G nor Wi-Fi) and expect your phone to show the local time and update its timezone, in order not to mess with existing alarms or calendar alerts.

	Shao Hang Kao
	Comment 20 • 11 years ago

(In reply to Fabien Cazenave [:kaze] from comment #18)
Hi Kaze,

> 
> It worries me a bit because they don’t require the same kind of
> connectivity.  Besides (if I understood correctly) SNTP only provides GMT
> time, not local time — hence, the signification of
> `time.automatic-update.[available|enabled]' would depend on the connectivity:
>  • when NITZ is available, the time is always properly updated;
>  • when NITZ is not available, the time is only updated when the data
> connection is active (3G or wifi) and if the timezone has been properly
> selected.

Yes, that's actually the way I just implemented. SNTP only update the system time when NITZ isn't available, also not be able to change the time zone since we have no idea about time zone for NTP protocol.

> 
> However, grouping these settings makes sense from a UX point of view.  If
> the `moztimechange' event provides enough information to know whether this
> time update depends is “absolute” (NITZ) or “relative” to a user-chosen
> timezone (NTP), I’m fine with this.

Sorry I didn't get it, why would you want to distinguish if the time change comes from NITZ or NTP for the event?

	Shao Hang Kao
	Comment 21 • 11 years ago

(In reply to Fabien Cazenave [:kaze] from comment #19)
> To (try to) clarify: the use case I have in mind is when you travel to a
> different timezone, get out of the plane (hence, no data communication —
> neither 3G nor Wi-Fi) and expect your phone to show the local time and
> update its timezone, in order not to mess with existing alarms or calendar
> alerts.

for this use case, if we only got wifi after out of the plane, you'll have to update the timezone manually. Either before / after the SNTP update starts are both fine. we can get the timezone info from GPS automatically when the task Andreas mentioned finished.

	Fabien Cazenave [:kaze]
	Comment 22 • 11 years ago

(In reply to Shao Hang Kao from comment #20)
> Sorry I didn't get it, why would you want to distinguish if the time change
> comes from NITZ or NTP for the event?

 • in the first case (NITZ), the timezone can stay undefined and the correct time will be displayed;
 • in the second case (NTP), we need to know the timezone before we can display the correct time.

Take the example of the traveler: leaving a country where the time was defined by NITZ, landing in a country where only NTP is available. Getting a `moztimechange' event should let us know that we get a GMT time and that the timezone should be properly set — either by using the GPS (comment #4) or by guessing it from the mcc/mnc tuple (bug 873962).

	Shao Hang Kao
	Comment 23 • 11 years ago

I see, I'll see how to distinguish them. Thanks!

	Shao Hang Kao
	Comment 24 • 11 years ago

added a fix to adjust the offset when the time was changed manually by user.

	Shao Hang Kao
	Comment 25 • 11 years ago

(In reply to Fabien Cazenave [:kaze] from comment #22)
> (In reply to Shao Hang Kao from comment #20)
> > Sorry I didn't get it, why would you want to distinguish if the time change
> > comes from NITZ or NTP for the event?
> 
>  • in the first case (NITZ), the timezone can stay undefined and the correct
> time will be displayed;
>  • in the second case (NTP), we need to know the timezone before we can
> display the correct time.
> 
> Take the example of the traveler: leaving a country where the time was
> defined by NITZ, landing in a country where only NTP is available. Getting a
> `moztimechange' event should let us know that we get a GMT time and that the
> timezone should be properly set — either by using the GPS (comment #4) or by
> guessing it from the mcc/mnc tuple (bug 873962).

Will it help if we fire a "mozsntpupdate" (or in a general name "moztimechangeneedtz") whenever we setup the time from sntp?

Also I have a question for this use case, what's the accuracy by guessing it from the mcc/mnc tuple? Say if you already change the time zone manually during the fly, you may not want the time zone get changed by a accidentally wrong guessing.

	Shao Hang Kao
	Comment 26 • 11 years ago

Also maybe Bug 861795 is a proper place to deal with time zones?

	Fabien Cazenave [:kaze]
	Comment 27 • 11 years ago

(In reply to Shao Hang Kao from comment #25)
> Will it help if we fire a "mozsntpupdate" (or in a general name
> "moztimechangeneedtz") whenever we setup the time from sntp?

This sounds messy to me. I’d much prefer to have this information attached as a detail of the “moztimechange” event. Especially in the case where we’d get an NITZ “moztimechange” event and an NTP “moztimechange” event a few milliseconds later.

Note that we don’t define new events without pinging the WebAPI team first.

> Also I have a question for this use case, what's the accuracy by guessing it
> from the mcc/mnc tuple? Say if you already change the time zone manually
> during the fly, you may not want the time zone get changed by a accidentally
> wrong guessing.

The guess is not 100% accurate, of course. We need UX input here but the goal would be to change the timezone automatically only when it’s not been set manually.

Thinking out loud… as a developer, if Gaia gets a “moztimechange” event and if there’s more than 15 min of time difference, then it could check:
 • if it’s an NITZ update — in which case, just leave the timezone undefined;
 • if it’s an NTP update — in which case, the timezone must be changed if it’s been defined automatically.

(In reply to Shao Hang Kao from comment #26)
> Also maybe Bug 861795 is a proper place to deal with time zones?

Yes. :-)  But unless I missed a point, we’ll still need some more context with the “moztimechange” event to know if we have to update the timezone or not.

	Shao Hang Kao
	Comment 28 • 11 years ago

(In reply to Fabien Cazenave [:kaze] from comment #27)
> (In reply to Shao Hang Kao from comment #25)
> > Will it help if we fire a "mozsntpupdate" (or in a general name
> > "moztimechangeneedtz") whenever we setup the time from sntp?
> 
> This sounds messy to me. I’d much prefer to have this information attached
> as a detail of the “moztimechange” event. Especially in the case where we’d
> get an NITZ “moztimechange” event and an NTP “moztimechange” event a few
> milliseconds later.

actually this won't happen, we won't set the time based on NTP if NITZ available.

> 
> Note that we don’t define new events without pinging the WebAPI team first.
> 
> > Also I have a question for this use case, what's the accuracy by guessing it
> > from the mcc/mnc tuple? Say if you already change the time zone manually
> > during the fly, you may not want the time zone get changed by a accidentally
> > wrong guessing.
> 
> The guess is not 100% accurate, of course. We need UX input here but the
> goal would be to change the timezone automatically only when it’s not been
> set manually.
> 
> Thinking out loud… as a developer, if Gaia gets a “moztimechange” event and
> if there’s more than 15 min of time difference, then it could check:
>  • if it’s an NITZ update — in which case, just leave the timezone undefined;
>  • if it’s an NTP update — in which case, the timezone must be changed if
> it’s been defined automatically.

Note that in most cases you won't get a "moztimechange" event with more than 15min of time difference. For example, if I take a fly from Taipei (UTC+8) to Tokyo (UTC+9). When the plane take off the local time in Taipei was 9:00am (UTC+8), after 2 hours it landed in Tokyo and suppose I have wifi connection. Then SNTP will set the time as 11:00am (UTC+8), which the difference should be < 1 min.

Also I'm wondering how Gaia knows the difference when getting a "moztimechange" event? I think it only throws the event without any additional info?

> 
> (In reply to Shao Hang Kao from comment #26)
> > Also maybe Bug 861795 is a proper place to deal with time zones?
> 
> Yes. :-)  But unless I missed a point, we’ll still need some more context
> with the “moztimechange” event to know if we have to update the timezone or
> not.

	Fabien Cazenave [:kaze]
	Comment 29 • 11 years ago

(In reply to Shao Hang Kao from comment #28)
> actually this won't happen, we won't set the time based on NTP if NITZ
> available.

But an NITZ update can still happen after we get the NTP one, if I understood correctly.

> Note that in most cases you won't get a "moztimechange" event with more than
> 15min of time difference.

Then I probably misunderstood something. I thought that:

 • NITZ updates give local time — no timezone info needed;
 • NTP updates give GMT time, and we need the proper timezone to get the local time.

If that’s right, what happens if:

 • I take off from Paris, where I had NITZ support and no timezone defined in the device;
 • I land in San Francisco, where I have no NITZ support (and still no timezone defined) but get an NTP update? Won’t that make a big time difference?

To put it another way round: how could we know that we need to change the timezone? If it’s impossible to know that the device has been moved to a new timezone just with NITZ/NTP information, then my comments regarding the additional info in “moztimechange” are irrelevant.

(In reply to Shao Hang Kao from comment #28)
> Also I'm wondering how Gaia knows the difference when getting a
> "moztimechange" event? I think it only throws the event without any
> additional info?

Right. This is a good point.

	Shao Hang Kao
	Comment 30 • 11 years ago

(In reply to Fabien Cazenave [:kaze] from comment #29)
> (In reply to Shao Hang Kao from comment #28)
> > actually this won't happen, we won't set the time based on NTP if NITZ
> > available.
> 
> But an NITZ update can still happen after we get the NTP one, if I
> understood correctly.

that's right :) NTP=>NITZ is possible, but not for NITZ=>NTP.

> 
> > Note that in most cases you won't get a "moztimechange" event with more than
> > 15min of time difference.
> 
> Then I probably misunderstood something. I thought that:
> 
>  • NITZ updates give local time — no timezone info needed;
>  • NTP updates give GMT time, and we need the proper timezone to get the
> local time.
> 
> If that’s right, what happens if:
> 
>  • I take off from Paris, where I had NITZ support and no timezone defined
> in the device;

Here you have NITZ so the timezone should set to France 

>  • I land in San Francisco, where I have no NITZ support (and still no
> timezone defined) but get an NTP update? Won’t that make a big time
> difference?

In the view of system time we always store the time elapsed from 1900 in GMT, and since GMT time won't change by different timezone you should get only a small offset to your system (maybe some micro seconds).

> 
> To put it another way round: how could we know that we need to change the
> timezone? If it’s impossible to know that the device has been moved to a new
> timezone just with NITZ/NTP information, then my comments regarding the
> additional info in “moztimechange” are irrelevant.

NITZ provides timezone information so we can change timezone based on it. But if we only have NTP available we have no idea if we need update timezone. The only thing we can do is to update the system time stamp in GMT and timezone remains unchanged.

> 
> (In reply to Shao Hang Kao from comment #28)
> > Also I'm wondering how Gaia knows the difference when getting a
> > "moztimechange" event? I think it only throws the event without any
> > additional info?
> 
> Right. This is a good point.

To summarize: we'll get the same "time" from NTP at the same moment wherever you are :) (actually we calculate an offset between your system time (GMT) & system time (GMT) on NTP server)

	Gene Lian (I already quit Mozilla)
	Comment 31 • 11 years ago

(the discussion is too long to follow all the details)

Anyway, my opinion is: *time zone* and *clock time* are 2 separate types of concepts. It's not really necessary to always update both of them at the same time. Updating only one of them can make the system time more correct, instead of making it worse.

The controversial point looks like: supposing the |time.automatic-update.[available|enabled]| are enabled and the device only supports NTP (which is only capable of adjusting *clock time*), the user might be confused why the system time is still not correct while travelling across countries (because the *time zone* is not updated).

Does it help if we separately consider the settings for *time zone* and *clock time*? Something like:

  - time.timezone.automatic-update.[available|enabled]
  - time.clock.automatic-update.[available|enabled]

where the former can only be adjusted by NITZ but the latter can be adjusted by NITZ/NTP.

	Shao Hang Kao
	Comment 32 • 11 years ago

Agreed, that's exactly how android handle this problem: they have two options "Automatic date & time" and "Automatic time zone". Also I would suggest not to hide the date & time & timezone settings when "automatic update" checked. Instead we can gray out them so user can check if they got correct time or timezone as expected.

	Gene Lian (I already quit Mozilla)
	Comment 33 • 11 years ago

Comment on attachment 755367 [details] [diff] [review]
Patch: RadioInterfaceLayer.js - v2.1

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

Great! The codes look simpler now. :) We still need to decide what the final settings entries are and I hope to take more reviews to go through the details of mathematical calculations. Please upload a new patch to address some nits and my questions as below. Thanks!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +54,5 @@
>  const kSysMsgListenerReadyObserverTopic  = "system-message-listener-ready";
>  const kSysClockChangeObserverTopic       = "system-clock-change";
> +// TODO: change the setting name to time.automatic-update.enabled/available
> +const kTimeAutomaticUpdateEnabled        = "time.nitz.automatic-update.enabled";
> +const kTimeAutomaticUpdateAvailable      = "time.nitz.available";

For internal namings, let's s/Automatic/Auto to make the codes shorter.

@@ +64,5 @@
>  const DOM_MOBILE_MESSAGE_DELIVERY_ERROR    = "error";
>  
>  const CALL_WAKELOCK_TIMEOUT              = 5000;
> +const MAX_SNTP_RETRY                     = 10;
> +const SNTP_REFRESH_PERIOD                = 86400 * 1000; // Daily.

Let's use:

const SNTP_MAX_RETRY_COUNT      = 10;
const SNTP_REFRESH_PERIOD_IN_MS = 86400 * 1000; // Daily.

Prefix them with SNTP look more readable.

@@ +319,5 @@
>    // we need to adjust the system clock time and time zone by NITZ.
> +  lock.get(kTimeAutomaticUpdateEnabled, this);
> +
> +  // Set "time.nitz.automatic-update.available" to false when starting up.
> +  this.setTimeAutomaticUpdateAvailable(false);

Ditto. The function is too long. Let's do:

s/setTimeAutomaticUpdateAvailable/setTimeAutoUpdateAvailable.

@@ +1803,5 @@
>      }
>    },
>  
> +  /**
> +   * Set system time by SNTP

Nit: add a period for a complete sentence in the comments.

@@ +1807,5 @@
> +   * Set system time by SNTP
> +   */
> +  setSntpTime: function setSntpTime(message) {
> +    if (this._lastNitzMessage) {
> +      debug("NITZ available, forget SNTP");

debug("SNTP: NITZ is available; don't use SNTP.");

Add "SNTP: " for all of your debug messages so that you can easily filter them in the adb logcat.

@@ +1812,5 @@
> +      return;
> +    }
> +    gTimeService.set(Date.now() + message.clockOffset);
> +    // We've set the time so the offset now becomes 0.
> +    message.clockOffset = 0;

I'm wondering you don't need to do this because you're already listened to |kSysClockChangeObserverTopic| to dynamically update the clock offset. They will get converged in the long run.

@@ +1814,5 @@
> +    gTimeService.set(Date.now() + message.clockOffset);
> +    // We've set the time so the offset now becomes 0.
> +    message.clockOffset = 0;
> +
> +    // schedule another update SNTP_REFRESH_PERIOD secs later.

// Schedule another update SNTP_REFRESH_PERIOD secs later.

A comment should be in the format of:

//{one_space}{one_upper_case}...{period}

@@ +1821,5 @@
> +    }
> +
> +    this._sntpTimer.initWithCallback(this.requestSntpTime.bind(this),
> +                          SNTP_REFRESH_PERIOD,
> +                          Ci.nsITimer.TYPE_ONE_SHOT);

Please align the parameters:

this._sntpTimer.initWithCallback(this.requestSntpTime.bind(this),
                                 SNTP_REFRESH_PERIOD,
                                 Ci.nsITimer.TYPE_ONE_SHOT);

@@ +1826,5 @@
> +    debug ("SNTP: scheduled SNTP daily update");
> +  },
> +
> +  /**
> +   * Handle the SNTP message (generated by ourself)

* Handle the SNTP message.

The "(generated by ourself)" isn't clear. Btw, it's "ourselves" not "ourself".

@@ +1829,5 @@
> +  /**
> +   * Handle the SNTP message (generated by ourself)
> +   */
> +  handleSntpTime: function handleSntpTime(message) {
> +    // We've success so clear the retry status.

s/success/succeeded

@@ +1831,5 @@
> +   */
> +  handleSntpTime: function handleSntpTime(message) {
> +    // We've success so clear the retry status.
> +    this._sntpRetryCount = 0;
> +    this._sntpRetryTimeOffset = 0;

I hope to rename this variable. Please see below.

@@ +1846,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Used for retry SNTP requests

Period, please.

@@ +1870,5 @@
> +    }
> +
> +    this._sntpTimer.initWithCallback(this.requestSntpTime.bind(this),
> +                          this._sntpRetryTimeOffset * 1000,
> +                          Ci.nsITimer.TYPE_ONE_SHOT);

this._sntpTimer.initWithCallback(this.requestSntpTime.bind(this),
                                 this._sntpRetryPeriodInSec,
                                 Ci.nsITimer.TYPE_ONE_SHOT);

@@ +1873,5 @@
> +                          this._sntpRetryTimeOffset * 1000,
> +                          Ci.nsITimer.TYPE_ONE_SHOT);
> +    debug ("SNTP: scheduled SNTP retry after "
> +            + this._sntpRetryTimeOffset
> +            + " seconds");

Can we have these in a single line?

@@ +1880,5 @@
> +  /**
> +   * Request SNTP
> +   */
> +  requestSntpTime: function requestSntpTime() {
> +

Don't need a blank line after the line of function declaration.

@@ +1919,5 @@
> +          this.retrySntpTime();
> +        }
> +      }.bind(this),
> +
> +      onDataAvailable: function onDataAvailable(request, context, inputStream, offset, count) {

Wrap this a bit like:

function onDataAvailable(request, context, inputStream,
                         offset, count)

@@ +1948,5 @@
> +
> +          let respondTimeInMS = Date.now();
> +          let clockOffset = Math.floor(
> +            ((receiveTimeInMS - originateTimeInMS) + (transmitTimeInMS - respondTimeInMS)) / 2
> +          );

let clockOffset =
  Math.floor(((receiveTimeInMS - originateTimeInMS) +
             (transmitTimeInMS - respondTimeInMS)) / 2);

Basically, we hope to have some words after the first "(" and don't leave ");" alone in one line.

@@ +2009,5 @@
> +    this._sntpRequesting = true;
> +
> +    let currentThread = Cc["@mozilla.org/thread-manager;1"]
> +                          .getService()
> +                          .currentThread;

let currentThread = Cc["@mozilla.org/thread-manager;1"]
                      .getService().currentThread;

@@ +2011,5 @@
> +    let currentThread = Cc["@mozilla.org/thread-manager;1"]
> +                          .getService()
> +                          .currentThread;
> +    let socketTransportService = Cc["@mozilla.org/network/socket-transport-service;1"].
> +                                 getService(Ci.nsISocketTransportService);

let socketTransportService =
  Cc["@mozilla.org/network/socket-transport-service;1"]
    .getService(Ci.nsISocketTransportService);

@@ +2013,5 @@
> +                          .currentThread;
> +    let socketTransportService = Cc["@mozilla.org/network/socket-transport-service;1"].
> +                                 getService(Ci.nsISocketTransportService);
> +    let pump = Cc["@mozilla.org/network/input-stream-pump;1"].
> +                createInstance(Ci.nsIInputStreamPump);

let pump = Cc["@mozilla.org/network/input-stream-pump;1"]
             .createInstance(Ci.nsIInputStreamPump);

@@ +2021,5 @@
> +                        1,
> +                        NTP_POOLS[Math.floor(NTP_POOLS.length * Math.random())],
> +                        NTP_PORT,
> +                        null
> +                      );

let transport = socketTransportService
  .createTransport(["udp"],
                   1,
                   NTP_POOLS[Math.floor(NTP_POOLS.length * Math.random())],
                   NTP_PORT,
                   null);

@@ +2215,5 @@
> +          debug ("SNTP: network connected");
> +          // Check NTP when we have data connection, this may not take
> +          // effect immediately before the setting get enabled.
> +          if ((!this._lastSntpMessage ||
> +              this._lastSntpMessage.respondTimeInMS < Date.now() - 86400000)) {

You only need one parentheses here.

@@ +2261,5 @@
> +  // Retry counter of SNTP.
> +  _sntpRetryCount: 0,
> +
> +  // Retry time offset (in seconds) of SNTP.
> +  _sntpRetryTimeOffset: 0,

Don't use "TimeOffset" again since you're dealing with lots of time offset things, which is confusing. Maybe |_sntpRetryPeriodInSec| is a bit clear.

@@ +2264,5 @@
> +  // Retry time offset (in seconds) of SNTP.
> +  _sntpRetryTimeOffset: 0,
> +
> +  // Timer for SNTP, used for retry & daily updates
> +  _sntpTimer: null,

s/_sntpTimer/_sntpRetryTimer

@@ +2273,5 @@
>    handleSettingsChange: function handleSettingsChange(aName, aResult, aMessage) {
>      // Don't allow any content processes to modify the setting
> +    // "time.automatic-update.available" except for the chrome process.
> +    let isTimeAutomaticUpdateAvailable = (this._lastNitzMessage !== null ||
> +                                          this._lastSntpMessage !== null);

let isTimeAutoUpdateAvailable = this._lastNitzMessage !== null ||
                                this._lastSntpMessage !== null;

@@ +2343,5 @@
> +          if (this._lastNitzMessage) {
> +            this.setNitzTime(this._lastNitzMessage);
> +          }
> +          if (gNetworkManager.active &&
> +            gNetworkManager.active.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {

So if NITZ is already available, why do you still attempt to set SNTP? This will override the NITZ effect. Right?

@@ +2345,5 @@
> +          }
> +          if (gNetworkManager.active &&
> +            gNetworkManager.active.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +            // Set the latest cached SNTP time if it's available.
> +            if (!this._lastSntpMessage ||

This is wrong. Why do you attempt to set the SNTP when the |this._lastSntpMessage| is NOT available?

@@ +2347,5 @@
> +            gNetworkManager.active.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +            // Set the latest cached SNTP time if it's available.
> +            if (!this._lastSntpMessage ||
> +                this._lastSntpMessage.respondTimeInMS >= Date.now() - 86400000) {
> +              this.setSntpTime(this._lastSntpMessage);

Don't you need to return here? You should call requestSntpTime() only when the last SNTP is not available. Right?

	Shao Hang Kao
	Comment 34 • 11 years ago

Comment on attachment 755367 [details] [diff] [review]
Patch: RadioInterfaceLayer.js - v2.1

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1873,5 @@
> +                          this._sntpRetryTimeOffset * 1000,
> +                          Ci.nsITimer.TYPE_ONE_SHOT);
> +    debug ("SNTP: scheduled SNTP retry after "
> +            + this._sntpRetryTimeOffset
> +            + " seconds");

then it will be too long...

@@ +2264,5 @@
> +  // Retry time offset (in seconds) of SNTP.
> +  _sntpRetryTimeOffset: 0,
> +
> +  // Timer for SNTP, used for retry & daily updates
> +  _sntpTimer: null,

this is not only used for retry, but also for daily updates

@@ +2343,5 @@
> +          if (this._lastNitzMessage) {
> +            this.setNitzTime(this._lastNitzMessage);
> +          }
> +          if (gNetworkManager.active &&
> +            gNetworkManager.active.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {

I'll fix it. Thanks!

@@ +2347,5 @@
> +            gNetworkManager.active.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +            // Set the latest cached SNTP time if it's available.
> +            if (!this._lastSntpMessage ||
> +                this._lastSntpMessage.respondTimeInMS >= Date.now() - 86400000) {
> +              this.setSntpTime(this._lastSntpMessage);

I was thinking of first set the time based on the cached one, and then get a refresh one for use. How do you think?

	Gene Lian (I already quit Mozilla)
	Comment 35 • 11 years ago

> this is not only used for retry, but also for daily updates

Maybe you can use _sntpUpdateTimer, or _sntpRequestTimer or something like that. It's not very clear to me what the "timer" is doing when reading _sntpTimer.

> I was thinking of first set the time based on the cached one, and then get a refresh one
> for use. How do you think?

I don't think so. If your original logic is updating the SNTP by a daily period, then we should follow that policy. You can imagine if the user keep toggling the enabled/disabled button, the device will send the request to the server frequently, which is a waste of CPU and network resource.

	Gene Lian (I already quit Mozilla)
	Comment 36 • 11 years ago

How do you feel about the comment #31 and the comment #32?

	Fabien Cazenave [:kaze]
	Comment 37 • 11 years ago

(In reply to Gene Lian [:gene] from comment #31)
> Does it help if we separately consider the settings for *time zone* and
> *clock time*? Something like:
> 
>   - time.timezone.automatic-update.[available|enabled]
>   - time.clock.automatic-update.[available|enabled]
> 
> where the former can only be adjusted by NITZ but the latter can be adjusted
> by NITZ/NTP.

This would make a lot of sense to me. Thanks for clarifying!

	Fabien Cazenave [:kaze]
	Comment 38 • 11 years ago

(In reply to Shao Hang Kao from comment #32)
> Agreed, that's exactly how android handle this problem: they have two
> options "Automatic date & time" and "Automatic time zone". Also I would
> suggest not to hide the date & time & timezone settings when "automatic
> update" checked. Instead we can gray out them so user can check if they got
> correct time or timezone as expected.

Makes sense to me as well, but we need some UX input on this.

	Shao Hang Kao
	Comment 40 • 11 years ago

Gene,

Is patch v2.2 ok for you?
Should I update to new settings and upload a new patch or should we wait for someone's response?

Thanks!

	Gene Lian (I already quit Mozilla)
	Comment 41 • 11 years ago

I'm struggling with some tef+ issues which need to be fixed and landed before the next Monday. I'll look into your patch ASAP. That would be nice if you can update your patch to the new settings we discussed at comment #31 before my review. However, it doesn't matter, though.

	Andreas Gal :gal Reporter
	Comment 42 • 11 years ago

Comment on attachment 755863 [details] [diff] [review]
Patch: RadioInterfaceLayer.js - v2.2

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

I would put the SNTP code itself into a separate file. RadioInterfaceLayer is getting really big and the SNTP code has nothing to do with it directly. I am totally fine with landing this without an enable/disable switch in settings. This stuff should be automatic anyway. gene is the right person to do the final review. Nice patch. Thanks!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +53,5 @@
>  const kMozSettingsChangedObserverTopic   = "mozsettings-changed";
>  const kSysMsgListenerReadyObserverTopic  = "system-message-listener-ready";
>  const kSysClockChangeObserverTopic       = "system-clock-change";
> +// TODO: change the setting name to time.[clock|timezone].automatic-update.[enabled|available]
> +const kTimeAutoUpdateEnabled        = "time.nitz.automatic-update.enabled";

Please fix the alignment here.

@@ +64,5 @@
>  const DOM_MOBILE_MESSAGE_DELIVERY_ERROR    = "error";
>  
>  const CALL_WAKELOCK_TIMEOUT              = 5000;
> +const SNTP_MAX_RETRY_COUNT               = 10;
> +const SNTP_REFRESH_PERIOD_IN_MS          = 86400 * 1000; // Daily.

Please make this a pref in b2g.js

@@ +1855,5 @@
> +      this._sntpRetryCount = 0;
> +      this._sntpRetryPeriodInSec = 0;
> +      return;
> +    }
> +    if (this._sntpRetryPeriodInSec == 0) {

How about

this._sntpRetryPeriodInSec = Math.min(1, this._sntpRetryPeriodInSec * 2)

@@ +1867,5 @@
> +      this._sntpUpdateTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +    }
> +
> +    this._sntpUpdateTimer.initWithCallback(this.requestSntpTime.bind(this),
> +                                     this._sntpRetryPeriodInSec * 1000,

align this with this above

@@ +1870,5 @@
> +    this._sntpUpdateTimer.initWithCallback(this.requestSntpTime.bind(this),
> +                                     this._sntpRetryPeriodInSec * 1000,
> +                                     Ci.nsITimer.TYPE_ONE_SHOT);
> +    debug ("SNTP: scheduled SNTP retry after " + this._sntpRetryPeriodInSec +
> +      " seconds");

align here please

@@ +1985,5 @@
> +    // Number of seconds between Jan 1, 1900 and Jan 1, 1970.
> +    // 70 years plus 17 leap days.
> +    let OFFSET_1900_TO_1970 = ((365 * 70) + 17) * 24 * 60 * 60;
> +    let NTP_PORT = 123;
> +    let NTP_POOLS = [

please make port and domain names a pref in b2g.js

@@ +1991,5 @@
> +      "1.pool.ntp.org",
> +      "2.pool.ntp.org",
> +      "3.pool.ntp.org"
> +    ];
> +    let NTP_TIMEOUT = 30;

please make this a pref in b2g.js too

	Shao Hang Kao
	Comment 43 • 11 years ago

Hello Andreas,

I was thinking of put SNTP stuff in a separate file too. But have no idea where is the proper place for it, could you suggest one? maybe under toolkit/modules/SNTP.jsm ?

	Shao Hang Kao
	Comment 44 • 11 years ago

And I think enable/disable setting is required for SNTP. ex. Some users may want their clock have a faster time of 5 minutes to reduce the probability of being late. If we always update the clock automatically for them they won't feel good.

	Andreas Gal :gal Reporter
	Comment 45 • 11 years ago

Definitely. There should be a way to disable setting time automatically. But we don't need to separate that from NITZ. Users don't care how the time is set. Just whether to set it automatically or not.

maybe components/time? toolkit is fine too I guess.

	Shao Hang Kao
	Comment 46 • 11 years ago

Thanks! Yeh we're going to share the enable/disable with NITZ, but with an additional setting for automatically update time zone which is currently available for NITZ now. (See Comment 31 & Comment 32 for the detail)

	Shao Hang Kao
	Comment 47 • 11 years ago

Comment on attachment 755863 [details] [diff] [review]
Patch: RadioInterfaceLayer.js - v2.2

Gene,

I'm moving SNTP codes to a separate jsm file. You can review after it (when you have time, of course :-) ), Thanks!

	Andreas Gal :gal Reporter
	Comment 48 • 11 years ago

After this is all done and landed we should see if we can change the FTU to no longer manually set time/timezone at all, and instead of a best guess of the time zone and time (via NITZ and SNTP), and the user can go to settings to change that. We need UX feedback for that.

	Shao Hang Kao
	Comment 49 • 11 years ago

A snapshot before moving SNTP codes, with some settings moved to b2g.js.

	Shao Hang Kao
	Comment 54 • 11 years ago

Comment on attachment 757123 [details] [diff] [review]
Patch v3.2

Hello Gene,

This is my final version so far, also I changed the settings. Since we're waiting UX for feedbacks I've create a testing patch for gaia so you can apply it if you want to test if the SNTP functions work. Thanks!

	Shao Hang Kao
	Comment 55 • 11 years ago

Sorry for another update, just remove one unused include of XPComUtil in Sntp.jsm.

	Gene Lian (I already quit Mozilla)
	Comment 56 • 11 years ago

Comment on attachment 757130 [details] [diff] [review]
Patch v3.2

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

Basically, codes look good. Overall, just hoping to rename some variables and make some comments more clear and explicit. I'd like to take a final review when the comments are solved and the final review should be very soon to be done. :)

Thanks Shao Hang!

::: b2g/app/b2g.js
@@ +729,5 @@
> +pref("network.sntp.pools", "0.pool.ntp.org;1.pool.ntp.org;2.pool.ntp.org;3.pool.ntp.org");
> +// SNTP Port.
> +pref("network.sntp.port", 123);
> +// Timeout setting in seconds.
> +pref("network.sntp.timeout", 30);

I think the comments are a bit redundant here since they are almost the same as the preference names. Please rewrite and align them as:

// SNTP preferences.
pref("network.sntp.maxRetryCount", 10);
pref("network.sntp.refreshPeriod", 86400); // In seconds.
pref("network.sntp.pools",                 // Servers separated by ';'.
     "0.pool.ntp.org;1.pool.ntp.org;2.pool.ntp.org;3.pool.ntp.org");
pref("network.sntp.port", 123);
pref("network.sntp.timeout", 30);          // In seconds.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +53,5 @@
>  const kSmsDeliveryErrorObserverTopic     = "sms-delivery-error";
>  const kMozSettingsChangedObserverTopic   = "mozsettings-changed";
>  const kSysMsgListenerReadyObserverTopic  = "system-message-listener-ready";
>  const kSysClockChangeObserverTopic       = "system-clock-change";
> +const kTimeAutoUpdateEnabled             = "time.clock.automatic-update.enabled";

s/kTimeAutoUpdateEnabled/kClockAutoUpdateEnabled

for all the codes below.

@@ +54,5 @@
>  const kMozSettingsChangedObserverTopic   = "mozsettings-changed";
>  const kSysMsgListenerReadyObserverTopic  = "system-message-listener-ready";
>  const kSysClockChangeObserverTopic       = "system-clock-change";
> +const kTimeAutoUpdateEnabled             = "time.clock.automatic-update.enabled";
> +const kTimeAutoUpdateAvailable           = "time.clock.automatic-update.available";

s/kTimeAutoUpdateAvailable/kClockAutoUpdateAvailable

for all the codes below.

I'd like to use "clock" to denote the system clock and use "timezone" to denote the system timezone. They can both affect our "system time".

@@ +380,5 @@
> +                        Services.prefs.getIntPref('network.sntp.refreshPeriod'),
> +                        Services.prefs.getIntPref('network.sntp.timeout'),
> +                        Services.prefs.getCharPref('network.sntp.pools').split(';'),
> +                        Services.prefs.getIntPref('network.sntp.port')
> +                        );

Move |);| up to the tail of the above line.

@@ +1773,2 @@
>     */
> +  setTimeAutoUpdateAvailable: function setTimeAutoUpdateAvailable(value) {

s/setTimeAutoUpdateAvailable/setClockAutoUpdateAvailable

for all the codes within this file.

@@ +1784,5 @@
>                                        "fromInternalSetting");
>    },
>  
>    /**
>     * Set the NITZ message in our system time.

Set the system clock by NITZ.

@@ +1786,5 @@
>  
>    /**
>     * Set the NITZ message in our system time.
>     */
>    setNitzTime: function setNitzTime(message) {

s/setNitzTime/setClockByNitz

@@ +1794,5 @@
>        message.networkTimeInMS + (Date.now() - message.receiveTimeInMS));
> +  },
> +
> +  /**
> +   * Set the NITZ message in our system time zone.

Set the system timezone by NITZ.

@@ +1796,5 @@
> +
> +  /**
> +   * Set the NITZ message in our system time zone.
> +   */
> +  setNitzTimezone: function setNitzTime(message) {

s/setNitzTime/setTimezoneByNitz

@@ +1825,5 @@
>  
>      // Cache the latest NITZ message whenever receiving it.
>      this._lastNitzMessage = message;
>  
>      // Set the received NITZ time if the setting is enabled.

// Set the received NITZ /clock/ if the setting is enabled.

@@ +1831,1 @@
>        this.setNitzTime(message);

Ditto: s/setNitzTime/setNitzClock

@@ +1836,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Set system time by SNTP.

Set the system clock by SNTP.

@@ +1838,5 @@
> +
> +  /**
> +   * Set system time by SNTP.
> +   */
> +  setSntpTime: function setSntpTime(offset) {

s/setSntpTime/setClockBySntp

@@ +1845,5 @@
> +    if (!this._timeAutoUpdateEnabled) {
> +      return;
> +    }
> +    if (this._lastNitzMessage) {
> +      debug("Sntp: nitz available, forget sntp");

s/Sntp/SNTP
s/nitz/NITZ

Please use them as a special term with upper cases if they are being put in a sentence.

Also, s/forget/discard because you're discarding it not forgetting it.

@@ +2027,5 @@
> +      case kNetworkInterfaceStateChangedTopic:
> +        let network = subject.QueryInterface(Ci.nsINetworkInterface);
> +        if (network.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +          // Check NTP when we have data connection, this may not take
> +          // effect immediately before the setting get enabled.

// Check /SNTP/ when we have data connection. This may not take
// effect immediately before the setting get enabled.

@@ +2028,5 @@
> +        let network = subject.QueryInterface(Ci.nsINetworkInterface);
> +        if (network.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +          // Check NTP when we have data connection, this may not take
> +          // effect immediately before the setting get enabled.
> +          if (!this._sntp.cacheValid()) {

s/cacheValid/isExpired

The caller (RadioInterfaceLayer) doesn't need to know the concept of "cache" when you wrap the SNTP into a utility.

@@ +2055,5 @@
>    dataCallSettingsSUPL: {},
>    _dataCallSettingsToRead: [],
>    _oldRilDataEnabledState: null,
>  
> +  // Flag to determine whether to update system time automatically. It

s/system time/system clock/

@@ +2057,5 @@
>    _oldRilDataEnabledState: null,
>  
> +  // Flag to determine whether to update system time automatically. It
> +  // corresponds to the 'time.clock.automatic-update.enabled' setting from the
> +  // UI.

Remove the sentence /from the UI/, since even Gecko code can change the setting.

@@ +2058,5 @@
>  
> +  // Flag to determine whether to update system time automatically. It
> +  // corresponds to the 'time.clock.automatic-update.enabled' setting from the
> +  // UI.
> +  _timeAutoUpdateEnabled: null,

s/_timeAutoUpdateEnabled/_clockAutoUpdateEnabled

for all the codes within this file.

@@ +2062,5 @@
> +  _timeAutoUpdateEnabled: null,
> +
> +  // Flag to determine whether to update system timezone automatically. It
> +  // corresponds to the 'time.clock.automatic-update.enabled' setting from the
> +  // UI.

Remove the sentence /from the UI/, since even Gecko code can change the setting.

@@ +2069,5 @@
>    // Remember the last NITZ message so that we can set the time based on
>    // the network immediately when users enable network-based time.
>    _lastNitzMessage: null,
>  
> +  // Object that handles sntp.

s/sntp/SNTP.

@@ +2079,5 @@
>    handleSettingsChange: function handleSettingsChange(aName, aResult, aMessage) {
>      // Don't allow any content processes to modify the setting
> +    // "time.clock.automatic-update.available" except for the chrome process.
> +    let isTimeAutoUpdateAvailable = this._lastNitzMessage !== null ||
> +                                    this._sntp.available();

s/isTimeAutoUpdateAvailable/isClockAutoUpdateAvailable/
s/available/isAvailable/

@@ +2097,5 @@
> +        aResult !== isTimezoneAutoUpdateAvailable) {
> +      debug("Content processes cannot modify 'time.timezone.automatic-update.available'. Restore!");
> +      // Restore the setting to the current value.
> +      this.setTimezoneAutoUpdateAvailable(isTimezoneAutoUpdateAvailable);
> +    }

Can you refactor the condition a bit? Like:

if (aMessage !== "fromInternalSetting") {
  ...
}

@@ +2153,5 @@
>          break;
> +      case kTimeAutoUpdateEnabled:
> +        this._timeAutoUpdateEnabled = aResult;
> +
> +        if (this._timeAutoUpdateEnabled) {

Please do early return:

if (!this._timeAutoUpdateEnabled) {
  return;
}

// The fallback codes.
// ...

@@ +2159,5 @@
> +          if (this._lastNitzMessage) {
> +            this.setNitzTime(this._lastNitzMessage);
> +          }
> +          else if (gNetworkManager.active &&
> +            gNetworkManager.active.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {

Please align this line by the above line from |gNetworkManager.active &&|. Or:

else if (gNetworkManager.active && gNetworkManager.active.state ==
         Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {

@@ +2161,5 @@
> +          }
> +          else if (gNetworkManager.active &&
> +            gNetworkManager.active.state == Ci.nsINetworkInterface.NETWORK_STATE_CONNECTED) {
> +            // Set the latest cached SNTP time if it's available.
> +            if (this._sntp.cacheValid()) {

s/cacheValid()/isExpired()/

@@ +2165,5 @@
> +            if (this._sntp.cacheValid()) {
> +              this.setSntpTime(this._sntp.getOffset());
> +            }
> +            else {
> +              // Or get a refresh one.

// Or refresh the SNTP.

@@ +2176,5 @@
> +        this._timezoneAutoUpdateEnabled = aResult;
> +
> +        if (this._timezoneAutoUpdateEnabled) {
> +          // Set the latest cached NITZ timezone if it's available.
> +          if (this._lastNitzMessage) {

Why not using the original code?

// Apply the latest cached NITZ for timezone if it's available.
if (this._timezoneAutoUpdateEnabled && this._lastNitzMessage) {
  this.setTimezoneByNitz(this._lastNitzMessage);
}

::: toolkit/modules/Sntp.jsm
@@ +12,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +// Set to true to see debug messages.
> +var DEBUG = true; // TODO: remember to set this to false before submit

OK.

@@ +14,5 @@
> +
> +// Set to true to see debug messages.
> +var DEBUG = true; // TODO: remember to set this to false before submit
> +
> +const SNTP_MAX_RETRY_COUNT      =

You don't need this since it's assigned by the constructor.

@@ +16,5 @@
> +var DEBUG = true; // TODO: remember to set this to false before submit
> +
> +const SNTP_MAX_RETRY_COUNT      =
> +  Services.prefs.getIntPref('network.sntp.maxRetryCount');
> +const SNTP_REFRESH_PERIOD_IN_MS =

You don't need this since it's assigned by the constructor.

@@ +22,5 @@
> +
> +/**
> + * Constructor of Sntp.
> + *
> + * @param availableCallback

s/availableCallback/dataAvailableCb

@@ +23,5 @@
> +/**
> + * Constructor of Sntp.
> + *
> + * @param availableCallback
> + *        Callback function get called when sntp offset available, signature

s/get/gets/
s/sntp/SNTP
s/available/is available/
s/, signature/Signature/

@@ +24,5 @@
> + * Constructor of Sntp.
> + *
> + * @param availableCallback
> + *        Callback function get called when sntp offset available, signature
> + *        is function availableCallback(offsetInMS).

s/availableCallback/dataAvailableCb

@@ +26,5 @@
> + * @param availableCallback
> + *        Callback function get called when sntp offset available, signature
> + *        is function availableCallback(offsetInMS).
> + * @param maxRetry
> + *        Maximum retry count when sntp failed to connect to server, set to

s/maxRetry/maxRetryCount/
s/, set/; set/

@@ +29,5 @@
> + * @param maxRetry
> + *        Maximum retry count when sntp failed to connect to server, set to
> + *        zero to disable the retry.
> + * @param refreshPeriodInSecs
> + *        Refresh period, set to zero to disable refresh.

s/, set/; set/

@@ +33,5 @@
> + *        Refresh period, set to zero to disable refresh.
> + * @param timeoutInSecs
> + *        Timeout value used for connection.
> + * @param pools
> + *        Sntp server lists, separate by ';'.

s/Sntp/SNTP/
s/, separate/separated/

@@ +35,5 @@
> + *        Timeout value used for connection.
> + * @param pools
> + *        Sntp server lists, separate by ';'.
> + * @param port
> + *        Sntp port.

s/Sntp/SNTP/

@@ +37,5 @@
> + *        Sntp server lists, separate by ';'.
> + * @param port
> + *        Sntp port.
> + */
> +this.Sntp = function Sntp(availableCallback, maxRetry, refreshPeriodInSecs,

s/availableCallback/dataAvailableCb
s/maxRetry/maxRetryCount

@@ +39,5 @@
> + *        Sntp port.
> + */
> +this.Sntp = function Sntp(availableCallback, maxRetry, refreshPeriodInSecs,
> +                          timeoutInSecs, pools, port) {
> +  if (availableCallback != null) {

s/availableCallback/dataAvailableCb

@@ +40,5 @@
> + */
> +this.Sntp = function Sntp(availableCallback, maxRetry, refreshPeriodInSecs,
> +                          timeoutInSecs, pools, port) {
> +  if (availableCallback != null) {
> +    this._availableCallback = availableCallback;

s/availableCallback/dataAvailableCb
s/_availableCallback/_dataAvailableCb

@@ +42,5 @@
> +                          timeoutInSecs, pools, port) {
> +  if (availableCallback != null) {
> +    this._availableCallback = availableCallback;
> +  }
> +  if (maxRetry != null) {

s/null/undefined

This can meet both |null| and |undefined|.

@@ +43,5 @@
> +  if (availableCallback != null) {
> +    this._availableCallback = availableCallback;
> +  }
> +  if (maxRetry != null) {
> +    this._maxRetry = maxRetry;

s_maxRetry/_maxRetryCount/

@@ +45,5 @@
> +  }
> +  if (maxRetry != null) {
> +    this._maxRetry = maxRetry;
> +  }
> +  if (refreshPeriodInSecs != null) {

s/null/undefined

@@ +48,5 @@
> +  }
> +  if (refreshPeriodInSecs != null) {
> +    this._refreshPeriodInMS = refreshPeriodInSecs * 1000;
> +  }
> +  if (timeoutInSecs != null) {

s/null/undefined

@@ +51,5 @@
> +  }
> +  if (timeoutInSecs != null) {
> +    this._timeoutInMS = timeoutInSecs * 1000;
> +  }
> +  if (pools != null && pools.length > 0) {

s/null/undefined

I don't think you need the check |pools.length > 0|. If you hope, please use |Array.isArray(...)|.

@@ +54,5 @@
> +  }
> +  if (pools != null && pools.length > 0) {
> +    this._pools = pools;
> +  }
> +  if (port != null) {

s/null/undefined

@@ +60,5 @@
> +  }
> +}
> +
> +Sntp.prototype = {
> +  available: function available() {

s/available/isAvailable/

@@ +64,5 @@
> +  available: function available() {
> +    return this._cachedOffset != null;
> +  },
> +
> +  cacheValid: function cacheValid() {

s/cacheValid/isExpired/

@@ +66,5 @@
> +  },
> +
> +  cacheValid: function cacheValid() {
> +    let valid = this._cachedOffset != null;
> +    valid = valid && this._cachedTimeInMS != null;

Combine this two lines:

let valid = this._cachedOffset != null &&
            this._cachedTimeInMS != null;

@@ +69,5 @@
> +    let valid = this._cachedOffset != null;
> +    valid = valid && this._cachedTimeInMS != null;
> +    if (this._refreshPeriodInMS > 0) {
> +      valid = valid && Date.now() <
> +        this._cachedTimeInMS + this._refreshPeriodInMS;

valid = valid && Date.now() < this._cachedTimeInMS +
                              this._refreshPeriodInMS;

@@ +83,5 @@
> +    return this._cachedOffset;
> +  },
> +
> +  /**
> +   * Indicates the system time has changed by [offset]ms so we need to

s/system time/system clock/
s/has/has been/

@@ +123,5 @@
> +    // Cache the latest SNTP offset whenever receiving it.
> +    this._cachedOffset = clockOffset;
> +    this._cachedTimeInMS = respondTimeInMS;
> +
> +    if (this._availableCallback != null) {

s/_availableCallback/_dataAvailableCb

@@ +124,5 @@
> +    this._cachedOffset = clockOffset;
> +    this._cachedTimeInMS = respondTimeInMS;
> +
> +    if (this._availableCallback != null) {
> +      this._availableCallback(clockOffset);

s/_availableCallback/_dataAvailableCb

@@ +136,5 @@
> +   */
> +  _retry: function _retry() {
> +    this._retryCount++;
> +    if (this._retryCount > this._maxRetry) {
> +      debug ("stop retry SNTP");

s/stop retry/stop retrying/

@@ +148,5 @@
> +    this._schedule(this._retryPeriodInMS);
> +  },
> +
> +  /**
> +   * Request SNTP

Add a period at the tail.

@@ +171,5 @@
> +      let buffer = new ArrayBuffer(NTP_PACKET_SIZE);
> +      let data = new DataView(buffer);
> +      data.setUint8(0, NTP_MODE_CLIENT | (NTP_VERSION << 3));
> +      data.setUint32(TRANSMIT_TIME_OFFSET, s, false);
> +      data.setUint32(TRANSMIT_TIME_OFFSET+4, f, false);

s/TRANSMIT_TIME_OFFSET+4/TRANSMIT_TIME_OFFSET + 4/

@@ +292,5 @@
> +    outStream.asyncWait(new SNTPRequester(), 0, 0, currentThread);
> +  },
> +
> +  // Callback function.
> +  _availableCallback: null,

s/_availableCallback/_dataAvailableCb

@@ +300,5 @@
> +      "0.pool.ntp.org",
> +      "1.pool.ntp.org",
> +      "2.pool.ntp.org",
> +      "3.pool.ntp.org"
> +    ],

Alignments:

_pools: [
  "0.pool.ntp.org",
  "1.pool.ntp.org",
  "2.pool.ntp.org",
  "3.pool.ntp.org"
],

@@ +305,5 @@
> +
> +  // The port.
> +  _port: 123,
> +
> +  // Maximum retry allowed when request failed.

s/retry/retry count/

@@ +314,5 @@
> +
> +  // Timeout value used for connecting.
> +  _timeoutInMS: 30 * 1000,
> +
> +  // Cached sntp offset.

s/sntp/SNTP/

@@ +317,5 @@
> +
> +  // Cached sntp offset.
> +  _cachedOffset: null,
> +
> +  // When we got the offset in cache.

This is a bit implicit. Maybe:

// The time point when we cache the offset.

@@ +329,5 @@
> +
> +  // Retry time offset (in seconds).
> +  _retryPeriodInMS: 0,
> +
> +  // Timer used for retry & daily updates

s/retry/retries/

Add a period.

	Gene Lian (I already quit Mozilla)
	Comment 57 • 11 years ago

(In reply to Andreas Gal :gal from comment #48)
> After this is all done and landed we should see if we can change the FTU to
> no longer manually set time/timezone at all, and instead of a best guess of
> the time zone and time (via NITZ and SNTP), and the user can go to settings
> to change that. We need UX feedback for that.

For the UI/UX feedback, we've already fired a bug for that at bug 878001, comment #8 and asked feedback from the UX team.

	Shao Hang Kao
	Comment 58 • 11 years ago

Comment on attachment 757130 [details] [diff] [review]
Patch v3.2

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

::: toolkit/modules/Sntp.jsm
@@ +51,5 @@
> +  }
> +  if (timeoutInSecs != null) {
> +    this._timeoutInMS = timeoutInSecs * 1000;
> +  }
> +  if (pools != null && pools.length > 0) {

Is it possible we got an empty array here?

	Gene Lian (I already quit Mozilla)
	Comment 59 • 11 years ago

Oh yeap! Maybe add both for safety.

	Gene Lian (I already quit Mozilla)
	Comment 61 • 11 years ago

I'll take the final review today. Btw, since you are a newbie, you probably need to learn our Mozilla Automatic Test. If you've already known, please ignore this message. :)

To make sure your codes don't break any existing functions, you need to learn how to commit your patches to the Mozilla Try Server [1] to run through the automatic tests. You can see your testing results at [2] to make sure each testing item is green before asking for a check-in (some non-green items might be due to random bugs, which can be just ignored). Btw, although you cannot commit the codes to the mozilla-central for now, you can commit them to the Try Server, where the former needs level-3 permission and the latter only needs level-1 permission. Therefore, please apply for your level-1 permission while you're working on this bug because that application usually takes some time and some paper work to be done [3].

Don't hesitate to ask questions when you're stuck. Thanks!

[1] https://wiki.mozilla.org/Build:TryServer
[2] https://tbpl.mozilla.org/?tree=Try
[3] http://www.mozilla.org/hacking/commit-access-policy/

	Shao Hang Kao
	Comment 62 • 11 years ago

Thanks, I already tried to push this patch to try server but with some permission problems and just got IT's feedback today. Will try it again tonight.

	Gene Lian (I already quit Mozilla)
	Comment 63 • 11 years ago

Comment on attachment 757894 [details] [diff] [review]
Patch v3.3

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

Excellent! Shao Hang! I only have some minor comments on nits. Giving your review+ which means this patch is allowed to be landed onto our code base. Please upload a new patch for fixing the nits and you can directly carry on the review+ flag. The next step you need to do is:

1. Upload a new patch with the following HG commit message:

   "Bug 874771 - Implement SNTP support (Gecko end). r=gene"

2. Run your patch on the Try Server.

3. Fix bug 878001 and get review+ as well.

4. Land the Gecko [note] and Gaia patches at the same time.

[note] If you have level-3 permission, you can do that by yourself. Since you only have level-1 for now, you can ask for help. That is, please put "checkin-needed" in the Keywords field and then someone would help you land this patch. That would be a bonus if you could please paste your try server result here before asking for "checkin-needed".

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2073,5 @@
>    // Cell Broadcast settings values.
>    _cellBroadcastSearchListStr: null,
>  
>    handleSettingsChange: function handleSettingsChange(aName, aResult, aMessage) {
> +    if (aMessage !== "fromInternalSetting" ) {

Sorry! After some thoughts, I think your original logic was better, because it could filter the |aName| first and makes the logic more clear, but we can still have some improvements here (use |isClockAutoUpdateAvailable| and |isTimezoneAutoUpdateAvailable| only when needed):

// Don't allow any content processes to modify the setting
// "time.clock.automatic-update.available" except for the chrome process.
if (aName === kClockAutoUpdateAvailable && aMessage !== "fromInternalSetting") {
  let isClockAutoUpdateAvailable = this._lastNitzMessage !== null ||
                                   this._sntp.isAvailable();
  if (aResult !== isClockAutoUpdateAvailable) {
    debug("Content processes cannot modify 'time.clock.automatic-update.available'. Restore!");
    // Restore the setting to the current value.
    this.setClockAutoUpdateAvailable(isClockAutoUpdateAvailable);
  }
}

// Don't allow any content processes to modify the setting
// "time.timezone.automatic-update.available" except for the chrome
// process.
if (aName === kTimezoneAutoUpdateAvailable && aMessage !== "fromInternalSetting") {
  let isTimezoneAutoUpdateAvailable = this._lastNitzMessage !== null;
  if (aResult !== isTimezoneAutoUpdateAvailable) {
    debug("Content processes cannot modify 'time.timezone.automatic-update.available'. Restore!");
    // Restore the setting to the current value.
    this.setTimezoneAutoUpdateAvailable(isTimezoneAutoUpdateAvailable);
  }
}

this.handle(aName, aResult);

@@ +2077,5 @@
> +    if (aMessage !== "fromInternalSetting" ) {
> +      // Don't allow any content processes to modify the setting
> +      // "time.clock.automatic-update.available" except for the chrome process.
> +      let isClockAutoUpdateAvailable = this._lastNitzMessage !== null ||
> +                                      this._sntp.isAvailable();

Align this line with the above |this|.

@@ +2164,5 @@
> +          // Set the latest cached SNTP time if it's available.
> +          if (!this._sntp.isExpired()) {
> +            this.setClockBySntp(this._sntp.getOffset());
> +          }
> +          else {

Move this line up to the above:

} else {

::: toolkit/modules/Sntp.jsm
@@ +9,5 @@
> +];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");

Why do you want to import this? Please remove this if it isn't needed.

@@ +12,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +// Set to true to see debug messages.
> +var DEBUG = false;

Please always use |let| in the Gecko codes.

@@ +21,5 @@
> + * @param dataAvailableCb
> + *        Callback function gets called when SNTP offset available. Signature
> + *        is function dataAvailableCb(offsetInMS).
> + * @param maxRetryCount
> + *        Maximum retry count when sntp failed to connect to server; set to

s/sntp/SNTP/ :P

@@ +28,5 @@
> + *        Refresh period; set to zero to disable refresh.
> + * @param timeoutInSecs
> + *        Timeout value used for connection.
> + * @param pools
> + *        SNTP server lists, separate by ';'.

s/, separate by/separated by/

@@ +34,5 @@
> + *        SNTP port.
> + */
> +this.Sntp = function Sntp(dataAvailableCb, maxRetryCount, refreshPeriodInSecs,
> +                          timeoutInSecs, pools, port) {
> +  if (dataAvailableCb != undefined) {

Sorry... it's my bad. You should use |!= null| in general, which can be matched by both |undefined| and |null|. Using |!= undefined| is fine but |!= null| looks better.

P.S. I originally misunderstood you were using |!== null|.

@@ +37,5 @@
> +                          timeoutInSecs, pools, port) {
> +  if (dataAvailableCb != undefined) {
> +    this._dataAvailableCb = dataAvailableCb;
> +  }
> +  if (maxRetryCount != undefined) {

Ditto. Please use |!= null|.

@@ +40,5 @@
> +  }
> +  if (maxRetryCount != undefined) {
> +    this._maxRetryCount = maxRetryCount;
> +  }
> +  if (refreshPeriodInSecs != undefined) {

Ditto. Please use |!= null|.

@@ +43,5 @@
> +  }
> +  if (refreshPeriodInSecs != undefined) {
> +    this._refreshPeriodInMS = refreshPeriodInSecs * 1000;
> +  }
> +  if (timeoutInSecs != undefined) {

Ditto. Please use |!= null|.

@@ +46,5 @@
> +  }
> +  if (timeoutInSecs != undefined) {
> +    this._timeoutInMS = timeoutInSecs * 1000;
> +  }
> +  if (pools != undefined && Array.isArray(pools) && pools.length > 0) {

Ditto. Please use |!= null|.

@@ +49,5 @@
> +  }
> +  if (pools != undefined && Array.isArray(pools) && pools.length > 0) {
> +    this._pools = pools;
> +  }
> +  if (port != undefined) {

Ditto. Please use |!= null|.

@@ +97,5 @@
> +
> +    this._updateTimer.initWithCallback(this._request.bind(this),
> +                                       timeInMS,
> +                                       Ci.nsITimer.TYPE_ONE_SHOT);
> +    debug("Scheduled sntp request in " + timeInMS + "ms");

s/sntp/SNTP. :P

@@ +196,5 @@
> +        debug ("Data available: " + count + " bytes");
> +
> +        try {
> +          let binaryInputStream = Cc["@mozilla.org/binaryinputstream;1"]
> +                                  .createInstance(Ci.nsIBinaryInputStream);

Align |.| with the above |[|.

@@ +214,5 @@
> +          this._handleSntp(originateTimeInMS, receiveTimeInMS,
> +                           transmitTimeInMS, respondTimeInMS);
> +          this._requesting = false;
> +        }
> +        catch (e) {

Move this line to the above:

} catch (e) {

@@ +232,5 @@
> +          let bytes_write = stream.write(data, data.length);
> +          debug ("SNTP: sent " + bytes_write + " bytes");
> +          stream.close();
> +        }
> +        catch (e) {

} catch (e) {

@@ +296,5 @@
> +    "2.pool.ntp.org",
> +    "3.pool.ntp.org"
> +  ],
> +
> +  // The port.

// The SNTP port.

	Shao Hang Kao
	Comment 64 • 11 years ago

try result:
https://tbpl.mozilla.org/?tree=Try&rev=0990ef41b480

(Gene, could you have a look? 2 tests failed and I'm not sure if it's normal. Thanks!)

	Gene Lian (I already quit Mozilla)
	Comment 65 • 11 years ago

(In reply to Shao Hang Kao from comment #64)
> try result:
> https://tbpl.mozilla.org/?tree=Try&rev=0990ef41b480
> 
> (Gene, could you have a look? 2 tests failed and I'm not sure if it's
> normal. Thanks!)

Sometimes we would have random bugs depending on the dynamics. These two fails seem to be having nothing to do with your patch. You can push the "+" sign in the left bottom corner to rerun the testing items (I just did that for you).

	Shao Hang Kao
	Comment 66 • 11 years ago

Sorry forgot to upload this, the try was tested with this patch.

	Shao Hang Kao
	Comment 67 • 11 years ago

fixed a bug that didn't change the usage from setNitzTime to SetClockByNitz

	Shao Hang Kao [:skao] Assignee
	Comment 68 • 11 years ago

rebased

	Shao Hang Kao [:skao] Assignee
	Comment 69 • 11 years ago

Comment on attachment 802915 [details] [diff] [review]
Patch v3.6 ready for checkin

Gene,

since the patch was too old so I just uploaded a rebased one, do you want to review again?

S.H.

	Gene Lian (I already quit Mozilla)
	Comment 70 • 11 years ago

Comment on attachment 802915 [details] [diff] [review]
Patch v3.6 ready for checkin

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

Basically, you don't need to ask for another review for rebasing if you've got a review+. :) But be careful the function still works as it worked before.

	Shao Hang Kao [:skao] Assignee
	Comment 71 • 11 years ago

I see, thanks!

	Shao Hang Kao [:skao] Assignee
	Comment 72 • 11 years ago

try result with Patch v3.6: https://tbpl.mozilla.org/?tree=Try&rev=e51e47c6cb64

	Gene Lian (I already quit Mozilla)
	Comment 73 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5ba2a8e039

Fixed some simple conflicts in the b2g.js. Please check it again if I did it right. Thanks!

	Shao Hang Kao [:skao] Assignee
	Comment 74 • 11 years ago

looks good to me, thanks! should I switch the status to RESOLVED ?

	Ryan VanderMeulen [:RyanVM]
	Comment 75 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/fb5ba2a8e039

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 76 • 11 years ago

Merged before branching.

	André Fiedler
	Comment 77 • 10 years ago

Can we make pool.ntp.org configurable? I want to pull the date/time from my own NTP Server at home. Would be nice if I can change this in setting.

	Ed Morley [:emorley]
	Comment 78 • 10 years ago

(In reply to Andreas Gal :gal from comment #42)
> @@ +1985,5 @@
> > +    // Number of seconds between Jan 1, 1900 and Jan 1, 1970.
> > +    // 70 years plus 17 leap days.
> > +    let OFFSET_1900_TO_1970 = ((365 * 70) + 17) * 24 * 60 * 60;
> > +    let NTP_PORT = 123;
> > +    let NTP_POOLS = [
> 
> please make port and domain names a pref in b2g.js
> 
> @@ +1991,5 @@
> > +      "1.pool.ntp.org",
> > +      "2.pool.ntp.org",
> > +      "3.pool.ntp.org"
> > +    ];
> > +    let NTP_TIMEOUT = 30;
> 
> please make this a pref in b2g.js too

These review comments were not fixed before landing; I'll file a followup, since this is impacting on bug 1052016.