Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
272763002:
Make code for the AutomaticProfileResetter more readabile (readability review) (Closed)
|
Created:
6 years, 7 months ago by engedy Modified:
6 years, 6 months ago CC:
robertshield, MAD Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake code for the AutomaticProfileResetter more readable (readability review)
The code was originally reviewed in:
* https://codereview.chromium.org/62193002
* https://codereview.chromium.org/27030002
Since then, there have been some changes by others, but the bulk of the code is still my original.
BUG=298036
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273873
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments. #
Total comments: 10
Patch Set 3 : Addressed comments by pkasting@. #Messages
Total messages: 13 (0 generated)
This is a very strong readability submission. I've asked for some additional comments in the code, and recommended only 1 change to the actual code. Please let me know if you have any questions. https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Please add a comment. https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:247: for (ScopedVector<TemplateURLData>::const_iterator it = engines.begin(); This can be rewritten more clearly as: for (const auto& engine : engines) { https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Please add a comment. https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:184: Profile* profile_; https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Please add comments for all class member variables. It's okay to use a single comment for similar variables.
@Jay, please see my responses inline. https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/05/25 21:21:58, laefer wrote: > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > Please add a comment. Done, although I could not come up with anything meaningful to say here. Please let me know if you see something that would be worth pointing out. https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:247: for (ScopedVector<TemplateURLData>::const_iterator it = engines.begin(); On 2014/05/25 21:21:58, laefer wrote: > This can be rewritten more clearly as: > > for (const auto& engine : engines) { Indeed, I could not agree more. -- However, for compatibility reasons, it is not allowed to use C++11 constructs in the Chromium code. :( https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/05/25 21:21:58, laefer wrote: > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > Please add a comment. Done. https://codereview.chromium.org/272763002/diff/1/chrome/browser/profile_reset... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:184: Profile* profile_; On 2014/05/25 21:21:58, laefer wrote: > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... > > Please add comments for all class member variables. It's okay to use a single > comment for similar variables. Done.
lgtm
@Peter, @Dominic: as the main reviewers for the original CLs, could you please take a quick look?
lgtm
LGTM https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:5: // Contains the implementation for AutomaticProfileResetterDelegateImpl. Personally, I'm skeptical that this comment adds anything useful to the reader. I realize the style guide requests file comments in all files, but I would put a file comment here only if you have anything to say about the actual implementation, or if you need to direct the reader to a specific place to find documentation about the contents of the file. https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:400: Nit: Don't add a trailing blank line https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:11: // On the other hand, it also becomes easier to verify that the state machine "On one/the other hand" implies two arguments that counter each other. In this case you're just giving two arguments that both suggest factoring things out is a good idea, so don't use this idiom. https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:241: Nit: Don't add a trailing blank line https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:605: Nit: Don't add a trailing blank line
https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc (right): https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:5: // Contains the implementation for AutomaticProfileResetterDelegateImpl. On 2014/05/28 18:46:32, Peter Kasting wrote: > Personally, I'm skeptical that this comment adds anything useful to the reader. > I realize the style guide requests file comments in all files, but I would put a > file comment here only if you have anything to say about the actual > implementation, or if you need to direct the reader to a specific place to find > documentation about the contents of the file. I agree, actually I have added this after quite some hesitation. Now I have removed it. https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc:400: On 2014/05/28 18:46:32, Peter Kasting wrote: > Nit: Don't add a trailing blank line Done. https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h (right): https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:11: // On the other hand, it also becomes easier to verify that the state machine On 2014/05/28 18:46:32, Peter Kasting wrote: > "On one/the other hand" implies two arguments that counter each other. In this > case you're just giving two arguments that both suggest factoring things out is > a good idea, so don't use this idiom. Fixed. Thanks for pointing this out! https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h:241: On 2014/05/28 18:46:32, Peter Kasting wrote: > Nit: Don't add a trailing blank line Done. https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... File chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc (right): https://codereview.chromium.org/272763002/diff/20001/chrome/browser/profile_r... chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:605: On 2014/05/28 18:46:32, Peter Kasting wrote: > Nit: Don't add a trailing blank line Done.
The CQ bit was checked by engedy@chromium.org
The CQ bit was unchecked by engedy@chromium.org
The CQ bit was checked by engedy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/engedy@chromium.org/272763002/40001
Message was sent while issue was closed.
Change committed as 273873 |
