Refactor MediaSourceDelegate destruction.

Refactor MediaSourceDelegate destruction.

This CL introduces a waiter in ~WebMediaPlayerAndroid() and makes sure objects
are destructed after MediaSourceDelegate is completly stopped.

BUG=417864
Committed: https://crrev.com/aaaae22e5c7a31658ecb7a562d4d6562d54fe2e7
Cr-Commit-Position: refs/heads/master@{#297031}

[xhwang, 2014-09-25 23:14:59]

xhwang@chromium.org changed reviewers:
+ qinmin@chromium.org, scherkus@chromium.org

[xhwang, 2014-09-25 23:14:59]

Please see the bug for how the old code could cause crash.

The fix is similar to what WMPI is currently doing.

PTAL

[scherkus (not reviewing), 2014-09-26 00:45:19]

lgtm w/ suggestion

https://codereview.chromium.org/605013002/diff/40001/content/renderer/media/a...
File content/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/605013002/diff/40001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.cc:145: }
perhaps add comment here that |this| may be destroyed at this point in time as a
result of running |stop_cb|

[xhwang, 2014-09-26 16:54:39]

qinmin: could you please also take a look?

[qinmin, 2014-09-26 18:06:50]

lgtm with comments

https://codereview.chromium.org/605013002/diff/40001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/605013002/diff/40001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:211:
base::Bind(&base::WaitableEvent::Signal, base::Unretained(&waiter)));
How about moving this waitableEvent into MediaSourceDelegate::Stop()? 
With the current change, MediaSourceDelegate::Stop() implicitly implies that
only a waitableEvent::signal callback can be accept as the callback will be
executed on another thread. So developers can easily make a mistake there if
they don't read the implementation.

By moving the waitableEvent into MediaSourceDelegate::Stop(), we don't need to
pass a CB into MSD and there is no need to worry about what kind of callbacks
can be passed.

[xhwang, 2014-09-26 19:24:23]

rebase only

[xhwang, 2014-09-26 19:26:41]

comments addressed

[xhwang, 2014-09-26 19:26:59]

https://codereview.chromium.org/605013002/diff/40001/content/renderer/media/a...
File content/renderer/media/android/media_source_delegate.cc (right):

https://codereview.chromium.org/605013002/diff/40001/content/renderer/media/a...
content/renderer/media/android/media_source_delegate.cc:145: }
On 2014/09/26 00:45:18, scherkus wrote:
> perhaps add comment here that |this| may be destroyed at this point in time as
a
> result of running |stop_cb|

Done.

[xhwang, 2014-09-26 21:06:58]

https://codereview.chromium.org/605013002/diff/40001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/605013002/diff/40001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:211:
base::Bind(&base::WaitableEvent::Signal, base::Unretained(&waiter)));
On 2014/09/26 18:06:50, qinmin wrote:
> How about moving this waitableEvent into MediaSourceDelegate::Stop()? 
> With the current change, MediaSourceDelegate::Stop() implicitly implies that
> only a waitableEvent::signal callback can be accept as the callback will be
> executed on another thread. So developers can easily make a mistake there if
> they don't read the implementation.
> 
> By moving the waitableEvent into MediaSourceDelegate::Stop(), we don't need to
> pass a CB into MSD and there is no need to worry about what kind of callbacks
> can be passed. 

My Clank checkout is broken.. I'll land this CL as is and address this comment
later.

[xhwang, 2014-09-26 21:07:02]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-09-26 21:08:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/605013002/80001

[commit-bot: I haz the power, 2014-09-26 21:13:31]

Committed patchset #5 (id:80001) as 2161ed0fb5d73ae35ad296517d4e4f2243dd898e

[commit-bot: I haz the power, 2014-09-26 21:14:03]

Patchset 5 (id:??) landed as
https://crrev.com/aaaae22e5c7a31658ecb7a562d4d6562d54fe2e7
Cr-Commit-Position: refs/heads/master@{#297031}