Refactor ContentViewClient (1/6)

Refactor ContentViewClient (1/6)

This aims to break down the interface ContentViewClient
and move its methods to more specific ones according
to their functionalities. Please see the bug description
for more details.

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172
NOTRY=true
NOPRESUBMIT=true

Committed: https://crrev.com/517f915d980c42abe234e5614fd8a7d284d5f890
Cr-Commit-Position: refs/heads/master@{#421750}

[Jinsuk Kim, 2016-09-22 00:47:52]

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

[commit-bot: I haz the power, 2016-09-22 00:48:14]

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-09-22 00:58:43]

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

[commit-bot: I haz the power, 2016-09-22 00:58:44]

Dry run: 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...)

[Jinsuk Kim, 2016-09-22 02:11:34]

Patchset #3 (id:40001) has been deleted

[Jinsuk Kim, 2016-09-22 02:15:03]

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

[commit-bot: I haz the power, 2016-09-22 02:15:25]

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-09-22 02:25:30]

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

[commit-bot: I haz the power, 2016-09-22 02:25:31]

Dry run: 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...)

[Jinsuk Kim, 2016-09-23 03:15:19]

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

[commit-bot: I haz the power, 2016-09-23 03:15:40]

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-09-23 03:24:29]

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

[commit-bot: I haz the power, 2016-09-23 03:24:29]

Dry run: 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...)

[Jinsuk Kim, 2016-09-26 04:14:01]

Patchset #6 (id:120001) has been deleted

[Jinsuk Kim, 2016-09-26 04:14:17]

Patchset #2 (id:20001) has been deleted

[Jinsuk Kim, 2016-09-26 04:14:36]

Patchset #2 (id:60001) has been deleted

[Jinsuk Kim, 2016-09-26 04:14:50]

Patchset #2 (id:80001) has been deleted

[Jinsuk Kim, 2016-09-26 04:15:02]

Patchset #2 (id:100001) has been deleted

[Jinsuk Kim, 2016-09-26 07:11:47]

Description was changed from

==========
Refactor ContentViewClient (1/6)

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172
==========

to

==========
Refactor ContentViewClient (1/6)

This aims to break down the interface ContentViewClient
and move its methods to more specific ones according
to their functionalities. Please see the bug description
for more details.

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172
==========

[Jinsuk Kim, 2016-09-26 07:17:07]

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

[commit-bot: I haz the power, 2016-09-26 07:17:20]

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-09-26 07:26:08]

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

[commit-bot: I haz the power, 2016-09-26 07:26:09]

Dry run: 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...)

[Jinsuk Kim, 2016-09-26 10:47:01]

jinsukkim@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2016-09-26 21:25:25]

reviewed android_webview, content, components

mostly minor things

