C++ Readability review for holte

components/rappor/log_uploader.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Change for readability
+
 #ifndef COMPONENTS_RAPPOR_LOG_UPLOADER_H_
 #define COMPONENTS_RAPPOR_LOG_UPLOADER_H_
 
 #include <queue>
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "url/gurl.h"
 
 namespace net {
 class URLFetcher;
 }

[ktl, 2014/03/17 11:22:25]

Add end-of-namespace comment.

[Ilya Sherman, 2014/03/17 19:14:36]

These are generally omitted in Chromium code for forward-declarations, since the
namespace block is so tiny.

 
 namespace rappor {
 
 // Handles uploading logs to an external server.
 class LogUploader : public net::URLFetcherDelegate {
  public:
   // Constructor takes the server_url that logs should be uploaded to, the
   // mime_type of the uploaded data, and request_context to create uploads
   // with.
   LogUploader(const GURL& server_url,
@@ skipping 18 matching lines @@
   // Starts transmission of the next log. Exposed for tests.
   void StartScheduledUpload();
 
   // Increases the upload interval each time it's called, to handle the case
   // where the server is having issues. Exposed for tests.
   static base::TimeDelta BackOffUploadInterval(base::TimeDelta);
 
  private:
   // Implementation of net::URLFetcherDelegate. Called after transmission
   // completes (either successfully or with failure).
   virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;

[ktl, 2014/03/17 11:22:25]

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;

[Ilya Sherman, 2014/03/17 19:14:36]

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...

 
   // Called when the upload is completed.
   void OnUploadFinished(bool server_is_healthy, bool more_logs_remaining);
 
   // The server URL to upload logs to.
   const GURL server_url_;
 
   // The mime type to specify on uploaded logs.
   const std::string mime_type_;
 
@@ skipping 15 matching lines @@
   // The interval to wait after an upload's URLFetcher completion before
   // starting the next upload attempt.
   base::TimeDelta upload_interval_;
 
   DISALLOW_COPY_AND_ASSIGN(LogUploader);
 };
 
 }  // namespace rappor
 
 #endif  // COMPONENTS_RAPPOR_LOG_UPLOADER_H_