Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396783002:
Add LanguageModel, a keyed service that collects language info from CLD. (Closed)
DescriptionAdd LanguageModel, a keyed service that collects language info from CLD.
Before this CL, user's statistics about language detection from the CLD3
library were not accessible to Chrome.
This CL introduces a new keyed service called LanguageModel that
collects such statistics and builds a simple model on top of that. This
allows other parts of Chrome to see top languages in which the user
consumes web content.
A design doc:
https://docs.google.com/a/google.com/document/d/1FBD79CEBUKkMtSJKk3fnF1xnlrXd23z2fHgIGQUFOE8/edit?usp=sharing
BUG=653058
Committed: https://crrev.com/c713e7093d3e93fabecd885f93c0522272325725
Cr-Commit-Position: refs/heads/master@{#423702}
Patch Set 1 #
Total comments: 21
Patch Set 2 : David's comments #Patch Set 3 : Bernhard's comments #
Total comments: 22
Patch Set 4 : David's comments #2 #
Total comments: 4
Patch Set 5 : Bernhard's comments #2 #
Total comments: 2
Patch Set 6 : Paweł's comment #Patch Set 7 : Fix the asan bug #Patch Set 8 : Fixing incognito #Messages
Total messages: 43 (21 generated)
jkrcal@chromium.org changed reviewers: + droger@chromium.org
David, could you PTAL at my implementation? Thanks for all your help, so far!
jkrcal@chromium.org changed reviewers: + bauerb@chromium.org, phajdan.jr@chromium.org
Bernhard, could you PTAL at browser_prefs.cc? Paweł, could you PTAL at keyed_service dependency?
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Some nits below, but I'll do another pass. Do you need this feature on iOS too? It's not quite clear for me yet that this code belongs to components/translate/core, and we should resolve that first. Other than that this looks pretty good. https://codereview.chromium.org/2396783002/diff/1/chrome/browser/translate/ch... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2396783002/diff/1/chrome/browser/translate/ch... chrome/browser/translate/chrome_translate_client.cc:55: translate_driver_.AddObserver(&language_model_observer_); Need to call RemoveObserver at some point. Maybe use base::ScopedObserver? https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... File components/translate/core/browser/language_model.cc (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... File components/translate/core/browser/language_model_unittest.cc (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model_unittest.cc:63: EXPECT_THAT(model.GetLanguageFrequency(kLang1), 0.75); Why not EXPECT_DOUBLE_EQ or EXPECT_NEAR? https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model_unittest.cc:80: EXPECT_THAT(model.GetLanguageFrequency(kLang2), EXPECT_GE(model.GetLanguageFrequency(kLang2), 0);
https://codereview.chromium.org/2396783002/diff/1/components/translate/conten... File components/translate/content/browser/language_model_observer.cc (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/conten... components/translate/content/browser/language_model_observer.cc:22: language_model_->OnPageVisited(details.cld_language); Actually you can probably simplify here: get rid of this class entirely and move these two lines of code to ChromeTranslateClient::OnLanguageDetermined.
I agree with David's comment about moving the counting to ChromeTranslateClient. https://codereview.chromium.org/2396783002/diff/1/components/translate/conten... File components/translate/content/browser/language_model_observer.h (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/conten... components/translate/content/browser/language_model_observer.h:21: explicit LanguageModelObserver(translate::LanguageModel* language_model); Please explain the lifetime of |language_model| https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... File components/translate/core/browser/language_model.cc (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model.cc:57: for (const std::string lang_to_remove : remove_keys) const ref https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... File components/translate/core/browser/language_model.h (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model.h:29: // The current estimated frequency of the language share, a number between 0 Nit: empty line before blocks of comments please, for readability (also below). https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model.h:31: // opens is in this language). Does that mean that the frequencies should sum to 1?
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks to both of you! David, LanguageModel will not be used on iOS in the nearest future but there are plans to use is there later. Thus, I would like to keep it in core. Does it make sense? https://codereview.chromium.org/2396783002/diff/1/chrome/browser/translate/ch... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2396783002/diff/1/chrome/browser/translate/ch... chrome/browser/translate/chrome_translate_client.cc:55: translate_driver_.AddObserver(&language_model_observer_); On 2016/10/05 13:11:32, droger wrote: > Need to call RemoveObserver at some point. > > Maybe use base::ScopedObserver? Fixed by removing the observer completely (and using |this| instead). https://codereview.chromium.org/2396783002/diff/1/components/translate/conten... File components/translate/content/browser/language_model_observer.cc (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/conten... components/translate/content/browser/language_model_observer.cc:22: language_model_->OnPageVisited(details.cld_language); On 2016/10/05 13:37:26, droger wrote: > Actually you can probably simplify here: > get rid of this class entirely and move these two lines of code to > ChromeTranslateClient::OnLanguageDetermined. Done. https://codereview.chromium.org/2396783002/diff/1/components/translate/conten... File components/translate/content/browser/language_model_observer.h (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/conten... components/translate/content/browser/language_model_observer.h:21: explicit LanguageModelObserver(translate::LanguageModel* language_model); On 2016/10/05 14:48:16, Bernhard Bauer wrote: > Please explain the lifetime of |language_model| This class got removed. https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... File components/translate/core/browser/language_model.cc (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/10/05 13:11:32, droger wrote: > 2016 Done. https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model.cc:57: for (const std::string lang_to_remove : remove_keys) On 2016/10/05 14:48:16, Bernhard Bauer wrote: > const ref Done. https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... File components/translate/core/browser/language_model.h (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model.h:29: // The current estimated frequency of the language share, a number between 0 On 2016/10/05 14:48:16, Bernhard Bauer wrote: > Nit: empty line before blocks of comments please, for readability (also below). Done. https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model.h:31: // opens is in this language). On 2016/10/05 14:48:16, Bernhard Bauer wrote: > Does that mean that the frequencies should sum to 1? Yes, added a comment. The sum is not guaranteed to be exactly one (rounding errors) but I think this is not worth stressing. https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... File components/translate/core/browser/language_model_unittest.cc (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/10/05 13:11:32, droger wrote: > 2016 Done. https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model_unittest.cc:63: EXPECT_THAT(model.GetLanguageFrequency(kLang1), 0.75); On 2016/10/05 13:11:32, droger wrote: > Why not EXPECT_DOUBLE_EQ or EXPECT_NEAR? Done. Staying with EXPECT_THAT (based on https://groups.google.com/a/chromium.org/d/topic/chromium-dev/e3ucMdpxEJ4/dis...). I also changed operator== for LanguageInfo so that frequencies are not checked in the other tests, just the language codes. https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model_unittest.cc:80: EXPECT_THAT(model.GetLanguageFrequency(kLang2), On 2016/10/05 13:11:32, droger wrote: > EXPECT_GE(model.GetLanguageFrequency(kLang2), 0); Do you insist? I'd like to stay with EXPECT_THAT (based on https://groups.google.com/a/chromium.org/d/topic/chromium-dev/e3ucMdpxEJ4/dis...). I do not have any strong opinion, I just would like to understand which way I can write test code so that it does not cause controversies in code review :)
https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... File components/translate/core/browser/language_model_unittest.cc (right): https://codereview.chromium.org/2396783002/diff/1/components/translate/core/b... components/translate/core/browser/language_model_unittest.cc:63: EXPECT_THAT(model.GetLanguageFrequency(kLang1), 0.75); On 2016/10/05 15:14:47, jkrcal wrote: > On 2016/10/05 13:11:32, droger wrote: > > Why not EXPECT_DOUBLE_EQ or EXPECT_NEAR? > > Done. Staying with EXPECT_THAT (based on > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/e3ucMdpxEJ4/dis...). > > I also changed operator== for LanguageInfo so that frequencies are not checked > in the other tests, just the language codes. Oops sorry, I totally missed this thread. Please ignore my comments.
On 2016/10/05 15:14:47, jkrcal wrote: > David, LanguageModel will not be used on iOS in the nearest future but there are > plans to use is there later. Thus, I would like to keep it in core. Does it make > sense? Yes, that makes sense. I think that's a good argument.
Thanks, only nits. https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:54: web_contents->GetBrowserContext())) { DCHECK(language_model_) ? https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... File chrome/browser/translate/language_model_factory.cc (right): https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... chrome/browser/translate/language_model_factory.cc:7: #include "base/macros.h" Not needed, and it's already in the .h https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... File chrome/browser/translate/language_model_factory.h (right): https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... chrome/browser/translate/language_model_factory.h:12: class Profile; Probably not needed. https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... File components/translate/core/browser/BUILD.gn (right): https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/BUILD.gn:61: "//components/translate/core/language_detection", Do you really need this? If so, we should at least update the README in components/translate. I would also be concerned that this introduces a dependency on CLD in the browser, leading to increased executable sizes. https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... File components/translate/core/browser/language_model.cc (right): https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:14: #include "components/translate/core/language_detection/language_detection_util.h" Could we remove this dep? https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:26: // Get the sum of the counter for all languages in the model. Gets https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:38: // Remove languages with small counter values and discount remaining counters. Removes https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:40: std::map<std::string, int> new_values; unused variable https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:61: // Transform the counters from prefs into a list of LanguageInfo structs. Transforms https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:63: const base::DictionaryValue* dict) { nit: not 100% sure, but I think the recommended style is to use const references rather than const pointers. https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... File components/translate/core/browser/language_model_unittest.cc (right): https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model_unittest.cc:7: #include "base/macros.h" Not needed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, David! https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:54: web_contents->GetBrowserContext())) { On 2016/10/05 15:39:59, droger wrote: > DCHECK(language_model_) ? Done. https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... File chrome/browser/translate/language_model_factory.cc (right): https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... chrome/browser/translate/language_model_factory.cc:7: #include "base/macros.h" On 2016/10/05 15:39:59, droger wrote: > Not needed, and it's already in the .h Done. https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... File chrome/browser/translate/language_model_factory.h (right): https://codereview.chromium.org/2396783002/diff/40001/chrome/browser/translat... chrome/browser/translate/language_model_factory.h:12: class Profile; On 2016/10/05 15:39:59, droger wrote: > Probably not needed. Oh, yes, thanks for spotting it! https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... File components/translate/core/browser/BUILD.gn (right): https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/BUILD.gn:61: "//components/translate/core/language_detection", On 2016/10/05 15:39:59, droger wrote: > Do you really need this? > > If so, we should at least update the README in components/translate. > > I would also be concerned that this introduces a dependency on CLD in the > browser, leading to increased executable sizes. Right, probably not needed. Let me test it on the testbots :) https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... File components/translate/core/browser/language_model.cc (right): https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:14: #include "components/translate/core/language_detection/language_detection_util.h" On 2016/10/05 15:39:59, droger wrote: > Could we remove this dep? True, I can! https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:26: // Get the sum of the counter for all languages in the model. On 2016/10/05 15:39:59, droger wrote: > Gets I meant it as an imperative. You are probably right that 3rd person indicative is more standard. https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:38: // Remove languages with small counter values and discount remaining counters. On 2016/10/05 15:40:00, droger wrote: > Removes Done. https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:40: std::map<std::string, int> new_values; On 2016/10/05 15:39:59, droger wrote: > unused variable Right, thanks! I am surprised the compiler does not err on this. https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:61: // Transform the counters from prefs into a list of LanguageInfo structs. On 2016/10/05 15:39:59, droger wrote: > Transforms Done. https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model.cc:63: const base::DictionaryValue* dict) { On 2016/10/05 15:40:00, droger wrote: > nit: not 100% sure, but I think the recommended style is to use const references > rather than const pointers. Thanks, you are right! https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... File components/translate/core/browser/language_model_unittest.cc (right): https://codereview.chromium.org/2396783002/diff/40001/components/translate/co... components/translate/core/browser/language_model_unittest.cc:7: #include "base/macros.h" On 2016/10/05 15:40:00, droger wrote: > Not needed? Done.
lgtm
https://codereview.chromium.org/2396783002/diff/60001/components/translate/co... File components/translate/core/browser/language_model.cc (right): https://codereview.chromium.org/2396783002/diff/60001/components/translate/co... components/translate/core/browser/language_model.cc:22: const float kCutoffRatio = 0.005f; Is the f necessary if you already use floating point notation? https://codereview.chromium.org/2396783002/diff/60001/components/translate/co... File components/translate/core/browser/language_model_unittest.cc (right): https://codereview.chromium.org/2396783002/diff/60001/components/translate/co... components/translate/core/browser/language_model_unittest.cc:83: EXPECT_THAT(model.GetLanguageFrequency(kLang2), Ge(0.0f)); Do you want Gt() here? GetLanguageFrequency returns 0 for an unknown language, right?
Thanks, Bernhard! Bernhard, I still need your lgtm for browser_prefs.cc. Paweł, could you PTAL at keyed_service dependency? https://codereview.chromium.org/2396783002/diff/60001/components/translate/co... File components/translate/core/browser/language_model.cc (right): https://codereview.chromium.org/2396783002/diff/60001/components/translate/co... components/translate/core/browser/language_model.cc:22: const float kCutoffRatio = 0.005f; On 2016/10/06 08:58:49, Bernhard Bauer wrote: > Is the f necessary if you already use floating point notation? Yes, windows compiler complained about that: e:\b\c\b\win\src\components\translate\core\browser\language_model.cc(23): error C2220: warning treated as error - no 'object' file generated e:\b\c\b\win\src\components\translate\core\browser\language_model.cc(23): warning C4305: 'initializing': truncation from 'double' to 'float' https://codereview.chromium.org/2396783002/diff/60001/components/translate/co... File components/translate/core/browser/language_model_unittest.cc (right): https://codereview.chromium.org/2396783002/diff/60001/components/translate/co... components/translate/core/browser/language_model_unittest.cc:83: EXPECT_THAT(model.GetLanguageFrequency(kLang2), Ge(0.0f)); On 2016/10/06 08:58:49, Bernhard Bauer wrote: > Do you want Gt() here? GetLanguageFrequency returns 0 for an unknown language, > right? Oh, true. Thanks for spotting it!
lgtm
jkrcal@chromium.org changed reviewers: + erg@chromium.org
Elliot, since Paweł is not responding, could you PTAL at the new keyed_service dependency?
LGTM w/nit https://codereview.chromium.org/2396783002/diff/80001/components/translate/co... File components/translate/core/browser/language_model.h (right): https://codereview.chromium.org/2396783002/diff/80001/components/translate/co... components/translate/core/browser/language_model.h:60: }; nit: Shouldn't this have DISALLOW_COPY_AND_ASSIGN?
Thanks! https://codereview.chromium.org/2396783002/diff/80001/components/translate/co... File components/translate/core/browser/language_model.h (right): https://codereview.chromium.org/2396783002/diff/80001/components/translate/co... components/translate/core/browser/language_model.h:60: }; On 2016/10/06 14:42:48, Paweł Hajdan Jr. wrote: > nit: Shouldn't this have DISALLOW_COPY_AND_ASSIGN? True! Done.
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, bauerb@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2396783002/#ps100001 (title: "Paweł's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, droger@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2396783002/#ps140001 (title: "Fixing incognito")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add LanguageModel, a keyed service that collects language info from CLD. Before this CL, user's statistics about language detection from the CLD3 library were not accessible to Chrome. This CL introduces a new keyed service called LanguageModel that collects such statistics and builds a simple model on top of that. This allows other parts of Chrome to see top languages in which the user consumes web content. A design doc: https://docs.google.com/a/google.com/document/d/1FBD79CEBUKkMtSJKk3fnF1xnlrXd... BUG=653058 ========== to ========== Add LanguageModel, a keyed service that collects language info from CLD. Before this CL, user's statistics about language detection from the CLD3 library were not accessible to Chrome. This CL introduces a new keyed service called LanguageModel that collects such statistics and builds a simple model on top of that. This allows other parts of Chrome to see top languages in which the user consumes web content. A design doc: https://docs.google.com/a/google.com/document/d/1FBD79CEBUKkMtSJKk3fnF1xnlrXd... BUG=653058 Committed: https://crrev.com/c713e7093d3e93fabecd885f93c0522272325725 Cr-Commit-Position: refs/heads/master@{#423702} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c713e7093d3e93fabecd885f93c0522272325725 Cr-Commit-Position: refs/heads/master@{#423702} |