https://codereview.chromium.org/2353063005/diff/140001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/2353063005/diff/140001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:292:
public void enterFullscreen() {
private

https://codereview.chromium.org/2353063005/diff/140001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:316:
public void exitFullscreen() {
private

https://codereview.chromium.org/2353063005/diff/140001/components/web_content...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java
(right):

https://codereview.chromium.org/2353063005/diff/140001/components/web_content...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:158:
public ContentVideoViewEmbedder getContentVideoViewEmbedder() {
is this needed at all?

actually this actually raises another question, is chrome also subclassing this?
then maybe the native plumbing can be shared in this component instead?

https://codereview.chromium.org/2353063005/diff/140001/content/browser/androi...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/2353063005/diff/140001/content/browser/androi...
content/browser/android/content_video_view.cc:165: JavaObjectWeakGlobalRef
ContentVideoView::CreateJavaObject(
issue with existing code, so doesn't need to be fixed here

this is broken. afaict nothing is keeping a strong ref to this (during creation
at least), so this could get collected at any time

https://codereview.chromium.org/2353063005/diff/140001/content/browser/androi...
File content/browser/android/content_video_view.h (right):

https://codereview.chromium.org/2353063005/diff/140001/content/browser/androi...
content/browser/android/content_video_view.h:17: namespace
web_contents_delegate_android {
this is unused (also layering violation?)

https://codereview.chromium.org/2353063005/diff/140001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/2353063005/diff/140001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.cc:217: if
(WebContentsDelegate* delegate = web_contents_->GetDelegate())
style: need braces if the body is more than one line

https://codereview.chromium.org/2353063005/diff/140001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.cc:455:
web_contents()->GetDelegate()->GetContentVideoViewEmbedder());
null check GetDelegate

https://codereview.chromium.org/2353063005/diff/140001/content/browser/media/...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/2353063005/diff/140001/content/browser/media/...
content/browser/media/android/browser_surface_view_manager.cc:90:
web_contents->GetDelegate()->GetContentVideoViewEmbedder(),
null check GetDelegate

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:116:
private static final ContentVideoViewEmbedder NULL_VIDEO_EMBEDDER =
why is needed now, but not before? seems like native side has a null check for
null embedder already that it won't even call into java, so this should never
happen?

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:142:
mEmbedder = (embedder != null) ? embedder : NULL_VIDEO_EMBEDDER;
nit: no need for brackets

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:495:
public ContentViewCore(Context context) {
remove this and fix all callers?

https://codereview.chromium.org/2353063005/diff/140001/content/public/browser...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/2353063005/diff/140001/content/public/browser...
content/public/browser/web_contents_delegate.cc:214: NOTREACHED() <<
"Implementation must be provided through subclass.";
generally content allow embedders to selectivey override methods, plus you
already have null checks I think, so just remove this line?

[Jinsuk Kim, 2016-09-27 10:05:47]

Patchset #3 (id:160001) has been deleted

[Jinsuk Kim, 2016-09-27 10:08:59]

https://codereview.chromium.org/2353063005/diff/140001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java
(right):

https://codereview.chromium.org/2353063005/diff/140001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:292:
public void enterFullscreen() {
On 2016/09/26 21:25:24, boliu wrote:
> private

Done.

https://codereview.chromium.org/2353063005/diff/140001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegateAdapter.java:316:
public void exitFullscreen() {
On 2016/09/26 21:25:24, boliu wrote:
> private

Done.

https://codereview.chromium.org/2353063005/diff/140001/components/web_content...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java
(right):

https://codereview.chromium.org/2353063005/diff/140001/components/web_content...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:158:
public ContentVideoViewEmbedder getContentVideoViewEmbedder() {
On 2016/09/26 21:25:24, boliu wrote:
> is this needed at all?
> 
> actually this actually raises another question, is chrome also subclassing
this?
> then maybe the native plumbing can be shared in this component instead?

WDITOT - moved the native plumbing of both chrome/webview to this class.

https://codereview.chromium.org/2353063005/diff/140001/content/browser/androi...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/2353063005/diff/140001/content/browser/androi...
content/browser/android/content_video_view.cc:165: JavaObjectWeakGlobalRef
ContentVideoView::CreateJavaObject(
On 2016/09/26 21:25:24, boliu wrote:
> issue with existing code, so doesn't need to be fixed here
> 
> this is broken. afaict nothing is keeping a strong ref to this (during
creation
> at least), so this could get collected at any time

Acknowledged. I'll leave a TODO for a reminder before landing.

https://codereview.chromium.org/2353063005/diff/140001/content/browser/androi...
File content/browser/android/content_video_view.h (right):

https://codereview.chromium.org/2353063005/diff/140001/content/browser/androi...
content/browser/android/content_video_view.h:17: namespace
web_contents_delegate_android {
On 2016/09/26 21:25:24, boliu wrote:
> this is unused (also layering violation?)

Done.

https://codereview.chromium.org/2353063005/diff/140001/content/browser/media/...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/2353063005/diff/140001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.cc:217: if
(WebContentsDelegate* delegate = web_contents_->GetDelegate())
On 2016/09/26 21:25:25, boliu wrote:
> style: need braces if the body is more than one line

Done.

https://codereview.chromium.org/2353063005/diff/140001/content/browser/media/...
content/browser/media/android/browser_media_player_manager.cc:455:
web_contents()->GetDelegate()->GetContentVideoViewEmbedder());
On 2016/09/26 21:25:25, boliu wrote:
> null check GetDelegate

Done.

https://codereview.chromium.org/2353063005/diff/140001/content/browser/media/...
File content/browser/media/android/browser_surface_view_manager.cc (right):

https://codereview.chromium.org/2353063005/diff/140001/content/browser/media/...
content/browser/media/android/browser_surface_view_manager.cc:90:
web_contents->GetDelegate()->GetContentVideoViewEmbedder(),
On 2016/09/26 21:25:25, boliu wrote:
> null check GetDelegate

Done.

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:116:
private static final ContentVideoViewEmbedder NULL_VIDEO_EMBEDDER =
On 2016/09/26 21:25:25, boliu wrote:
> why is needed now, but not before? seems like native side has a null check for
> null embedder already that it won't even call into java, so this should never
> happen?

OverlayPanelContent.java was passing an empty impl of ContentVideoViewEmbedder -
in fact this has a bug but fixed in the new patch - I think it would be better
to have the definition here rather than adding native part, going up all the way
to Java layer only to get the empty impl. Also there can be other instances
which also injects an empty impl, for which having it here will be better.

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:142:
mEmbedder = (embedder != null) ? embedder : NULL_VIDEO_EMBEDDER;
On 2016/09/26 21:25:25, boliu wrote:
> nit: no need for brackets

Done.

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:495:
public ContentViewCore(Context context) {
On 2016/09/26 21:25:25, boliu wrote:
> remove this and fix all callers?

Done.

https://codereview.chromium.org/2353063005/diff/140001/content/public/browser...
File content/public/browser/web_contents_delegate.cc (right):

https://codereview.chromium.org/2353063005/diff/140001/content/public/browser...
content/public/browser/web_contents_delegate.cc:214: NOTREACHED() <<
"Implementation must be provided through subclass.";
On 2016/09/26 21:25:25, boliu wrote:
> generally content allow embedders to selectivey override methods, plus you
> already have null checks I think, so just remove this line?

Should have been removed. Done.

[boliu, 2016-09-27 17:24:44]

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:116:
private static final ContentVideoViewEmbedder NULL_VIDEO_EMBEDDER =
On 2016/09/27 10:08:59, Jinsuk wrote:
> On 2016/09/26 21:25:25, boliu wrote:
> > why is needed now, but not before? seems like native side has a null check
for
> > null embedder already that it won't even call into java, so this should
never
> > happen?
> 
> OverlayPanelContent.java was passing an empty impl of ContentVideoViewEmbedder
-
> in fact this has a bug but fixed in the new patch - I think it would be better
> to have the definition here rather than adding native part, going up all the
way
> to Java layer only to get the empty impl. Also there can be other instances
> which also injects an empty impl, for which having it here will be better.

I can't tell if you agree with me or not..

native ContentVideoView::CreateJavaObject already has a null check for the
embedder, which means in the java ContentVideoView constructor, the embedder can
never be null, so this NULL_VIDEO_EMBEDDER will never be needed.

Or did I miss something?

[Jinsuk Kim, 2016-09-27 22:17:44]

On 2016/09/27 17:24:44, boliu wrote:
>
https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
> (right):
> 
>
https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:116:
> private static final ContentVideoViewEmbedder NULL_VIDEO_EMBEDDER =
> On 2016/09/27 10:08:59, Jinsuk wrote:
> > On 2016/09/26 21:25:25, boliu wrote:
> > > why is needed now, but not before? seems like native side has a null check
> for
> > > null embedder already that it won't even call into java, so this should
> never
> > > happen?
> > 
> > OverlayPanelContent.java was passing an empty impl of
ContentVideoViewEmbedder
> -
> > in fact this has a bug but fixed in the new patch - I think it would be
better
> > to have the definition here rather than adding native part, going up all the
> way
> > to Java layer only to get the empty impl. Also there can be other instances
> > which also injects an empty impl, for which having it here will be better.
> 
> I can't tell if you agree with me or not..
> 
> native ContentVideoView::CreateJavaObject already has a null check for the
> embedder, which means in the java ContentVideoView constructor, the embedder
can
> never be null, so this NULL_VIDEO_EMBEDDER will never be needed.
> 
> Or did I miss something?

You are right; put the null check out of habit... I think I should remove it
to allow for nulled-out embedder object. That will make the change functionally
identical before and after the change. Does it make sense?

[boliu, 2016-09-27 22:19:00]

On 2016/09/27 22:17:44, Jinsuk wrote:
> On 2016/09/27 17:24:44, boliu wrote:
> >
>
https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2353063005/diff/140001/content/public/android...
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:116:
> > private static final ContentVideoViewEmbedder NULL_VIDEO_EMBEDDER =
> > On 2016/09/27 10:08:59, Jinsuk wrote:
> > > On 2016/09/26 21:25:25, boliu wrote:
> > > > why is needed now, but not before? seems like native side has a null
check
> > for
> > > > null embedder already that it won't even call into java, so this should
> > never
> > > > happen?
> > > 
> > > OverlayPanelContent.java was passing an empty impl of
> ContentVideoViewEmbedder
> > -
> > > in fact this has a bug but fixed in the new patch - I think it would be
> better
> > > to have the definition here rather than adding native part, going up all
the
> > way
> > > to Java layer only to get the empty impl. Also there can be other
instances
> > > which also injects an empty impl, for which having it here will be better.
> > 
> > I can't tell if you agree with me or not..
> > 
> > native ContentVideoView::CreateJavaObject already has a null check for the
> > embedder, which means in the java ContentVideoView constructor, the embedder
> can
> > never be null, so this NULL_VIDEO_EMBEDDER will never be needed.
> > 
> > Or did I miss something?
> 
> You are right; put the null check out of habit... I think I should remove it
> to allow for nulled-out embedder object. That will make the change
functionally
> identical before and after the change. Does it make sense?

sure

lgtm after that

[Jinsuk Kim, 2016-09-27 22:20:26]

nyquist@: Please review changes in chrome/

[Jinsuk Kim, 2016-09-27 22:23:34]

jinsukkim@chromium.org changed reviewers:
+ nyquist@chromium.org

[Jinsuk Kim, 2016-09-27 22:33:57]

jinsukkim@chromium.org changed reviewers:
+ creis@chromium.org, halliwell@chromium.org

[Jinsuk Kim, 2016-09-27 22:33:58]

creis@chromium.org: Please review changes in content/public (2 new interface in
WebContentsDelegate) and content/shell 

halliwell@chromium.org: Please review changes in chromecast/

[boliu, 2016-09-27 23:28:45]

scanned through chrome and chromecast too, I probably shoulda done that in the
first place. nothing major, just noticing probable existing bugs...

https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:213:
return new ContentViewCore(activity, "");
I wonder if this is an existing bug, that this CVC doesn't have the version
number?

https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
(right):

https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:124:
mUiCVC = new ContentViewCore(mActivity, "");
ditto, this one has to be wrong..

https://codereview.chromium.org/2353063005/diff/180001/chrome/browser/android...
File chrome/browser/android/tab_web_contents_delegate_android.h (right):

https://codereview.chromium.org/2353063005/diff/180001/chrome/browser/android...
chrome/browser/android/tab_web_contents_delegate_android.h:74: 
random new line

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java
(right):

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java:132:
mContentViewCore = new ContentViewCore(context, "");
ditto

[Simeon, 2016-09-28 00:24:51]

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

[Simeon, 2016-09-28 00:24:52]

I was summoned by halliwell@ to help look at the Java stuff in chromecast/

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
(right):

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:120:
shellView.setContentVideoViewEmbedder(
I think a better way to do this is to pass the ContentVideoViewEmbedder into
launchCastWindow().

So we could have:

mNativeCastWindow = mCastWindowManager.launchCastWindow(url, new
ActivityContentVideoViewEmbedder(this));

Then, in cast_window_manager.cc:

jlong LaunchCastWindow(JNIEnv* env,
                       const JavaParamRef<jclass>& clazz,
                       const JavaParamRef<jstring>& jurl,
                       const JavaParamRef<jobject>&
j_content_video_view_embedder) {
  GURL url(base::android::ConvertJavaStringToUTF8(env, jurl));
  return reinterpret_cast<jlong>(
      CastWindowAndroid::CreateNewWindow(
          CastBrowserProcess::GetInstance()->browser_context(),
          url, j_content_video_view_embedder));
}


And in cast_window_android.cc:

// static
CastWindowAndroid* CastWindowAndroid::CreateNewWindow(
    content::BrowserContext* browser_context,
    const GURL& url,
    const JavaParamRef<jobject>& j_content_video_view_embedder) {
  CastWindowAndroid* window_android = new CastWindowAndroid(browser_context,
j_content_video_view_embedder);
  window_android->Initialize();

  if (!url.is_empty())
    window_android->LoadURL(url);

  content::RenderWidgetHostView* rwhv =
      window_android->web_contents_->GetRenderWidgetHostView();
  if (rwhv) {
    rwhv->SetBackgroundColor(SK_ColorBLACK);
  }

  return window_android;
}

CastWindowAndroid::CastWindowAndroid(
    content::BrowserContext* browser_context, 
    const JavaParamRef<jobject>& j_content_video_view_embedder)
    : browser_context_(browser_context),
      content_window_(new CastContentWindow),
      j_content_video_view_embedder_(j_content_video_view_embedder),
      weak_factory_(this) {
}
...
base::android::ScopedJavaLocalRef<jobject>
CastWindowAndroid::GetContentVideoViewEmbedder() {
  return j_content_video_view_embedder_;
}

And of course, create a private member for j_content_video_view_embedder_ in
cast_window_android.h.

(I'm not 100% sure it's right to use JavaParamRef as I did above, but it looks
to me like the lifetimes of these objects fit together so this works. I might be
missing some fundamental reason we can't do it this way, of course.)

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:121:
new ActivityContentVideoViewEmbedder(CastShellActivity.this));
Now that this line isn't in an anonymous class, we can use "this" instead of
"CastShellActivity.this"

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java
(right):

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java:166:
public ContentVideoViewEmbedder getContentVideoViewEmbedder() {
This does not need to be public.

But if you manage to get my suggestion in CastShellActivity.java to work, you
won't need any changes in CastWindowAndroid.java.

[Jinsuk Kim, 2016-09-28 00:49:00]

On 2016/09/28 00:24:52, Simeon wrote:
> I was summoned by halliwell@ to help look at the Java stuff in chromecast/
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
> File
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
> (right):
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:120:
> shellView.setContentVideoViewEmbedder(
> I think a better way to do this is to pass the ContentVideoViewEmbedder into
> launchCastWindow().
> 
> So we could have:
> 
> mNativeCastWindow = mCastWindowManager.launchCastWindow(url, new
> ActivityContentVideoViewEmbedder(this));
> 
> Then, in cast_window_manager.cc:
> 
> jlong LaunchCastWindow(JNIEnv* env,
>                        const JavaParamRef<jclass>& clazz,
>                        const JavaParamRef<jstring>& jurl,
>                        const JavaParamRef<jobject>&
> j_content_video_view_embedder) {
>   GURL url(base::android::ConvertJavaStringToUTF8(env, jurl));
>   return reinterpret_cast<jlong>(
>       CastWindowAndroid::CreateNewWindow(
>           CastBrowserProcess::GetInstance()->browser_context(),
>           url, j_content_video_view_embedder));
> }
> 
> 
> And in cast_window_android.cc:
> 
> // static
> CastWindowAndroid* CastWindowAndroid::CreateNewWindow(
>     content::BrowserContext* browser_context,
>     const GURL& url,
>     const JavaParamRef<jobject>& j_content_video_view_embedder) {
>   CastWindowAndroid* window_android = new CastWindowAndroid(browser_context,
> j_content_video_view_embedder);
>   window_android->Initialize();
> 
>   if (!url.is_empty())
>     window_android->LoadURL(url);
> 
>   content::RenderWidgetHostView* rwhv =
>       window_android->web_contents_->GetRenderWidgetHostView();
>   if (rwhv) {
>     rwhv->SetBackgroundColor(SK_ColorBLACK);
>   }
> 
>   return window_android;
> }
> 
> CastWindowAndroid::CastWindowAndroid(
>     content::BrowserContext* browser_context, 
>     const JavaParamRef<jobject>& j_content_video_view_embedder)
>     : browser_context_(browser_context),
>       content_window_(new CastContentWindow),
>       j_content_video_view_embedder_(j_content_video_view_embedder),
>       weak_factory_(this) {
> }
> ...
> base::android::ScopedJavaLocalRef<jobject>
> CastWindowAndroid::GetContentVideoViewEmbedder() {
>   return j_content_video_view_embedder_;
> }
> 
> And of course, create a private member for j_content_video_view_embedder_ in
> cast_window_android.h.
> 
> (I'm not 100% sure it's right to use JavaParamRef as I did above, but it looks
> to me like the lifetimes of these objects fit together so this works. I might
be
> missing some fundamental reason we can't do it this way, of course.)
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:121:
> new ActivityContentVideoViewEmbedder(CastShellActivity.this));
> Now that this line isn't in an anonymous class, we can use "this" instead of
> "CastShellActivity.this"
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
> File
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java
> (right):
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java:166:
> public ContentVideoViewEmbedder getContentVideoViewEmbedder() {
> This does not need to be public.
> 
> But if you manage to get my suggestion in CastShellActivity.java to work, you
> won't need any changes in CastWindowAndroid.java.

Thanks for the code snippet for suggestion!
It makes me see another potential (hopefully simpler) approach. Is the |context|
of CastWindowAndroid always associated with an activity (CastShellActivity in
this case)? Then I can create the embedder instance in
CastWindowAndroid.getContentVideoViewEmbedder directly upon request, without
having to pass it from CastShellActivity.

[Jinsuk Kim, 2016-09-28 01:09:11]

> > 
> > And of course, create a private member for j_content_video_view_embedder_ in
> > cast_window_android.h.
> > 

I'd like to avoid storing Java obj in native if possible - be it weak or strong.
From what I've seen, this has been an easy road to memory leak.

[boliu, 2016-09-28 01:14:31]

On 2016/09/28 01:09:11, Jinsuk wrote:
> > > 
> > > And of course, create a private member for j_content_video_view_embedder_
in
> > > cast_window_android.h.
> > > 
> 
> I'd like to avoid storing Java obj in native if possible - be it weak or
strong.
> From what I've seen, this has been an easy road to memory leak.

fwiw gc leaks is only a big issue in android webview and any code that webview
uses, which includes content

I don't know the specifics here, but holding a strong reference in native in
chrome/ or chromecast/ isn't necessarily a red flag

[Jinsuk Kim, 2016-09-28 01:32:35]

On 2016/09/28 01:14:31, boliu wrote:
> On 2016/09/28 01:09:11, Jinsuk wrote:
> > > > 
> > > > And of course, create a private member for
j_content_video_view_embedder_
> in
> > > > cast_window_android.h.
> > > > 
> > 
> > I'd like to avoid storing Java obj in native if possible - be it weak or
> strong.
> > From what I've seen, this has been an easy road to memory leak.
> 
> fwiw gc leaks is only a big issue in android webview and any code that webview
> uses, which includes content
> 
> I don't know the specifics here, but holding a strong reference in native in
> chrome/ or chromecast/ isn't necessarily a red flag

Point taken. Anyway it is an unnecessary reference to the object which
can be created on the fly and passed to ContentVideoView. I was holding it
in Java CastWindowAndroid. Now I'm trying to see if the reference can be removed
at all now that it is changing the location to native.

[Jinsuk Kim, 2016-09-28 03:58:52]

https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:213:
return new ContentViewCore(activity, "");
On 2016/09/27 23:28:45, boliu wrote:
> I wonder if this is an existing bug, that this CVC doesn't have the version
> number?

Maybe a harmless bug. Used chrome version.

https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
(right):

https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:124:
mUiCVC = new ContentViewCore(mActivity, "");
On 2016/09/27 23:28:45, boliu wrote:
> ditto, this one has to be wrong..

ChromeVersionInfo.getProductVersion().

https://codereview.chromium.org/2353063005/diff/180001/chrome/browser/android...
File chrome/browser/android/tab_web_contents_delegate_android.h (right):

https://codereview.chromium.org/2353063005/diff/180001/chrome/browser/android...
chrome/browser/android/tab_web_contents_delegate_android.h:74: 
On 2016/09/27 23:28:45, boliu wrote:
> random new line

Removed.

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
(right):

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:120:
shellView.setContentVideoViewEmbedder(
On 2016/09/28 00:24:52, Simeon wrote:
> I think a better way to do this is to pass the ContentVideoViewEmbedder into
> launchCastWindow().
> 
> So we could have:
> 
> mNativeCastWindow = mCastWindowManager.launchCastWindow(url, new
> ActivityContentVideoViewEmbedder(this));
> 
> Then, in cast_window_manager.cc:
> 
> jlong LaunchCastWindow(JNIEnv* env,
>                        const JavaParamRef<jclass>& clazz,
>                        const JavaParamRef<jstring>& jurl,
>                        const JavaParamRef<jobject>&
> j_content_video_view_embedder) {
>   GURL url(base::android::ConvertJavaStringToUTF8(env, jurl));
>   return reinterpret_cast<jlong>(
>       CastWindowAndroid::CreateNewWindow(
>           CastBrowserProcess::GetInstance()->browser_context(),
>           url, j_content_video_view_embedder));
> }
> 
> 
> And in cast_window_android.cc:
> 
> // static
> CastWindowAndroid* CastWindowAndroid::CreateNewWindow(
>     content::BrowserContext* browser_context,
>     const GURL& url,
>     const JavaParamRef<jobject>& j_content_video_view_embedder) {
>   CastWindowAndroid* window_android = new CastWindowAndroid(browser_context,
> j_content_video_view_embedder);
>   window_android->Initialize();
> 
>   if (!url.is_empty())
>     window_android->LoadURL(url);
> 
>   content::RenderWidgetHostView* rwhv =
>       window_android->web_contents_->GetRenderWidgetHostView();
>   if (rwhv) {
>     rwhv->SetBackgroundColor(SK_ColorBLACK);
>   }
> 
>   return window_android;
> }
> 
> CastWindowAndroid::CastWindowAndroid(
>     content::BrowserContext* browser_context, 
>     const JavaParamRef<jobject>& j_content_video_view_embedder)
>     : browser_context_(browser_context),
>       content_window_(new CastContentWindow),
>       j_content_video_view_embedder_(j_content_video_view_embedder),
>       weak_factory_(this) {
> }
> ...
> base::android::ScopedJavaLocalRef<jobject>
> CastWindowAndroid::GetContentVideoViewEmbedder() {
>   return j_content_video_view_embedder_;
> }
> 
> And of course, create a private member for j_content_video_view_embedder_ in
> cast_window_android.h.
> 
> (I'm not 100% sure it's right to use JavaParamRef as I did above, but it looks
> to me like the lifetimes of these objects fit together so this works. I might
be
> missing some fundamental reason we can't do it this way, of course.)

While appreciating your suggestion, I tried a simpler approach not holding a
reference to ContentVideoViewEmbedder after verifying that
CastWindow{Manager|Android} are inflated and associated with the activity
context of CastShellActivity. Please review it and see if it looks okay.

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:121:
new ActivityContentVideoViewEmbedder(CastShellActivity.this));
On 2016/09/28 00:24:52, Simeon wrote:
> Now that this line isn't in an anonymous class, we can use "this" instead of
> "CastShellActivity.this"

Removed.

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java
(right):

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java:132:
mContentViewCore = new ContentViewCore(context, "");
On 2016/09/27 23:28:45, boliu wrote:
> ditto

sanfin@ please see if the product version I picked up is a good fit for this.

https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java:166:
public ContentVideoViewEmbedder getContentVideoViewEmbedder() {
On 2016/09/28 00:24:52, Simeon wrote:
> This does not need to be public.
> 
> But if you manage to get my suggestion in CastShellActivity.java to work, you
> won't need any changes in CastWindowAndroid.java.

Changed to private. I think the context of CastWindowAndroid is activity context
which allows for this casting - used it to create the embedder instance here
instead of having it passed from CastShellActivity.

[Simeon, 2016-09-28 16:12:22]

On 2016/09/28 03:58:52, Jinsuk wrote:
>
https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
> (right):
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:213:
> return new ContentViewCore(activity, "");
> On 2016/09/27 23:28:45, boliu wrote:
> > I wonder if this is an existing bug, that this CVC doesn't have the version
> > number?
> 
> Maybe a harmless bug. Used chrome version.
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
> File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java
> (right):
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chrome/android/java/sr...
> chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:124:
> mUiCVC = new ContentViewCore(mActivity, "");
> On 2016/09/27 23:28:45, boliu wrote:
> > ditto, this one has to be wrong..
> 
> ChromeVersionInfo.getProductVersion().
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chrome/browser/android...
> File chrome/browser/android/tab_web_contents_delegate_android.h (right):
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chrome/browser/android...
> chrome/browser/android/tab_web_contents_delegate_android.h:74: 
> On 2016/09/27 23:28:45, boliu wrote:
> > random new line
> 
> Removed.
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
> File
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
> (right):
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:120:
> shellView.setContentVideoViewEmbedder(
> On 2016/09/28 00:24:52, Simeon wrote:
> > I think a better way to do this is to pass the ContentVideoViewEmbedder into
> > launchCastWindow().
> > 
> > So we could have:
> > 
> > mNativeCastWindow = mCastWindowManager.launchCastWindow(url, new
> > ActivityContentVideoViewEmbedder(this));
> > 
> > Then, in cast_window_manager.cc:
> > 
> > jlong LaunchCastWindow(JNIEnv* env,
> >                        const JavaParamRef<jclass>& clazz,
> >                        const JavaParamRef<jstring>& jurl,
> >                        const JavaParamRef<jobject>&
> > j_content_video_view_embedder) {
> >   GURL url(base::android::ConvertJavaStringToUTF8(env, jurl));
> >   return reinterpret_cast<jlong>(
> >       CastWindowAndroid::CreateNewWindow(
> >           CastBrowserProcess::GetInstance()->browser_context(),
> >           url, j_content_video_view_embedder));
> > }
> > 
> > 
> > And in cast_window_android.cc:
> > 
> > // static
> > CastWindowAndroid* CastWindowAndroid::CreateNewWindow(
> >     content::BrowserContext* browser_context,
> >     const GURL& url,
> >     const JavaParamRef<jobject>& j_content_video_view_embedder) {
> >   CastWindowAndroid* window_android = new CastWindowAndroid(browser_context,
> > j_content_video_view_embedder);
> >   window_android->Initialize();
> > 
> >   if (!url.is_empty())
> >     window_android->LoadURL(url);
> > 
> >   content::RenderWidgetHostView* rwhv =
> >       window_android->web_contents_->GetRenderWidgetHostView();
> >   if (rwhv) {
> >     rwhv->SetBackgroundColor(SK_ColorBLACK);
> >   }
> > 
> >   return window_android;
> > }
> > 
> > CastWindowAndroid::CastWindowAndroid(
> >     content::BrowserContext* browser_context, 
> >     const JavaParamRef<jobject>& j_content_video_view_embedder)
> >     : browser_context_(browser_context),
> >       content_window_(new CastContentWindow),
> >       j_content_video_view_embedder_(j_content_video_view_embedder),
> >       weak_factory_(this) {
> > }
> > ...
> > base::android::ScopedJavaLocalRef<jobject>
> > CastWindowAndroid::GetContentVideoViewEmbedder() {
> >   return j_content_video_view_embedder_;
> > }
> > 
> > And of course, create a private member for j_content_video_view_embedder_ in
> > cast_window_android.h.
> > 
> > (I'm not 100% sure it's right to use JavaParamRef as I did above, but it
looks
> > to me like the lifetimes of these objects fit together so this works. I
might
> be
> > missing some fundamental reason we can't do it this way, of course.)
> 
> While appreciating your suggestion, I tried a simpler approach not holding a
> reference to ContentVideoViewEmbedder after verifying that
> CastWindow{Manager|Android} are inflated and associated with the activity
> context of CastShellActivity. Please review it and see if it looks okay.
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:121:
> new ActivityContentVideoViewEmbedder(CastShellActivity.this));
> On 2016/09/28 00:24:52, Simeon wrote:
> > Now that this line isn't in an anonymous class, we can use "this" instead of
> > "CastShellActivity.this"
> 
> Removed.
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
> File
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java
> (right):
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java:132:
> mContentViewCore = new ContentViewCore(context, "");
> On 2016/09/27 23:28:45, boliu wrote:
> > ditto
> 
> sanfin@ please see if the product version I picked up is a good fit for this.
> 
>
https://codereview.chromium.org/2353063005/diff/180001/chromecast/browser/and...
>
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java:166:
> public ContentVideoViewEmbedder getContentVideoViewEmbedder() {
> On 2016/09/28 00:24:52, Simeon wrote:
> > This does not need to be public.
> > 
> > But if you manage to get my suggestion in CastShellActivity.java to work,
you
> > won't need any changes in CastWindowAndroid.java.
> 
> Changed to private. I think the context of CastWindowAndroid is activity
context
> which allows for this casting - used it to create the embedder instance here
> instead of having it passed from CastShellActivity.

Thanks! I like this version a lot more. The use of
CastWindowAndroid.getContext() and PRODUCT_VERSION look right. chromecast/ lgtm
aside from the nit re: casting CastWindowAndroid.getContext().

[Simeon, 2016-09-28 16:12:36]

https://codereview.chromium.org/2353063005/diff/200001/chromecast/browser/and...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java
(right):

https://codereview.chromium.org/2353063005/diff/200001/chromecast/browser/and...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java:166:
return new ActivityContentVideoViewEmbedder((CastShellActivity) getContext());
I think you should only have to cast getContext() to Activity.

[nyquist, 2016-09-28 17:36:41]

chrome lgtm

[Charlie Reis, 2016-09-28 21:13:45]

RS LGTM for content/public, based on boliu's review.

[halliwell, 2016-09-28 21:37:24]

On 2016/09/28 21:13:45, Charlie Reis wrote:
> RS LGTM for content/public, based on boliu's review.

rs-lgtm for chromecast/

[Jinsuk Kim, 2016-09-29 00:06:34]

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

[commit-bot: I haz the power, 2016-09-29 00:06:58]

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-09-29 00:16:35]

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

[commit-bot: I haz the power, 2016-09-29 00:16:37]

Dry run: 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...)

[Jinsuk Kim, 2016-09-29 03:42:12]

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

[commit-bot: I haz the power, 2016-09-29 03:42: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-09-29 03:52:10]

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

[commit-bot: I haz the power, 2016-09-29 03:52:11]

Dry run: 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...)

[Jinsuk Kim, 2016-09-29 04:43:45]

Patchset #5 (id:220001) has been deleted

[Jinsuk Kim, 2016-09-29 04:43:53]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-09-29 04:43:54]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
nyquist@chromium.org, creis@chromium.org, halliwell@chromium.org,
sanfin@chromium.org
Link to the patchset: https://codereview.chromium.org/2353063005/#ps240001
(title: "cast to activity")

[commit-bot: I haz the power, 2016-09-29 04:44:13]

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-09-29 04:49:56]

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

[commit-bot: I haz the power, 2016-09-29 04:49:58]

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...)

[boliu, 2016-09-29 04:53:08]

On 2016/09/29 04:49:58, commit-bot: I haz the power wrote:
> 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...)

AlertDialog presubmit check should be limited to chrome/ code. You can add
NOTRY=true, assuming all other bots are green

[Jinsuk Kim, 2016-09-29 04:55:10]

Description was changed from

==========
Refactor ContentViewClient (1/6)

This aims to break down the interface ContentViewClient
and move its methods to more specific ones according
to their functionalities. Please see the bug description
for more details.

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172
==========

to

==========
Refactor ContentViewClient (1/6)

This aims to break down the interface ContentViewClient
and move its methods to more specific ones according
to their functionalities. Please see the bug description
for more details.

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172

NOTRY=true
==========

[Jinsuk Kim, 2016-09-29 04:55:27]

The CQ bit was checked by jinsukkim@chromium.org

[commit-bot: I haz the power, 2016-09-29 04:55:48]

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

[boliu, 2016-09-29 05:04:39]

err.... oops, should be NOPRESUBMIT=true

[commit-bot: I haz the power, 2016-09-29 05:05:24]

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

[commit-bot: I haz the power, 2016-09-29 05:05:26]

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...)

[Jinsuk Kim, 2016-09-29 05:07:33]

Description was changed from

==========
Refactor ContentViewClient (1/6)

This aims to break down the interface ContentViewClient
and move its methods to more specific ones according
to their functionalities. Please see the bug description
for more details.

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172

NOTRY=true
==========

to

==========
Refactor ContentViewClient (1/6)

This aims to break down the interface ContentViewClient
and move its methods to more specific ones according
to their functionalities. Please see the bug description
for more details.

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172

NOTRY=true
NOPRESUBMIT=true
==========

[Jinsuk Kim, 2016-09-29 05:07:44]

The CQ bit was checked by jinsukkim@chromium.org

[commit-bot: I haz the power, 2016-09-29 05:07:59]

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-09-29 05:10:13]

Description was changed from

==========
Refactor ContentViewClient (1/6)

This aims to break down the interface ContentViewClient
and move its methods to more specific ones according
to their functionalities. Please see the bug description
for more details.

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172

NOTRY=true
NOPRESUBMIT=true
==========

to

==========
Refactor ContentViewClient (1/6)

This aims to break down the interface ContentViewClient
and move its methods to more specific ones according
to their functionalities. Please see the bug description
for more details.

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172

NOTRY=true
NOPRESUBMIT=true
==========

[commit-bot: I haz the power, 2016-09-29 05:10:15]

Committed patchset #5 (id:240001)

[commit-bot: I haz the power, 2016-09-29 05:15:02]

Description was changed from

==========
Refactor ContentViewClient (1/6)

This aims to break down the interface ContentViewClient
and move its methods to more specific ones according
to their functionalities. Please see the bug description
for more details.

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172

NOTRY=true
NOPRESUBMIT=true
==========

to

==========
Refactor ContentViewClient (1/6)

This aims to break down the interface ContentViewClient
and move its methods to more specific ones according
to their functionalities. Please see the bug description
for more details.

Moves following methods to WebContentsDelegateAndroid:
 - getContentVideoViewEmbedder
 - shouldBlockMediaRequest

Passes through ContentViewCore contstructor:
 - getProductVersion

BUG=620172

NOTRY=true
NOPRESUBMIT=true

Committed: https://crrev.com/517f915d980c42abe234e5614fd8a7d284d5f890
Cr-Commit-Position: refs/heads/master@{#421750}
==========

[commit-bot: I haz the power, 2016-09-29 05:15:04]

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

[Jinsuk Kim, 2016-09-29 05:15:48]

On 2016/09/29 05:04:39, boliu wrote:
> err.... oops, should be NOPRESUBMIT=true

Thanks for the tip!