The packet format is ril-specific, but the NITZ processing is generic. It would be nice to have that be reusable. This comes across as #define RIL_UNSOL_NITZ_TIME_RECEIVED 1008

Ok, I tried several attempts to make the clock general purpose, but calling from the ril_worker thread, and being called before the browser is fully up, caused undue complications. So I simplified it and added the actual clock setting to SystemWorkerManager. Android seems to require the use of an ioctl to set the clock, but I left in the alternate settimeofday system call. Apparently gecko doesn't see the HAVE_ANDROID_OS define, so it is just defined locally. If there is a better test for this, please let me know. I left in one debug output to show the actual string received from the RIL. Googling around seems to indicate that the string may vary in content from one carrier to another, so I want to easily be able to get the string from people who encounter problems with automatically setting the network time.

Comment on attachment 601737 [details] [diff] [review] Patch to Gecko to watch for NITZ RIL messages Review of attachment 601737 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/b2g/SystemWorkerManager.cpp @@ +63,5 @@ > #include "nsRadioInterfaceLayer.h" > #include "nsWifiWorker.h" > > +#define HAVE_ANDROID_OS 1 > + Uh, why not #ifdef MOZ_WIDGET_GONK? ::: dom/system/b2g/ril_worker.js @@ +1261,5 @@ > RIL[UNSOLICITED_ON_USSD] = null; > RIL[UNSOLICITED_ON_USSD_REQUEST] = null; > + > +RIL[UNSOLICITED_NITZ_TIME_RECEIVED] = function UNSOLICITED_NITZ_TIME_RECEIVED() { > + let dateString = Buf.readString(); Nit: two space indentation please. @@ +1277,5 @@ > + let hours = parseInt(dateString.substr(9,2)); > + let minutes = parseInt(dateString.substr(12,2)); > + let seconds = parseInt(dateString.substr(15,2)); > + let offset = parseInt(dateString.substr(17,3)); // offset is in 15 minute units > + let dst = parseInt(dateString.substr(21,2)); // dst already is in local time Always call parseInt with the base argument to keep it from accidentally reading 0-leading numbers in the octal representation, so e.g. parseInt(..., 10). @@ +1278,5 @@ > + let minutes = parseInt(dateString.substr(12,2)); > + let seconds = parseInt(dateString.substr(15,2)); > + let offset = parseInt(dateString.substr(17,3)); // offset is in 15 minute units > + let dst = parseInt(dateString.substr(21,2)); // dst already is in local time > + if (DEBUG) debug("year="+year+" month="+month+" day="+day+" hours="+hours+" minutes="+minutes+" seconds="+seconds+" offset="+offset+" dst="+dst); Nit: over long line needs to be wrapped (and then the `if` needs braces), spaces around operators. @@ +1281,5 @@ > + let dst = parseInt(dateString.substr(21,2)); // dst already is in local time > + if (DEBUG) debug("year="+year+" month="+month+" day="+day+" hours="+hours+" minutes="+minutes+" seconds="+seconds+" offset="+offset+" dst="+dst); > + if (!setDateTime(seconds, minutes, hours, day, month, year, offset)) { > + debug("setDateTime failed"); > + } Ok, so you added setDateTime which directly modifies the system time in the RIL worker. I think that's making it a bit too easy. It's also way beyond the scope of this bug while cutting corners on the general date/time system we want to implement (see bug 714349). If I understand the overall goal correctly, we want to (a) bubble the system time notification to the main thread (RadioInterfaceLayer.js) (b) from there notify whatever component tracks time. For the lack of a better word, let's call it the System Time Manager. (c) the System Time Manager decides whether we want to take the network time at all. We might be configured to get our time from the internet rather than the network. (d) once the System Time Manager has decided what to set the system time to, it does so using whatever system call it needs to do (e) if the system time or timezone changed as a result of (d), the System Time Manager notifies consumers (most notably DOM content) of that change. If I understand the scope of this bug correctly, it would be (a) and (b). (c) through (e) is various bugs tracked by bug 714349.

Fixed the critiques, changed setDateTime to convertDateTime to convert into a more useful time value. The time, the timezone, and timestamp are then communicated to the RadioInterfaceLayer where they await a time manager.

