removed AndroidOverlayFactory

Remove AndroidOverlayFactory

AndroidOverlayFactory is replaced with a callback.  This simplifies it.

Also, it makes plumbing a mojo factory callback from the GPU service
much easier.  The factory type, AndroidOverlayFactoryCB can be compiled
on any platform, to prevent a lot of #ifdef's later.

As part of this, the base::Callback instances are replaced with
RepeatingCallbacks or OnceCallbacks, as appropriate.

BUG=718577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2856253004
Cr-Commit-Position: refs/heads/master@{#469742}
Committed: https://chromium.googlesource.com/chromium/src/+/c9baea5c9d9d2b658d91c48b053ddde9bf9df7b7

[liberato (no reviews please), 2017-05-04 18:08:39]

Description was changed from

==========
removed AndroidOverlayFactory


removed some unused classes


tests pass


tests compile


compiles, except for unit tests


split into config

BUG=
==========

to

==========
removed AndroidOverlayFactory


removed some unused classes


tests pass


tests compile


compiles, except for unit tests


split into config

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2017-05-04 20:38:46]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 20:39:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-05-04 20:49:00]

Description was changed from

==========
removed AndroidOverlayFactory


removed some unused classes


tests pass


tests compile


compiles, except for unit tests


split into config

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove AndroidOverlayFactory

AndroidOverlayFactory is replaced with a callback.  This simplifies it.

Also, it makes plumbing a mojo factory callback from the GPU service
much easier.

As part of this, the base::Callback instances are replaced with
RepeatingCallbacks or OnceCallbacks, as appropriate.

BUG=718577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2017-05-04 20:49:18]

Description was changed from

==========
Remove AndroidOverlayFactory

AndroidOverlayFactory is replaced with a callback.  This simplifies it.

Also, it makes plumbing a mojo factory callback from the GPU service
much easier.

As part of this, the base::Callback instances are replaced with
RepeatingCallbacks or OnceCallbacks, as appropriate.

BUG=718577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove AndroidOverlayFactory

AndroidOverlayFactory is replaced with a callback.  This simplifies it.

Also, it makes plumbing a mojo factory callback from the GPU service
much easier.

As part of this, the base::Callback instances are replaced with
RepeatingCallbacks or OnceCallbacks, as appropriate.

BUG=718577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2017-05-04 20:49:18]

liberato@chromium.org changed reviewers:
+ tguilbert@chromium.org, watk@chromium.org

[liberato (no reviews please), 2017-05-04 20:49:22]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 20:49:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-05-04 20:51:45]

turns out that it's really hard to plumb the mojo stuff from gpu_service without
doing so as a factory callback.  so, i'm taking watk's suggestion sooner than i
thought, and replacing all AndroidOverlayFactory instances with
AndroidOverlayFactoryCB .  i've made them compile on non-android platforms to
avoid a bunch of ifdefs in platform-independent code later.

i'm introducing the mojo factory type here too.  don't really need to, but i
didn't want to forget why i did this :)

thanks
-fl

[commit-bot: I haz the power, 2017-05-04 21:01:18]

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

[commit-bot: I haz the power, 2017-05-04 21:01:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[liberato (no reviews please), 2017-05-04 21:21:30]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 21:21:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-05-04 21:32:05]

Description was changed from

==========
Remove AndroidOverlayFactory

AndroidOverlayFactory is replaced with a callback.  This simplifies it.

Also, it makes plumbing a mojo factory callback from the GPU service
much easier.

As part of this, the base::Callback instances are replaced with
RepeatingCallbacks or OnceCallbacks, as appropriate.

BUG=718577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove AndroidOverlayFactory

AndroidOverlayFactory is replaced with a callback.  This simplifies it.

Also, it makes plumbing a mojo factory callback from the GPU service
much easier.  The factory type, AndroidOverlayFactoryCB can be compiled
on any platform, to prevent a lot of #ifdef's later.

As part of this, the base::Callback instances are replaced with
RepeatingCallbacks or OnceCallbacks, as appropriate.

BUG=718577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[tguilbert, 2017-05-04 21:52:05]

LGTM % nits

https://codereview.chromium.org/2856253004/diff/60001/media/base/android_over...
File media/base/android_overlay_config.h (right):

