Refactored DataBuffer to use unix_hacker style methods.

Refactored DataBuffer to use unix_hacker style methods.

DataBuffer went from having pure virtual methods to having concrete
implementations.  However, the style of the method name remained
in UpperCamelCase.  This patch renames the methods to fit with
the unix_hacker_style that is used for concrete implementations.

BUG=251986
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208453

[Ty Overby, 2013-06-20 21:09:15]

In response to Issue 251986, this patch refactors DataBuffer to fit with the
google style guidelines.

[scherkus (not reviewing), 2013-06-20 21:29:13]

similar story w/ this CL

https://codereview.chromium.org/17315021/diff/2001/media/base/data_buffer.h
File media/base/data_buffer.h (right):

https://codereview.chromium.org/17315021/diff/2001/media/base/data_buffer.h#n...
media/base/data_buffer.h:33: static scoped_refptr<DataBuffer> copy_from(const
uint8* data, int size);
ditto w/ changing too much -- this should be reverted

https://codereview.chromium.org/17315021/diff/2001/media/base/data_buffer.h#n...
media/base/data_buffer.h:39: static scoped_refptr<DataBuffer>
create_eos_buffer();
ditto

https://codereview.chromium.org/17315021/diff/2001/media/base/data_buffer.h#n...
media/base/data_buffer.h:41: base::TimeDelta get_timestamp() const;
drop get_ prefixes from these setters

https://codereview.chromium.org/17315021/diff/2001/media/base/data_buffer.h#n...
media/base/data_buffer.h:57: bool is_end_of_stream() const;
drop is_ prefix

https://codereview.chromium.org/17315021/diff/2001/media/base/data_buffer_uni...
File media/base/data_buffer_unittest.cc (right):

