Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
593243002:
Profile_Metrics integration with Keystone (Closed)
|
Created:
6 years, 3 months ago by Mike Lerman Modified:
5 years, 9 months ago CC:
chromium-reviews, borisv Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionProfile_Metrics integration with Keystone
Chromium side of the implementation of tracking profile stats (number of profiles, number of signed in profiles) through the Omaha channel for Mac. Design doc: https://docs.google.com/a/google.com/document/d/15Ou8VCYNCZvke8Bw9bF3vYqD67voJBPyx_GBR8ONcCw/edit?disco=AAAAAKU7Zzg (Googler Only)
BUG=409895
Committed: https://crrev.com/0bcaa8e521eae39a81b4754f8b4ac9de1f0463d5
Cr-Commit-Position: refs/heads/master@{#318729}
Patch Set 1 #Patch Set 2 : Better error handling #
Total comments: 10
Patch Set 3 : bcwhite comments #Patch Set 4 : Borisv comments #
Total comments: 19
Patch Set 5 : Rebase #Patch Set 6 : Comments (mark and others) #
Total comments: 12
Patch Set 7 : Rebase #Patch Set 8 : Remove separate attribute header; refactor setActive methods. #
Total comments: 8
Patch Set 9 : Rework integer types #Patch Set 10 : Rebase #Patch Set 11 : Post-rebase #Patch Set 12 : Rebase #Patch Set 13 : Rebase #Patch Set 14 : Properly calls keystone code #Patch Set 15 : Better creation of KSReportingAttributes #
Total comments: 19
Patch Set 16 : Comments from mark@ and grt@. Also, make things compile. #Patch Set 17 : Small tweak before review #
Total comments: 2
Patch Set 18 : Google Updater handles unsigned values #
Total comments: 2
Patch Set 19 : Comment refined #Patch Set 20 : Rebase #Messages
Total messages: 45 (7 generated)
mlerman@chromium.org changed reviewers: + bcwhite@chromium.org
On 2014/09/23 19:41:07, Mike Lerman wrote: Hi Brian, Can you take a look at this for me before I send it out to the Keystone-Omaha and Mac folks? Thanks! Mike
Boris, here's the CL for you to look at.
https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:499: if (numSignedInProfiles_ > 0) { Should this be "numProfiles_"? You could have existing profiles with none signed it and we'd want to report that. https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:535: [ksr setActive]; How about moving this out of the condition and eliminating the "else" block? https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:536: return; Unneeded. https://codereview.chromium.org/593243002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_metrics.cc (right): https://codereview.chromium.org/593243002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.cc:162: void ProfileMetrics::UpdateReportedOSProfileStatistics( Any particular reason you moved the block down here from its original position? Makes the diff less obvious.
mlerman@chromium.org changed reviewers: + borisv@chromium.org
Hi Boris, I feel like, if any call returns in error, rather than calling setActive, I'll want to call setActiveWithReportAttributes passing in an array with two reporting attributes with zero values, with the idea that any error means I should zero out the results and set active. I'd then want to check, and if that fails, just call setActive. Sounds good? https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:499: if (numSignedInProfiles_ > 0) { On 2014/09/23 20:32:58, bcwhite wrote: > Should this be "numProfiles_"? You could have existing profiles with none > signed it and we'd want to report that. Done. https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:535: [ksr setActive]; On 2014/09/23 20:32:58, bcwhite wrote: > How about moving this out of the condition and eliminating the "else" block? I flipped the upper condition around. https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:536: return; On 2014/09/23 20:32:58, bcwhite wrote: > Unneeded. Done. https://codereview.chromium.org/593243002/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_metrics.cc (right): https://codereview.chromium.org/593243002/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics.cc:162: void ProfileMetrics::UpdateReportedOSProfileStatistics( On 2014/09/23 20:32:58, bcwhite wrote: > Any particular reason you moved the block down here from its original position? > Makes the diff less obvious. The method was previously in the anonymous namespace. The method is now a static member of the class, so was moved down to the class methods.
LGTM https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:535: [ksr setActive]; On 2014/09/26 19:52:30, Mike Lerman wrote: > On 2014/09/23 20:32:58, bcwhite wrote: > > How about moving this out of the condition and eliminating the "else" block? > > I flipped the upper condition around. I like the idea where you optionally add things to the ksr and then just call setActive at the very end.
Concern about the check for numProfiles_ to 0 https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:501: return; This will break in the following case: 1. User A (first user) signs in. numProfiles_ becomes equal to 1 and Keystone remembers to report numProfiles_=1 for 28 days. 2. The user signs out, goes to privacy settings and clears all history. numProfiles_ becomes 0, so Keystone is never updated. 3. Keystone will continue to report incorrectly numProfiles_ = 1 for 28 more days. I'd suggest that you remove this check for 0 altogether and always report the number.
mlerman@chromium.org changed reviewers: + mark@chromium.org
Hi Mark, I'm still getting reviews, but can you take a high level design review at this CL as I proceed with details? I can't land this CL until the appropriate version of Keystone is landed within Chromium.
I’d like to see the upstream Keystone change(s) too. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:124: - (void)setRegistrationActive:(KSRegistration*)ksr; Why is this taking ksr as a parameter when registration_ is already available? https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_registration.h (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_registration.h:65: - (BOOL)setActiveWithReportingAttributes:(NSArray *)reportingAttributes Chrome style nestles the * up against the type name. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_reporting_attribute.h (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_reporting_attribute.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. I think this should just go in-line in keystone_registration.h. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_reporting_attribute.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. No (c) in new files. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_metrics_mac.mm (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics_mac.mm:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. No (c) in new files. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics_mac.mm:9: #if defined(OS_MACOSX) Not necessary. _mac.mm files will only be built on Mac.
Hi Mark, Here's the Keystone CL: https://critique.corp.google.com/#review/75217474 https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/20001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:535: [ksr setActive]; On 2014/09/26 20:41:20, bcwhite wrote: > On 2014/09/26 19:52:30, Mike Lerman wrote: > > On 2014/09/23 20:32:58, bcwhite wrote: > > > How about moving this out of the condition and eliminating the "else" block? > > > > I flipped the upper condition around. > > I like the idea where you optionally add things to the ksr and then just call > setActive at the very end. We don't always call setActive, we call it only if setActiveWithReportingAttributes fails. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:124: - (void)setRegistrationActive:(KSRegistration*)ksr; On 2014/10/02 16:07:23, Mark Mentovai wrote: > Why is this taking ksr as a parameter when registration_ is already available? when it's called from markActive (when the timer triggers), don't I need to get the ksr from the timer's userInfo? https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:501: return; On 2014/09/26 20:57:00, Boris Vidolov wrote: > This will break in the following case: > 1. User A (first user) signs in. numProfiles_ becomes equal to 1 and Keystone > remembers to report numProfiles_=1 for 28 days. > 2. The user signs out, goes to privacy settings and clears all history. > numProfiles_ becomes 0, so Keystone is never updated. > 3. Keystone will continue to report incorrectly numProfiles_ = 1 for 28 more > days. > I'd suggest that you remove this check for 0 altogether and always report the > number. In step 2 here, after signing out, numAccounts will become 0, but numProfiles will continue to be 1. You always have 1 profile - having 0 would be an error condition for the profiles code. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_registration.h (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_registration.h:65: - (BOOL)setActiveWithReportingAttributes:(NSArray *)reportingAttributes On 2014/10/02 16:07:23, Mark Mentovai wrote: > Chrome style nestles the * up against the type name. Thanks https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_reporting_attribute.h (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_reporting_attribute.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/10/02 16:07:23, Mark Mentovai wrote: > I think this should just go in-line in keystone_registration.h. I was mirroring the file structure of the Keystone files. I'll let you review the Keystone CL - let me know if you still think I should inline. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_metrics_mac.mm (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics_mac.mm:9: #if defined(OS_MACOSX) On 2014/10/02 16:07:23, Mark Mentovai wrote: > Not necessary. _mac.mm files will only be built on Mac. They don't come on iOS as well?
https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:124: - (void)setRegistrationActive:(KSRegistration*)ksr; Mike Lerman wrote: > On 2014/10/02 16:07:23, Mark Mentovai wrote: > > Why is this taking ksr as a parameter when registration_ is already available? > > when it's called from markActive (when the timer triggers), don't I need to get > the ksr from the timer's userInfo? -markActive: is called on the KeystoneGlue instance, so why wouldn’t registration_ be valid? I don’t think we need to supply registration_ to the timer as userInfo at all. If you’re going to supply it, all you should do is DCHECK that it’s the same as registration_ in the -markActive: timer callback. But what’s the point? I see no reason that -setRegistrationActive: needs to accept a KSRegistration* argument at all. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_reporting_attribute.h (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_reporting_attribute.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. Mike Lerman wrote: > On 2014/10/02 16:07:23, Mark Mentovai wrote: > > I think this should just go in-line in keystone_registration.h. > > I was mirroring the file structure of the Keystone files. I'll let you review > the Keystone CL - let me know if you still think I should inline. Still do. This header doesn’t have to be thought of as a copy of KeystoneRegistration.h, it’s just the declarations that we need in Chrome to use KeystoneRegistration.framework. It’s OK if not all of those come from KeystoneRegistration.h upstream. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_metrics_mac.mm (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics_mac.mm:9: #if defined(OS_MACOSX) Mike Lerman wrote: > On 2014/10/02 16:07:23, Mark Mentovai wrote: > > Not necessary. _mac.mm files will only be built on Mac. > > They don't come on iOS as well? _mac.mm files are built on Mac only, not on iOS. See build/filename_rules.gypi. https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.h (right): https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.h:75: @class KSReportingAttriburte; This header doesn’t make use of KSReportingAttribute or KSUnsignedReportingAttribute, so these declarations don’t belong here. KSReportingAttribute is misspelled as KSReportingAttriburte here, anyway. https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.h:182: - (void)updateProfileCountsWithNumProfiles:(size_t)numProfiles The caller’s interface uses int and Keystone’s interface uses uint32_t. You’re now adding a third distinct type, size_t. The most prudent thing to do when adapting the two dissimilar types (int and uint32_t) would be to check that the source value is in range for the destination type. base/numerics/safe_conversions.h can help. The least prudent thing to do is to introduce a third type and not have any range checking on the values at all. https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:501: return; Every early return in this function is preceded by [ksr setActive]. Seems like some light refactoring is in order. Like this: - (BOOL)setRegistrationActiveWithReportingAttribute; - (void)setRegistrationActive { if (![self setRegistrationActiveWithReportingAttribute]) { [registration_ setActive]; } } https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:512: LOG(ERROR) << [reportingAttributeError localizedDescription]; I don’t know that ERROR is the appropriate severity for these. They don’t really affect the user or the running of the app. INFO, perhaps?
Suggestion to simplify the Chrome code and push the setting of active in case of reporting attributes error to the Keystone framework https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:501: return; On 2014/10/06 18:35:13, Mark Mentovai wrote: > Every early return in this function is preceded by [ksr setActive]. Seems like > some light refactoring is in order. Like this: > > - (BOOL)setRegistrationActiveWithReportingAttribute; > > - (void)setRegistrationActive { > if (![self setRegistrationActiveWithReportingAttribute]) { > [registration_ setActive]; > } > } I'd rather put this as a requirement on the Keystone registration framework. KSRegistration::SetActiveWithReportingAttributes should attempt to set as Active first, even if reporting attributes fail. This would avoid all of the registration framework users to write a complex code like this.
I’d support that.
https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_glue.mm:124: - (void)setRegistrationActive:(KSRegistration*)ksr; On 2014/10/06 18:35:13, Mark Mentovai wrote: > Mike Lerman wrote: > > On 2014/10/02 16:07:23, Mark Mentovai wrote: > > > Why is this taking ksr as a parameter when registration_ is already > available? > > > > when it's called from markActive (when the timer triggers), don't I need to > get > > the ksr from the timer's userInfo? > > -markActive: is called on the KeystoneGlue instance, so why wouldn’t > registration_ be valid? > > I don’t think we need to supply registration_ to the timer as userInfo at all. > If you’re going to supply it, all you should do is DCHECK that it’s the same as > registration_ in the -markActive: timer callback. But what’s the point? > > I see no reason that -setRegistrationActive: needs to accept a KSRegistration* > argument at all. Ok, removed it, now referencing the object's registration_. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... File chrome/browser/mac/keystone_reporting_attribute.h (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/mac/keyst... chrome/browser/mac/keystone_reporting_attribute.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/10/06 18:35:13, Mark Mentovai wrote: > Mike Lerman wrote: > > On 2014/10/02 16:07:23, Mark Mentovai wrote: > > > I think this should just go in-line in keystone_registration.h. > > > > I was mirroring the file structure of the Keystone files. I'll let you review > > the Keystone CL - let me know if you still think I should inline. > > Still do. This header doesn’t have to be thought of as a copy of > KeystoneRegistration.h, it’s just the declarations that we need in Chrome to use > KeystoneRegistration.framework. It’s OK if not all of those come from > KeystoneRegistration.h upstream. Ok. Done. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/profiles/... File chrome/browser/profiles/profile_metrics_mac.mm (right): https://codereview.chromium.org/593243002/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics_mac.mm:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/10/02 16:07:23, Mark Mentovai wrote: > No (c) in new files. Done. https://codereview.chromium.org/593243002/diff/60001/chrome/browser/profiles/... chrome/browser/profiles/profile_metrics_mac.mm:9: #if defined(OS_MACOSX) On 2014/10/06 18:35:13, Mark Mentovai wrote: > Mike Lerman wrote: > > On 2014/10/02 16:07:23, Mark Mentovai wrote: > > > Not necessary. _mac.mm files will only be built on Mac. > > > > They don't come on iOS as well? > > _mac.mm files are built on Mac only, not on iOS. See build/filename_rules.gypi. Cool, good to know. Done. https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.h (right): https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.h:75: @class KSReportingAttriburte; On 2014/10/06 18:35:13, Mark Mentovai wrote: > This header doesn’t make use of KSReportingAttribute or > KSUnsignedReportingAttribute, so these declarations don’t belong here. > > KSReportingAttribute is misspelled as KSReportingAttriburte here, anyway. Done. In a previous local incarnation I had direct member objects. https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.h:182: - (void)updateProfileCountsWithNumProfiles:(size_t)numProfiles On 2014/10/06 18:35:13, Mark Mentovai wrote: > The caller’s interface uses int and Keystone’s interface uses uint32_t. You’re > now adding a third distinct type, size_t. > > The most prudent thing to do when adapting the two dissimilar types (int and > uint32_t) would be to check that the source value is in range for the > destination type. base/numerics/safe_conversions.h can help. > > The least prudent thing to do is to introduce a third type and not have any > range checking on the values at all. Ah, indeed. Used to using size_t in the chromium code. I'll change this to the uint32_t and do the conversion in profile_metrics_mac.mm. https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:501: return; On 2014/10/06 20:25:52, Boris Vidolov wrote: > On 2014/10/06 18:35:13, Mark Mentovai wrote: > > Every early return in this function is preceded by [ksr setActive]. Seems like > > some light refactoring is in order. Like this: > > > > - (BOOL)setRegistrationActiveWithReportingAttribute; > > > > - (void)setRegistrationActive { > > if (![self setRegistrationActiveWithReportingAttribute]) { > > [registration_ setActive]; > > } > > } > I'd rather put this as a requirement on the Keystone registration framework. > KSRegistration::SetActiveWithReportingAttributes should attempt to set as Active > first, even if reporting attributes fail. This would avoid all of the > registration framework users to write a complex code like this. I understand this to mean that the SetActiveWithReportingAttributes (a) can accept null's or invalid ReportingAttribute objects in the NSArray, and (b) can be assumed to call [ setActive] if is it all possible, if either there are null objects within the NSArray or if sending the ReportingAttribute fails. Is that accurate, boris? if so, that's reflected now here. https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:512: LOG(ERROR) << [reportingAttributeError localizedDescription]; On 2014/10/06 18:35:13, Mark Mentovai wrote: > I don’t know that ERROR is the appropriate severity for these. They don’t really > affect the user or the running of the app. INFO, perhaps? Changed to VLOG(INFO). What do you think of also logging something via UMA?
https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:512: LOG(ERROR) << [reportingAttributeError localizedDescription]; Mike Lerman wrote: > On 2014/10/06 18:35:13, Mark Mentovai wrote: > > I don’t know that ERROR is the appropriate severity for these. They don’t > really > > affect the user or the running of the app. INFO, perhaps? > > Changed to VLOG(INFO). > What do you think of also logging something via UMA? Doubt it’s necessary. https://codereview.chromium.org/593243002/diff/140001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.h (right): https://codereview.chromium.org/593243002/diff/140001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.h:106: size_t numProfiles_; Just use uint32_t, since that’s what we have on both sides of the interface now. size_t is wider than uint32_t in the 64-bit environment, what’s the point of expanding a uint32_t to fill a size_t only to truncate it back down to uint32_t when you give it to Keystone? But see the “other option” comment in profile_metrics.cc. https://codereview.chromium.org/593243002/diff/140001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/140001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:499: [registration_ setActive]; What’s the point of this? Why wouldn’t we want to push “0” back to Keystone? It’s quite possible that you have a compelling reason for this, but it’s totally unclear from here. You’d need a comment to explain the intended logic. https://codereview.chromium.org/593243002/diff/140001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:511: VLOG(INFO) << [reportingError localizedDescription]; What is VLOG(INFO)? VLOG() takes an integer argument, the verbosity level. LOG() takes severity levels like INFO. Did you want VLOG(1)? https://codereview.chromium.org/593243002/diff/140001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.cc (right): https://codereview.chromium.org/593243002/diff/140001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.cc:146: int limited_total = counts.total; Other option: change these to size_t. All of the fields in ProfileCounts are already size_t. You will need to change the definition of UpdateReportedOSProfileStatistics to take two size_t arguments. You will need to update the Mac and Windows code to deal with size_t. You will need to check in keystone_glue.mm that you’re not overflowing uint32_t where the conversion is made. You may also need to check this in the Windows file. The existing use of “int” here does seem sloppy.
Patchset #9 (id:240001) has been deleted
https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:512: LOG(ERROR) << [reportingAttributeError localizedDescription]; On 2014/10/09 21:26:20, Mark Mentovai wrote: > Mike Lerman wrote: > > On 2014/10/06 18:35:13, Mark Mentovai wrote: > > > I don’t know that ERROR is the appropriate severity for these. They don’t > > really > > > affect the user or the running of the app. INFO, perhaps? > > > > Changed to VLOG(INFO). > > What do you think of also logging something via UMA? > > Doubt it’s necessary. Acknowledged. https://codereview.chromium.org/593243002/diff/140001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.h (right): https://codereview.chromium.org/593243002/diff/140001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.h:106: size_t numProfiles_; On 2014/10/09 21:26:20, Mark Mentovai wrote: > Just use uint32_t, since that’s what we have on both sides of the interface now. > > size_t is wider than uint32_t in the 64-bit environment, what’s the point of > expanding a uint32_t to fill a size_t only to truncate it back down to uint32_t > when you give it to Keystone? > > But see the “other option” comment in profile_metrics.cc. Back to uint32_t for everything in keystone. https://codereview.chromium.org/593243002/diff/140001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/140001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:499: [registration_ setActive]; On 2014/10/09 21:26:20, Mark Mentovai wrote: > What’s the point of this? Why wouldn’t we want to push “0” back to Keystone? > > It’s quite possible that you have a compelling reason for this, but it’s totally > unclear from here. You’d need a comment to explain the intended logic. There should never be zero profiles - that's invalid for Chrome. For this class, it means that the value hasn't been set yet, or something else very wonky is going on. Added a comment. https://codereview.chromium.org/593243002/diff/140001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:511: VLOG(INFO) << [reportingError localizedDescription]; On 2014/10/09 21:26:20, Mark Mentovai wrote: > What is VLOG(INFO)? VLOG() takes an integer argument, the verbosity level. LOG() > takes severity levels like INFO. Did you want VLOG(1)? I suppose so! VLOG and LOG should have a similar interface :P changed. https://codereview.chromium.org/593243002/diff/140001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.cc (right): https://codereview.chromium.org/593243002/diff/140001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.cc:146: int limited_total = counts.total; On 2014/10/09 21:26:21, Mark Mentovai wrote: > Other option: change these to size_t. All of the fields in ProfileCounts are > already size_t. You will need to change the definition of > UpdateReportedOSProfileStatistics to take two size_t arguments. You will need to > update the Mac and Windows code to deal with size_t. You will need to check in > keystone_glue.mm that you’re not overflowing uint32_t where the conversion is > made. You may also need to check this in the Windows file. > > The existing use of “int” here does seem sloppy. Ok. I changed the profile_metrics code to work with size_t, which is internally consistent. Windows code updated, which was simpler, as all I end up doing is converting to a string, so I used a different converstion function. I added a check in profile_metrics_mac.mm that calls IsValueInRangeForNumericType before casting to a uint32_t and calling keystone_glue (it seemed clean to do the conversion in there and leave the interface to keystone_glue.mm with a uint32_t type).
Publishing some comments. https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/100001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:501: return; On 2014/10/09 20:41:01, Mike Lerman wrote: > On 2014/10/06 20:25:52, Boris Vidolov wrote: > > On 2014/10/06 18:35:13, Mark Mentovai wrote: > > > Every early return in this function is preceded by [ksr setActive]. Seems > like > > > some light refactoring is in order. Like this: > > > > > > - (BOOL)setRegistrationActiveWithReportingAttribute; > > > > > > - (void)setRegistrationActive { > > > if (![self setRegistrationActiveWithReportingAttribute]) { > > > [registration_ setActive]; > > > } > > > } > > I'd rather put this as a requirement on the Keystone registration framework. > > KSRegistration::SetActiveWithReportingAttributes should attempt to set as > Active > > first, even if reporting attributes fail. This would avoid all of the > > registration framework users to write a complex code like this. > > I understand this to mean that the SetActiveWithReportingAttributes (a) can > accept null's or invalid ReportingAttribute objects in the NSArray, and (b) can > be assumed to call [ setActive] if is it all possible, if either there are null > objects within the NSArray or if sending the ReportingAttribute fails. Is that > accurate, boris? if so, that's reflected now here. Well, it is. The idea is that you don't have to fall back to setActive everywhere. Create a mutable dictionary, fill it in with all attributes you can create, logging local errors whenever they show. At the end pass this dictionary filled with whatever you have to setActiveWithReportingAttributes and if it errors out, you log the error. This is it. No need for additional calls to SetActive in case of error, no need to bail if one or more attributes cannot be created - just log the error and continue with the rest of the attributes. Also, don't forget my other comment about always sending these out, even if numProfiles is 0. The issue is when they were not 0 before and just changed to 0. Keystone will continue reporting the non-zero amount as it does not see a change in the attribute value.
mlerman@chromium.org changed reviewers: + grt@chromium.org
Okay. This has been through a few levels of iteration and now actually works!!! The KeystoneRegistration that's in src-internal needs to be updated, but I verified the functionality against a version Boris provided me. Please take a look. +grt for changes to chrome/installer/util. Thanks!
https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.h (right): https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.h:180: - (void)updateProfileCountsWithNumProfiles:(uint32_t)numProfiles I don’t think you need “num” in any of these, same in the .mm file. It’s obvious that a count is going to be a number. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:222: NSBundle* ksrBundle; Don’t store this in a global. Use an instance variable. But if all you need is a single class (KSUnsignedReportingAttribute) from this, maybe you should store that class instead of the whole NSBundle. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:540: VLOG(1) << [reportingError localizedDescription]; {brace} this because its controlling condition takes up multiple lines and the ragged indentation makes it hard to follow the control flow. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_registration.h (right): https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... chrome/browser/mac/keystone_registration.h:90: /* We don’t need to copy any of the documentation. See above, the docs aren’t copied there. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.cc (right): https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.cc:195: // The MACOSX implementation of this function is in profile_metrics_mac.mm. What is MACOSX? https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics_mac.mm (right): https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics_mac.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics_mac.mm:12: if (base::IsValueInRangeForNumericType<uint32_t, size_t>(active) && I don’t think the second template parameter is necessary, it’s implied by the type of the function argument. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics_mac.mm:15: updateProfileCountsWithNumProfiles:(uint32_t)active I don’t think these casts are necessary.
https://codereview.chromium.org/593243002/diff/430001/chrome/installer/util/g... File chrome/installer/util/google_update_settings.h (right): https://codereview.chromium.org/593243002/diff/430001/chrome/installer/util/g... chrome/installer/util/google_update_settings.h:221: static void UpdateProfileCounts(size_t profiles_active, Why size_t? http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types
https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.h (right): https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.h:180: - (void)updateProfileCountsWithNumProfiles:(uint32_t)numProfiles On 2015/02/20 21:54:18, Mark Mentovai wrote: > I don’t think you need “num” in any of these, same in the .mm file. It’s obvious > that a count is going to be a number. Done - though the class' member variables (numProfiles_ and numSignedInProfiles_) should probably keep the prefix since they're not explicitly parameters to a method called "count" - their meaning needs to stand alone a bit better. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:222: NSBundle* ksrBundle; On 2015/02/20 21:54:18, Mark Mentovai wrote: > Don’t store this in a global. Use an instance variable. > > But if all you need is a single class (KSUnsignedReportingAttribute) from this, > maybe you should store that class instead of the whole NSBundle. Done. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:540: VLOG(1) << [reportingError localizedDescription]; On 2015/02/20 21:54:18, Mark Mentovai wrote: > {brace} this because its controlling condition takes up multiple lines and the > ragged indentation makes it hard to follow the control flow. Indeed - thanks. Done. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_registration.h (right): https://codereview.chromium.org/593243002/diff/430001/chrome/browser/mac/keys... chrome/browser/mac/keystone_registration.h:90: /* On 2015/02/20 21:54:19, Mark Mentovai wrote: > We don’t need to copy any of the documentation. See above, the docs aren’t > copied there. Done. I kept the location of the source file, similar to what's on line 11-12. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.cc (right): https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.cc:195: // The MACOSX implementation of this function is in profile_metrics_mac.mm. On 2015/02/20 21:54:19, Mark Mentovai wrote: > What is MACOSX? That's the name of the OS constant we use for ifdef's.. OS_MACOSX. Since the UpdateReportedOSProfileStatistics method is defined once in the header file with #if defined(OS_WIN) || defined(OS_MACOSX), but here I only have the (OS_WIN) implementation, I wanted to call out where the other implementation is. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics_mac.mm (right): https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics_mac.mm:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/02/20 21:54:19, Mark Mentovai wrote: > 2015 Done. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics_mac.mm:12: if (base::IsValueInRangeForNumericType<uint32_t, size_t>(active) && On 2015/02/20 21:54:19, Mark Mentovai wrote: > I don’t think the second template parameter is necessary, it’s implied by the > type of the function argument. Done. https://codereview.chromium.org/593243002/diff/430001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics_mac.mm:15: updateProfileCountsWithNumProfiles:(uint32_t)active On 2015/02/20 21:54:19, Mark Mentovai wrote: > I don’t think these casts are necessary. Done. https://codereview.chromium.org/593243002/diff/430001/chrome/installer/util/g... File chrome/installer/util/google_update_settings.h (right): https://codereview.chromium.org/593243002/diff/430001/chrome/installer/util/g... chrome/installer/util/google_update_settings.h:221: static void UpdateProfileCounts(size_t profiles_active, On 2015/02/21 00:08:47, grt wrote: > Why size_t? > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types I went with size_t because that's what we tend to use in profiles when there's a non-negative number, e.g. a count of profiles, an iterator over profiles, or an index into the ProfileInfoCache. That link you mention notes that size_t is acceptable to use. mark@ prefers the parameter passed to Keystone be of an unsigned type, so I wanted to be consistent to the windows side as well.
https://codereview.chromium.org/593243002/diff/430001/chrome/installer/util/g... File chrome/installer/util/google_update_settings.h (right): https://codereview.chromium.org/593243002/diff/430001/chrome/installer/util/g... chrome/installer/util/google_update_settings.h:221: static void UpdateProfileCounts(size_t profiles_active, On 2015/02/23 16:12:12, Mike Lerman wrote: > On 2015/02/21 00:08:47, grt wrote: > > Why size_t? > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types > > I went with size_t because that's what we tend to use in profiles when there's a > non-negative number, e.g. a count of profiles, an iterator over profiles, or an > index into the ProfileInfoCache. That link you mention notes that size_t is > acceptable to use. > > mark@ prefers the parameter passed to Keystone be of an unsigned type, so I > wanted to be consistent to the windows side as well. That seems to run contrary to "In particular, do not use unsigned types to say a number will never be negative." size_t is unambiguously correct when used to index into a std container (e.g., iterating over a vector), but it's less clear to me that "there will never be a negative number of profiles" is a good reason. My take on the style guide is that int is the correct type for this. Mark: why do you prefer an unsigned type for Keystone?
I don’t have a preference for signed vs. unsigned, but I do have a preference to maintain the same signedness all the way through a pipeline. The existing infrastructure in Chrome to track this (the ProfileCounts type) already dealt in size_t exclusively, and the Keystone interface deals with uint32_t. I wanted to eliminate the flip-flopping on the type. Since it was already unsigned on both ends, there was no reason to take a trip through signed in the middle. I don’t particularly care if you want to change all of these to int or some other signed type, but I do feel that the same signedness should be respected throughout, so it would have to change in ProfileCounts and Keystone’s interface as well.
On 2015/02/23 21:20:36, Mark Mentovai wrote: > I don’t have a preference for signed vs. unsigned, but I do have a preference to > maintain the same signedness all the way through a pipeline. The existing > infrastructure in Chrome to track this (the ProfileCounts type) already dealt in > size_t exclusively, and the Keystone interface deals with uint32_t. I wanted to > eliminate the flip-flopping on the type. Since it was already unsigned on both > ends, there was no reason to take a trip through signed in the middle. > > I don’t particularly care if you want to change all of these to int or some > other signed type, but I do feel that the same signedness should be respected > throughout, so it would have to change in ProfileCounts and Keystone’s interface > as well. A change to this would push us back a milestone. The profile_metrics effort would be a refactor, but changing the Keystone API I think would involve a new release of that product, which is slow. I don't think using size_t is unambiguously wrong; much as an index into an array is 0+, a count of profiles is also always positive. The other consumer of these counts are UMA_HISTOGRAM_COUNTS metrics, which take positive numbers (although can take int type). I've even had other reviewers recommend that size_t makes sense for these counts (although I can't find the CL).
Okay, size_t SGTM. Thanks for the background, Mark. https://codereview.chromium.org/593243002/diff/470001/chrome/installer/util/g... File chrome/installer/util/google_update_settings.cc (right): https://codereview.chromium.org/593243002/diff/470001/chrome/installer/util/g... chrome/installer/util/google_update_settings.cc:93: return (key.WriteValue(uniquename.c_str(), value) == ERROR_SUCCESS); presumably this is using the DWORD WriteValue, where DWORD is an unsigned type (uint32_t, effectively). how about changing |value| in this function from an int to a size_t and adding a check that the value doesn't exceed the bounds of a DWORD with something like: #include <limits> DWORD dword_value = (value > std::numeric_limits<DWORD>::max() ? std::numeric_limits<DWORD>::max() : static_cast<DWORD>(value)); return (key.WriteValue(uniquename.c_str(), dword_value) == ERROR_SUCCESS); this lets you get rid of the unsafe c-style cast in GoogleUpdateSettings::UpdateProfileCounts.
On 2015/02/24 17:19:54, Mike Lerman wrote: > On 2015/02/23 21:20:36, Mark Mentovai wrote: > > I don’t have a preference for signed vs. unsigned, but I do have a preference > to > > maintain the same signedness all the way through a pipeline. The existing > > infrastructure in Chrome to track this (the ProfileCounts type) already dealt > in > > size_t exclusively, and the Keystone interface deals with uint32_t. I wanted > to > > eliminate the flip-flopping on the type. Since it was already unsigned on both > > ends, there was no reason to take a trip through signed in the middle. > > > > I don’t particularly care if you want to change all of these to int or some > > other signed type, but I do feel that the same signedness should be respected > > throughout, so it would have to change in ProfileCounts and Keystone’s > interface > > as well. > > A change to this would push us back a milestone. The profile_metrics effort > would be a refactor, but changing the Keystone API I think would involve a new > release of that product, which is slow. > > I don't think using size_t is unambiguously wrong; much as an index into an > array is 0+, a count of profiles is also always positive. The other consumer of > these counts are UMA_HISTOGRAM_COUNTS metrics, which take positive numbers > (although can take int type). I've even had other reviewers recommend that > size_t makes sense for these counts (although I can't find the CL). Keystone will be happy to change our APIs from unsigned to signed. It will take us around a week. Just confirm that you need it. Such change will have no impact on the file formats, so everything will work fine. Just give us heads up, so that we can start the work as early as possible.
https://codereview.chromium.org/593243002/diff/470001/chrome/installer/util/g... File chrome/installer/util/google_update_settings.cc (right): https://codereview.chromium.org/593243002/diff/470001/chrome/installer/util/g... chrome/installer/util/google_update_settings.cc:93: return (key.WriteValue(uniquename.c_str(), value) == ERROR_SUCCESS); On 2015/02/24 17:57:00, grt wrote: > presumably this is using the DWORD WriteValue, where DWORD is an unsigned type > (uint32_t, effectively). how about changing |value| in this function from an int > to a size_t and adding a check that the value doesn't exceed the bounds of a > DWORD with something like: > > #include <limits> > > DWORD dword_value = (value > std::numeric_limits<DWORD>::max() ? > std::numeric_limits<DWORD>::max() : > static_cast<DWORD>(value)); > return (key.WriteValue(uniquename.c_str(), dword_value) == ERROR_SUCCESS); > > this lets you get rid of the unsafe c-style cast in > GoogleUpdateSettings::UpdateProfileCounts. Ah, great! I didn't realize DWORDs were unsigned. This works perfectly.
chrome/installer/util/* lgtm
Thanks, Greg. Mark, any further comments on this CL?
LGTM now https://codereview.chromium.org/593243002/diff/490001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.cc (right): https://codereview.chromium.org/593243002/diff/490001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.cc:198: // The MACOS implementation of this function is in profile_metrics_mac.mm. Mac OS X or OS_MACOSX.
Thanks, Mark! I'll wait until you and Boris have the updated Keystone library landed into Chrome's src_internal before I send this into the CQ. Let me know when that's done, please. https://codereview.chromium.org/593243002/diff/490001/chrome/browser/profiles... File chrome/browser/profiles/profile_metrics.cc (right): https://codereview.chromium.org/593243002/diff/490001/chrome/browser/profiles... chrome/browser/profiles/profile_metrics.cc:198: // The MACOS implementation of this function is in profile_metrics_mac.mm. On 2015/02/25 21:38:06, Mark Mentovai wrote: > Mac OS X or OS_MACOSX. OS_MACOSX :)
If the Keystone that you want is checked in to googlemac/Releases already, why don’t you import it into Chrome and send me the reviews?
I've landed the Keystone libraries into src-internal, and verified that my ToT with this patch works. Landing!
The CQ bit was checked by mlerman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bcwhite@chromium.org, mark@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/593243002/#ps530001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/593243002/530001
Message was sent while issue was closed.
Committed patchset #20 (id:530001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/0bcaa8e521eae39a81b4754f8b4ac9de1f0463d5 Cr-Commit-Position: refs/heads/master@{#318729}
Message was sent while issue was closed.
I just crashed in this code (either KeystoneRegistration or keystone_glue). http://crbug.com/463781.
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:530001) has been created in https://codereview.chromium.org/974423002/ by mlerman@chromium.org. The reason for reverting is: crbug.com/463900. Mac crashed, like, all the time.. |
