Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
561223002:
Updates the histograms for IMF and IMEs according to the new design. (Closed)
|
Created:
6 years, 3 months ago by Shu Chen Modified:
6 years, 3 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, asvitkine+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUpdates the histograms for IMF and IMEs according to the new design.
BUG=367631
TEST=Verified on linux_chromeos.
Committed: https://crrev.com/0d95894f4e846d7352d9813cd34f43efdb6e1e4e
Cr-Commit-Position: refs/heads/master@{#295417}
Patch Set 1 #Patch Set 2 : fixed unit_tests. #
Total comments: 6
Patch Set 3 : nits. #Patch Set 4 : #
Total comments: 57
Patch Set 5 : revised per comments. #
Total comments: 20
Patch Set 6 : revised per comments. #
Messages
Total messages: 15 (2 generated)
shuchen@chromium.org changed reviewers: + isherman@chromium.org, nona@chromium.org
+isherman@ for histograms.xml. +nona@ for IMF changes. Please note I've added some additional histograms meta which is not used in this cl. Those will be used in IME extension JS. Thanks!
almost lgtm. please address small comments https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_engine.cc (right): https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_engine.cc:264: UMA_HISTOGRAM_COUNTS("InputMethod.CommitLength", nit: please wrap {} for two-line body. https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_engine_unittest.cc (right): https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_engine_unittest.cc:287: engine_->CommitText(1, "入力", &error); I prefer "\xE5\x85\xA5\xE5\x8A\x9B" with comment like "// two UTF-8 encoded unicode characters" some editor may not be able to show this characters. https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:853: std::map<int, std::vector<std::string>> buckets; I'm sorry I'm not familiar with recent Chrome OS build env but can we C++11 features? If not, please replace ">>" with "> >". If C++11 is allowed in Chrome OS, please keep as it.
https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_engine.cc (right): https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_engine.cc:264: UMA_HISTOGRAM_COUNTS("InputMethod.CommitLength", On 2014/09/12 02:15:37, Seigo Nonaka wrote: > nit: please wrap {} for two-line body. Done. https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_engine_unittest.cc (right): https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_engine_unittest.cc:287: engine_->CommitText(1, "入力", &error); On 2014/09/12 02:15:37, Seigo Nonaka wrote: > I prefer "\xE5\x85\xA5\xE5\x8A\x9B" with comment like "// two UTF-8 encoded > unicode characters" some editor may not be able to show this characters. Done. https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/561223002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:853: std::map<int, std::vector<std::string>> buckets; On 2014/09/12 02:15:37, Seigo Nonaka wrote: > I'm sorry I'm not familiar with recent Chrome OS build env but can we C++11 > features? > If not, please replace ">>" with "> >". If C++11 is allowed in Chrome OS, please > keep as it. Done. Changed it as "> >" to be safe.
Kindly pinging. isherman@, do you have time to review this cl? For the input method ID usage stats, I found an example for languageCode: https://code.google.com/p/chromium/codesearch#chromium/src/components/languag... It uses UMA_HISTOGRAM_SPARSE_SLOWLY, while in my CL, "InputMethod.ID" uses CustomHistogram. isherman@, do you have suggestion/preference about which one I should use? Thanks, Shu
https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_engine_unittest.cc (right): https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_engine_unittest.cc:171: } Please use base::HistogramTester [1] rather than re-implementing some of the functionality. (Fine to make this change in a follow-up CL, but please do clean this up!) [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:62: INPUT_METHOD_CATEGORY_MAX nit: I find these names needlessly cryptic. Would it be reasonable to use less cryptic names? If not, please at least add comments to provide documentation. https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:875: base::HistogramBase::kUmaTargetedHistogramFlag); Hmm, why do you capture this reference, rather than using a macro? I don't fully understand what the IDs you're trying to emit are; but I bet that a sparse histogram would work nicely for your needs. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9713: + The suggestion index of the suggestion list which user chooses to commit. nit: "of the suggestion list" -> "of the suggestion list item" https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9724: + suggestion. 5: User reverts the committed text. nit: It might be more appropriate to move the bucket explanations into the enum. You can write an enum value as <int value="0" label="short label">Long textual explanation that's shown on mouse hover</int> https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9728: +<histogram name="IME.VK.InitLatency" units="milliseconds"> nit: I'd prefer that you use "VirtualKeyboard" rather than "VK". In general, it's preferable to spell out acronyms, since not everyone on the team is going to be familiar with the acronyms that you use. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9737: + <summary>The usage for active layout of on-screen keyboard.</summary> nit: Please rephrase to something like "The layout of the on-screen keyboard. Logged when [insert condition here]." For "[insert condition here]", I'm guessing it's something like "the keyboard is shown or the layout is changed", but if it's something else, please describe that. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9742: + <summary>The count of switching layouts on the on-screen keyboard.</summary> I don't understand what this means. It would probably help to document when this is logged, and what the count is relative to (i.e. when, if ever, the count is reset back to 0). https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9899: +<histogram name="InputMethod.ActiveCount"> What is the difference between "IME" and "InputMethod"? Can we group these histograms together under a single prefix? https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9902: + The number of active input methods recorded when user is logged in to CrOS. What is an "input method", exactly? Is it the number of virtual keyboard layouts the that the user has enabled? https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9902: + The number of active input methods recorded when user is logged in to CrOS. nit: "input methods recorded when" -> "input methods. Recorded when" https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9902: + The number of active input methods recorded when user is logged in to CrOS. nit: "when user is logged in to CrOS" -> "when the user logs in to Chrome OS". https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9909: + The breakdown of input method usage by input method category, which is nit: ", which is recorded" -> ". Recorded" https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9910: + recorded when user presses keys on physical or on-screen keyboard. So, this is recorded once per key press? https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9910: + recorded when user presses keys on physical or on-screen keyboard. nit: "user" -> "the user" https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9922: + The breakdown of input method usage by input method IDs, which is recorded nit: ", which is recorded" -> ". Recorded" https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9923: + when user presses keys on physical or on-screen keyboard. nit: "user" -> "the user" https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9923: + when user presses keys on physical or on-screen keyboard. I don't really understand what these IDs look like. Could you associate an enum with this histogram? https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43496: + <int value="3" label="X -> Y(1)"/> nit: "(0)" and "(1)" are not very clear, IMO. Are there any other representations that you can think of that might be a little clearer? https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43504: + <int value="2" label="CompactMore"/> Optional nit: WDYT of something like "Compact w/ symbols" and "Extra compact" (assuming that's what the third item means)? https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43567: + <int value="0" label="XKB"/> Isn't there an UNK(nown) bucket? https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43572: + <int value="5" label="T13N"/> These labels are super cryptic. Can you use less cryptic labels? https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:52811: + <affected-histogram name="IME.Commit.Index.FR"/> Hmm, why is this histogram not listed together with the other three, below? https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:52834: + <suffix name="LayoutLoaded" label="LayoutLoaded"/> nit: These labels will be appended to the histogram summary, so please format them as English sentences.
Thanks for your review! Please take a look at the latest patchset. https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_engine_unittest.cc (right): https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_engine_unittest.cc:171: } On 2014/09/16 00:00:28, Ilya Sherman wrote: > Please use base::HistogramTester [1] rather than re-implementing some of the > functionality. (Fine to make this change in a follow-up CL, but please do clean > this up!) > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... Done. But the test failed at ExpectUniqueSample(), expect 1 but actual 3. Do you have any ideas? https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:62: INPUT_METHOD_CATEGORY_MAX On 2014/09/16 00:00:28, Ilya Sherman wrote: > nit: I find these names needlessly cryptic. Would it be reasonable to use less > cryptic names? If not, please at least add comments to provide documentation. Done. I've added comments. https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:875: base::HistogramBase::kUmaTargetedHistogramFlag); On 2014/09/16 00:00:28, Ilya Sherman wrote: > Hmm, why do you capture this reference, rather than using a macro? I don't > fully understand what the IDs you're trying to emit are; but I bet that a sparse > histogram would work nicely for your needs. Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9713: + The suggestion index of the suggestion list which user chooses to commit. On 2014/09/16 00:00:29, Ilya Sherman wrote: > nit: "of the suggestion list" -> "of the suggestion list item" Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9724: + suggestion. 5: User reverts the committed text. On 2014/09/16 00:00:30, Ilya Sherman wrote: > nit: It might be more appropriate to move the bucket explanations into the enum. > You can write an enum value as > > <int value="0" label="short label">Long textual explanation that's shown on > mouse hover</int> Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9728: +<histogram name="IME.VK.InitLatency" units="milliseconds"> On 2014/09/16 00:00:29, Ilya Sherman wrote: > nit: I'd prefer that you use "VirtualKeyboard" rather than "VK". In general, > it's preferable to spell out acronyms, since not everyone on the team is going > to be familiar with the acronyms that you use. Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9737: + <summary>The usage for active layout of on-screen keyboard.</summary> On 2014/09/16 00:00:29, Ilya Sherman wrote: > nit: Please rephrase to something like "The layout of the on-screen keyboard. > Logged when [insert condition here]." For "[insert condition here]", I'm > guessing it's something like "the keyboard is shown or the layout is changed", > but if it's something else, please describe that. Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9742: + <summary>The count of switching layouts on the on-screen keyboard.</summary> On 2014/09/16 00:00:29, Ilya Sherman wrote: > I don't understand what this means. It would probably help to document when > this is logged, and what the count is relative to (i.e. when, if ever, the count > is reset back to 0). Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9899: +<histogram name="InputMethod.ActiveCount"> On 2014/09/16 00:00:29, Ilya Sherman wrote: > What is the difference between "IME" and "InputMethod"? Can we group these > histograms together under a single prefix? "InputMethod" prefix is for histograms logged in IMF (Input Method Framework) of Chrome OS. "IME" prefix is for histograms logged in IME extension JS through metricsPrivate API. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9902: + The number of active input methods recorded when user is logged in to CrOS. On 2014/09/16 00:00:29, Ilya Sherman wrote: > nit: "when user is logged in to CrOS" -> "when the user logs in to Chrome OS". Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9902: + The number of active input methods recorded when user is logged in to CrOS. On 2014/09/16 00:00:30, Ilya Sherman wrote: > nit: "input methods recorded when" -> "input methods. Recorded when" Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9902: + The number of active input methods recorded when user is logged in to CrOS. On 2014/09/16 00:00:29, Ilya Sherman wrote: > What is an "input method", exactly? Is it the number of virtual keyboard > layouts the that the user has enabled? it's not only for virtual keyboard but also for physical keyboard. for example, a typical chinese pinyin input method user will enable both US keyboard and Chinese Pinyin IME. So when she logs in to Chrome OS, the active input method count is 2. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9909: + The breakdown of input method usage by input method category, which is On 2014/09/16 00:00:29, Ilya Sherman wrote: > nit: ", which is recorded" -> ". Recorded" Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9910: + recorded when user presses keys on physical or on-screen keyboard. On 2014/09/16 00:00:29, Ilya Sherman wrote: > So, this is recorded once per key press? I've changed the meaning. Please let me know whether it's still confusing. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9910: + recorded when user presses keys on physical or on-screen keyboard. On 2014/09/16 00:00:30, Ilya Sherman wrote: > nit: "user" -> "the user" Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9922: + The breakdown of input method usage by input method IDs, which is recorded On 2014/09/16 00:00:29, Ilya Sherman wrote: > nit: ", which is recorded" -> ". Recorded" Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9923: + when user presses keys on physical or on-screen keyboard. On 2014/09/16 00:00:29, Ilya Sherman wrote: > nit: "user" -> "the user" Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9923: + when user presses keys on physical or on-screen keyboard. On 2014/09/16 00:00:29, Ilya Sherman wrote: > I don't really understand what these IDs look like. Could you associate an enum > with this histogram? Please check the comment at line 262 in input_method_manager_impl.h. Can I not create the enum for the IDs? It would be a long list. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9923: + when user presses keys on physical or on-screen keyboard. On 2014/09/16 00:00:29, Ilya Sherman wrote: > I don't really understand what these IDs look like. Could you associate an enum > with this histogram? Please check the comment at line 262 in input_method_manager_impl.h. Can I not create the enum for the IDs? It would be a long list. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43496: + <int value="3" label="X -> Y(1)"/> On 2014/09/16 00:00:29, Ilya Sherman wrote: > nit: "(0)" and "(1)" are not very clear, IMO. Are there any other > representations that you can think of that might be a little clearer? Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43504: + <int value="2" label="CompactMore"/> On 2014/09/16 00:00:29, Ilya Sherman wrote: > Optional nit: WDYT of something like "Compact w/ symbols" and "Extra compact" > (assuming that's what the third item means)? I would prefer to use them as it is used in discussions, designs and coding. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43567: + <int value="0" label="XKB"/> On 2014/09/16 00:00:29, Ilya Sherman wrote: > Isn't there an UNK(nown) bucket? Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43572: + <int value="5" label="T13N"/> On 2014/09/16 00:00:30, Ilya Sherman wrote: > These labels are super cryptic. Can you use less cryptic labels? Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:52811: + <affected-histogram name="IME.Commit.Index.FR"/> On 2014/09/16 00:00:29, Ilya Sherman wrote: > Hmm, why is this histogram not listed together with the other three, below? Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:52834: + <suffix name="LayoutLoaded" label="LayoutLoaded"/> On 2014/09/16 00:00:29, Ilya Sherman wrote: > nit: These labels will be appended to the histogram summary, so please format > them as English sentences. Done.
https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_engine_unittest.cc (right): https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_engine_unittest.cc:171: } On 2014/09/16 16:36:29, Shu Chen wrote: > On 2014/09/16 00:00:28, Ilya Sherman wrote: > > Please use base::HistogramTester [1] rather than re-implementing some of the > > functionality. (Fine to make this change in a follow-up CL, but please do > clean > > this up!) > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... > > Done. But the test failed at ExpectUniqueSample(), expect 1 but actual 3. Do you > have any ideas? Thanks for updating the test code! ExpectUniqueSample() is intended to be used for a histogram where only one bucket is filled. Since that's not the case that you're testing, you want to use ExpectBucketCount() instead. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9899: +<histogram name="InputMethod.ActiveCount"> On 2014/09/16 16:36:30, Shu Chen wrote: > On 2014/09/16 00:00:29, Ilya Sherman wrote: > > What is the difference between "IME" and "InputMethod"? Can we group these > > histograms together under a single prefix? > > "InputMethod" prefix is for histograms logged in IMF (Input Method Framework) of > Chrome OS. > "IME" prefix is for histograms logged in IME extension JS through metricsPrivate > API. It's best not to introduce too many top-level names (prefixes), so I'll strongly encourage you to combine these two sets of histograms under a single prefix. If you think it's useful to distinguish them, you can use secondary prefixes, e.g. "Input.IME.*" and "Input.InputMethod.*". https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9902: + The number of active input methods recorded when user is logged in to CrOS. On 2014/09/16 16:36:30, Shu Chen wrote: > On 2014/09/16 00:00:29, Ilya Sherman wrote: > > What is an "input method", exactly? Is it the number of virtual keyboard > > layouts the that the user has enabled? > > it's not only for virtual keyboard but also for physical keyboard. > > for example, a typical chinese pinyin input method user will enable both US > keyboard and Chinese Pinyin IME. > So when she logs in to Chrome OS, the active input method count is 2. I think this would be useful to clarify in the histogram's <summary>. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9923: + when user presses keys on physical or on-screen keyboard. On 2014/09/16 16:36:31, Shu Chen wrote: > On 2014/09/16 00:00:29, Ilya Sherman wrote: > > I don't really understand what these IDs look like. Could you associate an > enum > > with this histogram? > > Please check the comment at line 262 in input_method_manager_impl.h. > Can I not create the enum for the IDs? It would be a long list. You don't need to generate an enum in C++, but it would be very helpful to have semantically meaningful labels on the histograms dashboard. It doesn't seem like it should be too hard to write a script that would generate all possible enum values with some reasonably small upper-bound on the index (the third part of the tuple). https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:56: INPUT_METHOD_CATEGORY_UNK = 0, nit: Can "UNK" be expanded to "Unknown"? https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:57: INPUT_METHOD_CATEGORY_XKB, // XKB input methods What is XKB? https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:862: std::vector<int> stat_id_custom_ranges; nit: Looks like this is now unused. https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:866: for (size_t j = 0; j < i->second.size(); ++j) { Somewhere, you should probably verify that size() does not exceed 100. https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right): https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.h:267: // 112 means the first char after prefix is 'p' of 'pinyin'; I assume you're restricting only to lower ASCII characters -- is that true? If so, it would probably be more convenient to use an index in the range 0 to 25 (or 1 to 26), rather than the raw ASCII value. https://codereview.chromium.org/561223002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/561223002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9722: + and chooses X as top suggestion. Hmm, why did you leave the label for bucket 0 while removing the others? https://codereview.chromium.org/561223002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9744: + The count of layout switching actions when virtual keyboard is alive. nit: "when" -> "while". Also, please document when this is recorded -- is it when the virtual keyboard is closed / destroyed? https://codereview.chromium.org/561223002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43651: + <int value="0" label="UNK"/> nit: "Unknown" https://codereview.chromium.org/561223002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43657: + <int value="6" label="T13N"/> I would at least include on-hover text that expands these abbreviations.
https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_engine_unittest.cc (right): https://codereview.chromium.org/561223002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_engine_unittest.cc:171: } On 2014/09/16 21:51:48, Ilya Sherman wrote: > On 2014/09/16 16:36:29, Shu Chen wrote: > > On 2014/09/16 00:00:28, Ilya Sherman wrote: > > > Please use base::HistogramTester [1] rather than re-implementing some of the > > > functionality. (Fine to make this change in a follow-up CL, but please do > > clean > > > this up!) > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/histogra... > > > > Done. But the test failed at ExpectUniqueSample(), expect 1 but actual 3. Do > you > > have any ideas? > > Thanks for updating the test code! > > ExpectUniqueSample() is intended to be used for a histogram where only one > bucket is filled. Since that's not the case that you're testing, you want to > use ExpectBucketCount() instead. Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9899: +<histogram name="InputMethod.ActiveCount"> On 2014/09/16 21:51:48, Ilya Sherman wrote: > On 2014/09/16 16:36:30, Shu Chen wrote: > > On 2014/09/16 00:00:29, Ilya Sherman wrote: > > > What is the difference between "IME" and "InputMethod"? Can we group these > > > histograms together under a single prefix? > > > > "InputMethod" prefix is for histograms logged in IMF (Input Method Framework) > of > > Chrome OS. > > "IME" prefix is for histograms logged in IME extension JS through > metricsPrivate > > API. > > It's best not to introduce too many top-level names (prefixes), so I'll strongly > encourage you to combine these two sets of histograms under a single prefix. If > you think it's useful to distinguish them, you can use secondary prefixes, e.g. > "Input.IME.*" and "Input.InputMethod.*". Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9902: + The number of active input methods recorded when user is logged in to CrOS. On 2014/09/16 21:51:48, Ilya Sherman wrote: > On 2014/09/16 16:36:30, Shu Chen wrote: > > On 2014/09/16 00:00:29, Ilya Sherman wrote: > > > What is an "input method", exactly? Is it the number of virtual keyboard > > > layouts the that the user has enabled? > > > > it's not only for virtual keyboard but also for physical keyboard. > > > > for example, a typical chinese pinyin input method user will enable both US > > keyboard and Chinese Pinyin IME. > > So when she logs in to Chrome OS, the active input method count is 2. > > I think this would be useful to clarify in the histogram's <summary>. Done. https://codereview.chromium.org/561223002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9923: + when user presses keys on physical or on-screen keyboard. On 2014/09/16 21:51:48, Ilya Sherman wrote: > On 2014/09/16 16:36:31, Shu Chen wrote: > > On 2014/09/16 00:00:29, Ilya Sherman wrote: > > > I don't really understand what these IDs look like. Could you associate an > > enum > > > with this histogram? > > > > Please check the comment at line 262 in input_method_manager_impl.h. > > Can I not create the enum for the IDs? It would be a long list. > > You don't need to generate an enum in C++, but it would be very helpful to have > semantically meaningful labels on the histograms dashboard. It doesn't seem > like it should be too hard to write a script that would generate all possible > enum values with some reasonably small upper-bound on the index (the third part > of the tuple). Done. https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:56: INPUT_METHOD_CATEGORY_UNK = 0, On 2014/09/16 21:51:48, Ilya Sherman wrote: > nit: Can "UNK" be expanded to "Unknown"? Done. https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:57: INPUT_METHOD_CATEGORY_XKB, // XKB input methods On 2014/09/16 21:51:48, Ilya Sherman wrote: > What is XKB? XKB (XKeyboard, used in XWindow) is the keyboard layout system on Linux. So "US keyboard" is an XKB based input method, "French keyboard" is another. https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:862: std::vector<int> stat_id_custom_ranges; On 2014/09/16 21:51:48, Ilya Sherman wrote: > nit: Looks like this is now unused. Done. https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:866: for (size_t j = 0; j < i->second.size(); ++j) { On 2014/09/16 21:51:48, Ilya Sherman wrote: > Somewhere, you should probably verify that size() does not exceed 100. Done. https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right): https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.h:267: // 112 means the first char after prefix is 'p' of 'pinyin'; On 2014/09/16 21:51:48, Ilya Sherman wrote: > I assume you're restricting only to lower ASCII characters -- is that true? If > so, it would probably be more convenient to use an index in the range 0 to 25 > (or 1 to 26), rather than the raw ASCII value. I'm not using range of 0 to 25 because it can be digit. e.g. "hangul_2set". https://codereview.chromium.org/561223002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/561223002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9722: + and chooses X as top suggestion. On 2014/09/16 21:51:48, Ilya Sherman wrote: > Hmm, why did you leave the label for bucket 0 while removing the others? Done. Errh, I should have been more careful when revising the code. https://codereview.chromium.org/561223002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:9744: + The count of layout switching actions when virtual keyboard is alive. On 2014/09/16 21:51:49, Ilya Sherman wrote: > nit: "when" -> "while". Also, please document when this is recorded -- is it > when the virtual keyboard is closed / destroyed? Done. https://codereview.chromium.org/561223002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43651: + <int value="0" label="UNK"/> On 2014/09/16 21:51:48, Ilya Sherman wrote: > nit: "Unknown" Done. https://codereview.chromium.org/561223002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:43657: + <int value="6" label="T13N"/> On 2014/09/16 21:51:49, Ilya Sherman wrote: > I would at least include on-hover text that expands these abbreviations. Done.
LGTM, thanks! https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:57: INPUT_METHOD_CATEGORY_XKB, // XKB input methods On 2014/09/17 15:17:28, Shu Chen wrote: > On 2014/09/16 21:51:48, Ilya Sherman wrote: > > What is XKB? > > XKB (XKeyboard, used in XWindow) is the keyboard layout system on Linux. > > So "US keyboard" is an XKB based input method, "French keyboard" is another. > My point was that you should expand out the acronym in the comment ;)
isherman@, thanks for your review! https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/561223002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:57: INPUT_METHOD_CATEGORY_XKB, // XKB input methods On 2014/09/17 18:49:48, Ilya Sherman wrote: > On 2014/09/17 15:17:28, Shu Chen wrote: > > On 2014/09/16 21:51:48, Ilya Sherman wrote: > > > What is XKB? > > > > XKB (XKeyboard, used in XWindow) is the keyboard layout system on Linux. > > > > So "US keyboard" is an XKB based input method, "French keyboard" is another. > > > > My point was that you should expand out the acronym in the comment ;) Sorry I wasn't clear enough. We rarely expand XKB to KXkeyboard, as XKB is a well known name. :)
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/561223002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as 7d2c1d9c7ad171d49be66cc9378a855810818fc1
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0d95894f4e846d7352d9813cd34f43efdb6e1e4e Cr-Commit-Position: refs/heads/master@{#295417} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