https://codereview.chromium.org/2856253004/diff/60001/media/base/android_over...
media/base/android_overlay_config.h:36: gfx::Rect rect;
NIT: can you add a small comment describing what |rect| is used for or what it
is?

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:413: factory =
base::Bind(&CreateContentVideoViewOverlay, config_.surface_id);
NIT: Remove brackets for single line if

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator_unittest.cc:351:
ASSERT_TRUE(!chooser_->factory_.is_null());
NIT: ASSERT_FALSE?

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator_unittest.cc:372:
ASSERT_TRUE(chooser_->factory_.is_null());
NIT: base::Callback has an explicit bool operator. You could forgo the is_null()
here and everywhere else in this CL

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
File media/gpu/android_video_surface_chooser_impl.cc (right):

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
media/gpu/android_video_surface_chooser_impl.cc:23: if
(!initial_factory.is_null()) {
NIT: Same "is_null() can be ommitted" comment.

[watk, 2017-05-04 22:03:09]

On 2017/05/04 21:52:05, tguilbert wrote:
> LGTM % nits
> 
>
https://codereview.chromium.org/2856253004/diff/60001/media/base/android_over...
> File media/base/android_overlay_config.h (right):
> 
>
https://codereview.chromium.org/2856253004/diff/60001/media/base/android_over...
> media/base/android_overlay_config.h:36: gfx::Rect rect;
> NIT: can you add a small comment describing what |rect| is used for or what it
> is?
> 
>
https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
> File media/gpu/android_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
> media/gpu/android_video_decode_accelerator.cc:413: factory =
> base::Bind(&CreateContentVideoViewOverlay, config_.surface_id);
> NIT: Remove brackets for single line if
> 
>
https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
> File media/gpu/android_video_decode_accelerator_unittest.cc (right):
> 
>
https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
> media/gpu/android_video_decode_accelerator_unittest.cc:351:
> ASSERT_TRUE(!chooser_->factory_.is_null());
> NIT: ASSERT_FALSE?
> 
>
https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
> media/gpu/android_video_decode_accelerator_unittest.cc:372:
> ASSERT_TRUE(chooser_->factory_.is_null());
> NIT: base::Callback has an explicit bool operator. You could forgo the
is_null()
> here and everywhere else in this CL
> 
>
https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
> File media/gpu/android_video_surface_chooser_impl.cc (right):
> 
>
https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
> media/gpu/android_video_surface_chooser_impl.cc:23: if
> (!initial_factory.is_null()) {
> NIT: Same "is_null() can be ommitted" comment.

I'll defer to tguilbert's review. +251 lines, -349 lines  :)

[commit-bot: I haz the power, 2017-05-05 01:32:52]

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

[commit-bot: I haz the power, 2017-05-05 01:32:52]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-05-05 16:47:44]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 16:48:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-05-05 16:50:23]

thanks for the feedback.

as an aside, when i removed is_null() i forgot to remove some of the negations. 
happily, the unit tests failed.  they even failed when i fixed only the
instances in the unit tests, intentionally leaving the implementation broken.

i wouldn't normally be so happy about unit tests, but... it's avda!  :)

thanks
-fl

https://codereview.chromium.org/2856253004/diff/60001/media/base/android_over...
File media/base/android_overlay_config.h (right):

https://codereview.chromium.org/2856253004/diff/60001/media/base/android_over...
media/base/android_overlay_config.h:36: gfx::Rect rect;
On 2017/05/04 21:52:04, tguilbert wrote:
> NIT: can you add a small comment describing what |rect| is used for or what it
> is?

Done.

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator.cc (right):

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator.cc:413: factory =
base::Bind(&CreateContentVideoViewOverlay, config_.surface_id);
On 2017/05/04 21:52:04, tguilbert wrote:
> NIT: Remove brackets for single line if

Done.

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
File media/gpu/android_video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator_unittest.cc:351:
ASSERT_TRUE(!chooser_->factory_.is_null());
On 2017/05/04 21:52:05, tguilbert wrote:
> NIT: ASSERT_FALSE?