https://codereview.chromium.org/17315021/diff/2001/media/base/data_buffer_uni...
media/base/data_buffer_unittest.cc:44: TEST(DataBufferTest, copy_from) {
gtest names are always CamelCase

https://codereview.chromium.org/17315021/diff/2001/media/filters/decrypting_a...
File media/filters/decrypting_audio_decoder_unittest.cc (right):

https://codereview.chromium.org/17315021/diff/2001/media/filters/decrypting_a...
media/filters/decrypting_audio_decoder_unittest.cc:65: MATCHER(is_end_of_stream,
"end of stream") {
revert this -- matchers aren't setters/getters and should be CamelCase

[Ty Overby, 2013-06-21 02:14:38]

I fixed all my previous mistakes with the refactor.

[scherkus (not reviewing), 2013-06-21 19:48:38]

looks like the style formatter went a bit overboard :(

in the future: try not to only apply style formatter to areas of code you're
changing, otherwise it can produce really large patches that make it hard to
review (I can't easily tell which changes are the "core" part of the change vs.
style cleanup)

https://codereview.chromium.org/17315021/diff/16001/media/base/seekable_buffe...
File media/base/seekable_buffer.cc (right):

https://codereview.chromium.org/17315021/diff/16001/media/base/seekable_buffe...
media/base/seekable_buffer.cc:54: if (current_buffer == buffers_.end()) return
false;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/base/seekable_buffe...
media/base/seekable_buffer.cc:82: if (forward_bytes_ >= forward_capacity_)
return false;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/base/seekable_buffe...
media/base/seekable_buffer.cc:106: if (size > forward_bytes_) return false;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/base/seekable_buffe...
media/base/seekable_buffer.cc:115: if (size > backward_bytes_) return false;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/base/seekable_buffe...
media/base/seekable_buffer.cc:147: if (current_buffer_ == buffers_.begin())
break;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/base/seekable_buffe...
media/base/seekable_buffer.cc:166: if (i == current_buffer_) break;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/base/seekable_buffe...
media/base/seekable_buffer.cc:202: memcpy(data + taken, buffer->data() +
current_buffer_offset,
can you fit "copied);" onto this line?

https://codereview.chromium.org/17315021/diff/16001/media/base/seekable_buffe...
media/base/seekable_buffer.cc:229: if (next == buffers_.end()) break;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:343: if (!need_another_buffer &&
!CanRead_Locked()) return;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:355: if (state_ == kUnderflow || state_ ==
kRebuffering) state_ = kPlaying;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:363: if (!buffer->end_of_stream())
algorithm_->EnqueueBuffer(buffer);
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:368: if (IsBeforePrerollTime(buffer))
return true;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:380: if (!buffer->end_of_stream())
algorithm_->EnqueueBuffer(buffer);
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:397: if (!CanRead_Locked()) return;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:481: if (!algorithm_) return 0;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:484: if (playback_rate == 0) return 0;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:486: if (state_ == kRebuffering &&
algorithm_->IsQueueFull()) state_ = kPlaying;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:489: if (state_ != kPlaying) return 0;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:574: if (!underflow_cb.is_null())
underflow_cb.Run();
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:582: if (frames_filled <= 0) return;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:609: if (status != PIPELINE_OK)
error_cb_.Run(status);
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:622: if (status != PIPELINE_OK)
error_cb_.Run(status);
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:130: if (!enter.succeeded())
return false;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:145: if (bytes_left <
kHeaderSize) return false;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:156: if (frame_size <= 0 ||
bytes_left < frame_size) return false;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:408: if
(!decrypt_cb.is_null()) decrypt_cb.Run(media::Decryptor::kSuccess, NULL);
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:594: if
(key_added_cb_.is_null()) return;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:609: if
(key_message_cb_.is_null()) return;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:636: if
(key_error_cb_.is_null()) return;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:757: if (!enter.succeeded())
return NULL;
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:935: while
(media_resource_size < data_size) media_resource_size *= 2;
revert this change -- we don't do single-line whiles

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:958: if (buffer_id)
free_buffers_.push(buffer_id);
revert this change -- we don't do single-line ifs

https://codereview.chromium.org/17315021/diff/16001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:965: if
(free_buffers_.empty()) return;
revert this change -- we don't do single-line ifs

[Ty Overby, 2013-06-21 20:58:17]

Style is now done according to the chromium style guide.

[scherkus (not reviewing), 2013-06-21 23:39:15]

forgot two spots

https://codereview.chromium.org/17315021/diff/22001/webkit/plugins/ppapi/cont...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/17315021/diff/22001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:73: if (array_size <
str.size()) return false;
hey!

https://codereview.chromium.org/17315021/diff/22001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:947: while
(media_resource_size < data_size) media_resource_size *= 2;
hey!

[Ty Overby (Google), 2013-06-22 00:19:30]

Weird, I have both of those changes on the branch that I swear I pushed via
changelist.  Should I 'git cl upload' again on that branch?  I suspect the
same might be true the other patch too.  I'll check.


On Fri, Jun 21, 2013 at 4:39 PM, <scherkus@chromium.org> wrote:

> forgot two spots
>
>
> https://codereview.chromium.**org/17315021/diff/22001/**
>
webkit/plugins/ppapi/content_**decryptor_delegate.cc<https://codereview.chromium.org/17315021/diff/22001/webkit/plugins/ppapi/content_decryptor_delegate.cc>
> File webkit/plugins/ppapi/content_**decryptor_delegate.cc (right):
>
> https://codereview.chromium.**org/17315021/diff/22001/**
>
webkit/plugins/ppapi/content_**decryptor_delegate.cc#**newcode73<https://codereview.chromium.org/17315021/diff/22001/webkit/plugins/ppapi/content_decryptor_delegate.cc#newcode73>
> webkit/plugins/ppapi/content_**decryptor_delegate.cc:73: if (array_size <
> str.size()) return false;
> hey!
>
> https://codereview.chromium.**org/17315021/diff/22001/**
>
webkit/plugins/ppapi/content_**decryptor_delegate.cc#**newcode947<https://codereview.chromium.org/17315021/diff/22001/webkit/plugins/ppapi/content_decryptor_delegate.cc#newcode947>
> webkit/plugins/ppapi/content_**decryptor_delegate.cc:947: while
>
> (media_resource_size < data_size) media_resource_size *= 2;
> hey!
>
>
https://codereview.chromium.**org/17315021/<https://codereview.chromium.org/1...
>

[scherkus (not reviewing), 2013-06-22 00:23:27]

when in doubt, make sure your branch has everything committed and re-run git cl
upload

[scherkus (not reviewing), 2013-06-22 00:36:59]

no problem! it's why we do code reviews ;)

lgtm

[commit-bot: I haz the power, 2013-06-22 00:40:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoverby@chromium.org/17315021/28001

[commit-bot: I haz the power, 2013-06-22 00:59:25]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Ty Overby, 2013-06-22 01:07:36]

On 2013/06/22 00:59:25, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

piman: OWNERS review for webkit/plugins/ppapi/content_decryptop_delegate.cc

[commit-bot: I haz the power, 2013-06-24 15:59:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoverby@chromium.org/17315021/28001

[commit-bot: I haz the power, 2013-06-24 16:07:11]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[brettw, 2013-06-24 20:25:41]

I made a few general comments. In general, I think too many fomatting things
were changed unnecessarily or for the worse.

https://codereview.chromium.org/17315021/diff/28001/webkit/plugins/ppapi/cont...
File webkit/plugins/ppapi/content_decryptor_delegate.cc (right):

https://codereview.chromium.org/17315021/diff/28001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:282:
weak_this_(weak_ptr_factory_.GetWeakPtr()) {}
This was better before.

https://codereview.chromium.org/17315021/diff/28001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:305:
->MakeArrayBufferPPVar(init_data_length, init_data);
You did a lot of random reformatting in this file. Some of it is fine, but to
me, this type of thing (-> at the beginning of a line) is strictly worse than
the old code.

https://codereview.chromium.org/17315021/diff/28001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:346: if
(!MakeMediaBufferResource(stream_type, encrypted_buffer,
I preferred the previous formatting,

https://codereview.chromium.org/17315021/diff/28001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:544:
plugin_decryption_interface_->DecryptAndDecode(
IMO this type of thing is equal either way and I don't think its worth changing.

https://codereview.chromium.org/17315021/diff/28001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:661: PP_DecryptorStreamType
decoder_type, uint32_t request_id, PP_Bool success) {
The previous formatting was correct. Arg decl and impl lists should go on
separate lines.

https://codereview.chromium.org/17315021/diff/28001/webkit/plugins/ppapi/cont...
webkit/plugins/ppapi/content_decryptor_delegate.cc:680:
base::ResetAndReturn(&pending_video_decoder_init_cb_)
Old formatting was better (avoid . at beginning of line)

[brettw, 2013-06-24 20:27:59]

I didn't check the other files, but I think my comments apply to the others as
well.

https://codereview.chromium.org/17315021/diff/28001/media/audio/linux/alsa_ou...
File media/audio/linux/alsa_output_unittest.cc (left):

https://codereview.chromium.org/17315021/diff/28001/media/audio/linux/alsa_ou...
media/audio/linux/alsa_output_unittest.cc:688: const char* kExpectedDeviceName[]
= { NULL,
I fould the old code here much easier to read.

[Ty Overby, 2013-06-24 20:57:29]

On 2013/06/24 20:27:59, brettw wrote:
> I didn't check the other files, but I think my comments apply to the others as
> well.
> 
>
https://codereview.chromium.org/17315021/diff/28001/media/audio/linux/alsa_ou...
> File media/audio/linux/alsa_output_unittest.cc (left):
> 
>
https://codereview.chromium.org/17315021/diff/28001/media/audio/linux/alsa_ou...
> media/audio/linux/alsa_output_unittest.cc:688: const char*
kExpectedDeviceName[]
> = { NULL,
> I fould the old code here much easier to read.

Thanks brett, I undid the style changes in the files that you mentioned.

[brettw, 2013-06-24 21:18:28]

Thanks, here is some other stuff I noticed now that I did a full pass.

https://codereview.chromium.org/17315021/diff/45001/media/base/data_buffer.h
File media/base/data_buffer.h (right):

https://codereview.chromium.org/17315021/diff/45001/media/base/data_buffer.h#...
media/base/data_buffer.h:41: base::TimeDelta timestamp() const;
For all of these, we say that anything named lika a variable has to be as cheap
as a variable, which means that it has to be inline.

https://codereview.chromium.org/17315021/diff/45001/media/base/seekable_buffe...
File media/base/seekable_buffer.cc (right):

https://codereview.chromium.org/17315021/diff/45001/media/base/seekable_buffe...
media/base/seekable_buffer.cc:24: SeekableBuffer::~SeekableBuffer() {}
I like the old way better.

https://codereview.chromium.org/17315021/diff/45001/media/filters/audio_rende...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/17315021/diff/45001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:62: actual_frames_per_buffer_(0) {}
I liked the old way better.

https://codereview.chromium.org/17315021/diff/45001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:195: void AudioRendererImpl::Initialize(
Old way was better

https://codereview.chromium.org/17315021/diff/45001/media/filters/audio_rende...
media/filters/audio_renderer_impl.cc:306: AudioDecoder::Status status, const
scoped_refptr<DataBuffer>& buffer) {
Old way was better.

[Ty Overby, 2013-06-24 22:29:06]

On 2013/06/24 21:18:28, brettw wrote:
> Thanks, here is some other stuff I noticed now that I did a full pass.
> 
> https://codereview.chromium.org/17315021/diff/45001/media/base/data_buffer.h
> File media/base/data_buffer.h (right):
> 
>
https://codereview.chromium.org/17315021/diff/45001/media/base/data_buffer.h#...
> media/base/data_buffer.h:41: base::TimeDelta timestamp() const;
> For all of these, we say that anything named lika a variable has to be as
cheap
> as a variable, which means that it has to be inline.
> 
>
https://codereview.chromium.org/17315021/diff/45001/media/base/seekable_buffe...
> File media/base/seekable_buffer.cc (right):
> 
>
https://codereview.chromium.org/17315021/diff/45001/media/base/seekable_buffe...
> media/base/seekable_buffer.cc:24: SeekableBuffer::~SeekableBuffer() {}
> I like the old way better.
> 
>
https://codereview.chromium.org/17315021/diff/45001/media/filters/audio_rende...
> File media/filters/audio_renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/17315021/diff/45001/media/filters/audio_rende...
> media/filters/audio_renderer_impl.cc:62: actual_frames_per_buffer_(0) {}
> I liked the old way better.
> 
>
https://codereview.chromium.org/17315021/diff/45001/media/filters/audio_rende...
> media/filters/audio_renderer_impl.cc:195: void AudioRendererImpl::Initialize(
> Old way was better
> 
>
https://codereview.chromium.org/17315021/diff/45001/media/filters/audio_rende...
> media/filters/audio_renderer_impl.cc:306: AudioDecoder::Status status, const
> scoped_refptr<DataBuffer>& buffer) {
> Old way was better.

Thanks Brett, I went through and tried to completely undo all style changes. 
The inlining will come in the next patch.

[Ty Overby, 2013-06-24 23:05:08]

Getters and Setters are now inline. 

Please commit if it looks good.

[brettw, 2013-06-25 06:06:08]

lgtm

[commit-bot: I haz the power, 2013-06-25 06:06:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tyoverby@chromium.org/17315021/54001

[commit-bot: I haz the power, 2013-06-25 08:41:04]

Change committed as 208453