Submitting ByteStream chrome class for Google C++ Readability Review.

Submitting ByteStream chrome class for Google C++ Readability Review.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143448

[Randy Smith (Not in Mondays), 2012-06-12 16:14:49]

Submitting for readability review.

[Randy Smith (Not in Mondays), 2012-06-18 19:10:02]

I should have said:

The original CL that created this class is available at  
http://codereview.chromium.org/10244001/.  
* There were some changes to these files in
https://chromiumcodereview.appspot.com/10392111, pretty much only name changes
(ByteStreamInput/Output -> ByteStreamWriter/Reader, and removing references to
pipes in favor of streams, for regularization of naming>0

[erikmc, 2012-06-18 19:25:44]

This is a very clean CL, this should be a quick review.  See my initial comments
below.

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
File content/browser/download/byte_stream.cc (right):

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:36: LifetimeFlag() : is_alive_(true) { }
Structs have different naming rules for data members.  Please remove the
trailing underscore in is_alive_.

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Variable_Names#Variab...

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:52:
ByteStreamWriterImpl(scoped_refptr<base::SequencedTaskRunner> task_runner,
align inputs here

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:441: 
Please remove empty line 441.

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
File content/browser/download/byte_stream.h (right):

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:59: // Class methods are virtual to allow
mocking for tests; these classes
Please include sample usage in the comments.

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Class_Comments#Class_...

[Randy Smith (Not in Mondays), 2012-06-18 20:38:23]

Glad it started out clean, though I suspect I've given some more grist for the
mill in the example usage code :-}.  PTAL.

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
File content/browser/download/byte_stream.cc (right):

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:36: LifetimeFlag() : is_alive_(true) { }
On 2012/06/18 19:25:44, erikmc wrote:
> Structs have different naming rules for data members.  Please remove the
> trailing underscore in is_alive_.
> 
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Variable_Names#Variab...

Done.

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:52:
ByteStreamWriterImpl(scoped_refptr<base::SequencedTaskRunner> task_runner,
On 2012/06/18 19:25:44, erikmc wrote:
> align inputs here

Huh.  Wonder how I managed that.  Done.

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.cc:441: 
On 2012/06/18 19:25:44, erikmc wrote:
> Please remove empty line 441.

Yep--this was to get Rietveld to "take" the patch.  Done, and done in
byte_stream.h as well; I was "lucky" enough to have this error to correct in
byte_stream_unittest.cc when I uploaded :-}.

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
File content/browser/download/byte_stream.h (right):

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:59: // Class methods are virtual to allow
mocking for tests; these classes
On 2012/06/18 19:25:44, erikmc wrote:
> Please include sample usage in the comments.
> 
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Class_Comments#Class_...

Tug on a string :-}.  This may have added 50% to the number of lines in the
header file.  Let me know what you think.

(Not arguing with the request at all; just amused at the amount of verbiage it
produces.)

[erikmc, 2012-06-21 17:39:44]

lgtm

Nice work on a extra-clean CL.

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
File content/browser/download/byte_stream.h (right):

http://codereview.chromium.org/10535116/diff/1/content/browser/download/byte_...
content/browser/download/byte_stream.h:59: // Class methods are virtual to allow
mocking for tests; these classes
Adding useful comment lines at the top of the file are never something to worry
about.  This looks good, and honestly given the complexity required to use this
class, it is great to have a concrete example.

On 2012/06/18 20:38:24, rdsmith wrote:
> On 2012/06/18 19:25:44, erikmc wrote:
> > Please include sample usage in the comments.
> > 
> >
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=Class_Comments#Class_...
> 
> Tug on a string :-}.  This may have added 50% to the number of lines in the
> header file.  Let me know what you think.
> 
> (Not arguing with the request at all; just amused at the amount of verbiage it
> produces.)

[Randy Smith (Not in Mondays), 2012-06-21 17:56:56]

Thanks, Erik!

I think I should probably get a chromium committer LGTM as well before landing. 
Ben, willing to rubberstamp?

[benjhayden, 2012-06-21 18:06:59]

lgtm

[commit-bot: I haz the power, 2012-06-21 18:13:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/10535116/11001

[commit-bot: I haz the power, 2012-06-21 20:46:46]

Change committed as 143448