after removing the is_null, it turns back into ASSERT_TRUE.

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
media/gpu/android_video_decode_accelerator_unittest.cc:372:
ASSERT_TRUE(chooser_->factory_.is_null());
On 2017/05/04 21:52:05, tguilbert wrote:
> NIT: base::Callback has an explicit bool operator. You could forgo the
is_null()
> here and everywhere else in this CL

Done.  i also turned this one into ASSERT_FALSE.

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
File media/gpu/android_video_surface_chooser_impl.cc (right):

https://codereview.chromium.org/2856253004/diff/60001/media/gpu/android_video...
media/gpu/android_video_surface_chooser_impl.cc:23: if
(!initial_factory.is_null()) {
On 2017/05/04 21:52:05, tguilbert wrote:
> NIT: Same "is_null() can be ommitted" comment.

Done.

[commit-bot: I haz the power, 2017-05-05 18:51:17]

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

[commit-bot: I haz the power, 2017-05-05 18:51:18]

Dry run: This issue passed the CQ dry run.

[liberato (no reviews please), 2017-05-05 18:53:06]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-05-05 18:53:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
tguilbert@chromium.org
Link to the patchset: https://codereview.chromium.org/2856253004/#ps80001
(title: "cl feedvback")

[commit-bot: I haz the power, 2017-05-05 18:53:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 18:59:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494010386841050,
"parent_rev": "89b9688d75599341703719d88b5f09841ac4d5de", "commit_rev":
"c9baea5c9d9d2b658d91c48b053ddde9bf9df7b7"}

[commit-bot: I haz the power, 2017-05-05 18:59:18]

Description was changed from

==========
Remove AndroidOverlayFactory

AndroidOverlayFactory is replaced with a callback.  This simplifies it.

Also, it makes plumbing a mojo factory callback from the GPU service
much easier.  The factory type, AndroidOverlayFactoryCB can be compiled
on any platform, to prevent a lot of #ifdef's later.

As part of this, the base::Callback instances are replaced with
RepeatingCallbacks or OnceCallbacks, as appropriate.

BUG=718577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Remove AndroidOverlayFactory

AndroidOverlayFactory is replaced with a callback.  This simplifies it.

Also, it makes plumbing a mojo factory callback from the GPU service
much easier.  The factory type, AndroidOverlayFactoryCB can be compiled
on any platform, to prevent a lot of #ifdef's later.

As part of this, the base::Callback instances are replaced with
RepeatingCallbacks or OnceCallbacks, as appropriate.

BUG=718577
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2856253004
Cr-Commit-Position: refs/heads/master@{#469742}
Committed:
https://chromium.googlesource.com/chromium/src/+/c9baea5c9d9d2b658d91c48b053d...
==========

[commit-bot: I haz the power, 2017-05-05 18:59:20]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/c9baea5c9d9d2b658d91c48b053d...

[xhwang, 2017-06-01 05:50:58]

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

[xhwang, 2017-06-01 05:50:59]

https://codereview.chromium.org/2856253004/diff/80001/media/base/android_over...
File media/base/android_overlay_mojo_factory.h (right):

https://codereview.chromium.org/2856253004/diff/80001/media/base/android_over...
media/base/android_overlay_mojo_factory.h:16:
AndroidOverlay>(base::UnguessableToken&, const AndroidOverlayConfig&)>
ISTM this file lives in media/base and this callback has nothing to do with
mojo. I wonder why we use "mojo" in the file/callback name? :)

[liberato (no reviews please), 2017-06-01 06:07:55]

https://codereview.chromium.org/2856253004/diff/80001/media/base/android_over...
File media/base/android_overlay_mojo_factory.h (right):

https://codereview.chromium.org/2856253004/diff/80001/media/base/android_over...
media/base/android_overlay_mojo_factory.h:16:
AndroidOverlay>(base::UnguessableToken&, const AndroidOverlayConfig&)>
On 2017/06/01 05:50:59, xhwang wrote:
> ISTM this file lives in media/base and this callback has nothing to do with
> mojo. I wonder why we use "mojo" in the file/callback name? :)

good point.

the need for a base::UnguessableToken is specific to the mojo implementation of
the AO api.  it identifies the render frame that owns the overlay.  that's the
only connection.

i'm not sure if it could be moved under media/mojo, or if there's a build rule
that prevented it.  i'll give it a try.