Comment on attachment 602554 [details] [diff] [review] Patch to Gecko to watch for NITZ RIL messages Review of attachment 602554 [details] [diff] [review]: ----------------------------------------------------------------- r- mostly for the use of C++ to reinvent the wheel. Also some code clarity points and various nits. ::: dom/system/b2g/RadioInterfaceLayer.js @@ +221,5 @@ > + if (DEBUG) debug("timeInSeconds is "+message.time.timeInSeconds); > + if (debug) debug("timeZoneInMinutes is " > + +message.time.timeZoneInMinutes); > + if (DEBUG) debug("timeStamp is "+message.time.timeStamp); > + break; Those debug statements seem superfluous. Please remove them. Instead I suggest adding a // TODO comment referring to the bug that will address this (you should probably file one for that blocking bug 714349.) ::: dom/system/b2g/ril_worker.js @@ +1265,5 @@ > + let dateString = Buf.readString(); > + > + // "data" is const char * pointing to NITZ time string > + // in the form "yy/mm/dd,hh:mm:ss(+/-)tz,dt" > + // for example: 12/02/16,03:36:08-20,00,310410 What is "data"? And there is no const char * in JS. @@ +1268,5 @@ > + // in the form "yy/mm/dd,hh:mm:ss(+/-)tz,dt" > + // for example: 12/02/16,03:36:08-20,00,310410 > + > + // this is being printed always to see what other carriers send > + debug("DateTimeZone string " + dateString); That comment is quite superfluous (and in any case, English grammar applies wrt capitalization and punctuation.) Please gate the debug statement with `if (DEBUG)`. @@ +1271,5 @@ > + // this is being printed always to see what other carriers send > + debug("DateTimeZone string " + dateString); > + > + let currentTime = new Date(); > + let now = currentTime.getTime(); let now = Date.now(); @@ +1280,5 @@ > + let hours = parseInt(dateString.substr(9,2),10); > + let minutes = parseInt(dateString.substr(12,2),10); > + let seconds = parseInt(dateString.substr(15,2),10); > + let offset = parseInt(dateString.substr(17,3),10); //offset is in 15 min units > + let dst = parseInt(dateString.substr(21,2),10); //dst already is in local time Coding style nit: spacer after the comma. @@ +1283,5 @@ > + let offset = parseInt(dateString.substr(17,3),10); //offset is in 15 min units > + let dst = parseInt(dateString.substr(21,2),10); //dst already is in local time > + if (DEBUG) debug("year="+year+" month="+month+" day="+day > + +" hours="+hours +" minutes="+minutes+" seconds="+seconds > + +" offset="+offset+" dst="+dst); Please clean this up (multiline statements need braces, spaces between operators, and perhaps a nicer way of displaying the info; debug statements are read by humans, even more so than code, so readability matters.) @@ +1285,5 @@ > + if (DEBUG) debug("year="+year+" month="+month+" day="+day > + +" hours="+hours +" minutes="+minutes+" seconds="+seconds > + +" offset="+offset+" dst="+dst); > + let timeInSeconds = convertDateTime(seconds, minutes, hours, > + day, month, year, offset); let timestamp = Date.UTC(year, month, day, hour, minute, second); timestamp += offset * 15 * 60 * 1000; For next time I suggest asking somebody before re-inventing the wheel... coulda saved you some work. :) @@ +1958,5 @@ > + onNITZ: function onNITZ(timeInSeconds, timeZoneInMinutes, timeStamp) { > + let message = {type: "nitzTime", > + time: {timeInSeconds: timeInSeconds, > + timeZoneInMinutes: timeZoneInMinutes, > + timeStamp: timeStamp}}; Why the sub-object (`time`)? Please just keep the objects flat. Also, it's not obvious to me that 'timeStamp' (usually we spell it timestamp) refers to system time whereas timeInSeconds refers to network time. I suggest naming these parameters and attributes appropriately (here and the variables in RIL[UNSOLICITED_NITZ_TIME_RECEIVED] as well.)

