478462 - Include locale, host application name/version and gecko version in CEIP

Status: RESOLVED FIXED

(Other Applications Graveyard :: ChatZilla, enhancement)

Description

[James Ross]

Currently, we only capture the ChatZilla version and version-suffix; Glenjamin asked about locale information and I've previously thought about the host application information. I believe we should capture all 4 parts:
 - selected locale (e.g. "en-US")
 - host application (e.g. "Firefox")
 - host version (e.g. "3.0.5")
 - host gecko version (e.g. "2008092521")

[Excluding the locale, we already use these to make up our UA string.]

These should go in the logger:start event, next to the two existing version fields.

Comment 1

[James Ross]

Mardeg brought up the point we have no *local* time information, so I suggest we should also include Date's getTimezoneOffset() value.

Comment 2

[James Ross]

Attachment: Log host details plus host/client locale

getWinterTimezoneOffset() is pretty magical, but it works. :)

Comment 3

[:Gijs (he/him)]

Comment on attachment 366206 [details] [diff] [review]
Log host details plus host/client locale

>Index: ceip/ceip.js
><snip>
>+CEIP.prototype.getWinterTimezoneOffset =
>+function ceip_getwintertimezoneoffset()
>+{
>+    /* getTimezoneOffset() is +ve for behind, -ve for ahead. We date two dates -
>+     * now and 6 months forward - and pick whichever has the bigger value, which
>+     * is the most behind.
>+     */
>+    var d1 = new Date();
>+    var d2 = new Date();
>+    d2.setMonth(d1.getMonth() + 6);
>+    return d1.getTimezoneOffset() > d2.getTimezoneOffset() ?
>+           d1.getTimezoneOffset() : d2.getTimezoneOffset();
>+}

This is a bit odd. What does "+ve" and "-ve" mean? Because I'm not sure. The documentation for getTimezoneOffset suggests it returns +/-int. Furthermore, why not just:

return Math.max(d1.getTimezoneOffset(), d2.getTimezoneOffset());

Finally, it is quite obvious that this doesn't always work because DST doesn't last exactly 6 months. Even if it did, the method name is misleading as it is quite possible for both d1 and d2 to not be in winter.

What exactly are you trying to achieve, and wouldn't it be better to just pick a date which is going to be in/out of DST whatever the case? 21st of Dec/Jun depending on what you want, which you can just pass directly to new Date().

r- on the above. Otherwise it looks good, but I really don't like this part of the story.

Oh, and we should probably have a bug on cleaning up getVersionInfo and initApplicationCompatibility. Makes no sense to gather the same kind of data twice.

Comment 4

[James Ross]

+ve and -ve are positive and negative, respectively. Math.max is just an oversight from the original code. The function's name is correct. d1/d2 set to Dec/Jun may be more reliable, but the function is having to create information which doesn't exist, it will never been 100% correct.

Comment 5

[:Gijs (he/him)]

(In reply to comment #4)
> +ve and -ve are positive and negative, respectively. Math.max is just an
> oversight from the original code. The function's name is correct. d1/d2 set to
> Dec/Jun may be more reliable, but the function is having to create information
> which doesn't exist, it will never been 100% correct.

Can you elaborate on when/why Dec/Jun would not be correct? I'm not aware of any type of DST in the world that is not in effect in either of those dates, nor any reason why it wouldn't be if it were introduced / rules were changed. If you're wanting to "save daylight time", you'd do it at least on the longest/shortest days...

Comment 6

[James Ross]

I did not say it was provably wrong, and if you mean what I think you mean by "save daylight time", you've completely failed to pay attention to the function's name. When I said that was correct, I meant it.

Comment 7

[:Gijs (he/him)]

Alright, seeing as you think I don't seem to understand what the function is supposed to do, can you explain it?

Comment 8

[James Ross]

It returns the timezone offset for standard time - not daylight time - for the current timezone.

Comment 9

[James Ross]

Attachment: Updated patch

Comment 10

[:Gijs (he/him)]

Comment on attachment 383247 [details] [diff] [review]
Updated patch

Looks great, thanks a lot! r=me :-)

Comment 11

[James Ross]

Checked in --> FIXED.