949345 - Get telemetry for non-default Cyrillic encodings

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Henri Sivonen (:hsivonen)]

To make a data-driven decision on bug 912466 and foreseeable similar requests, we should get telemetry for the non-windows-1251 Cyrillic encodings.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Add telemetry probes

Comment 2

[Henri Sivonen (:hsivonen)]

Note: I don't expect us to remove ISO-8859-5 or KOI8-R. They are for reference to understanding the relative magnitude of IBM866.

Comment 3

[Henri Sivonen (:hsivonen)]

Comment on attachment 8346518 [details] [diff] [review]
Add telemetry probes

As far as privacy goes, this is the same sort of stuff as bug 935453, but requesting privacy review anyway to be sure to comply with the rules.

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 8346518 [details] [diff] [review]
Add telemetry probes

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

::: intl/uconv/ucvlatin/nsKOI8UToUnicode.cpp
@@ +19,5 @@
>    static const uint16_t g_utMappingTable[] = {
>  #include "koi8u.ut"
>    };
>  
> +  Telemetry::Accumulate(Telemetry::DECODER_INSTANTIATED_KOI8R, true);

DECODER_INSTANTIATED_KOI8U

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Curtis, I assume you want a privacy review but you didn't say anything when you assigned review to me.  What do you need?

Comment 6

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #5)
> Curtis, I assume you want a privacy review but you didn't say anything when
> you assigned review to me.  What do you need?

I was tagged to privacy review the patch, but I don't think I'm qualified for that and since you did the review for bug 935453 I shifted the flag to you, sorry, guess I should have been more explicit when I flipped the flags. It's my first go around on something like this.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8346518 [details] [diff] [review]
Add telemetry probes

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

f=me.

Not r+ because this isn't a code review.  This is indeed the same type of thing as bug 935453, so has the same privacy risk and implications (minor).  Looks fine.

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to Masatoshi Kimura [:emk] from comment #4)
> > +  Telemetry::Accumulate(Telemetry::DECODER_INSTANTIATED_KOI8R, true);
> 
> DECODER_INSTANTIATED_KOI8U

Good catch! Landed with that fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a5cd771bfe

Thanks for the reviews/feedback.

Comment 9

[Henri Sivonen (:hsivonen)]

Comment on attachment 8346518 [details] [diff] [review]
Add telemetry probes

> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 

This is not a bug fix but a telemetry addition. If the took this (as landed with review comments addressed; https://hg.mozilla.org/integration/mozilla-inbound/rev/81a5cd771bfe ) to Aurora, we'd get results 6 weeks earlier.

> User impact if declined: 

Firefox changes that could happen 6 weeks earlier in response to data will happen later.

> Testing completed (on m-c, etc.): 

Nothing to test really.

> Risk to taking this patch (and alternatives if risky): 

Very low risk. Adds telemetry probes using an already-understood pattern.

> String or IDL/UUID changes made by this patch:

None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81a5cd771bfe

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Patch as landad

[Approval Request Comment]

(Attaching the patch as landed  in order to be able to request approval for the patch that addresses review comments in order to avoid accidents with the branch landing.)
 
> Bug caused by (feature/regressing bug #): 

This is not a bug fix but a telemetry addition. If we took this to Aurora, we'd get results 6 weeks earlier.

> User impact if declined: 

6-week delay for whatever the reaction to the telemetry data ends up being.

> Testing completed (on m-c, etc.): 

Landed on m-c. The tree did not burn.

> Risk to taking this patch (and alternatives if risky): 

Very low risk.  This adds telemetry according to a pre-existing pattern.

> String or IDL/UUID changes made by this patch:

None.

Comment 12

[Henri Sivonen (:hsivonen)]

Thanks:
https://hg.mozilla.org/releases/mozilla-aurora/rev/0fa91246cc63