Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
714273004:
mac: Expose keychain access frequency to Telemetry. (Closed)
|
Created:
6 years, 1 month ago by erikchen Modified:
5 years, 11 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, telemetry+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@mock_keychain_sleep Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmac: Expose keychain access frequency to Telemetry.
Accessing the keychain is really slow. Telemetry tests use a mock keychain,
which is abnormally fast. This CL exposes the number of times the mock keychain
is accessed via a local histogram, and 4 telemetry measurements record that
number as a metric.
Those tests are:
- speedometer
- page_cycler
- startup
- tab_switching
Once this metric is collected, alerts will be added to the Telemetry dashboard
to help catch regressions to the number of times the keychain gets accessed.
BUG=
Committed: https://crrev.com/95dc1d73331ddd636c4abb6bf4b9d5e665fd5449
Cr-Commit-Position: refs/heads/master@{#311744}
Patch Set 1 #Patch Set 2 : Add a common subclass to measurements. #
Total comments: 12
Patch Set 3 : Remove dependency on another CL. Respond to comments from jeremy. #Patch Set 4 : Respond to offline comments from tonyg. #
Total comments: 11
Patch Set 5 : Comments from dtu & tonyg. #Patch Set 6 : Rebase against top of tree. #Patch Set 7 : Import ordering. #Patch Set 8 : Comments from dtu. #
Total comments: 18
Patch Set 9 : Comments from jeremy. #
Total comments: 1
Patch Set 10 : Comments from jeremy. #Patch Set 11 : Minor updates to base/mac/keychain_metrics.h. #
Total comments: 2
Patch Set 12 : Comments from thestig. #Patch Set 13 : Comments from jeremy - remove duplicate method. #Patch Set 14 : Rebase against top of tree. #Patch Set 15 : Move GetEncryptionPassword so that it gets compiled on iOS. #
Messages
Total messages: 61 (20 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
erikchen@chromium.org changed reviewers: + jeremy@chromium.org
jeremy: Please review.
https://codereview.chromium.org/714273004/diff/80001/base/perf_util.cc File base/perf_util.cc (right): https://codereview.chromium.org/714273004/diff/80001/base/perf_util.cc#newcode19 base/perf_util.cc:19: } Shouldn't be part of this CL. https://codereview.chromium.org/714273004/diff/80001/base/perf_util.cc#newcode22 base/perf_util.cc:22: void EmitAccessKeychainHistogram() { nit: rename IncrementKeychainAccessHistogram(). https://codereview.chromium.org/714273004/diff/80001/base/perf_util.cc#newcode23 base/perf_util.cc:23: LOCAL_HISTOGRAM_BOOLEAN("OSX.Keychain.Access", true); needs a comment here that explains what this histogram is, why we need it and that it's read from Telemetry. https://codereview.chromium.org/714273004/diff/80001/tools/perf/measurements/... File tools/perf/measurements/__init__.py (right): https://codereview.chromium.org/714273004/diff/80001/tools/perf/measurements/... tools/perf/measurements/__init__.py:10: class PageTestMeasurement(page_test.PageTest): I'd just add the flag and stat as part of the information reported by StartupMetric, is there a reason that wouldn't work? https://codereview.chromium.org/714273004/diff/80001/tools/perf/measurements/... tools/perf/measurements/__init__.py:23: '--enable-stats-collection-bindings' Is there a reason we need to collect this for tests other than the startup tests? If so, this flag is already passed in measurements/startup.py https://codereview.chromium.org/714273004/diff/80001/tools/perf/metrics/keych... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/80001/tools/perf/metrics/keych... tools/perf/metrics/keychain_metric.py:28: if result: Do you think we should fail if there are no entries for this histogram?
https://codereview.chromium.org/714273004/diff/80001/tools/perf/measurements/... File tools/perf/measurements/__init__.py (right): https://codereview.chromium.org/714273004/diff/80001/tools/perf/measurements/... tools/perf/measurements/__init__.py:10: class PageTestMeasurement(page_test.PageTest): On 2014/11/13 11:31:30, jeremy wrote: > I'd just add the flag and stat as part of the information reported by > StartupMetric, is there a reason that wouldn't work? I'm not sure that I understand your comment. I thought that we wanted to collect this stat for all Telemetry tests? Whereas in your suggestion, the stat would only be collected for Telemetry measurements that use StartupMetric.
On 2014/11/13 18:11:09, erikchen wrote: > I'm not sure that I understand your comment. I thought that we wanted to collect > this stat for all Telemetry tests? Whereas in your suggestion, the stat would > only be collected for Telemetry measurements that use StartupMetric. The way I understand things, the aim here is to prevent a regression in the number of calls to the keychain, right? Are there any tests other than the startup tests that access the keychain in interesting ways? I may be wrong, but my understanding is that most of the Telemetry tests we have launch the browser with a fresh profile then load some pages and run benchmarks on them, nothing that should access the keychain.
On 2014/11/13 18:30:48, jeremy wrote: > On 2014/11/13 18:11:09, erikchen wrote: > > I'm not sure that I understand your comment. I thought that we wanted to > collect > > this stat for all Telemetry tests? Whereas in your suggestion, the stat would > > only be collected for Telemetry measurements that use StartupMetric. > > The way I understand things, the aim here is to prevent a regression in the > number of calls to the keychain, right? > Are there any tests other than the startup tests that access the keychain in > interesting ways? > > I may be wrong, but my understanding is that most of the Telemetry tests we have > launch the browser with a fresh profile then load some pages and run benchmarks > on them, nothing that should access the keychain. The two regressions we have found so far have been in the startup realm. The goal of this CL is to prevent regressions in all parts of Chrome, not just startup. I agree that the other tests shouldn't be accessing the keychain. That's exactly why I want to measure keychain access for them. :)
On 2014/11/13 18:42:24, erikchen wrote: > The two regressions we have found so far have been in the startup realm. The > goal of this CL is to prevent regressions in all parts of Chrome, not just > startup. > > I agree that the other tests shouldn't be accessing the keychain. That's exactly > why I want to measure keychain access for them. :) Most of the telemetry tests are starting a browser with a fresh profile and loading a few sites or running a benchmark, I don't think we expect those to call through to the keychain. Lets focus on stemming regressions in the places we've seen them, correct me if I'm wrong but those would have been caught by the startup tests and a sync test (if we had it), adding to the page cycler is a stretch but I guess conceivably loading a site in one of the pagesets could access the keychain. IMHO we can just add this as part of the startup metric, if you think there are cases we wouldn't catch with that then you can create a new metric that follows the pattern set by startupmetric then in each measurement you can add that metric, I don't understand why we need the PageTestMeasurement class?
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
https://codereview.chromium.org/714273004/diff/80001/base/perf_util.cc File base/perf_util.cc (right): https://codereview.chromium.org/714273004/diff/80001/base/perf_util.cc#newcode19 base/perf_util.cc:19: } On 2014/11/13 11:31:29, jeremy wrote: > Shouldn't be part of this CL. Right. I removed the dependency of this CL on the CL that added SpinCpuMicroseconds. https://codereview.chromium.org/714273004/diff/80001/base/perf_util.cc#newcode22 base/perf_util.cc:22: void EmitAccessKeychainHistogram() { On 2014/11/13 11:31:29, jeremy wrote: > nit: rename IncrementKeychainAccessHistogram(). Done. https://codereview.chromium.org/714273004/diff/80001/base/perf_util.cc#newcode23 base/perf_util.cc:23: LOCAL_HISTOGRAM_BOOLEAN("OSX.Keychain.Access", true); On 2014/11/13 11:31:29, jeremy wrote: > needs a comment here that explains what this histogram is, why we need it and > that it's read from Telemetry. I added copious documentation to the header file, which seemed like a better location than the implementation file. https://codereview.chromium.org/714273004/diff/80001/tools/perf/measurements/... File tools/perf/measurements/__init__.py (right): https://codereview.chromium.org/714273004/diff/80001/tools/perf/measurements/... tools/perf/measurements/__init__.py:23: '--enable-stats-collection-bindings' On 2014/11/13 11:31:30, jeremy wrote: > Is there a reason we need to collect this for tests other than the startup > tests? > If so, this flag is already passed in measurements/startup.py Yes, as per our offline discussion. We will want all telemetry tests on Mac to check that the keychain isn't accessed more than 10 times. https://codereview.chromium.org/714273004/diff/80001/tools/perf/metrics/keych... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/80001/tools/perf/metrics/keych... tools/perf/metrics/keychain_metric.py:28: if result: On 2014/11/13 11:31:30, jeremy wrote: > Do you think we should fail if there are no entries for this histogram? If the keychain is never accessed, then there are no entries for the histogram, right? If that is true, then this would probably apply to most of the telemetry tests.
erikchen@chromium.org changed reviewers: + dtu@chromium.org
dtu: Please review. Also, please confirm that this is the implementation you expected.
I chatted with dtu briefly about this, from what I understand he says we shouldn't be using the alerting infrastructure at all but just failing the telemetry test if the number of accesses changes. He said to add an expectations file to the source tree, and assert if the number exceeds the expectation for the current test.
On 2014/11/14 18:57:15, jeremy wrote: > I chatted with dtu briefly about this, from what I understand he says we > shouldn't be using the alerting infrastructure at all but just failing the > telemetry test if the number of accesses changes. > He said to add an expectations file to the source tree, and assert if the number > exceeds the expectation for the current test. We talked about this possibility yesterday. This has the problem that anyone who legitimately changes the number of keychain accesses will then cause failures of the telemetry bots. The solution is not robust and just makes more work for the perf sheriffs.
dtu: ping?
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
dtu: Please review.
dtu: Ping?
dtu@chromium.org changed reviewers: + tonyg@chromium.org
You might need an OWNER for the C++ code too. https://codereview.chromium.org/714273004/diff/220001/tools/perf/measurements... File tools/perf/measurements/page_cycler_unittest.py (right): https://codereview.chromium.org/714273004/diff/220001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:106: cycler.SetInUnitTest(True) Why can't we have this enabled in unit tests as well? https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:30: def GetKeychainAccessCount(tab): telemetry.value.histogram_util.GetHistogramSum https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:55: def ShouldCollectKeychainMetrics(): Since all callers are going to check ShouldCollectKeychainMetrics, maybe it's safer and more modular if the check goes in this class's methods?
https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:50: if access_count > KeychainMetric.MAX_KEYCHAIN_ACCESS_COUNT: Correct me if I'm mistaken, but didn't we land on the idea of just reporting this to the dashboard to be monitored there rather than failing hard?
Ah, you're right. I misinterpreted the notes from our discussion. Reproducing them here for convenience: """ 1. Track # of keychain accesses in certain telemetry measurements. Startup, page-cycler, tab switcher, speedometer. Emit a result just like other metrics. Write an absolute alerting rule (>10). owner: erikchen """ On Thu, Dec 4, 2014 at 7:12 PM, <tonyg@chromium.org> wrote: > > https://codereview.chromium.org/714273004/diff/220001/ > tools/perf/metrics/keychain_metric.py > File tools/perf/metrics/keychain_metric.py (right): > > https://codereview.chromium.org/714273004/diff/220001/ > tools/perf/metrics/keychain_metric.py#newcode50 > tools/perf/metrics/keychain_metric.py:50: if access_count > > KeychainMetric.MAX_KEYCHAIN_ACCESS_COUNT: > Correct me if I'm mistaken, but didn't we land on the idea of just > reporting this to the dashboard to be monitored there rather than > failing hard? > > https://codereview.chromium.org/714273004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
dtu: PTAL https://codereview.chromium.org/714273004/diff/220001/tools/perf/measurements... File tools/perf/measurements/page_cycler_unittest.py (right): https://codereview.chromium.org/714273004/diff/220001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:106: cycler.SetInUnitTest(True) On 2014/12/05 01:12:54, dtu wrote: > Why can't we have this enabled in unit tests as well? I don't understand your comment. My intention is for all unit tests that use a PageCycler to call this method. This appears to be the best place to do so. I don't know of any unit tests that I've missed. https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:30: def GetKeychainAccessCount(tab): On 2014/12/05 01:12:54, dtu wrote: > telemetry.value.histogram_util.GetHistogramSum Done. https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:50: if access_count > KeychainMetric.MAX_KEYCHAIN_ACCESS_COUNT: On 2014/12/05 03:12:03, tonyg wrote: > Correct me if I'm mistaken, but didn't we land on the idea of just reporting > this to the dashboard to be monitored there rather than failing hard? Yup, removed this line. https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:55: def ShouldCollectKeychainMetrics(): On 2014/12/05 01:12:54, dtu wrote: > Since all callers are going to check ShouldCollectKeychainMetrics, maybe it's > safer and more modular if the check goes in this class's methods? Hm. You're right. I moved the logic into this file. In addition, I renamed the methods to CustomizeBrowserOptionsMac and AddResultsMac. AddResults throws an assert. (If the methods aren't renamed, then KeychainMetric().AddResults(...) has different effects based on the platform, which is not at all clear based on the name of the method.)
https://codereview.chromium.org/714273004/diff/220001/tools/perf/measurements... File tools/perf/measurements/page_cycler_unittest.py (right): https://codereview.chromium.org/714273004/diff/220001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:106: cycler.SetInUnitTest(True) On 2014/12/08 23:41:25, erikchen wrote: > On 2014/12/05 01:12:54, dtu wrote: > > Why can't we have this enabled in unit tests as well? > > I don't understand your comment. My intention is for all unit tests that use a > PageCycler to call this method. This appears to be the best place to do so. I > don't know of any unit tests that I've missed. Sorry, I mean that I think it's generally dangerous and messy to have a custom code path just for unit tests. Why are you disabling the metric for unit tests? https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/220001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:55: def ShouldCollectKeychainMetrics(): On 2014/12/08 23:41:25, erikchen wrote: > On 2014/12/05 01:12:54, dtu wrote: > > Since all callers are going to check ShouldCollectKeychainMetrics, maybe it's > > safer and more modular if the check goes in this class's methods? > > Hm. You're right. > > I moved the logic into this file. In addition, I renamed the methods to > CustomizeBrowserOptionsMac and AddResultsMac. AddResults throws an assert. > > (If the methods aren't renamed, then KeychainMetric().AddResults(...) has > different effects based on the platform, which is not at all clear based on the > name of the method.) Makes sense.
dtu: PTAL https://codereview.chromium.org/714273004/diff/220001/tools/perf/measurements... File tools/perf/measurements/page_cycler_unittest.py (right): https://codereview.chromium.org/714273004/diff/220001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:106: cycler.SetInUnitTest(True) On 2014/12/09 00:25:03, dtu wrote: > On 2014/12/08 23:41:25, erikchen wrote: > > On 2014/12/05 01:12:54, dtu wrote: > > > Why can't we have this enabled in unit tests as well? > > > > I don't understand your comment. My intention is for all unit tests that use a > > PageCycler to call this method. This appears to be the best place to do so. I > > don't know of any unit tests that I've missed. > > Sorry, I mean that I think it's generally dangerous and messy to have a custom > code path just for unit tests. Why are you disabling the metric for unit tests? Good point. I've removed the method "SetInUnitTest" and its corresponding member. I've introduced platform-dependent code into the unit test to account for the different behavior of the class on Mac vs. non-Mac.
Can you update the CL description please https://codereview.chromium.org/714273004/diff/300001/base/perf_util.h File base/perf_util.h (right): https://codereview.chromium.org/714273004/diff/300001/base/perf_util.h#newcode1 base/perf_util.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. I'm not a base owner but I'd prefer putting this somewhere less generic - base/mac/keychain_metrics.h ? https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... File tools/perf/measurements/page_cycler_unittest.py (right): https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:59: if script == get_keychain_script: # Fake data for keychain metric. if keychain_histogram_name in script: return .... https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:233: if sys.platform =='darwin': space after == https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:234: value_count = value_count + 1 value_count += 1 https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:242: for value, expected in zip(values[1:4], ['gpu', 'renderer', 'browser']): I don't like the hardcoded 4 here, how about: foo = ['gpu', ...] for ... in zip(values[1:len(foo)+1], foo) https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... File tools/perf/measurements/session_restore.py (left): https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... tools/perf/measurements/session_restore.py:78: startup_metric.StartupMetric().AddResults(tab, results) Why remove this? https://codereview.chromium.org/714273004/diff/300001/tools/perf/metrics/keyc... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/300001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:22: def CustomizeBrowserOptionsMac(cls, options): I'd remove Mac from this and the other method name to match other Metrics. https://codereview.chromium.org/714273004/diff/300001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:28: options.AppendExtraBrowserArgs(['--enable-stats-collection-bindings']) I guess nothing bad will happen if this is appended twice, I wonder if we should just always launch Chrome with this option in Telemetry?
lgtm barring jeremy's comments https://codereview.chromium.org/714273004/diff/300001/tools/perf/metrics/keyc... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/300001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:22: def CustomizeBrowserOptionsMac(cls, options): On 2014/12/09 14:57:17, jeremy wrote: > I'd remove Mac from this and the other method name to match other Metrics. Either way is fine to me. https://codereview.chromium.org/714273004/diff/300001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:28: options.AppendExtraBrowserArgs(['--enable-stats-collection-bindings']) On 2014/12/09 14:57:17, jeremy wrote: > I guess nothing bad will happen if this is appended twice, I wonder if we should > just always launch Chrome with this option in Telemetry? I think the AppendExtraBrowserArgs method removes duplicates. I like that it's declared where it's being used.
https://codereview.chromium.org/714273004/diff/300001/tools/perf/metrics/keyc... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/300001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:28: options.AppendExtraBrowserArgs(['--enable-stats-collection-bindings']) Good to know, please ignore my comment then :)
Patchset #9 (id:320001) has been deleted
jeremy: PTAL https://codereview.chromium.org/714273004/diff/300001/base/perf_util.h File base/perf_util.h (right): https://codereview.chromium.org/714273004/diff/300001/base/perf_util.h#newcode1 base/perf_util.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/12/09 14:57:16, jeremy wrote: > I'm not a base owner but I'd prefer putting this somewhere less generic - > base/mac/keychain_metrics.h ? Done. https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... File tools/perf/measurements/page_cycler_unittest.py (right): https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:59: if script == get_keychain_script: On 2014/12/09 14:57:16, jeremy wrote: > # Fake data for keychain metric. > if keychain_histogram_name in script: > return .... Done. https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:233: if sys.platform =='darwin': On 2014/12/09 14:57:16, jeremy wrote: > space after == Done. https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:234: value_count = value_count + 1 On 2014/12/09 14:57:16, jeremy wrote: > value_count += 1 Done. https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... tools/perf/measurements/page_cycler_unittest.py:242: for value, expected in zip(values[1:4], ['gpu', 'renderer', 'browser']): On 2014/12/09 14:57:16, jeremy wrote: > I don't like the hardcoded 4 here, how about: > > foo = ['gpu', ...] > for ... in zip(values[1:len(foo)+1], foo) Done. https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... File tools/perf/measurements/session_restore.py (left): https://codereview.chromium.org/714273004/diff/300001/tools/perf/measurements... tools/perf/measurements/session_restore.py:78: startup_metric.StartupMetric().AddResults(tab, results) On 2014/12/09 14:57:17, jeremy wrote: > Why remove this? The superclass implementation of ValidateAndMeasurePage already includes this metric. https://codereview.chromium.org/714273004/diff/300001/tools/perf/metrics/keyc... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/300001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:22: def CustomizeBrowserOptionsMac(cls, options): On 2014/12/09 22:05:20, dtu wrote: > On 2014/12/09 14:57:17, jeremy wrote: > > I'd remove Mac from this and the other method name to match other Metrics. > > Either way is fine to me. I intentionally changed the names to include the suffix Mac, as I explained to dtu earlier in the reply chain. These methods only have an effect when run on Macs. Naming these methods like the other Metrics will cause confusion, since the other Metrics have an effect on all OSes.
lgtm https://codereview.chromium.org/714273004/diff/340001/tools/perf/metrics/keyc... File tools/perf/metrics/keychain_metric.py (right): https://codereview.chromium.org/714273004/diff/340001/tools/perf/metrics/keyc... tools/perf/metrics/keychain_metric.py:30: def AddResultsMac(self, tab, results): I feel quite strongly that these should just be called AddResults/CustomizeBrowserOptions - I think the platform constraints are clear from the class name. Having said that I'm not an owner so dtu gets the final say.
On 2014/12/14 08:55:50, jeremy wrote: > lgtm > > https://codereview.chromium.org/714273004/diff/340001/tools/perf/metrics/keyc... > File tools/perf/metrics/keychain_metric.py (right): > > https://codereview.chromium.org/714273004/diff/340001/tools/perf/metrics/keyc... > tools/perf/metrics/keychain_metric.py:30: def AddResultsMac(self, tab, results): > I feel quite strongly that these should just be called > AddResults/CustomizeBrowserOptions - I think the platform constraints are clear > from the class name. > > Having said that I'm not an owner so dtu gets the final say. dtu mentioned he was okay with either way. Since you feel strongly about this, I will change the code to match your expectations.
erikchen@chromium.org changed reviewers: + davidben@chromium.org, thestig@chromium.org
thestig: Looking for an OWNER review of components/os_crypt/ and base/. davidben: Looking for an OWNER review of crypto/
https://codereview.chromium.org/714273004/diff/380001/base/mac/keychain_metri... File base/mac/keychain_metrics.h (right): https://codereview.chromium.org/714273004/diff/380001/base/mac/keychain_metri... base/mac/keychain_metrics.h:18: BASE_EXPORT void IncrementKeychainAccessHistogram(); How about moving this to crypto/apple_keychain.h ? components/os_crypt/ can access crypto/.
thestig: PTAL https://codereview.chromium.org/714273004/diff/380001/base/mac/keychain_metri... File base/mac/keychain_metrics.h (right): https://codereview.chromium.org/714273004/diff/380001/base/mac/keychain_metri... base/mac/keychain_metrics.h:18: BASE_EXPORT void IncrementKeychainAccessHistogram(); On 2014/12/15 22:27:02, Lei Zhang wrote: > How about moving this to crypto/apple_keychain.h ? components/os_crypt/ can > access crypto/. Good suggestion. I moved all histogram emission into mock_apple_keychain.
lgtm
Rietveld is down but just wanted to point out that AddResults is defined twice in keychain_metric.py On Tue, Dec 16, 2014 at 1:22 AM, <thestig@chromium.org> wrote: > > lgtm > > > > https://codereview.chromium.org/714273004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
thanks, fixed in patch set 13.
erikchen@chromium.org changed reviewers: + agl@chromium.org - davidben@chromium.org
agl: Looking for an OWNER review of crypto/ -davidben, as he's OOO
erikchen@chromium.org changed reviewers: + davidben@chromium.org - agl@chromium.org
-agl On the premise that he appears too busy to review this CL. +davidben now that the holiday season is over. Looking for an OWNER review of crypto/*
crypto lgtm. Sorry about the holiday coordination delays.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/714273004/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/714273004/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/714273004/460001
Message was sent while issue was closed.
Committed patchset #15 (id:460001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/95dc1d73331ddd636c4abb6bf4b9d5e665fd5449 Cr-Commit-Position: refs/heads/master@{#311744} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
