Take advantage of the new Pass() machinery on scoped_ptr{,_malloc}.

Take advantage of the new Pass() machinery on scoped_ptr{,_malloc}.

Pass() was announced in 
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/RTd7rNxHjqk/discussion
This CL replaces comments about ownership transfer (in all files whose paths
contain media/) with the explicit passing of the appropriate scoper.

The exceptions that are not touched by this CL:
- scoped_refptr<> doesn't support Pass() and so is untouched.
- media/audio code defines its own callback machinery, mimicking the old-style
  callbacks (pass by pointer, explicit deletes).  I think that whole pile needs
  to be replaced with new-style (Bind) callbacks, so left it alone for now.

BUG=none
TEST=trybots


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

[Ami GONE FROM CHROMIUM, 2011-12-30 02:09:27]

This is a speculative CL to see what happens if we apply Albert's spiffy new
Pass() machinery to our codebase.

I think the end result is worth it, but since this is a largish change to our
coding conventions wanted to make sure we have consensus.

scherkus/acolwell: please review.
videostack-eng: FYI, but please speak up if you don't like what this is doing to
your codebase.
ajwong: FYI.

This CL depends on http://codereview.chromium.org/8968032/ (and patches in its
non-test bits to scoped_ptr.h); that one must land before this one does (or a
bunch of extra intermediates need to be introduced).

http://codereview.chromium.org/9015015/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

http://codereview.chromium.org/9015015/diff/1/base/memory/scoped_ptr.h#newcod...
base/memory/scoped_ptr.h:129: // Constructor.  Allows construction from a
scoped_ptr rvalue for a
Changes in this file are patched in from http://codereview.chromium.org/8968032/

I don't expect to land this CL before that one lands.

[Sergey Ulanov, 2012-01-02 20:22:40]

http://codereview.chromium.org/9015015/diff/15001/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/9015015/diff/15001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.h:36: #include "media/ffmpeg/ffmpeg_common.h"
Is it possible to forward declare ScopedPtrAVFreePacket instead of adding this
include?

[Ami GONE FROM CHROMIUM, 2012-01-03 00:35:59]

http://codereview.chromium.org/9015015/diff/15001/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/9015015/diff/15001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.h:36: #include "media/ffmpeg/ffmpeg_common.h"
On 2012/01/02 20:22:40, sergeyu wrote:
> Is it possible to forward declare ScopedPtrAVFreePacket instead of adding this
> include?

Done.

[acolwell GONE FROM CHROMIUM, 2012-01-03 19:27:06]

LGTM

http://codereview.chromium.org/9015015/diff/20001/media/base/mock_filters.cc
File media/base/mock_filters.cc (right):

