Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1237363002:
Adds support for Macedonian keyboard. (Closed)
|
Created:
5 years, 5 months ago by Shu Chen Modified:
5 years, 5 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, asvitkine+watch_chromium.org, nona+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds support for Macedonian keyboard.
BUG=352459
TEST=Verified on linux_chromeos.
Committed: https://crrev.com/3fc5b7b9fe3bc156d6d72457c28c8d95b0484faf
Cr-Commit-Position: refs/heads/master@{#339334}
Patch Set 1 #
Total comments: 7
Patch Set 2 : rebased. #Patch Set 3 : #
Messages
Total messages: 26 (5 generated)
shuchen@chromium.org changed reviewers: + isherman@chromium.org
isherman@, can you please take a look at the change in histograms.xml? I can take care of the rest changes.
https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59086: + <int value="110901" label="xkb:mn::mon">Mongolian keyboard</int> Hmm, why did this value change? https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59113: + <int value="111711" label="xkb:us:intl:por">US International keyboard</int> Likewise, why did these values change?
On 2015/07/16 04:04:52, Shu Chen wrote: > I can take care of the rest changes. Could you please clarify what you mean by this? Even if you are an owner for the remaining files, you should still have them reviewed by someone else.
On 2015/07/16 04:50:30, Ilya Sherman (Away 7.18-8.03) wrote: > On 2015/07/16 04:04:52, Shu Chen wrote: > > I can take care of the rest changes. > > Could you please clarify what you mean by this? Even if you are an owner for > the remaining files, you should still have them reviewed by someone else. I've added suzhe@chromium.org as TBR.
https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59086: + <int value="110901" label="xkb:mn::mon">Mongolian keyboard</int> On 2015/07/16 04:49:47, Ilya Sherman (Away 7.18-8.03) wrote: > Hmm, why did this value change? The ID sequence matters, because the input method ID for statistics has some rules: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59113: + <int value="111711" label="xkb:us:intl:por">US International keyboard</int> On 2015/07/16 04:49:47, Ilya Sherman (Away 7.18-8.03) wrote: > Likewise, why did these values change? Ditto. US Dvorak Programmer keyboard was added in cl https://codereview.chromium.org/1113683003, but forgot to change the histograms.xml.
On 2015/07/16 05:10:03, Shu Chen wrote: > On 2015/07/16 04:50:30, Ilya Sherman (Away 7.18-8.03) wrote: > > On 2015/07/16 04:04:52, Shu Chen wrote: > > > I can take care of the rest changes. > > > > Could you please clarify what you mean by this? Even if you are an owner for > > the remaining files, you should still have them reviewed by someone else. > > I've added mailto:suzhe@chromium.org as TBR. This does not seem to me like an appropriate use of TBR. Could you please explain why you are seeking to avoid a proper review for these files? If the changes are fairly trivial, then it should be easy for a fellow developer to quickly review them. https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59086: + <int value="110901" label="xkb:mn::mon">Mongolian keyboard</int> On 2015/07/16 05:13:35, Shu Chen wrote: > On 2015/07/16 04:49:47, Ilya Sherman (Away 7.18-8.03) wrote: > > Hmm, why did this value change? > > The ID sequence matters, because the input method ID for statistics has some > rules: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... You should find a mapping that is more stable than what you're using. It's not appropriate for bucket value "110900" to mean "Mongolian Keyboard" for some versions of Chrome, and "Macedonian keyboard" for others. Otherwise, you'll end up with misleading data on the histograms dashboard, which will always use the latest label definition from histograms.xml.
On 2015/07/16 06:25:31, Ilya Sherman (Away 7.18-8.03) wrote: > On 2015/07/16 05:10:03, Shu Chen wrote: > > On 2015/07/16 04:50:30, Ilya Sherman (Away 7.18-8.03) wrote: > > > On 2015/07/16 04:04:52, Shu Chen wrote: > > > > I can take care of the rest changes. > > > > > > Could you please clarify what you mean by this? Even if you are an owner > for > > > the remaining files, you should still have them reviewed by someone else. > > > > I've added mailto:suzhe@chromium.org as TBR. > > This does not seem to me like an appropriate use of TBR. Could you please > explain why you are seeking to avoid a proper review for these files? If the > changes are fairly trivial, then it should be easy for a fellow developer to > quickly review them. suzhe@ is on vacation today. So I added him as TBR for such trivial/structural change. Now I've added him as reviewer.
shuchen@chromium.org changed reviewers: + suzhe@chromium.org
https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59086: + <int value="110901" label="xkb:mn::mon">Mongolian keyboard</int> On 2015/07/16 06:25:31, Ilya Sherman (Away 7.18-8.03) wrote: > On 2015/07/16 05:13:35, Shu Chen wrote: > > On 2015/07/16 04:49:47, Ilya Sherman (Away 7.18-8.03) wrote: > > > Hmm, why did this value change? > > > > The ID sequence matters, because the input method ID for statistics has some > > rules: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > You should find a mapping that is more stable than what you're using. It's not > appropriate for bucket value "110900" to mean "Mongolian Keyboard" for some > versions of Chrome, and "Macedonian keyboard" for others. Otherwise, you'll end > up with misleading data on the histograms dashboard, which will always use the > latest label definition from histograms.xml. Do you have any idea of making the mapping stable? I am not sure whether UMA histograms support some kinds of hash values? The input method ID histograms are used for short-terms usage findings. So usually we are not interested in long term trend. We often need to know something like "for now how many 7-day users for this input method". Considering we won't add new input methods from day to day, I think we can endure such kind of instability.
On 2015/07/16 06:38:30, Shu Chen wrote: > On 2015/07/16 06:25:31, Ilya Sherman (Away 7.18-8.03) wrote: > > On 2015/07/16 05:10:03, Shu Chen wrote: > > > On 2015/07/16 04:50:30, Ilya Sherman (Away 7.18-8.03) wrote: > > > > On 2015/07/16 04:04:52, Shu Chen wrote: > > > > > I can take care of the rest changes. > > > > > > > > Could you please clarify what you mean by this? Even if you are an owner > > for > > > > the remaining files, you should still have them reviewed by someone else. > > > > > > I've added mailto:suzhe@chromium.org as TBR. > > > > This does not seem to me like an appropriate use of TBR. Could you please > > explain why you are seeking to avoid a proper review for these files? If the > > changes are fairly trivial, then it should be easy for a fellow developer to > > quickly review them. > > suzhe@ is on vacation today. So I added him as TBR for such trivial/structural > change. > > Now I've added him as reviewer. Thanks. I've also removed him from being TBR'ed. Note that if you believe the changes to be sufficiently simple, it's fine to ask someone who isn't an owner of the code to review the code. So, if no owner is available, you can ask any other reviewer to look at it. (If you'd like, I could look at it.) It's then up to that reviewer to either agree to review the code, or bow out, because they don't fully understand it. But it's not appropriate to TBR code just because someone is out on vacation. Apologies if this sounds harsh, but I think this is very important for the Chromium culture. https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1237363002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:59086: + <int value="110901" label="xkb:mn::mon">Mongolian keyboard</int> On 2015/07/16 06:38:44, Shu Chen wrote: > On 2015/07/16 06:25:31, Ilya Sherman (Away 7.18-8.03) wrote: > > On 2015/07/16 05:13:35, Shu Chen wrote: > > > On 2015/07/16 04:49:47, Ilya Sherman (Away 7.18-8.03) wrote: > > > > Hmm, why did this value change? > > > > > > The ID sequence matters, because the input method ID for statistics has some > > > rules: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > You should find a mapping that is more stable than what you're using. It's > not > > appropriate for bucket value "110900" to mean "Mongolian Keyboard" for some > > versions of Chrome, and "Macedonian keyboard" for others. Otherwise, you'll > end > > up with misleading data on the histograms dashboard, which will always use the > > latest label definition from histograms.xml. > > Do you have any idea of making the mapping stable? > I am not sure whether UMA histograms support some kinds of hash values? > > The input method ID histograms are used for short-terms usage findings. > So usually we are not interested in long term trend. > We often need to know something like "for now how many 7-day users for this > input method". > Considering we won't add new input methods from day to day, I think we can > endure such kind of instability. The easiest thing to do would be to just compute a hash of the keyboard name -- that would be stable. Any hashing algorithm that tends to avoid collisions in a 32-bit output space ought to be fine. Even if you don't anticipate tracking long-term trends, I bet that you often look at data collected from multiple versions of Chrome at once -- the current stable, beta, dev, and canary channels, as well as the sizable population of users who are stuck on older builds. For this reason, it's really fairly important to maintain the stability of the histogram bucket value semantics, even if you only look at short-term data.
shuchen@chromium.org changed reviewers: + nona@chromium.org
+nona@ to cover IME stuff.
On 2015/07/16 06:47:46, Ilya Sherman (Away 7.18-8.03) wrote: ... > Thanks. I've also removed him from being TBR'ed. Note that if you believe the > changes to be sufficiently simple, it's fine to ask someone who isn't an owner > of the code to review the code. So, if no owner is available, you can ask any > other reviewer to look at it. (If you'd like, I could look at it.) It's then > up to that reviewer to either agree to review the code, or bow out, because they > don't fully understand it. But it's not appropriate to TBR code just because > someone is out on vacation. Apologies if this sounds harsh, but I think this is > very important for the Chromium culture. Thanks. I've just added nona@ as reviewer. ... > The easiest thing to do would be to just compute a hash of the keyboard name -- > that would be stable. Any hashing algorithm that tends to avoid collisions in a > 32-bit output space ought to be fine. > > Even if you don't anticipate tracking long-term trends, I bet that you often > look at data collected from multiple versions of Chrome at once -- the current > stable, beta, dev, and canary channels, as well as the sizable population of > users who are stuck on older builds. For this reason, it's really fairly > important to maintain the stability of the histogram bucket value semantics, > even if you only look at short-term data. Any specific hashing algorithm you would suggest?
On 2015/07/16 06:57:01, Shu Chen wrote: > > The easiest thing to do would be to just compute a hash of the keyboard name > -- > > that would be stable. Any hashing algorithm that tends to avoid collisions in > a > > 32-bit output space ought to be fine. > > > > Even if you don't anticipate tracking long-term trends, I bet that you often > > look at data collected from multiple versions of Chrome at once -- the current > > stable, beta, dev, and canary channels, as well as the sizable population of > > users who are stuck on older builds. For this reason, it's really fairly > > important to maintain the stability of the histogram bucket value semantics, > > even if you only look at short-term data. > Any specific hashing algorithm you would suggest? You could try the hashing algorithm in base/hash.h and see whether it has any collisions for your dataset. If it works, it looks pretty straightforward to use.
lgtm for ime stuff, please get lgtm from Ilya. Thank you.
LGTM for IME part.
On 2015/07/16 07:03:28, Ilya Sherman (Away 7.18-8.03) wrote: > On 2015/07/16 06:57:01, Shu Chen wrote: > > > The easiest thing to do would be to just compute a hash of the keyboard name > > -- > > > that would be stable. Any hashing algorithm that tends to avoid collisions > in > > a > > > 32-bit output space ought to be fine. > > > > > > Even if you don't anticipate tracking long-term trends, I bet that you often > > > look at data collected from multiple versions of Chrome at once -- the > current > > > stable, beta, dev, and canary channels, as well as the sizable population of > > > users who are stuck on older builds. For this reason, it's really fairly > > > important to maintain the stability of the histogram bucket value semantics, > > > even if you only look at short-term data. > > Any specific hashing algorithm you would suggest? > > You could try the hashing algorithm in base/hash.h and see whether it has any > collisions for your dataset. If it works, it looks pretty straightforward to > use. I've made a cl https://codereview.chromium.org/1237493003 to use hash. I've verified there is no collisions.
Please ping this review once your CL to use a hash for histograms has landed, and you've rebase this CL on top of it. Thanks!
On 2015/07/17 01:56:01, Ilya Sherman (Away 7.18-8.03) wrote: > Please ping this review once your CL to use a hash for histograms has landed, > and you've rebase this CL on top of it. Thanks! I've uploaded the new patchset. Please take another look. thanks
histograms.xml lgtm, thanks
The CQ bit was checked by isherman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from suzhe@chromium.org, nona@chromium.org Link to the patchset: https://codereview.chromium.org/1237363002/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1237363002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3fc5b7b9fe3bc156d6d72457c28c8d95b0484faf Cr-Commit-Position: refs/heads/master@{#339334} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
