[Chromecast] Turn CastContentWindow into an abstract interface.

[Chromecast] Turn CastContentWindow into an abstract interface.

CastContentWindowImpl is the implementation of CastContentWindow
now. Add a CastContentWindowAndroid specific for Android platform.

BUG=NONE
TEST=cast_shell_apk

Change-Id: I6092f733b54638256bd3028ac3d889f6b52a02fe
Review-Url: https://codereview.chromium.org/2570623003
Cr-Commit-Position: refs/heads/master@{#443358}
Committed: https://chromium.googlesource.com/chromium/src/+/504caf2ab4aa7bd4689957423b352ea135508f17

[derekjchow1, 2016-12-12 23:30:06]

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

[commit-bot: I haz the power, 2016-12-12 23:34:05]

Dry run: 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, 2016-12-13 00:00:45]

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

[commit-bot: I haz the power, 2016-12-13 00:00:46]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[derekjchow1, 2016-12-13 00:01:53]

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

[commit-bot: I haz the power, 2016-12-13 00:03:54]

Dry run: 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, 2016-12-13 00:37:14]

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

[commit-bot: I haz the power, 2016-12-13 00:37:15]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[derekjchow1, 2016-12-13 00:45:33]

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

[commit-bot: I haz the power, 2016-12-13 00:46:18]

Dry run: 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, 2016-12-13 01:07:14]

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

[commit-bot: I haz the power, 2016-12-13 01:07:15]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[derekjchow1, 2016-12-13 01:16:11]

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

[commit-bot: I haz the power, 2016-12-13 01:17:29]

Dry run: 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, 2016-12-13 01:38:50]

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

[commit-bot: I haz the power, 2016-12-13 01:38:51]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[derekjchow1, 2016-12-13 18:29:10]

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

[commit-bot: I haz the power, 2016-12-13 18:29:39]

Dry run: 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, 2016-12-13 21:13:45]

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

[commit-bot: I haz the power, 2016-12-13 21:13:46]

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

[derekjchow1, 2016-12-15 00:29:39]

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

[commit-bot: I haz the power, 2016-12-15 00:30:28]

Dry run: 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, 2016-12-15 00:38:56]

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

[commit-bot: I haz the power, 2016-12-15 00:38:57]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[derekjchow1, 2016-12-15 18:31:20]

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

[commit-bot: I haz the power, 2016-12-15 18:31:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[derekjchow1, 2016-12-15 18:34:38]

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

[commit-bot: I haz the power, 2016-12-15 18:34:54]

Dry run: 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, 2016-12-15 19:27:30]

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

[commit-bot: I haz the power, 2016-12-15 19:27:30]

Dry run: This issue passed the CQ dry run.

[derekjchow1, 2016-12-15 19:30:41]

derekjchow@chromium.org changed reviewers:
+ alokp@chromium.org, byungchul@chromium.org, halliwell@chromium.org,
sanfin@chromium.org, tsunghung@chromium.org

[derekjchow1, 2016-12-15 19:30:42]

Moving the refactor upstream.

Patchset 1 represents where we left of in our internal review.
Patchset 7 contains build/lint fixes for Chromium CQ. PTAL

[derekjchow1, 2016-12-16 19:35:40]

Ping.

[derekjchow1, 2016-12-16 19:49:23]

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