http://codereview.chromium.org/9015015/diff/20001/media/base/mock_filters.cc#...
media/base/mock_filters.cc:70: return
scoped_ptr<DemuxerFactory>(reinterpret_cast<DemuxerFactory*>(
Why is the reinterpret_cast needed here?

http://codereview.chromium.org/9015015/diff/20001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/9015015/diff/20001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:29: AVPacketBuffer(scoped_ptr_malloc<AVPacket,
ScopedPtrAVFreePacket> packet,
Wow. This says way more about this pointer than the old code.

http://codereview.chromium.org/9015015/diff/20001/media/webm/cluster_builder.h
File media/webm/cluster_builder.h (right):

http://codereview.chromium.org/9015015/diff/20001/media/webm/cluster_builder....
media/webm/cluster_builder.h:38: Cluster* Finish();
I think this could become a scoped_ptr too.

http://codereview.chromium.org/9015015/diff/20001/webkit/media/buffered_resou...
File webkit/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/9015015/diff/20001/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.cc:185: active_loader_.reset(new
ActiveLoader(scoped_ptr<WebURLLoader>(loader)));
Perhaps make loader scoped_ptr since we've got ownership transfer happening
above?

[Ami GONE FROM CHROMIUM, 2012-01-03 20:58:34]

http://codereview.chromium.org/9015015/diff/20001/media/base/mock_filters.cc
File media/base/mock_filters.cc (right):

http://codereview.chromium.org/9015015/diff/20001/media/base/mock_filters.cc#...
media/base/mock_filters.cc:70: return
scoped_ptr<DemuxerFactory>(reinterpret_cast<DemuxerFactory*>(
On 2012/01/03 19:27:06, acolwell wrote:
> Why is the reinterpret_cast needed here?

Leftover.  Gone now.

http://codereview.chromium.org/9015015/diff/20001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/9015015/diff/20001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:29: AVPacketBuffer(scoped_ptr_malloc<AVPacket,
ScopedPtrAVFreePacket> packet,
On 2012/01/03 19:27:06, acolwell wrote:
> Wow. This says way more about this pointer than the old code.

...and yet, it says no more than the comment that *should* have accompanied the
old code would have said: "takes ownership of |packet|, assumes it can be freed
with ScopedPtrAVFreePacket()".

http://codereview.chromium.org/9015015/diff/20001/media/webm/cluster_builder.h
File media/webm/cluster_builder.h (right):

http://codereview.chromium.org/9015015/diff/20001/media/webm/cluster_builder....
media/webm/cluster_builder.h:38: Cluster* Finish();
On 2012/01/03 19:27:06, acolwell wrote:
> I think this could become a scoped_ptr too.

Done.

http://codereview.chromium.org/9015015/diff/20001/webkit/media/buffered_resou...
File webkit/media/buffered_resource_loader.cc (right):

http://codereview.chromium.org/9015015/diff/20001/webkit/media/buffered_resou...
webkit/media/buffered_resource_loader.cc:185: active_loader_.reset(new
ActiveLoader(scoped_ptr<WebURLLoader>(loader)));
On 2012/01/03 19:27:06, acolwell wrote:
> Perhaps make loader scoped_ptr since we've got ownership transfer happening
> above?

Done.

[acolwell GONE FROM CHROMIUM, 2012-01-03 22:46:40]

http://codereview.chromium.org/9015015/diff/23005/media/base/mock_filters.cc
File media/base/mock_filters.cc (right):

http://codereview.chromium.org/9015015/diff/23005/media/base/mock_filters.cc#...
media/base/mock_filters.cc:150: demuxer_factory.release()));
// TODO(fischman): replace the extra scoped_ptr+release() with Pass() when
// http://crbug.com/109026 is fixed.

http://codereview.chromium.org/9015015/diff/23005/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/9015015/diff/23005/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:60: return new DataBuffer(scoped_array<uint8>(0),
0);
nit: 0 not necessary in constructor.

http://codereview.chromium.org/9015015/diff/23005/media/tools/player_wtl/movi...
File media/tools/player_wtl/movie.cc (right):

http://codereview.chromium.org/9015015/diff/23005/media/tools/player_wtl/movi...
media/tools/player_wtl/movie.cc:77: collection->SetDemuxerFactory(
Remove comment per discussion

[Ami GONE FROM CHROMIUM, 2012-01-03 22:52:02]

http://codereview.chromium.org/9015015/diff/23005/media/base/mock_filters.cc
File media/base/mock_filters.cc (right):

http://codereview.chromium.org/9015015/diff/23005/media/base/mock_filters.cc#...
media/base/mock_filters.cc:150: demuxer_factory.release()));
On 2012/01/03 22:46:40, acolwell wrote:
> // TODO(fischman): replace the extra scoped_ptr+release() with Pass() when
> // http://crbug.com/109026 is fixed.

Done.

http://codereview.chromium.org/9015015/diff/23005/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/9015015/diff/23005/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:60: return new DataBuffer(scoped_array<uint8>(0),
0);
On 2012/01/03 22:46:40, acolwell wrote:
> nit: 0 not necessary in constructor.

Actually, scoped_array<uint8>(0) is not necessary.

http://codereview.chromium.org/9015015/diff/23005/media/tools/player_wtl/movi...
File media/tools/player_wtl/movie.cc (right):

http://codereview.chromium.org/9015015/diff/23005/media/tools/player_wtl/movi...
media/tools/player_wtl/movie.cc:77: collection->SetDemuxerFactory(
On 2012/01/03 22:46:40, acolwell wrote:
> Remove comment per discussion

Done.

[scherkus (not reviewing), 2012-01-10 02:11:30]

LGTM w/ nit on missed indents

http://codereview.chromium.org/9015015/diff/28001/media/base/media_log.cc
File media/base/media_log.cc (right):

http://codereview.chromium.org/9015015/diff/28001/media/base/media_log.cc#new...
media/base/media_log.cc:165: const char* property, bool value) {
indent here + below

[Ami GONE FROM CHROMIUM, 2012-01-10 03:25:59]

Thanks for the review.
sergeyu: removing you from reviewers b/c I'm guessing it was unintentional on
your part to add yourself as a reviewer.

http://codereview.chromium.org/9015015/diff/28001/media/base/media_log.cc
File media/base/media_log.cc (right):

http://codereview.chromium.org/9015015/diff/28001/media/base/media_log.cc#new...
media/base/media_log.cc:165: const char* property, bool value) {
On 2012/01/10 02:11:30, scherkus wrote:
> indent here + below

Done.

[commit-bot: I haz the power, 2012-01-10 03:26:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/9015015/33001

[commit-bot: I haz the power, 2012-01-10 03:26:36]

Presubmit check for 9015015-33001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    media/tools/player_wtl/movie.cc:94

Presubmit checks took 3.8s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-01-10 03:34:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/9015015/38001

[commit-bot: I haz the power, 2012-01-10 04:47:47]

Change committed as 117009