(In reply to Philipp von Weitershausen [:philikon] from comment #4) > @@ +1285,5 @@ > > + if (DEBUG) debug("year="+year+" month="+month+" day="+day > > + +" hours="+hours +" minutes="+minutes+" seconds="+seconds > > + +" offset="+offset+" dst="+dst); > > + let timeInSeconds = convertDateTime(seconds, minutes, hours, > > + day, month, year, offset); > > let timestamp = Date.UTC(year, month, day, hour, minute, second); > timestamp += offset * 15 * 60 * 1000; Note that Date.UTC() expects its parameters to be 0-based, so you want to subtract 1 from all of them (here or in the parseInt() lines.) See GsmPDUHelper.readMessage() [1] for an example usage of Date.UTC(), btw. [1] https://mxr.mozilla.org/mozilla-central/source/dom/system/b2g/ril_worker.js#2560

Used Date.UTC() as suggested. Removed some of the debug output. Note that I left in the one debug without an |if (DEBUG)| as I want to get people to send me the string on other carriers. I'll add in the test in the future. If you feel strongly about it, I'll put the test in. Also note that UTC only needs the -1 on the month (same as mktime). This might be a bug in GsmPDUHelper.readMessage.

(In reply to Jim Straus from comment #6) > Also note that UTC only needs the -1 on the month (same as mktime). This > might be a bug in GsmPDUHelper.readMessage. Um yeah, good point! Thanks for spotting that. Filed bug 733674.

Comment on attachment 603589 [details] [diff] [review] Patch to Gecko to watch for NITZ RIL messages Review of attachment 603589 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! r=me with nits addressed. ::: dom/system/b2g/ril_worker.js @@ +1275,5 @@ > + > + // Temporarily printing this without the if (DEBUG) as there are indications > + // on the web that some carriers don't completely fill this out. I would like > + // to be able to collect this without having to have people turn on DEBUG. > + debug("DateTimeZone string " + dateString); Fair enough. I suggest filing a bug about collecting this data and referencing it in this comment (also, it's not obvious who "I" is, so better to leave it out), e.g.: // Always print the NITZ info so we can collection what different providers send down the pipe (see bug XXX). @@ +1286,5 @@ > + let hours = parseInt(dateString.substr(9,2), 10); > + let minutes = parseInt(dateString.substr(12,2), 10); > + let seconds = parseInt(dateString.substr(15,2), 10); > + let offset = parseInt(dateString.substr(17,3), 10); //offset is in 15 min units > + let dst = parseInt(dateString.substr(21,2), 10); //dst already is in local time Coding style nit: space after commas and proper capitalization + punctuation in comments (also, space after //) @@ +1288,5 @@ > + let seconds = parseInt(dateString.substr(15,2), 10); > + let offset = parseInt(dateString.substr(17,3), 10); //offset is in 15 min units > + let dst = parseInt(dateString.substr(21,2), 10); //dst already is in local time > + > + let timeInSeconds = Date.UTC(year+2000, month-1+, day, hours, minutes, seconds) / 1000; Nits: * Spaces around all operators * Please use a const for 2000. You can reuse PDU_TIMESTAMP_YEAR_OFFSET, the one we use in the SMS code. @@ +1290,5 @@ > + let dst = parseInt(dateString.substr(21,2), 10); //dst already is in local time > + > + let timeInSeconds = Date.UTC(year+2000, month-1+, day, hours, minutes, seconds) / 1000; > + > + if (timeInSeconds == -1) { Date.UTC doesn't return -1, it returns NaN in case of invalid input. Use isNaN() to check. @@ +1962,5 @@ > + onNITZ: function onNITZ(timeInSeconds, timeZoneInMinutes, timeStamp) { > + let message = {type: "nitzTime", > + networkTimeInSeconds: timeInSeconds, > + networkTimeZoneInMinutes: timeZoneInMinutes, > + timestamp: timeStamp}; I would still suggest renaming `timestamp` to localTimestamp or something like that, and not just in the object but also in the parameter names and RIL[UNSOLICITED_NITZ_TIME_RECEIVED]. Neither "now" nor "timeStamp" are very descriptive when time is relative ;)

If you concur, can you please push this up?

Comment on attachment 604681 [details] [diff] [review] Patch to Gecko to watch for NITZ RIL messages Looks great, thanks!

Hi Jim :) I'm working on hooking this up to the B2G time system, which is going to be done very soon. Before that, I'd like to confirm some details with you to make sure I didn't misunderstand anything: 1. I saw you left a comment in the patch: // DST already is in local time. I don't understand why DST has something to do with local time. Could you please elaborate more on that? 2. Does message.networkTimeZoneInMinutes already include the DST part? If not, we can calculate the actual timezone offset by: message.networkTimeZoneInMinutes + message.dstFlag * 60 where the dstFlag can be 0, 1 or 2. Right? 3. What would the following debug message look like in Europe or North America? I can get one in Taipei but just hope to recheck the plus/minus signs of timezones when crossing UTC/GMT. debug("nitzTime networkTime=" + message.networkTimeInSeconds + " timezone=" + message.networkTimeZoneInMinutes + " dst=" + message.dstFlag + " timestamp=" + message.localTimeStampInMS); Any quick responses are highly appreciated! Thanks!

(In reply to Gene Lian [:gene] from comment #12) > I'm working on hooking this up to the B2G time system, which is going to be > done very soon. Before that, I'd like to confirm some details with you to > make sure I didn't misunderstand anything: Thanks for looking into this, Gene, but it's not good Bugzilla etiquette to comment on closed bugs. Please start a thread on dev-b2g or file new bugs. > 1. I saw you left a comment in the patch: // DST already is in local time. I > don't understand why DST has something to do with local time. Could you > please elaborate more on that? I don't understand this either. Remove? > 2. Does message.networkTimeZoneInMinutes already include the DST part? If > not, we can calculate the actual timezone offset by: > > message.networkTimeZoneInMinutes + message.dstFlag * 60 > > where the dstFlag can be 0, 1 or 2. Right? We could, but I'm not sure it's a good idea to throw that information away. Don't we want to set the timezone based on `networkTimeZoneInMinutes`? Btw, `dstFlag` is a terrible name if it's really hours. We should just compute `dstInMinutes` (analogous to `networkTimeZoneInMinutes`) and send that over. Consistent units FTW! Anyway, please address these issues in a separate bug (either the one you're working on right now or a new one.) Thanks!

Comment on attachment 604681 [details] [diff] [review] Patch to Gecko to watch for NITZ RIL messages Review of attachment 604681 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/b2g/ril_worker.js @@ +1322,5 @@ > + // for example: 12/02/16,03:36:08-20,00,310410 > + > + // Always print the NITZ info so we can collection what different providers > + // send down the pipe (see bug XXX). > + // TODO once data is collected, add in |if (DEBUG)| We're to hide all RIL debug messages in bug 960961. Please file a bug if this still applies.