media: Add unittest for MojoRenderer{Impl|Service}

media: Add unittest for MojoRenderer{Impl|Service}

This setup the following chain to test both MojoRendererImpl and
MojoRendererService:

MojoRendererTest -> MojoRendererImpl -> (mojom::Renderer) ->
MojoRendererService -> MockRenderer

This CL also fixes some real issues in MojoRendererImpl:
- After connection error, we may have outstanding callbacks not fired
- If we call MojoRendererImpl after error, callbacks should still be
  fired.

BUG=617204
TEST=This adds new unittests.

Committed: https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe
Cr-Commit-Position: refs/heads/master@{#400581}

[xhwang, 2016-06-16 06:44:49]

xhwang@chromium.org changed reviewers:
+ sandersd@chromium.org

[xhwang, 2016-06-16 06:44:50]

sandersd: PTAL
alokp: FYI

[xhwang, 2016-06-16 06:45:55]

BTW, this test is not enabled on any bots. My other CL
(https://chromiumcodereview.appspot.com/2069043003/) will at least enable it on
a FYI bot, and later we'll enable it on more try bots.

[sandersd (OOO until July 31), 2016-06-16 18:10:06]

https://chromiumcodereview.appspot.com/2070753002/diff/1/media/mojo/clients/B...
File media/mojo/clients/BUILD.gn (right):

https://chromiumcodereview.appspot.com/2070753002/diff/1/media/mojo/clients/B...
media/mojo/clients/BUILD.gn:10: # TODO(xhwang): Only allow
//media/mojo/services:media_mojo_shell_unittests
Comment is now on the wrong line.

https://chromiumcodereview.appspot.com/2070753002/diff/1/media/mojo/clients/m...
File media/mojo/clients/mojo_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/2070753002/diff/1/media/mojo/clients/m...
media/mojo/clients/mojo_renderer_impl.cc:38: FireAllPendingCallbacks();
Why does the destructor need to call this?

https://chromiumcodereview.appspot.com/2070753002/diff/1/media/mojo/clients/m...
media/mojo/clients/mojo_renderer_impl.cc:290: void
MojoRendererImpl::FireAllPendingCallbacks() {
This name should probably indicate that the callbacks are failed. Perhaps
CancelPendingCallbacks()?

[xhwang, 2016-06-17 06:58:18]

comments addressed

[xhwang, 2016-06-17 06:59:00]

PTAL again

https://chromiumcodereview.appspot.com/2070753002/diff/1/media/mojo/clients/B...
File media/mojo/clients/BUILD.gn (right):

https://chromiumcodereview.appspot.com/2070753002/diff/1/media/mojo/clients/B...
media/mojo/clients/BUILD.gn:10: # TODO(xhwang): Only allow
//media/mojo/services:media_mojo_shell_unittests
On 2016/06/16 18:10:06, sandersd wrote:
> Comment is now on the wrong line.

Done.

https://chromiumcodereview.appspot.com/2070753002/diff/1/media/mojo/clients/m...
File media/mojo/clients/mojo_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/2070753002/diff/1/media/mojo/clients/m...
media/mojo/clients/mojo_renderer_impl.cc:38: FireAllPendingCallbacks();
On 2016/06/16 18:10:06, sandersd wrote:
> Why does the destructor need to call this?

The Renderer API says all pending callbacks should be fired during destruction:

https://cs.chromium.org/chromium/src/media/base/renderer.h?rcl=0&l=28

Even though typically the caller will use weak ptr and ignore these callbacks,
but I feel it's still good practice to always fire all callbacks.

https://chromiumcodereview.appspot.com/2070753002/diff/1/media/mojo/clients/m...
media/mojo/clients/mojo_renderer_impl.cc:290: void
MojoRendererImpl::FireAllPendingCallbacks() {
On 2016/06/16 18:10:06, sandersd wrote:
> This name should probably indicate that the callbacks are failed. Perhaps
> CancelPendingCallbacks()?

Done.

[sandersd (OOO until July 31), 2016-06-17 17:44:58]

lgtm

[xhwang, 2016-06-17 18:31:52]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2016-06-17 18:31:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2070753002/#ps40001
(title: "fix visibility :(")

[commit-bot: I haz the power, 2016-06-17 18:32:36]

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

[commit-bot: I haz the power, 2016-06-17 20:26:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-17 20:26:45]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xhwang, 2016-06-18 05:02:33]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2016-06-18 05:02:47]

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

[commit-bot: I haz the power, 2016-06-18 06:33:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-18 06:35:18]

Description was changed from

==========
media: Add unittest for MojoRenderer{Impl|Service}

This setup the following chain to test both MojoRendererImpl and
MojoRendererService:

MojoRendererTest -> MojoRendererImpl -> (mojom::Renderer) ->
MojoRendererService -> MockRenderer

This CL also fixes some real issues in MojoRendererImpl:
- After connection error, we may have outstanding callbacks not fired
- If we call MojoRendererImpl after error, callbacks should still be
  fired.

BUG=617204
TEST=This adds new unittests.
==========

to

==========
media: Add unittest for MojoRenderer{Impl|Service}

This setup the following chain to test both MojoRendererImpl and
MojoRendererService:

MojoRendererTest -> MojoRendererImpl -> (mojom::Renderer) ->
MojoRendererService -> MockRenderer

This CL also fixes some real issues in MojoRendererImpl:
- After connection error, we may have outstanding callbacks not fired
- If we call MojoRendererImpl after error, callbacks should still be
  fired.

BUG=617204
TEST=This adds new unittests.

Committed: https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe
Cr-Commit-Position: refs/heads/master@{#400581}
==========

[commit-bot: I haz the power, 2016-06-18 06:35:20]

Patchset 3 (id:??) landed as
https://crrev.com/58cf81460b741ef317f54de1756e5cd855034bfe
Cr-Commit-Position: refs/heads/master@{#400581}