|
|
Created:
12 years ago by scherkus (not reviewing) Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionChecking in media buffer pure interfaces.
R=cpu,darin
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=6167
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 18
Patch Set 4 : Updated media/base/buffers.h based on Darin's comments. #
Total comments: 1
Messages
Total messages: 8 (0 generated)
Hi can you add darin to the reviewers? thanks
Some basic style nits. BTW, where is this set of files located with respect to the chromium tree? http://codereview.chromium.org/12701/diff/1/3 File media/base/buffers.h (right): http://codereview.chromium.org/12701/diff/1/3#newcode7 Line 7: I was sort of expecting #include "base/basictypes.h" here. See below for rationale. http://codereview.chromium.org/12701/diff/1/3#newcode77 Line 77: I think we favor the google3 base types like int32. I favor size_t for unsigned values. http://codereview.chromium.org/12701/diff/1/3#newcode120 Line 120: // pointer of the object to receive the NotifyComplete callback. It seems that OwnerType* is really AssignableInterface<T>* or did I misread the contract? http://codereview.chromium.org/12701/diff/1/3#newcode136 Line 136: virtual void OnAssignment() { maybe the DCHECK is better here than in the ctor? http://codereview.chromium.org/12701/diff/1/3#newcode142 Line 142: scoped_refptr<BufferType> buffer_; I don't see the include to use scoped_reptr. http://codereview.chromium.org/12701/diff/1/4 File media/base/media.cc (right): http://codereview.chromium.org/12701/diff/1/4#newcode6 Line 6: #include "media/base/buffers.h" Ah I see what you are doing now. We like to not have include dependencies. I mean having to include common.h before buffers.h I think that is why we arrange the includes alphabetically. http://codereview.chromium.org/12701/diff/1/4#newcode9 Line 9: periods at the end of all comments please.
Fixed up header includes, data types and comments. http://codereview.chromium.org/12701/diff/1/3 File media/base/buffers.h (right): http://codereview.chromium.org/12701/diff/1/3#newcode7 Line 7: On 2008/11/27 02:13:14, cpu wrote: > I was sort of expecting #include "base/basictypes.h" here. See below for > rationale. I removed common.h and now include basictypes and ref_counted http://codereview.chromium.org/12701/diff/1/3#newcode77 Line 77: On 2008/11/27 02:13:14, cpu wrote: > I think we favor the google3 base types like int32. I favor size_t for unsigned > values. Makes sense. Since they're all unsigned I've changed them to size_t. http://codereview.chromium.org/12701/diff/1/3#newcode120 Line 120: // pointer of the object to receive the NotifyComplete callback. On 2008/11/27 02:13:14, cpu wrote: > It seems that OwnerType* is really AssignableInterface<T>* or did I misread the > contract? OwnerType is usually something that owns/creates multiple buffers, so by implementing AssignableInterface<T> it would break the pattern by making OwnerType appear as a single buffer. I might have to rethink a bit of this. http://codereview.chromium.org/12701/diff/1/3#newcode136 Line 136: virtual void OnAssignment() { On 2008/11/27 02:13:14, cpu wrote: > maybe the DCHECK is better here than in the ctor? I thought it was better to fail early, or am I misunderstanding the purpose of DCHECK? http://codereview.chromium.org/12701/diff/1/3#newcode142 Line 142: scoped_refptr<BufferType> buffer_; On 2008/11/27 02:13:14, cpu wrote: > I don't see the include to use scoped_reptr. > > Fixed. http://codereview.chromium.org/12701/diff/1/4 File media/base/media.cc (right): http://codereview.chromium.org/12701/diff/1/4#newcode6 Line 6: #include "media/base/buffers.h" On 2008/11/27 02:13:14, cpu wrote: > Ah I see what you are doing now. We like to not have include dependencies. I > mean having to include common.h before buffers.h > > I think that is why we arrange the includes alphabetically. > > Fixed! http://codereview.chromium.org/12701/diff/1/4#newcode9 Line 9: On 2008/11/27 02:13:14, cpu wrote: > periods at the end of all comments please. Fixed!
LGTM On 2008/12/01 20:08:58, scherkus wrote: > Fixed up header includes, data types and comments. > > http://codereview.chromium.org/12701/diff/1/3 > File media/base/buffers.h (right): > > http://codereview.chromium.org/12701/diff/1/3#newcode7 > Line 7: > On 2008/11/27 02:13:14, cpu wrote: > > I was sort of expecting #include "base/basictypes.h" here. See below for > > rationale. > > I removed common.h and now include basictypes and ref_counted > > http://codereview.chromium.org/12701/diff/1/3#newcode77 > Line 77: > On 2008/11/27 02:13:14, cpu wrote: > > I think we favor the google3 base types like int32. I favor size_t for > unsigned > > values. > > Makes sense. Since they're all unsigned I've changed them to size_t. > > http://codereview.chromium.org/12701/diff/1/3#newcode120 > Line 120: // pointer of the object to receive the NotifyComplete callback. > On 2008/11/27 02:13:14, cpu wrote: > > It seems that OwnerType* is really AssignableInterface<T>* or did I misread > the > > contract? > > OwnerType is usually something that owns/creates multiple buffers, so by > implementing AssignableInterface<T> it would break the pattern by making > OwnerType appear as a single buffer. > > I might have to rethink a bit of this. > > http://codereview.chromium.org/12701/diff/1/3#newcode136 > Line 136: virtual void OnAssignment() { > On 2008/11/27 02:13:14, cpu wrote: > > maybe the DCHECK is better here than in the ctor? > > I thought it was better to fail early, or am I misunderstanding the purpose of > DCHECK? > > http://codereview.chromium.org/12701/diff/1/3#newcode142 > Line 142: scoped_refptr<BufferType> buffer_; > On 2008/11/27 02:13:14, cpu wrote: > > I don't see the include to use scoped_reptr. > > > > > > Fixed. > > http://codereview.chromium.org/12701/diff/1/4 > File media/base/media.cc (right): > > http://codereview.chromium.org/12701/diff/1/4#newcode6 > Line 6: #include "media/base/buffers.h" > On 2008/11/27 02:13:14, cpu wrote: > > Ah I see what you are doing now. We like to not have include dependencies. I > > mean having to include common.h before buffers.h > > > > I think that is why we arrange the includes alphabetically. > > > > > > Fixed! > > http://codereview.chromium.org/12701/diff/1/4#newcode9 > Line 9: > On 2008/11/27 02:13:14, cpu wrote: > > periods at the end of all comments please. > > Fixed!
http://codereview.chromium.org/12701/diff/403/405 File media/base/buffers.h (right): http://codereview.chromium.org/12701/diff/403/405#newcode4 Line 4: nit: please add a block comment here describing the contents of this file. we have not been very good about doing this throughout the chrome codebase, but we are trying to make an effort to change. see base/file_path.h and net/base/file_stream.h for examples. http://codereview.chromium.org/12701/diff/403/405#newcode9 Line 9: #include "base/ref_counted.h" nit: ref_counted.h already includes basictypes.h, so there is no need to repeat the include of basictypes.h. it is common practice in the chrome code base to reduce unnecessary headers like this. http://codereview.chromium.org/12701/diff/403/405#newcode17 Line 17: // current type of the pointer (StreamSampleInterface vs. SomeImplementation). nit: this comment about subclassing RefCountedThreadSafe makes no sense to me. the AddRef and Release methods are not virtual, so you would not really be able to subclass them. plus, i think it is fine to expect readers to be familiar with RefCountedThreadSafe or to go read about it first. its semantics do not really need to be defined here. http://codereview.chromium.org/12701/diff/403/405#newcode21 Line 21: virtual ~StreamSampleInterface() {} you should consider moving this destructor into a protected section. then make base::RefCountedThreadSafe<...> be a friend. http://codereview.chromium.org/12701/diff/403/405#newcode32 Line 32: virtual ~BufferInterface() {} nit: this destructor does not need to be declared. http://codereview.chromium.org/12701/diff/403/405#newcode44 Line 44: virtual ~WritableBufferInterface() {} nit: this destructor does not need to be declared. http://codereview.chromium.org/12701/diff/403/405#newcode96 Line 96: virtual ~VideoFrameInterface() {} nit: this destructor does not need to be declared. http://codereview.chromium.org/12701/diff/403/405#newcode102 Line 102: // Unlocks the underlying surface, any previous VideoSurfaces are no longer why is VideoSurface pluralized? can you call Lock many times before calling Unlock? http://codereview.chromium.org/12701/diff/403/405#newcode124 Line 124: class AssignableBuffer : public AssignableInterface<BufferType>, hmm... it is not clear why AssignableInterface<T> exists. it is a bit odd to have a templatized interface. are you planning on passing around AssignableInterface<T> pointers?
Updated based on Darin's comments. http://codereview.chromium.org/12701/diff/403/405 File media/base/buffers.h (right): http://codereview.chromium.org/12701/diff/403/405#newcode4 Line 4: On 2008/12/08 18:05:18, darin wrote: > nit: please add a block comment here describing the contents of this file. we > have not been very good about doing this throughout the chrome codebase, but we > are trying to make an effort to change. see base/file_path.h and > net/base/file_stream.h for examples. Done. http://codereview.chromium.org/12701/diff/403/405#newcode9 Line 9: #include "base/ref_counted.h" On 2008/12/08 18:05:18, darin wrote: > nit: ref_counted.h already includes basictypes.h, so there is no need to repeat > the include of basictypes.h. it is common practice in the chrome code base to > reduce unnecessary headers like this. Done. http://codereview.chromium.org/12701/diff/403/405#newcode17 Line 17: // current type of the pointer (StreamSampleInterface vs. SomeImplementation). On 2008/12/08 18:05:18, darin wrote: > nit: this comment about subclassing RefCountedThreadSafe makes no sense to me. > the AddRef and Release methods are not virtual, so you would not really be able > to subclass them. plus, i think it is fine to expect readers to be familiar > with RefCountedThreadSafe or to go read about it first. its semantics do not > really need to be defined here. Done. http://codereview.chromium.org/12701/diff/403/405#newcode21 Line 21: virtual ~StreamSampleInterface() {} On 2008/12/08 18:05:18, darin wrote: > you should consider moving this destructor into a protected section. then make > base::RefCountedThreadSafe<...> be a friend. Done. http://codereview.chromium.org/12701/diff/403/405#newcode32 Line 32: virtual ~BufferInterface() {} On 2008/12/08 18:05:18, darin wrote: > nit: this destructor does not need to be declared. Done. http://codereview.chromium.org/12701/diff/403/405#newcode44 Line 44: virtual ~WritableBufferInterface() {} On 2008/12/08 18:05:18, darin wrote: > nit: this destructor does not need to be declared. Done. http://codereview.chromium.org/12701/diff/403/405#newcode96 Line 96: virtual ~VideoFrameInterface() {} On 2008/12/08 18:05:18, darin wrote: > nit: this destructor does not need to be declared. Done. http://codereview.chromium.org/12701/diff/403/405#newcode102 Line 102: // Unlocks the underlying surface, any previous VideoSurfaces are no longer On 2008/12/08 18:05:18, darin wrote: > why is VideoSurface pluralized? can you call Lock many times before calling > Unlock? No you can't call Lock many times, comment clarified http://codereview.chromium.org/12701/diff/403/405#newcode124 Line 124: class AssignableBuffer : public AssignableInterface<BufferType>, On 2008/12/08 18:05:18, darin wrote: > hmm... it is not clear why AssignableInterface<T> exists. it is a bit odd to > have a templatized interface. are you planning on passing around > AssignableInterface<T> pointers? We're passing around AssignableInterface<T> pointers (see media/base/filters.h).
LGTM, thanks! http://codereview.chromium.org/12701/diff/209/210 File media/base/buffers.h (right): http://codereview.chromium.org/12701/diff/209/210#newcode5 Line 5: // Defines various types of timestamped media buffers used for transporting nice! |