[commit-bot: I haz the power, 2016-12-16 19:50:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[byungchul, 2016-12-16 22:52:23]

https://codereview.chromium.org/2570623003/diff/140001/chromecast/android/DEPS
File chromecast/android/DEPS (right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/android/DEP...
chromecast/android/DEPS:5: "+chromecast/browser/android",
Why?

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/BUI...
File chromecast/browser/BUILD.gn (right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/BUI...
chromecast/browser/BUILD.gn:122: "cast_content_window_linux.h",
In the past, gyp file specified all files and let ninja pick platform-specific
files accordingly. Is it changed for BUILD.gn?

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java
(right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java:57:
.scheme(CastWebContentsActivity.ACTION_DATA_SCHEME)
Is it a right indentation?

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java
(right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:279:
.getMethod("isStreamMute", int.class)
indentation?

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:324:
FrameLayout.LayoutParams.MATCH_PARENT));
This style doesn't look readable

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:345:
.scheme(CastWebContentsActivity.ACTION_DATA_SCHEME)
ditto

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/cas...
File chromecast/browser/cast_content_window.h (right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/cas...
chromecast/browser/cast_content_window.h:29: virtual void
OnKeyDown(ui::KeyboardCode keycode) = 0;
Need a protected dtor.

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/cas...
File chromecast/browser/cast_content_window_linux.cc (right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.cc:70: }
{}

[commit-bot: I haz the power, 2016-12-17 00:04:07]

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

[commit-bot: I haz the power, 2016-12-17 00:04:08]

Dry run: This issue passed the CQ dry run.

[derekjchow1, 2016-12-17 00:13:08]

Most of your linter comments argue against clang-format.

https://codereview.chromium.org/2570623003/diff/140001/chromecast/android/DEPS
File chromecast/android/DEPS (right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/android/DEP...
chromecast/android/DEPS:5: "+chromecast/browser/android",
On 2016/12/16 22:52:22, byungchul wrote:
> Why?

cast_jni_registrar.cc needs to register JNI functions defined in
chroemcast/browser/android.

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/BUI...
File chromecast/browser/BUILD.gn (right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/BUI...
chromecast/browser/BUILD.gn:122: "cast_content_window_linux.h",
On 2016/12/16 22:52:22, byungchul wrote:
> In the past, gyp file specified all files and let ninja pick platform-specific
> files accordingly. Is it changed for BUILD.gn?

It's not changed but it's not well known either. I've been bitten by this
behaviour before actually (I though an internal file named *_linux file could be
included in an android build).

My preference is to be explicit and leave them in these if statements, rather
than depend on the BUILD.gn behaviour.

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java
(right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java:57:
.scheme(CastWebContentsActivity.ACTION_DATA_SCHEME)
On 2016/12/16 22:52:22, byungchul wrote:
> Is it a right indentation?

Apparently as I ran clang-format-diff. Java has 100 char line widths so these
are correct (they only look weird because Rietveld is wrapping at 80 lines).

FWIW, I agree that this looks ugly. If you feel strongly, I'll let you have
final say over the linter.

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java
(right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:279:
.getMethod("isStreamMute", int.class)
On 2016/12/16 22:52:22, byungchul wrote:
> indentation?

See other comment (clang-format-diff did it)!

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:324:
FrameLayout.LayoutParams.MATCH_PARENT));
On 2016/12/16 22:52:22, byungchul wrote:
> This style doesn't look readable

ditto.

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:345:
.scheme(CastWebContentsActivity.ACTION_DATA_SCHEME)
On 2016/12/16 22:52:22, byungchul wrote:
> ditto

ditto

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/cas...
File chromecast/browser/cast_content_window.h (right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/cas...
chromecast/browser/cast_content_window.h:29: virtual void
OnKeyDown(ui::KeyboardCode keycode) = 0;
On 2016/12/16 22:52:22, byungchul wrote:
> Need a protected dtor.

Done.

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/cas...
File chromecast/browser/cast_content_window_linux.cc (right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.cc:70: }
On 2016/12/16 22:52:22, byungchul wrote:
> {}

Done.

[byungchul, 2016-12-17 00:57:37]

https://codereview.chromium.org/2570623003/diff/140001/chromecast/android/DEPS
File chromecast/android/DEPS (right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/android/DEP...
chromecast/android/DEPS:5: "+chromecast/browser/android",
On 2016/12/17 00:13:07, derekjchow1 wrote:
> On 2016/12/16 22:52:22, byungchul wrote:
> > Why?
> 
> cast_jni_registrar.cc needs to register JNI functions defined in
> chroemcast/browser/android.

That's the problem I think. It should not depend on shell. shell/android should
have its own jni registration function.

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java
(right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java:57:
.scheme(CastWebContentsActivity.ACTION_DATA_SCHEME)
On 2016/12/17 00:13:07, derekjchow1 wrote:
> On 2016/12/16 22:52:22, byungchul wrote:
> > Is it a right indentation?
> 
> Apparently as I ran clang-format-diff. Java has 100 char line widths so these
> are correct (they only look weird because Rietveld is wrapping at 80 lines).
> 
> FWIW, I agree that this looks ugly. If you feel strongly, I'll let you have
> final say over the linter.

I didn't mean line width. I meant the location of first character. Shouldn't
".scheme" start 8 char after of "Url"?

[derekjchow1, 2016-12-19 17:51:31]

https://codereview.chromium.org/2570623003/diff/140001/chromecast/android/DEPS
File chromecast/android/DEPS (right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/android/DEP...
chromecast/android/DEPS:5: "+chromecast/browser/android",
On 2016/12/17 00:57:37, byungchul wrote:
> On 2016/12/17 00:13:07, derekjchow1 wrote:
> > On 2016/12/16 22:52:22, byungchul wrote:
> > > Why?
> > 
> > cast_jni_registrar.cc needs to register JNI functions defined in
> > chroemcast/browser/android.
> 
> That's the problem I think. It should not depend on shell. shell/android
should
> have its own jni registration function.

Done. I creates a browser/android specific jni registration function. Also when
looking closer at this directory, I determined we can remove this dependency
completely.

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java
(right):

https://codereview.chromium.org/2570623003/diff/140001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java:57:
.scheme(CastWebContentsActivity.ACTION_DATA_SCHEME)
On 2016/12/17 00:57:37, byungchul wrote:
> On 2016/12/17 00:13:07, derekjchow1 wrote:
> > On 2016/12/16 22:52:22, byungchul wrote:
> > > Is it a right indentation?
> > 
> > Apparently as I ran clang-format-diff. Java has 100 char line widths so
these
> > are correct (they only look weird because Rietveld is wrapping at 80 lines).
> > 
> > FWIW, I agree that this looks ugly. If you feel strongly, I'll let you have
> > final say over the linter.
> 
> I didn't mean line width. I meant the location of first character. Shouldn't
> ".scheme" start 8 char after of "Url"?

PTAL. I decided to ignore clang-format and used 8 spaces.

[byungchul, 2017-01-03 22:56:44]

lgtm

[derekjchow1, 2017-01-05 20:16:52]

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

[commit-bot: I haz the power, 2017-01-05 20:17:27]

Dry run: 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-01-05 20:29:43]

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

[commit-bot: I haz the power, 2017-01-05 20:29:44]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[derekjchow1, 2017-01-05 21:39:39]

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

[commit-bot: I haz the power, 2017-01-05 21:40:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alokp, 2017-01-05 22:04:51]

lgtm ++

https://codereview.chromium.org/2570623003/diff/200001/chromecast/browser/and...
File chromecast/browser/android/DEPS (right):

https://codereview.chromium.org/2570623003/diff/200001/chromecast/browser/and...
chromecast/browser/android/DEPS:2: "+chromecast/browser"
This should not be necessary since this DEPS is for a parent directory.

https://codereview.chromium.org/2570623003/diff/200001/chromecast/browser/cas...
File chromecast/browser/cast_content_window_linux.cc (right):

https://codereview.chromium.org/2570623003/diff/200001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.cc:66: return
base::MakeUnique<CastContentWindowLinux>();
Is dropping the delegate intentional?

[derekjchow1, 2017-01-05 22:27:10]

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

[derekjchow1, 2017-01-05 22:27:25]

https://codereview.chromium.org/2570623003/diff/200001/chromecast/browser/and...
File chromecast/browser/android/DEPS (right):

https://codereview.chromium.org/2570623003/diff/200001/chromecast/browser/and...
chromecast/browser/android/DEPS:2: "+chromecast/browser"
On 2017/01/05 22:04:51, alokp wrote:
> This should not be necessary since this DEPS is for a parent directory.

Done.

https://codereview.chromium.org/2570623003/diff/200001/chromecast/browser/cas...
File chromecast/browser/cast_content_window_linux.cc (right):

https://codereview.chromium.org/2570623003/diff/200001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.cc:66: return
base::MakeUnique<CastContentWindowLinux>();
On 2017/01/05 22:04:51, alokp wrote:
> Is dropping the delegate intentional?

Correct. The Linux version of CastContentWindow doesn't use the delegate .

[commit-bot: I haz the power, 2017-01-05 22:27:31]

Dry run: 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-01-05 23:42:41]

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

[commit-bot: I haz the power, 2017-01-05 23:42:42]

Dry run: This issue passed the CQ dry run.

[derekjchow1, 2017-01-07 00:09:49]

The CQ bit was checked by derekjchow@chromium.org

[derekjchow1, 2017-01-07 00:09:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
byungchul@chromium.org, alokp@chromium.org
Link to the patchset: https://codereview.chromium.org/2570623003/#ps220001
(title: "Remove duplicate dependency")

[commit-bot: I haz the power, 2017-01-07 00:10:12]

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-01-07 00:19:41]

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

[commit-bot: I haz the power, 2017-01-07 00:19:42]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[derekjchow1, 2017-01-07 00:27:53]

+wez, +boliu

wez, could you take a look at chromecast/browser/cast_content_window.h and see
if our dependency on ui/events/keycodes/keyboard_codes.h is ok?

boliu, could you look at the new dependency we've added on
content/public/android in chromecast/browser?

[derekjchow1, 2017-01-09 17:53:21]

derekjchow@chromium.org changed reviewers:
+ boliu@chromium.org, wez@chromium.org

[derekjchow1, 2017-01-10 19:15:16]

Ping on this issue.

[boliu, 2017-01-10 19:20:19]

On 2017/01/10 19:15:16, derekjchow1 wrote:
> Ping on this issue.

content/public/android DEPS lgtm

Sorry.. for some reason, the email from yesterday was sent out to boliu+cc,
instead of boliu+reviewer, so it didn't go into my review queue

[Wez, 2017-01-11 20:47:55]

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/and...
File chromecast/browser/android/cast_content_window_android.h (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/and...
chromecast/browser/android/cast_content_window_android.h:32: explicit
CastContentWindowAndroid(CastContentWindow::Delegate* delegate);
As noted in the Linux impl, it doesn't seem that this class needs to be publicly
visible - you can move the definition into the .cc and rely on callers to
Create() it, I think?

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
File chromecast/browser/cast_content_window.h (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window.h:29: virtual void
OnKeyDown(ui::KeyboardCode keycode) = 0;
KeyboardCode is deprecated (though it seems we still use it under Android, and
don't generate the newer |key| and |code| values on events ):

I'd suggest having this interface be OnKeyEvent(const ui::KeyEvent&), so that
it's not tied to the deprecated keyCode field, even though the receiving code
will need to use that field for now, in the absence of |key| and |code|.

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window.h:37: CastContentWindow::Delegate*
delegate);
nit: Please be specific about the lifetime requirement on |delegate|, e.g. I
assume it must out-live CastContentWindow?

Is it beneficial to have |delegate| be a non-owned interface like this, or could
it be an owned interface, i.e. the Create() call would receive a
unique_ptr<Delegate> rather than bare-pointer?

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window.h:46: virtual void
ShowWebContents(content::WebContents* web_contents) = 0;
Similarly, please clarify the lifetime guarantee on |web_contents| that is
expected of the caller.

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
File chromecast/browser/cast_content_window_linux.cc (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.cc:66: return
base::MakeUnique<CastContentWindowLinux>();
nit: Suggest DCHECK(delegate) here since although you don't actually use the
Delegate on Linux, it being non-null is part of the cross-platform interface
contract for Create().

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
File chromecast/browser/cast_content_window_linux.h (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.h:30: public
CastVSyncSettings::Observer {
IIUC you only ever create this class via Create(), so there is no need for this
.h file - you can move the class definition into cast_content_window_linux.cc
and avoid it being publicly visible.

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.h:46: const MediaPlayerId& id)
override;
nit: Why did you move this? It reads more naturally to see Started before
Stopped! :)

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.h:56:
std::unique_ptr<aura::WindowTreeHost> window_tree_host_;
Rather than have this class' impl become a no-op in non-Aura builds, now that
you have an abstract interface you could have Create() simply return null, and
if necessary provide a no-op fallback implementation of the API to use in that
case.

[derekjchow1, 2017-01-11 21:54:32]

derekjchow@chromium.org changed reviewers:
+ sadrul@chromium.org

[derekjchow1, 2017-01-11 21:54:34]

Thanks for the comments Wez.

+sadrul, could you check if our new dependency on ui/events is ok?

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/and...
File chromecast/browser/android/cast_content_window_android.h (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/and...
chromecast/browser/android/cast_content_window_android.h:32: explicit
CastContentWindowAndroid(CastContentWindow::Delegate* delegate);
On 2017/01/11 20:47:54, Wez wrote:
> As noted in the Linux impl, it doesn't seem that this class needs to be
publicly
> visible - you can move the definition into the .cc and rely on callers to
> Create() it, I think?

Your point on constructor visibility is valid, but I think the class definition
should stay visible in it's own header file (at least for the Android case). The
key reason is we need the RegisterJni function to remain public, and I'd rather
keep this function as a static class method to make it obvious what Jni
functions are being registered.

In terms of constructor visibility, I've made the constructor private and added
CastContentWindow as a friend class. This way it can only be constructed via
CastContentWindow::Create. For consistency between platform implementations I've
done the same thing to CastContentWindowLinux.

Does this sound ok?

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
File chromecast/browser/cast_content_window.h (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window.h:29: virtual void
OnKeyDown(ui::KeyboardCode keycode) = 0;
On 2017/01/11 20:47:54, Wez wrote:
> KeyboardCode is deprecated (though it seems we still use it under Android, and
> don't generate the newer |key| and |code| values on events ):
> 
> I'd suggest having this interface be OnKeyEvent(const ui::KeyEvent&), so that
> it's not tied to the deprecated keyCode field, even though the receiving code
> will need to use that field for now, in the absence of |key| and |code|.

SGTM. Done.

Unfortunately, this changes our new dependency from ui/events/keycodes to
ui/events (meaning we need a new OWNERS reviewer for approval). +sadrul

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window.h:37: CastContentWindow::Delegate*
delegate);
On 2017/01/11 20:47:54, Wez wrote:
> nit: Please be specific about the lifetime requirement on |delegate|, e.g. I
> assume it must out-live CastContentWindow?
> 
> Is it beneficial to have |delegate| be a non-owned interface like this, or
could
> it be an owned interface, i.e. the Create() call would receive a
> unique_ptr<Delegate> rather than bare-pointer?

Added comment about |delegate| outliving CastContentWindow.

The delegate interface should be non-owning. Internally, the parent object of
CastContentWindow will pass itself as the delegate to be notified when the
window is destroyed or keys are pressed.

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window.h:46: virtual void
ShowWebContents(content::WebContents* web_contents) = 0;
On 2017/01/11 20:47:54, Wez wrote:
> Similarly, please clarify the lifetime guarantee on |web_contents| that is
> expected of the caller.

Done.

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
File chromecast/browser/cast_content_window_linux.cc (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.cc:66: return
base::MakeUnique<CastContentWindowLinux>();
On 2017/01/11 20:47:54, Wez wrote:
> nit: Suggest DCHECK(delegate) here since although you don't actually use the
> Delegate on Linux, it being non-null is part of the cross-platform interface
> contract for Create().

Done.

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
File chromecast/browser/cast_content_window_linux.h (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.h:30: public
CastVSyncSettings::Observer {
On 2017/01/11 20:47:55, Wez wrote:
> IIUC you only ever create this class via Create(), so there is no need for
this
> .h file - you can move the class definition into cast_content_window_linux.cc
> and avoid it being publicly visible.

Made constructor private, using CastContentWindow as friend class. See comment
in Android implementation file.

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.h:46: const MediaPlayerId& id)
override;
On 2017/01/11 20:47:54, Wez wrote:
> nit: Why did you move this? It reads more naturally to see Started before
> Stopped! :)

Done.

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.h:56:
std::unique_ptr<aura::WindowTreeHost> window_tree_host_;
On 2017/01/11 20:47:55, Wez wrote:
> Rather than have this class' impl become a no-op in non-Aura builds, now that
> you have an abstract interface you could have Create() simply return null, and
> if necessary provide a no-op fallback implementation of the API to use in that
> case.

We can't quite do that. Consumers of CastContentWindow also depend on the
"CreateWebContents" interface to configure the WebContents appropriate for the
platform.

[derekjchow1, 2017-01-11 22:01:01]

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

[commit-bot: I haz the power, 2017-01-11 22:02:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[derekjchow1, 2017-01-11 22:18:58]

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

[commit-bot: I haz the power, 2017-01-11 22:20:53]

Dry run: 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-01-11 22:50:50]

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

[commit-bot: I haz the power, 2017-01-11 22:50:51]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[sadrul, 2017-01-12 02:55:13]

On 2017/01/11 21:54:34, derekjchow1 wrote:
> Thanks for the comments Wez.
> 
> +sadrul, could you check if our new dependency on ui/events is ok?

Yep. lgtm

[derekjchow1, 2017-01-12 17:50:11]

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

[commit-bot: I haz the power, 2017-01-12 17:50:47]

Dry run: 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-01-12 18:53:24]

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

[commit-bot: I haz the power, 2017-01-12 18:53:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[derekjchow1, 2017-01-12 20:56:36]

The CQ bit was checked by derekjchow@chromium.org

[derekjchow1, 2017-01-12 20:56:36]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
alokp@chromium.org, byungchul@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2570623003/#ps280001
(title: "Fix browser test")

[commit-bot: I haz the power, 2017-01-12 20:57:21]

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

[Wez, 2017-01-12 21:04:29]

LGTM FWIW - thanks for making those changes. :)

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/and...
File chromecast/browser/android/cast_content_window_android.h (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/and...
chromecast/browser/android/cast_content_window_android.h:32: explicit
CastContentWindowAndroid(CastContentWindow::Delegate* delegate);
On 2017/01/11 21:54:33, derekjchow1 wrote:
> On 2017/01/11 20:47:54, Wez wrote:
> > As noted in the Linux impl, it doesn't seem that this class needs to be
> publicly
> > visible - you can move the definition into the .cc and rely on callers to
> > Create() it, I think?
> 
> Your point on constructor visibility is valid, but I think the class
definition
> should stay visible in it's own header file (at least for the Android case).
The
> key reason is we need the RegisterJni function to remain public, and I'd
rather
> keep this function as a static class method to make it obvious what Jni
> functions are being registered.
> 
> In terms of constructor visibility, I've made the constructor private and
added
> CastContentWindow as a friend class. This way it can only be constructed via
> CastContentWindow::Create. For consistency between platform implementations
I've
> done the same thing to CastContentWindowLinux.
> 
> Does this sound ok?

Yes, SGTM - thanks. :)

(If you would prefer to avoid friending, you could instead have a static
Create() directly on this impl class, with an identical signature, that the
generic one simply calls on to.)

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
File chromecast/browser/cast_content_window.h (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window.h:37: CastContentWindow::Delegate*
delegate);
On 2017/01/11 21:54:34, derekjchow1 wrote:
> On 2017/01/11 20:47:54, Wez wrote:
> > nit: Please be specific about the lifetime requirement on |delegate|, e.g. I
> > assume it must out-live CastContentWindow?
> > 
> > Is it beneficial to have |delegate| be a non-owned interface like this, or
> could
> > it be an owned interface, i.e. the Create() call would receive a
> > unique_ptr<Delegate> rather than bare-pointer?
> 
> Added comment about |delegate| outliving CastContentWindow.
> 
> The delegate interface should be non-owning. Internally, the parent object of
> CastContentWindow will pass itself as the delegate to be notified when the
> window is destroyed or keys are pressed.

Acknowledged.

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
File chromecast/browser/cast_content_window_linux.h (right):

https://codereview.chromium.org/2570623003/diff/220001/chromecast/browser/cas...
chromecast/browser/cast_content_window_linux.h:56:
std::unique_ptr<aura::WindowTreeHost> window_tree_host_;
On 2017/01/11 21:54:34, derekjchow1 wrote:
> On 2017/01/11 20:47:55, Wez wrote:
> > Rather than have this class' impl become a no-op in non-Aura builds, now
that
> > you have an abstract interface you could have Create() simply return null,
and
> > if necessary provide a no-op fallback implementation of the API to use in
that
> > case.
> 
> We can't quite do that. Consumers of CastContentWindow also depend on the
> "CreateWebContents" interface to configure the WebContents appropriate for the
> platform.

Aha, I see!  Worth mention in the bug you have filed for cleaning up
createWebContents() that this file can be cleaned up at that time as well?

[commit-bot: I haz the power, 2017-01-12 21:04:31]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1484254596328920,
"parent_rev": "ea4f0f86c0606655aed4ed99b76e6bcbc179d64a", "commit_rev":
"504caf2ab4aa7bd4689957423b352ea135508f17"}

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

Description was changed from

==========
[Chromecast] Turn CastContentWindow into an abstract interface.

CastContentWindowImpl is the implementation of CastContentWindow
now. Add a CastContentWindowAndroid specific for Android platform.

BUG=NONE
TEST=cast_shell_apk

Change-Id: I6092f733b54638256bd3028ac3d889f6b52a02fe
==========

to

==========
[Chromecast] Turn CastContentWindow into an abstract interface.

CastContentWindowImpl is the implementation of CastContentWindow
now. Add a CastContentWindowAndroid specific for Android platform.

BUG=NONE
TEST=cast_shell_apk

Change-Id: I6092f733b54638256bd3028ac3d889f6b52a02fe
Review-Url: https://codereview.chromium.org/2570623003
Cr-Commit-Position: refs/heads/master@{#443358}
Committed:
https://chromium.googlesource.com/chromium/src/+/504caf2ab4aa7bd4689957423b35...
==========

[commit-bot: I haz the power, 2017-01-12 21:05:08]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/504caf2ab4aa7bd4689957423b35...