Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1334943003:
metrics: Add RandomSelector, a class that randomly selects items with odds (Closed)
Descriptionmetrics: Add RandomSelector, a class that randomly selects items with odds
This class was originally added to debugd in ChromeOS to select perf
command lines for CWP. That command selection is being moved up into
Chrome, so this class needs to be moved here. It will be used by
PerfProvider for the same purpose.
Committed: https://crrev.com/85c2c2c4b76b0e6112dc03f640772bf164a548b9
Cr-Commit-Position: refs/heads/master@{#349298}
Patch Set 1 #Patch Set 2 : Add constructor for OddsAndValue #
Total comments: 17
Patch Set 3 : Switch to string values. Respond to comments on PS2 #Patch Set 4 : Moved to metrics/perf/ directory #
Total comments: 21
Patch Set 5 : Respond to comments on PS4 #
Total comments: 10
Patch Set 6 : Respond to comments on PS5 #
Total comments: 4
Patch Set 7 : Respond to comments on PS6 #
Total comments: 1
Patch Set 8 : Fix dumb error #
Messages
Total messages: 28 (8 generated)
dhsharp@chromium.org changed reviewers: + asvitkine@chromium.org, sque@chromium.org
Not sure if this is the best place to put this code. I just put it near where it will be used.
On 2015/09/11 00:53:31, dhsharp wrote: > Not sure if this is the best place to put this code. I just put it near where it > will be used. Add a link to the DebugD code in Chrome OS. https://chromium.googlesource.com/chromiumos/platform2/+/master/debugd/
https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/random_selector.cc (right): https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector.cc:5: #include "chrome/browser/metrics/random_selector.h" I'm not convinced of this living in chrome/browser/metrics. This directory is already pretty crowded. However, I see that the code that will be using it - perf_provider.cc - is in the same directory. How about making a new-subdirectory under chrome/browser/metrics - chrome/browser/metrics/perf/ and moving all of these files there? (Probably in a separate CL that can land before this one.) https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector.cc:10: #include <base/logging.h> Nit: "'s for project includes. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/random_selector.h (right): https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector.h:35: OddsAndValue(double weight_, const char* (&array)[N]) Nit: No _'s for params. I think using the same name as the field name should work fine. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector.h:72: }; DISALLOW_COPY_AND_ASSIGN(). https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/random_selector_unittest.cc (right): https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector_unittest.cc:9: #include <base/strings/string_util.h> ""'s not <>'s. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector_unittest.cc:12: #include "chrome/browser/metrics/random_selector.h" Should be first include. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector_unittest.cc:14: using metrics::RandomSelector; Put this code in the metrics namespace instead. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector_unittest.cc:46: }; DISALLOW_COPY_AND_ASSIGN()
https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/random_selector.cc (right): https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector.cc:5: #include "chrome/browser/metrics/random_selector.h" On 2015/09/11 21:00:54, Alexei Svitkine wrote: > I'm not convinced of this living in chrome/browser/metrics. This directory is > already pretty crowded. > > However, I see that the code that will be using it - perf_provider.cc - is in > the same directory. How about making a new-subdirectory under > chrome/browser/metrics - chrome/browser/metrics/perf/ and moving all of these > files there? (Probably in a separate CL that can land before this one.) Works for me. Will work on that. (Not sure if you noticed I raised this question in my review email.) https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector.cc:10: #include <base/logging.h> On 2015/09/11 21:00:54, Alexei Svitkine wrote: > Nit: "'s for project includes. Oops. Done. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/random_selector.h (right): https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector.h:35: OddsAndValue(double weight_, const char* (&array)[N]) On 2015/09/11 21:00:54, Alexei Svitkine wrote: > Nit: No _'s for params. I think using the same name as the field name should > work fine. Oh, okay. Was thinking that wouldn't work. Done. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector.h:72: }; On 2015/09/11 21:00:54, Alexei Svitkine wrote: > DISALLOW_COPY_AND_ASSIGN(). Done. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/random_selector_unittest.cc (right): https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector_unittest.cc:9: #include <base/strings/string_util.h> On 2015/09/11 21:00:55, Alexei Svitkine wrote: > ""'s not <>'s. Done. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector_unittest.cc:10: #include <gtest/gtest.h> For gtest/gtest.h, I looked around and saw a ~50/50 split between <> and "". Not sure which it should be... https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector_unittest.cc:12: #include "chrome/browser/metrics/random_selector.h" On 2015/09/11 21:00:54, Alexei Svitkine wrote: > Should be first include. Done. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector_unittest.cc:14: using metrics::RandomSelector; On 2015/09/11 21:00:55, Alexei Svitkine wrote: > Put this code in the metrics namespace instead. Done. https://codereview.chromium.org/1334943003/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/random_selector_unittest.cc:46: }; On 2015/09/11 21:00:55, Alexei Svitkine wrote: > DISALLOW_COPY_AND_ASSIGN() Done.
Okay, moved to metrics/perf.
https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector.cc (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:47: const std::string& RandomSelector::GetKeyOf(double value) { The name key in the function name is confusing, since you don't use that terminology anywhere else. In the struct, you name it value - so you can change the name to that term (and rename to param to something else - or perhaps GetStringFor(double value). https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:51: if (value < current) { Nit: No {}'s https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:56: return base::EmptyString(); The comment on base::EmptyString() suggest that it's often better to just return std::string(). If there's a good reason to use base::EmptyString() here instead, it should be documented. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector.h (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:13: namespace metrics { I think 'metrics' namespace is for things in components/metrics. I think the code in c/b/metrics generally doesn't use a namespace, so I'd drop it. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:50: // Get the next randomly picked string in |next|. Update comment. No |next|. Also, document semantics of GetNext() - i.e. mention that each pick is independent. Or maybe even rename this to not use the "next" term, since that implies the order is relevant. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:54: size_t GetNumValues() const { Nit: User hacker_style for inline trivial functions. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:66: // vector. Stores the result in |key|. Comment seems incorrect - there's no |key|. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector_unittest.cc (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector_unittest.cc:11: #include <gtest/gtest.h> Can this include "testing/gtest/include/gtest/gtest.h" instead?
https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector.cc (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:47: const std::string& RandomSelector::GetKeyOf(double value) { On 2015/09/14 19:12:33, Alexei Svitkine wrote: > The name key in the function name is confusing, since you don't use that > terminology anywhere else. > > In the struct, you name it value - so you can change the name to that term (and > rename to param to something else - or perhaps GetStringFor(double value). Done. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:51: if (value < current) { On 2015/09/14 19:12:33, Alexei Svitkine wrote: > Nit: No {}'s Done. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:56: return base::EmptyString(); On 2015/09/14 19:12:33, Alexei Svitkine wrote: > The comment on base::EmptyString() suggest that it's often better to just return > std::string(). > > If there's a good reason to use base::EmptyString() here instead, it should be > documented. `return std::string()` would be returning a reference to an a temporary which is about to go out of scope, which is undefined behavior, and results in a compiler warning/error. Reading the doc of EmptyString, this is exactly what it is intended for, despite the scary language around it. Does using this function as intended really need to be specially explained? https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector.h (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:13: namespace metrics { On 2015/09/14 19:12:33, Alexei Svitkine wrote: > I think 'metrics' namespace is for things in components/metrics. I think the > code in c/b/metrics generally doesn't use a namespace, so I'd drop it. You just told me to put it in 'metrics'... Done. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:50: // Get the next randomly picked string in |next|. On 2015/09/14 19:12:33, Alexei Svitkine wrote: > Update comment. No |next|. > > Also, document semantics of GetNext() - i.e. mention that each pick is > independent. Or maybe even rename this to not use the "next" term, since that > implies the order is relevant. Renamed "Select". https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:54: size_t GetNumValues() const { On 2015/09/14 19:12:33, Alexei Svitkine wrote: > Nit: User hacker_style for inline trivial functions. Done. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:66: // vector. Stores the result in |key|. On 2015/09/14 19:12:33, Alexei Svitkine wrote: > Comment seems incorrect - there's no |key|. Done. Sorry, old comments from debugd. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector_unittest.cc (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector_unittest.cc:11: #include <gtest/gtest.h> On 2015/09/14 19:12:33, Alexei Svitkine wrote: > Can this include "testing/gtest/include/gtest/gtest.h" instead? Done. https://codereview.chromium.org/1334943003/diff/60001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1875: 'browser/metrics/perf/random_selector.cc', Question: should this be under 'chrome_browser_chromeos_sources' instead? That list says # ChromeOS-sources not ending in _chromeos (which would be included in # other sections and filtered out for non-ChromeOS platforms.
https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector.cc (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:56: return base::EmptyString(); On 2015/09/15 00:00:21, dhsharp wrote: > On 2015/09/14 19:12:33, Alexei Svitkine wrote: > > The comment on base::EmptyString() suggest that it's often better to just > return > > std::string(). > > > > If there's a good reason to use base::EmptyString() here instead, it should be > > documented. > > `return std::string()` would be returning a reference to an a temporary which is > about to go out of scope, which is undefined behavior, and results in a compiler > warning/error. Reading the doc of EmptyString, this is exactly what it is > intended for, despite the scary language around it. Does using this function as > intended really need to be specially explained? Gotcha. Fair enough. https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector.h (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:13: namespace metrics { On 2015/09/15 00:00:21, dhsharp wrote: > On 2015/09/14 19:12:33, Alexei Svitkine wrote: > > I think 'metrics' namespace is for things in components/metrics. I think the > > code in c/b/metrics generally doesn't use a namespace, so I'd drop it. > > You just told me to put it in 'metrics'... Done. Sorry, my previous comment was about your "using" statement in the test - the point was to make the test be in the same namespace as the code it's testing so it didn't need a using statement. https://codereview.chromium.org/1334943003/diff/60001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1875: 'browser/metrics/perf/random_selector.cc', On 2015/09/15 00:00:21, dhsharp wrote: > Question: should this be under 'chrome_browser_chromeos_sources' instead? That > list says > > # ChromeOS-sources not ending in _chromeos (which would be included in > # other sections and filtered out for non-ChromeOS platforms. Putting it under chrome_browser_chromeos_sources makes sense to me. https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector.cc (right): https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:8: #include <vector> Nit: These two are not needed, because the header includes them. https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:16: RandomSelector::RandomSelector() {} Nit: Initialize sum_of_odds_ - or tools like coverty will complain. https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector.h (right): https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:48: // Get the next randomly picked string. Nit: Update comment (don't use the term "next" since it's confusing - also mention that this selects one randomly per their weights). https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:71: double sum_of_odds_; Nit: Add a comment. https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector_unittest.cc (right): https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector_unittest.cc:57: // 1%). Does this mean the test is expected to be flaky some percent of the time?
https://codereview.chromium.org/1334943003/diff/60001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/1334943003/diff/60001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1875: 'browser/metrics/perf/random_selector.cc', On 2015/09/15 15:31:18, Alexei Svitkine wrote: > On 2015/09/15 00:00:21, dhsharp wrote: > > Question: should this be under 'chrome_browser_chromeos_sources' instead? That > > list says > > > > # ChromeOS-sources not ending in _chromeos (which would be included in > > # other sections and filtered out for non-ChromeOS platforms. > > Putting it under chrome_browser_chromeos_sources makes sense to me. Done. https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector.cc (right): https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:8: #include <vector> On 2015/09/15 15:31:18, Alexei Svitkine wrote: > Nit: These two are not needed, because the header includes them. Done. https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.cc:16: RandomSelector::RandomSelector() {} On 2015/09/15 15:31:18, Alexei Svitkine wrote: > Nit: Initialize sum_of_odds_ - or tools like coverty will complain. Done. https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector.h (right): https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:48: // Get the next randomly picked string. On 2015/09/15 15:31:18, Alexei Svitkine wrote: > Nit: Update comment (don't use the term "next" since it's confusing - also > mention that this selects one randomly per their weights). Done. https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector.h:71: double sum_of_odds_; On 2015/09/15 15:31:18, Alexei Svitkine wrote: > Nit: Add a comment. Done. https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... File chrome/browser/metrics/perf/random_selector_unittest.cc (right): https://codereview.chromium.org/1334943003/diff/80001/chrome/browser/metrics/... chrome/browser/metrics/perf/random_selector_unittest.cc:57: // 1%). On 2015/09/15 15:31:18, Alexei Svitkine wrote: > Does this mean the test is expected to be flaky some percent of the time? No, not since we added the non-random "CustomRNG". But there will still be some fuzz.
lgtm https://codereview.chromium.org/1334943003/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/perf/random_selector.h (right): https://codereview.chromium.org/1334943003/diff/100001/chrome/browser/metrics... chrome/browser/metrics/perf/random_selector.h:30: // std::vector<std::string>& selection = random_selector.Select(); Nit: I find the nested comment above a bit messy. How about moving this code line right below line 25 and moving the text on line 27 to below it, unnesting the comment and changing it to say "The above should return ...". https://codereview.chromium.org/1334943003/diff/100001/chrome/browser/metrics... chrome/browser/metrics/perf/random_selector.h:38: double weight; Nit: Add a comment mention it's the probability weight. Also add a comment about the value field below.
https://codereview.chromium.org/1334943003/diff/100001/chrome/browser/metrics... File chrome/browser/metrics/perf/random_selector.h (right): https://codereview.chromium.org/1334943003/diff/100001/chrome/browser/metrics... chrome/browser/metrics/perf/random_selector.h:30: // std::vector<std::string>& selection = random_selector.Select(); On 2015/09/15 18:36:12, Alexei Svitkine (slow) wrote: > Nit: I find the nested comment above a bit messy. How about moving this code > line right below line 25 and moving the text on line 27 to below it, unnesting > the comment and changing it to say "The above should return ...". Done. https://codereview.chromium.org/1334943003/diff/100001/chrome/browser/metrics... chrome/browser/metrics/perf/random_selector.h:38: double weight; On 2015/09/15 18:36:12, Alexei Svitkine (slow) wrote: > Nit: Add a comment mention it's the probability weight. > > Also add a comment about the value field below. Done.
The CQ bit was checked by dhsharp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1334943003/#ps120001 (title: "Respond to comments on PS6")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334943003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334943003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/09/16 01:10:17, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Seems like a flake as it fails even without patch.
The CQ bit was checked by dhsharp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334943003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334943003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1334943003/diff/120001/chrome/browser/metrics... File chrome/browser/metrics/perf/random_selector.cc (right): https://codereview.chromium.org/1334943003/diff/120001/chrome/browser/metrics... chrome/browser/metrics/perf/random_selector.cc:15: RandomSelector::~RandomSelector() : sum_of_weights_(0) {} You have constructor and destructor switched around.
The CQ bit was checked by dhsharp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1334943003/#ps140001 (title: "Fix dumb error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1334943003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1334943003/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/85c2c2c4b76b0e6112dc03f640772bf164a548b9 Cr-Commit-Position: refs/heads/master@{#349298} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
