Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
188103004:
C++ Readability review for holte (Closed)
|
Created:
6 years, 9 months ago by Steven Holte Modified:
6 years, 9 months ago CC:
chromium-reviews, Ilya Sherman, Alexei Svitkine (slow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionC++ Readability review for holte
Original change: https://codereview.chromium.org/49753002
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259469
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Added header files #
Total comments: 51
Patch Set 4 : Fixed comments #
Total comments: 20
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #Patch Set 7 : Braced Initialization #Patch Set 8 : #Patch Set 9 : Fix struct/class declaration #
Messages
Total messages: 22 (0 generated)
Overall this is very nicely written code, I only have a few minor comments. (Please note that a few comments, like the use of scoped_ptr, occur multiple times in the CL, but I only commented on it once.) https://codereview.chromium.org/188103004/diff/40001/components/rappor/log_up... File components/rappor/log_uploader.h (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/log_up... components/rappor/log_uploader.h:22: } Add end-of-namespace comment. https://codereview.chromium.org/188103004/diff/40001/components/rappor/log_up... components/rappor/log_uploader.h:61: virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE; Where's OVERRIDE defined? The current google3 preference would be to use only one of virtual/override. Since override is more useful than restating the function is virtual, I would write void Function() override; https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_metric.h (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:7: #ifndef COMPONENTS_RAPPOR_RAPPOR_H_ The include guard should read COMPONENTS_RAPPOR_RAPPOR_METRIC_H_. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:19: // and generates randomized reports about the collected data. Maybe a small usage example could be added here. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:28: // functions and used in the bloom filter. is used? Capitalize "Bloom", for consistency. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:29: explicit RapporMetric(const std::string& metric_name, "explicit" should only be used for single-argument constructors. (It prevents implicit conversions and these would not be tried if there are multiple arguments.) https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:31: int32_t cohort); Include <stdint.h> or any project-specific file where the type int32_t is defined. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:53: DISALLOW_COPY_AND_ASSIGN(RapporMetric); You should include an appropriate header for this macro. (In google3, that would be base/macros.h.) https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.cc (left): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:45: { // ETLD_PLUS_ONE_RAPPOR_TYPE The style guide says to format braced initializer lists like function calls, which would mean the following in this case: ____{// Comment _____16 /* comment */, _____2 /* comment */ (that is, four and five spaces, respectively) https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.cc (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:70: DCHECK(!uploader_); My preference is to compare pointers explicitly against nullptr instead of relying on bool conversion. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:96: // static I don't think "static" comments are very useful: * The difference between static and non-static member functions is mostly interesting to the clients * Comments often diverge from the code, so I would still need to look at the .h file to see if the comment was still valid or not. (This is my view; the style guide doesn't prescribe anything about this.) https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:103: DCHECK_EQ(cohort_, -1); Maybe check for !IsInitialized()? https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:106: return; I don't quite understand this part. Is the idea that you'll assign the cohort value only once, and this return is taken when the value is already set? https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:137: for (std::map<std::string, RapporMetric*>::iterator it = metrics_map_.begin(); Iterating is simpler with range-for statement: for (const auto& key_val : metrics_map_) { const RapporMetric* metric = key_val.second; ... (Note that the loop variable is not an iterator, it's pair<Key,Value>.) Side note: for old-style iteration you should use map::const_iterator. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:146: STLDeleteValues(&metrics_map_); The documentation for this function doesn't mention that all values are deleted. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:168: Remove this blank line? https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:176: std::map<std::string, RapporMetric*>::iterator it = This is a case where using auto is warranted, since the actual type is long and uninteresting. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.h (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:26: ETLD_PLUS_ONE_RAPPOR_TYPE = 0, Comment? Is the zero value significant somewhere? If not, you could leave it out. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:38: void Start(PrefService* pref_service, net::URLRequestContextGetter* context); Does URLRequestContextGetter come from you direct include files? If not, you should forward-declare it yourself. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:96: // Timer which schedules calls to OnLogInterval() Add period. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:100: scoped_ptr<LogUploader> uploader_; Do you still use scoped_ptr<> instead of more standard unique_ptr<>? In any case, please include the appropriate header. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:104: std::map<std::string, RapporMetric*> metrics_map_; #include <map> In modern C++, it would be preferable to codify the ownership and write std::map<std::string, std::unique_ptr<RapporMetric>> instead. If your standard library supports that, you could try it instead. (Then no STLDeleteValues will be needed, of course.)
Providing some context specific to the Chromium codebase: https://codereview.chromium.org/188103004/diff/40001/components/rappor/log_up... File components/rappor/log_uploader.h (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/log_up... components/rappor/log_uploader.h:22: } On 2014/03/17 11:22:25, ktl wrote: > Add end-of-namespace comment. These are generally omitted in Chromium code for forward-declarations, since the namespace block is so tiny. https://codereview.chromium.org/188103004/diff/40001/components/rappor/log_up... components/rappor/log_uploader.h:61: virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE; On 2014/03/17 11:22:25, ktl wrote: > Where's OVERRIDE defined? The current google3 preference would be to use only > one of virtual/override. Since override is more useful than restating the > function is virtual, I would write > > void Function() override; The style used here is the prevailing Chromium style. OVERRIDE is defined here: https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_spec... https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.cc (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:70: DCHECK(!uploader_); On 2014/03/17 11:22:25, ktl wrote: > My preference is to compare pointers explicitly against nullptr instead of > relying on bool conversion. FWIW, this is the prevailing style in Chromium code. Again, we don't have nullptr available in Chromium code, because we can't yet support C++11 across all of our toolchains. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:96: // static On 2014/03/17 11:22:25, ktl wrote: > I don't think "static" comments are very useful: > > * The difference between static and non-static member functions is mostly > interesting to the clients > * Comments often diverge from the code, so I would still need to look at the .h > file to see if the comment was still valid or not. > > (This is my view; the style guide doesn't prescribe anything about this.) Agreed, but this is the prevailing Chromium style, for whatever reason. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:137: for (std::map<std::string, RapporMetric*>::iterator it = metrics_map_.begin(); On 2014/03/17 11:22:25, ktl wrote: > Iterating is simpler with range-for statement: > > for (const auto& key_val : metrics_map_) { > const RapporMetric* metric = key_val.second; > ... > > (Note that the loop variable is not an iterator, it's pair<Key,Value>.) Can't do this due to lack of C++11. > Side note: for old-style iteration you should use map::const_iterator. So we should do this instead :) https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:176: std::map<std::string, RapporMetric*>::iterator it = On 2014/03/17 11:22:25, ktl wrote: > This is a case where using auto is warranted, since the actual type is long and > uninteresting. Again, can't use it due to lack of C++11. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.h (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:100: scoped_ptr<LogUploader> uploader_; On 2014/03/17 11:22:25, ktl wrote: > Do you still use scoped_ptr<> instead of more standard unique_ptr<>? In any > case, please include the appropriate header. Chromium code does not yet have unique_ptr support, due to needing to support a wide variety of older compiler toolchains. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:104: std::map<std::string, RapporMetric*> metrics_map_; On 2014/03/17 11:22:25, ktl wrote: > #include <map> > > In modern C++, it would be preferable to codify the ownership and write > > std::map<std::string, std::unique_ptr<RapporMetric>> > > instead. If your standard library supports that, you could try it instead. (Then > no STLDeleteValues will be needed, of course.) Unfortunately, Chromium code cannot yet use C++11.
Thanks, Ilya, for responding to many comments. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_metric.h (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:7: #ifndef COMPONENTS_RAPPOR_RAPPOR_H_ On 2014/03/17 11:22:25, ktl wrote: > The include guard should read COMPONENTS_RAPPOR_RAPPOR_METRIC_H_. Done. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:19: // and generates randomized reports about the collected data. On 2014/03/17 11:22:25, ktl wrote: > Maybe a small usage example could be added here. Added a comment that metrics should be recorded through RapporService::RecordSample. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:28: // functions and used in the bloom filter. On 2014/03/17 11:22:25, ktl wrote: > is used? Capitalize "Bloom", for consistency. Done. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:29: explicit RapporMetric(const std::string& metric_name, On 2014/03/17 11:22:25, ktl wrote: > "explicit" should only be used for single-argument constructors. (It prevents > implicit conversions and these would not be tried if there are multiple > arguments.) Done. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:31: int32_t cohort); On 2014/03/17 11:22:25, ktl wrote: > Include <stdint.h> or any project-specific file where the type int32_t is > defined. Done. #include "base/basictypes.h" https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_metric.h:53: DISALLOW_COPY_AND_ASSIGN(RapporMetric); On 2014/03/17 11:22:25, ktl wrote: > You should include an appropriate header for this macro. (In google3, that would > be base/macros.h.) Done. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.cc (left): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:45: { // ETLD_PLUS_ONE_RAPPOR_TYPE On 2014/03/17 11:22:25, ktl wrote: > The style guide says to format braced initializer lists like function calls, > which would mean the following in this case: > > ____{// Comment > _____16 /* comment */, > _____2 /* comment */ > > (that is, four and five spaces, respectively) Done. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.cc (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:103: DCHECK_EQ(cohort_, -1); On 2014/03/17 11:22:25, ktl wrote: > Maybe check for !IsInitialized()? Done. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:106: return; On 2014/03/17 11:22:25, ktl wrote: > I don't quite understand this part. Is the idea that you'll assign the cohort > value only once, and this return is taken when the value is already set? Yes, that's correct. Added a couple comments to clarify. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:137: for (std::map<std::string, RapporMetric*>::iterator it = metrics_map_.begin(); On 2014/03/17 19:14:36, Ilya Sherman wrote: > On 2014/03/17 11:22:25, ktl wrote: > > Iterating is simpler with range-for statement: > > > > for (const auto& key_val : metrics_map_) { > > const RapporMetric* metric = key_val.second; > > ... > > > > (Note that the loop variable is not an iterator, it's pair<Key,Value>.) > > Can't do this due to lack of C++11. > > > Side note: for old-style iteration you should use map::const_iterator. > > So we should do this instead :) Changed to const_iterator. The way this wraps makes me sad. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:146: STLDeleteValues(&metrics_map_); On 2014/03/17 11:22:25, ktl wrote: > The documentation for this function doesn't mention that all values are deleted. Done. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:168: On 2014/03/17 11:22:25, ktl wrote: > Remove this blank line? Done. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.h (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:26: ETLD_PLUS_ONE_RAPPOR_TYPE = 0, On 2014/03/17 11:22:25, ktl wrote: > Comment? Is the zero value significant somewhere? If not, you could leave it > out. Added comments. The 0 value is used implicitly because the enum values are indexes into kRapporParametersForType. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:38: void Start(PrefService* pref_service, net::URLRequestContextGetter* context); On 2014/03/17 11:22:25, ktl wrote: > Does URLRequestContextGetter come from you direct include files? If not, you > should forward-declare it yourself. Done. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:96: // Timer which schedules calls to OnLogInterval() On 2014/03/17 11:22:25, ktl wrote: > Add period. Done. https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.h:100: scoped_ptr<LogUploader> uploader_; On 2014/03/17 11:22:25, ktl wrote: > Do you still use scoped_ptr<> instead of more standard unique_ptr<>? In any > case, please include the appropriate header. Added include.
https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.cc (right): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:70: DCHECK(!uploader_); On 2014/03/17 19:14:36, Ilya Sherman wrote: > On 2014/03/17 11:22:25, ktl wrote: > > My preference is to compare pointers explicitly against nullptr instead of > > relying on bool conversion. > > FWIW, this is the prevailing style in Chromium code. Again, we don't have > nullptr available in Chromium code, because we can't yet support C++11 across > all of our toolchains. Ilya, thanks for putting me right on Chromium style details and the lack of C++11. Like I said, it's my first ever look at Chromium code. https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... File components/rappor/log_uploader.cc (right): https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... components/rappor/log_uploader.cc:107: int response_code = source->GetResponseCode(); This variable (and upload_succeeded and server_is_healthy) can be const. While marking locals as const can help the compiler, it's a benefit for the reader who doesn't have to scan the code to see if there's a later assignment to it. https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... File components/rappor/log_uploader.h (right): https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... components/rappor/log_uploader.h:28: // Handles uploading logs to an external server. This class could use a longer comment. When do uploads happen, for example? Where is this class used? What happens to queued comments when the instance is destructed? (That last thing could be added to the destructor documentation.) https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... components/rappor/log_uploader.h:31: // Constructor takes the server_url that logs should be uploaded to, the In other places you use |name| convention for variable names. https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... components/rappor/log_uploader.h:46: // Check if an upload has been scheduled. "Checks", for consistency. https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... File components/rappor/rappor_metric.h (right): https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_metric.h:24: // instantiating RapporMetric objects directly. Is this class then not supposed to be used by clients? If that's the case, you should state it explicitly. https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... File components/rappor/rappor_service.cc (right): https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_service.cc:109: // This is the first time the client has started the service (or thier Spelling: their https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_service.cc:117: std::string secret_base64 = This fits on one line. https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_service.cc:141: metrics_map_.begin(); This would need one more space; the four space indentation is counted from the left parenthesis. (Clang-format also does this.) https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_service.cc:142: metrics_map_.end() != it; I've not seen this style (putting the variable iterator to the right in comparison) before, is this also project-specific? Note that Chromium style guide prefers foo == 0 to 0 == foo. https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... File components/rappor/rappor_service.h (right): https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_service.h:35: // Counter and end marker. The first comment is good, but I think the meaning of this is obvious enough; consider removing.
https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... File components/rappor/log_uploader.cc (right): https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... components/rappor/log_uploader.cc:107: int response_code = source->GetResponseCode(); On 2014/03/18 08:35:01, ktl wrote: > This variable (and upload_succeeded and server_is_healthy) can be const. While > marking locals as const can help the compiler, it's a benefit for the reader who > doesn't have to scan the code to see if there's a later assignment to it. Done. https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... File components/rappor/log_uploader.h (right): https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... components/rappor/log_uploader.h:28: // Handles uploading logs to an external server. On 2014/03/18 08:35:01, ktl wrote: > This class could use a longer comment. When do uploads happen, for example? > Where is this class used? What happens to queued comments when the instance is > destructed? (That last thing could be added to the destructor documentation.) Done https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... components/rappor/log_uploader.h:31: // Constructor takes the server_url that logs should be uploaded to, the On 2014/03/18 08:35:01, ktl wrote: > In other places you use |name| convention for variable names. Done. https://codereview.chromium.org/188103004/diff/60001/components/rappor/log_up... components/rappor/log_uploader.h:46: // Check if an upload has been scheduled. On 2014/03/18 08:35:01, ktl wrote: > "Checks", for consistency. Done. https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... File components/rappor/rappor_metric.h (right): https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_metric.h:24: // instantiating RapporMetric objects directly. On 2014/03/18 08:35:01, ktl wrote: > Is this class then not supposed to be used by clients? If that's the case, you > should state it explicitly. Reworded. https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... File components/rappor/rappor_service.cc (right): https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_service.cc:109: // This is the first time the client has started the service (or thier On 2014/03/18 08:35:01, ktl wrote: > Spelling: their Done. https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_service.cc:117: std::string secret_base64 = On 2014/03/18 08:35:01, ktl wrote: > This fits on one line. Done. https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_service.cc:141: metrics_map_.begin(); On 2014/03/18 08:35:01, ktl wrote: > This would need one more space; the four space indentation is counted from the > left parenthesis. (Clang-format also does this.) Done. https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_service.cc:142: metrics_map_.end() != it; On 2014/03/18 08:35:01, ktl wrote: > I've not seen this style (putting the variable iterator to the right in > comparison) before, is this also project-specific? > > Note that Chromium style guide prefers foo == 0 to 0 == foo. base/metrics/... seems to use that style in a few places, but I think that is not the norm in chrome code. Fixed. https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... File components/rappor/rappor_service.h (right): https://codereview.chromium.org/188103004/diff/60001/components/rappor/rappor... components/rappor/rappor_service.h:35: // Counter and end marker. On 2014/03/18 08:35:01, ktl wrote: > The first comment is good, but I think the meaning of this is obvious enough; > consider removing. Done.
Just one more thing I noticed, I think we're done after this. https://codereview.chromium.org/188103004/diff/80001/components/rappor/log_up... File components/rappor/log_uploader.h (right): https://codereview.chromium.org/188103004/diff/80001/components/rappor/log_up... components/rappor/log_uploader.h:5: // Change for readability Don't forget to remove these. https://codereview.chromium.org/188103004/diff/80001/components/rappor/rappor... File components/rappor/rappor_service.h (right): https://codereview.chromium.org/188103004/diff/80001/components/rappor/rappor... components/rappor/rappor_service.h:16: #include "base/prefs/pref_service.h" Seems like you don't really need to #include this file, you could just forward-declare PrefService. The same goes for LogUploader (scoped_ptr works with forward-declared classes).
https://codereview.chromium.org/188103004/diff/80001/components/rappor/log_up... File components/rappor/log_uploader.h (right): https://codereview.chromium.org/188103004/diff/80001/components/rappor/log_up... components/rappor/log_uploader.h:5: // Change for readability On 2014/03/21 13:40:02, ktl wrote: > Don't forget to remove these. Done. https://codereview.chromium.org/188103004/diff/80001/components/rappor/rappor... File components/rappor/rappor_service.h (right): https://codereview.chromium.org/188103004/diff/80001/components/rappor/rappor... components/rappor/rappor_service.h:16: #include "base/prefs/pref_service.h" On 2014/03/21 13:40:02, ktl wrote: > Seems like you don't really need to #include this file, you could just > forward-declare PrefService. > > The same goes for LogUploader (scoped_ptr works with forward-declared classes). Done.
lgtm
LGTM. Adding Alexei as an OWNER of the code.
https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.cc (left): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:45: { // ETLD_PLUS_ONE_RAPPOR_TYPE On 2014/03/17 11:22:25, ktl wrote: > The style guide says to format braced initializer lists like function calls, > which would mean the following in this case: > > ____{// Comment > _____16 /* comment */, > _____2 /* comment */ > > (that is, four and five spaces, respectively) Hmm, I'm not sure I agree with this suggestion. For one, the suggested formatting breaks the "two spaces before start of comment" rule from the style guide. But also, if we're following the same conventions as function calls, shouldn't the start brace be on the previous line in this case? (Which would then change how the lines below would be indented.)
https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.cc (left): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:45: { // ETLD_PLUS_ONE_RAPPOR_TYPE On 2014/03/24 17:59:53, Alexei Svitkine wrote: > On 2014/03/17 11:22:25, ktl wrote: > > The style guide says to format braced initializer lists like function calls, > > which would mean the following in this case: > > > > ____{// Comment > > _____16 /* comment */, > > _____2 /* comment */ > > > > (that is, four and five spaces, respectively) > > Hmm, I'm not sure I agree with this suggestion. > > For one, the suggested formatting breaks the "two spaces before start of > comment" rule from the style guide. Please see the example in style guide section "Line Comments". (There's a discussion in c-style titled "Formatting of comments in initializer lists".) > But also, if we're following the same > conventions as function calls, shouldn't the start brace be on the previous line > in this case? (Which would then change how the lines below would be indented.) I don't see why that would follow. It would be legal to have const Type name[NUM] = f( g(param, ... // This line with 4-space indent. when the first line is long enough that the arguments to f() don't fit.
https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.cc (left): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:45: { // ETLD_PLUS_ONE_RAPPOR_TYPE On 2014/03/24 18:26:04, ktl wrote: > On 2014/03/24 17:59:53, Alexei Svitkine wrote: > > On 2014/03/17 11:22:25, ktl wrote: > > > The style guide says to format braced initializer lists like function calls, > > > which would mean the following in this case: > > > > > > ____{// Comment > > > _____16 /* comment */, > > > _____2 /* comment */ > > > > > > (that is, four and five spaces, respectively) > > > > Hmm, I'm not sure I agree with this suggestion. > > > > For one, the suggested formatting breaks the "two spaces before start of > > comment" rule from the style guide. > > Please see the example in style guide section "Line Comments". (There's a > discussion in c-style titled "Formatting of comments in initializer lists".) > > > But also, if we're following the same > > conventions as function calls, shouldn't the start brace be on the previous > line > > in this case? (Which would then change how the lines below would be indented.) > > I don't see why that would follow. It would be legal to have > const Type name[NUM] = f( > g(param, ... // This line with 4-space indent. > when the first line is long enough that the arguments to f() don't fit. Thanks for the explanation. I understand the suggestion now. However, in this case "// ETLD_PLUS_ONE_RAPPOR_TYPE" is not a 1-line comment describing the first arg, but rather describing the initializer block (i.e. it would be equivalent to a comment explaining the g() call itself in your example). So given the above, wouldn't it make more sense for the comment to be on the previous line instead, before the {?
https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... File components/rappor/rappor_service.cc (left): https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... components/rappor/rappor_service.cc:45: { // ETLD_PLUS_ONE_RAPPOR_TYPE On 2014/03/24 18:55:21, Alexei Svitkine wrote: > On 2014/03/24 18:26:04, ktl wrote: > > On 2014/03/24 17:59:53, Alexei Svitkine wrote: > > > On 2014/03/17 11:22:25, ktl wrote: > > > > The style guide says to format braced initializer lists like function > calls, > > > > which would mean the following in this case: > > > > > > > > ____{// Comment > > > > _____16 /* comment */, > > > > _____2 /* comment */ > > > > > > > > (that is, four and five spaces, respectively) > > > > > > Hmm, I'm not sure I agree with this suggestion. > > > > > > For one, the suggested formatting breaks the "two spaces before start of > > > comment" rule from the style guide. > > > > Please see the example in style guide section "Line Comments". (There's a > > discussion in c-style titled "Formatting of comments in initializer lists".) > > > > > But also, if we're following the same > > > conventions as function calls, shouldn't the start brace be on the previous > > line > > > in this case? (Which would then change how the lines below would be > indented.) > > > > I don't see why that would follow. It would be legal to have > > const Type name[NUM] = f( > > g(param, ... // This line with 4-space indent. > > when the first line is long enough that the arguments to f() don't fit. > > Thanks for the explanation. I understand the suggestion now. > > However, in this case "// ETLD_PLUS_ONE_RAPPOR_TYPE" is not a 1-line comment > describing the first arg, but rather describing the initializer block (i.e. it > would be equivalent to a comment explaining the g() call itself in your > example). So given the above, wouldn't it make more sense for the comment to be > on the previous line instead, before the {? I agree. Updated to match. Also I think the closing brace for the initializer goes on the last line. (And I'm assuming that this shouldn't apply to array initializer, because it's intended to be matching a constructor style call.) Also updated initializers in unittest files.
On 2014/03/24 19:13:36, Steven Holte wrote: > https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... > File components/rappor/rappor_service.cc (left): > > https://codereview.chromium.org/188103004/diff/40001/components/rappor/rappor... > components/rappor/rappor_service.cc:45: { // ETLD_PLUS_ONE_RAPPOR_TYPE > On 2014/03/24 18:55:21, Alexei Svitkine wrote: > > On 2014/03/24 18:26:04, ktl wrote: > > > On 2014/03/24 17:59:53, Alexei Svitkine wrote: > > > > On 2014/03/17 11:22:25, ktl wrote: > > > > > The style guide says to format braced initializer lists like function > > calls, > > > > > which would mean the following in this case: > > > > > > > > > > ____{// Comment > > > > > _____16 /* comment */, > > > > > _____2 /* comment */ > > > > > > > > > > (that is, four and five spaces, respectively) > > > > > > > > Hmm, I'm not sure I agree with this suggestion. > > > > > > > > For one, the suggested formatting breaks the "two spaces before start of > > > > comment" rule from the style guide. > > > > > > Please see the example in style guide section "Line Comments". (There's a > > > discussion in c-style titled "Formatting of comments in initializer lists".) > > > > > > > But also, if we're following the same > > > > conventions as function calls, shouldn't the start brace be on the > previous > > > line > > > > in this case? (Which would then change how the lines below would be > > indented.) > > > > > > I don't see why that would follow. It would be legal to have > > > const Type name[NUM] = f( > > > g(param, ... // This line with 4-space indent. > > > when the first line is long enough that the arguments to f() don't fit. > > > > Thanks for the explanation. I understand the suggestion now. > > > > However, in this case "// ETLD_PLUS_ONE_RAPPOR_TYPE" is not a 1-line comment > > describing the first arg, but rather describing the initializer block (i.e. it > > would be equivalent to a comment explaining the g() call itself in your > > example). So given the above, wouldn't it make more sense for the comment to > be > > on the previous line instead, before the {? > > I agree. Updated to match. Also I think the closing brace for the initializer > goes on the last line. (And I'm assuming that this shouldn't apply to array > initializer, because it's intended to be matching a constructor style call.) > Also updated initializers in unittest files. I agree as well, the latest iteration looks good.
LGTM
The CQ bit was checked by holte@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/holte@chromium.org/188103004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on mac_chromium_rel
The CQ bit was checked by holte@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/holte@chromium.org/188103004/390001
Message was sent while issue was closed.
Change committed as 259469 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
