Refactoring ContentVideoViewContextDelegate.java

Main changes are

- Rename ContentVideoViewContextDelegate to ContentVideoViewClient, instead of having the getContext() method, passing the context to ContentVideoView when it is created.

- Add getContentVideoViewClient method in ContentViewClient, so it will be created when it actually used.



BUG=http://b/8315237

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204353

[joth, 2013-04-05 01:56:40]

https://codereview.chromium.org/13669003/diff/2001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/13669003/diff/2001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:25:
import android.webkit.WebChromeClient.CustomViewCallback;
rather than import, could you use 'WebChromeClient.CustomViewCallback' where
needed?

https://codereview.chromium.org/13669003/diff/2001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:218:
public void onDestroyContentVideoView() {
this needs to call onHideCustomView. (is there an instrumentation test to catch
this?)

https://codereview.chromium.org/13669003/diff/2001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:366:
return mContentVideoViewClient;
I'm tempted to remove the mContentVideoViewClient member field and just return
new instance everytime, so we can have the real implementation consistent with
the test code.

This does mean that if the client app does not hold onto the CustomViewCallback
instance we pass them, the ContentVideoViewClient instance will also be GCed
meaning they would not in turn receive the onHideCustomView call.... hmm subtle.

ANother option is to enforce this in base ContentViewClient: have a public final
method there that calls a protected getContentVideoViewClient(), and then caches
the result. (template method pattern).
This means the two lifetimes are necessarily the same, so at that point we could
just remove ContentVideoViewClient and push its methods straight onto
ContentViewClient. :-/

alternatives?

https://codereview.chromium.org/13669003/diff/2001/content/browser/android/co...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/13669003/diff/2001/content/browser/android/co...
content/browser/android/content_video_view.cc:34: void
ContentVideoView::CreateContentVideoView(
what if j_content_video_view_ were not null? I think we should call
DestroyContentVideoView() here first to be safe?
(maybe worth a tests case to cover that too)

- we could make this safer by having a wrapper object that holds the java ref
and also does Java_ContentVideoView_destroyContentVideoView() in its destructor,
and then just hold a scoped_ptr to that wrapper inside this class.

Or even better... why not call Java_ContentVideoView_createContentVideoView from
ContentVideoView c'tor, and destroyContentVideoView from the d'tor, and remove
these two explicit create/destroy methods? that way, the native and java objects
have the same lifetime, and it's much easier to see what's going on.

https://codereview.chromium.org/13669003/diff/2001/content/browser/android/co...
File content/browser/android/content_video_view.h (right):

https://codereview.chromium.org/13669003/diff/2001/content/browser/android/co...
content/browser/android/content_video_view.h:74:
base::android::ScopedJavaGlobalRef<jobject> j_content_video_view_;
this is a problem: holding a strong ref to the java object will stop it being
GC'd so long as we're in "full screen" (which may mean nothing, depending on how
the app is structured). this in turn may mean the WebView itself never gets
GCed. (As AwContentsClient indirectly references it).

So this should be a WeakJavaRef thingy, and all the usage of it use the "promote
to ScopedLocalRef and check that for null" pattern.

Although: it's only by fixing this problem that you will cause the
possibly-too-early GC of ContentVideoViewClient problem I mentioned above, so
perhaps best to solve this piece in a separate CL. (could raise a bug for it, it
should be a very rare corner leak case so no immediate priority)

[michaelbai, 2013-04-05 21:06:56]

https://codereview.chromium.org/13669003/diff/2001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/13669003/diff/2001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:25:
import android.webkit.WebChromeClient.CustomViewCallback;
On 2013/04/05 01:56:40, joth wrote:
> rather than import, could you use 'WebChromeClient.CustomViewCallback' where
> needed?

Done.

https://codereview.chromium.org/13669003/diff/2001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:218:
public void onDestroyContentVideoView() {

Right, AwContentsClientFullScreenVideoTest test this, but I didn't run it


On 2013/04/05 01:56:40, joth wrote:
> this needs to call onHideCustomView. (is there an instrumentation test to
catch
> this?)

https://codereview.chromium.org/13669003/diff/2001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:366:
return mContentVideoViewClient;
I may misunderstood you goal here. It seemed that you don't want to call
onHideCustomView if the client app don't hold the CustomViewCallback we gave
them. If so, return new instance every time will not help on it except we have a
weak reference of CustomViewCallback, even though the goal is not guaranteed, it
depends on when the GC happen. 

So what's your goal?

On 2013/04/05 01:56:40, joth wrote:
> I'm tempted to remove the mContentVideoViewClient member field and just return
> new instance everytime, so we can have the real implementation consistent with
> the test code.
> 
> This does mean that if the client app does not hold onto the
CustomViewCallback
> instance we pass them, the ContentVideoViewClient instance will also be GCed
> meaning they would not in turn receive the onHideCustomView call.... hmm
subtle.
> 
> ANother option is to enforce this in base ContentViewClient: have a public
final
> method there that calls a protected getContentVideoViewClient(), and then
caches
> the result. (template method pattern).
> This means the two lifetimes are necessarily the same, so at that point we
could
> just remove ContentVideoViewClient and push its methods straight onto
> ContentViewClient. :-/
> 
> alternatives?

https://codereview.chromium.org/13669003/diff/2001/content/browser/android/co...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/13669003/diff/2001/content/browser/android/co...
content/browser/android/content_video_view.cc:34: void
ContentVideoView::CreateContentVideoView(
On 2013/04/05 01:56:40, joth wrote:
> what if j_content_video_view_ were not null? I think we should call
> DestroyContentVideoView() here first to be safe?
> (maybe worth a tests case to cover that too)
> 

if j_content_video_view_ was not null, the same object will be returned (see
java file), the reference count of the object was just decreased/increased
again. it seemed no side effect.


> - we could make this safer by having a wrapper object that holds the java ref
> and also does Java_ContentVideoView_destroyContentVideoView() in its
destructor,
> and then just hold a scoped_ptr to that wrapper inside this class.
> 
> Or even better... why not call Java_ContentVideoView_createContentVideoView
from
> ContentVideoView c'tor, and destroyContentVideoView from the d'tor, and remove
> these two explicit create/destroy methods? that way, the native and java
objects
> have the same lifetime, and it's much easier to see what's going on.

Talked to min, there is a feature here, In the case, there are multiple video
need to play, we don't want to exit full screen mode, then enter again. the
previous code Java_ContentVideoView_openVideo is for this. so it might not good
to put Create/DestoryContentVideoView in c'tor/d'tor.

https://codereview.chromium.org/13669003/diff/2001/content/browser/android/co...
File content/browser/android/content_video_view.h (right):

https://codereview.chromium.org/13669003/diff/2001/content/browser/android/co...
content/browser/android/content_video_view.h:74:
base::android::ScopedJavaGlobalRef<jobject> j_content_video_view_;
On 2013/04/05 01:56:40, joth wrote:
> this is a problem: holding a strong ref to the java object will stop it being
> GC'd so long as we're in "full screen" (which may mean nothing, depending on
how
> the app is structured). this in turn may mean the WebView itself never gets
> GCed. (As AwContentsClient indirectly references it).
> 

Why WebView should be GCed? I think we need to back to WebView once exit full
screen video.  

> So this should be a WeakJavaRef thingy, and all the usage of it use the
"promote
> to ScopedLocalRef and check that for null" pattern.
> 
> Although: it's only by fixing this problem that you will cause the
> possibly-too-early GC of ContentVideoViewClient problem I mentioned above, so
> perhaps best to solve this piece in a separate CL. (could raise a bug for it,
it
> should be a very rare corner leak case so no immediate priority)

[joth, 2013-04-08 18:47:34]

https://codereview.chromium.org/13669003/diff/2001/content/browser/android/co...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/13669003/diff/2001/content/browser/android/co...
content/browser/android/content_video_view.cc:34: void
ContentVideoView::CreateContentVideoView(
On 2013/04/05 21:06:56, michaelbai wrote:
> On 2013/04/05 01:56:40, joth wrote:
> > what if j_content_video_view_ were not null? I think we should call
> > DestroyContentVideoView() here first to be safe?
> > (maybe worth a tests case to cover that too)
> > 
> 
> if j_content_video_view_ was not null, the same object will be returned (see
> java file), the reference count of the object was just decreased/increased
> again. it seemed no side effect.
> 
> 
> > - we could make this safer by having a wrapper object that holds the java
ref
> > and also does Java_ContentVideoView_destroyContentVideoView() in its
> destructor,
> > and then just hold a scoped_ptr to that wrapper inside this class.
> > 
> > Or even better... why not call Java_ContentVideoView_createContentVideoView
> from
> > ContentVideoView c'tor, and destroyContentVideoView from the d'tor, and
remove
> > these two explicit create/destroy methods? that way, the native and java
> objects
> > have the same lifetime, and it's much easier to see what's going on.
> 
> Talked to min, there is a feature here, In the case, there are multiple video
> need to play, we don't want to exit full screen mode, then enter again. the
> previous code Java_ContentVideoView_openVideo is for this. so it might not
good
> to put Create/DestoryContentVideoView in c'tor/d'tor.

Right, the java-side object is a singleton but the native side isn't, so can't
have same lifetimes.

https://codereview.chromium.org/13669003/diff/13001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/13669003/diff/13001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:71:
private static ContentVideoView sContentVideoView;
this could stop the WebView getting GC'd anyway.

moving the singleton instance 'cache' to the native side would solve this. (so
long as native has weak ref to java, as discussed)

https://codereview.chromium.org/13669003/diff/13001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:554:
private static ContentVideoView createContentVideoView(Context context,
getContentVideoVIewInstance

(or getInstance() )

https://codereview.chromium.org/13669003/diff/13001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:588:
setVisibility(View.GONE);
sContentVideoView = null;

https://codereview.chromium.org/13669003/diff/13001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:591:
public static ContentVideoView getContentVideoView() {
getForTets... ?

will need to fetch via native now.

[michaelbai, 2013-04-10 18:27:03]

PTAL

https://codereview.chromium.org/13669003/diff/13001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/13669003/diff/13001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:71:
private static ContentVideoView sContentVideoView;
On 2013/04/08 18:47:34, joth wrote:
> this could stop the WebView getting GC'd anyway.
> 
> moving the singleton instance 'cache' to the native side would solve this. (so
> long as native has weak ref to java, as discussed)
> 

Done.

https://codereview.chromium.org/13669003/diff/13001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:554:
private static ContentVideoView createContentVideoView(Context context,
Keep the name, as we don't have singleton in Java side now.

On 2013/04/08 18:47:34, joth wrote:
> getContentVideoVIewInstance
> 
> (or getInstance() )

https://codereview.chromium.org/13669003/diff/13001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:588:
setVisibility(View.GONE);
This is not applicable now

On 2013/04/08 18:47:34, joth wrote:
> sContentVideoView = null;

https://codereview.chromium.org/13669003/diff/13001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:591:
public static ContentVideoView getContentVideoView() {
Changed to fetch from naive, but it is not just for testing, 

On 2013/04/08 18:47:34, joth wrote:
> getForTets... ?
> 
> will need to fetch via native now.

[michaelbai, 2013-04-12 16:09:38]

PTAL, when you have time.

[qinmin, 2013-04-12 16:41:09]

contentVideoView lgtm

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:552:
private static ContentVideoView createContentVideoView(Context context,
move context to the next line so all params stay in the same line

[joth, 2013-04-12 17:56:24]

Sorry we seem to be circling a bit here, but the 'maybe create video view'
method is bugging me even more now it takes parameters that will be ignored in
the 'reuse & open instead of create' path.

https://codereview.chromium.org/13669003/diff/33001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/13669003/diff/33001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:203:
public class AwContentVideoViewClient implements ContentVideoViewClient {
nit - no need to be public

https://codereview.chromium.org/13669003/diff/33001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:211:
contentVideoView.destroyContentVideoView();
destroyContentVideoView could just be a static method? I'm fine with it this way
too though

https://codereview.chromium.org/13669003/diff/33001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:215:
ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED,
call 2 param version already?

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:21: // WeakRef Reference to the
Java object.
Add "there can only be one content video view at a time, this holds onto that
singleton instance"

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:22: static
JavaObjectWeakGlobalRef* g_content_video_view_ = NULL;
no trailing _ on globals

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:24: static
ScopedJavaLocalRef<jobject> GetContentVideoView(JNIEnv* env) {
instead of static, put these two  into anon namespace

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:28: return
ScopedJavaLocalRef<jobject>();
I think this whole file will be easier to read if you leave the WeakRef as a
member of the native ContentVideoView class, and stash a raw (non-owning)
ContentVideoView* pointer here as the way to find the singleton instance. (i.e.
set it in the c'tor, clear in d'tor, with appropriate DCHECKs to ensure the
singleton invariant)

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:32: return
GetContentVideoView(env).obj();
I think you need to use Release() here, otherwise the jobject returned is
referring to the one in the temporary ScopedLocalJaveRef that will be destructed
before the caller has got the jobject return value.

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:53: g_content_video_view_ = new
JavaObjectWeakGlobalRef(env,
g_content_video_view_ might already hold an instance?

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:58:
Java_ContentVideoView_openVideo(env, content_video_view.obj());
in the else branch we ignore the content_video_view_client. Is that right?

Cramming these two flows into one method (called create, but that may not
actually do any create) is increasingly awkward. IMO this would be clearer to
have the native and java objects be same lifetime, and leave it to the user of
ContentVideoView to make the choice about re-use vs making a new one (as it
would know if the client has changed or not, etc)

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
File content/browser/android/content_video_view.h (right):

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.h:45:
base::android::ScopedJavaLocalRef<jobject> content_video_view_client);
you can pass these as "const JavaRef<jobject>&" to save on some ref count churn

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:35: // TODO(jrg): this is a
shell.  Upstream the rest.
think this can go now :-)

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:278:
base::android::ScopedJavaLocalRef<jobject> GetContext();
This is misleading, as CVC can be used without an activity, so having this
getter could lead to mistakes.
(for fullscreen video, we accept that it's just going to fail if the webview is
used with a non-activity context, but I'm worried the getter here will cause new
code to make incorrect assumptions here)

With getContext on ContentVideoView pushed the policy of how to deal this up to
the embedder, and was clear that we are asking for a context specifically for
hosting the fullscreen video, whereas here it's ambiguous what this is good for.
Maybe it's OK just to comment this is the context that the CVC was created with,
which would typically be an Activity context for an onscreen view.

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:555:
if (client == null) return null;
can this reasonably happen? Seems like an error, and returning null just likely
to trip up the caller

that said...
given discussion above about context possibly being non-activity, then it's
quite likely in that case we'd get exception here anyway when we attempt to
contruct the base View against inappropriate context type.
So maybe the answer is to catch any excption in this method, log it, and return
null, and then make sure the native side can deal with null return. (I think
it's already robust to that anyway, especially now we using weak ref)

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2646:
getContentViewClient().onExternalVideoSurfaceRequested(playerId);
TODO move this method to ContentVideoClient?

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2651:
return mContentViewClient.getContentVideoViewClient();
as the native side only holds a weak ref, I think you'll need to keep an
appropriately life-timed  strong ref on the java side to stop it getting
premeturely GCed.
Stashing that ref in ContentViewCore, here, would make sense.
if (mContentViewClient == null) mContentViewClient = 
mContentViewClient.getContentVideoViewClient();
return mContentViewClient;

incude a comment on the member to explain it is required for ensuring correct
lifetime of the java object

(mRetainedJavaScriptObjects is doing something similar, on a larger scale)

[joth, 2013-04-13 01:18:35]

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:555:
if (client == null) return null;
On 2013/04/12 17:56:24, joth wrote:
> can this reasonably happen? Seems like an error, and returning null just
likely
> to trip up the caller
> 
> that said...
> given discussion above about context possibly being non-activity, then it's
> quite likely in that case we'd get exception here anyway when we attempt to
> contruct the base View against inappropriate context type.
> So maybe the answer is to catch any excption in this method, log it, and
return
> null, and then make sure the native side can deal with null return. (I think
> it's already robust to that anyway, especially now we using weak ref)

hmm one option is to just require 'context instanceof Activity' here, like we do
for alert dialogs (see canShowAlertDialog() in webview classic)
It's a bit hacky, but would avoid random exceptions all down the line when
trying to add views to an inappropriate context type

[michaelbai, 2013-04-18 18:21:58]

Made a lot of changes, PTAL

Main changes
1. Make the native ContentVideoView and Weak reference of Java object have the
same life cycle.

2. Unified the way to exit fullscreen video, now the path to exit fullscreen is
destroyContentVideoView(java) -> ExitFullscreen(native) -> ContentVideView
d'tor().

https://codereview.chromium.org/13669003/diff/33001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/13669003/diff/33001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:203:
public class AwContentVideoViewClient implements ContentVideoViewClient {
On 2013/04/12 17:56:24, joth wrote:
> nit - no need to be public

Done.

https://codereview.chromium.org/13669003/diff/33001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:211:
contentVideoView.destroyContentVideoView();

Thanks

On 2013/04/12 17:56:24, joth wrote:
> destroyContentVideoView could just be a static method? I'm fine with it this
way
> too though

https://codereview.chromium.org/13669003/diff/33001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:215:
ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED,
On 2013/04/12 17:56:24, joth wrote:
> call 2 param version already?

Done, actually, this patch was created before that change.

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:21: // WeakRef Reference to the
Java object.
On 2013/04/12 17:56:24, joth wrote:
> Add "there can only be one content video view at a time, this holds onto that
> singleton instance"

Done.

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:22: static
JavaObjectWeakGlobalRef* g_content_video_view_ = NULL;
On 2013/04/12 17:56:24, joth wrote:
> no trailing _ on globals

Done.

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:24: static
ScopedJavaLocalRef<jobject> GetContentVideoView(JNIEnv* env) {
On 2013/04/12 17:56:24, joth wrote:
> instead of static, put these two  into anon namespace

Done.

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:28: return
ScopedJavaLocalRef<jobject>();
On 2013/04/12 17:56:24, joth wrote:
> I think this whole file will be easier to read if you leave the WeakRef as a
> member of the native ContentVideoView class, and stash a raw (non-owning)
> ContentVideoView* pointer here as the way to find the singleton instance.
(i.e.
> set it in the c'tor, clear in d'tor, with appropriate DCHECKs to ensure the
> singleton invariant)

Done.

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.cc:32: return
GetContentVideoView(env).obj();
On 2013/04/12 17:56:24, joth wrote:
> I think you need to use Release() here, otherwise the jobject returned is
> referring to the one in the temporary ScopedLocalJaveRef that will be
destructed
> before the caller has got the jobject return value.

Done.

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
File content/browser/android/content_video_view.h (right):

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_video_view.h:45:
base::android::ScopedJavaLocalRef<jobject> content_video_view_client);
On 2013/04/12 17:56:24, joth wrote:
> you can pass these as "const JavaRef<jobject>&" to save on some ref count
churn

Done.

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
File content/browser/android/content_view_core_impl.h (right):

https://codereview.chromium.org/13669003/diff/33001/content/browser/android/c...
content/browser/android/content_view_core_impl.h:278:
base::android::ScopedJavaLocalRef<jobject> GetContext();
On 2013/04/12 17:56:24, joth wrote:
> This is misleading, as CVC can be used without an activity, so having this
> getter could lead to mistakes.
> (for fullscreen video, we accept that it's just going to fail if the webview
is
> used with a non-activity context, but I'm worried the getter here will cause
new
> code to make incorrect assumptions here)
> 
> With getContext on ContentVideoView pushed the policy of how to deal this up
to
> the embedder, and was clear that we are asking for a context specifically for
> hosting the fullscreen video, whereas here it's ambiguous what this is good
for.
> Maybe it's OK just to comment this is the context that the CVC was created
with,
> which would typically be an Activity context for an onscreen view.

Done.

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:552:
private static ContentVideoView createContentVideoView(Context context,
On 2013/04/12 16:41:09, qinmin wrote:
> move context to the next line so all params stay in the same line

Done.

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:555:
if (client == null) return null;
Actually, this couldn't happen, if the webview is not on screen view, there is
no way to switch to full screen video. 

Do you want the hacky code here?


On 2013/04/13 01:18:35, joth wrote:
> On 2013/04/12 17:56:24, joth wrote:
> > can this reasonably happen? Seems like an error, and returning null just
> likely
> > to trip up the caller
> > 
> > that said...
> > given discussion above about context possibly being non-activity, then it's
> > quite likely in that case we'd get exception here anyway when we attempt to
> > contruct the base View against inappropriate context type.
> > So maybe the answer is to catch any excption in this method, log it, and
> return
> > null, and then make sure the native side can deal with null return. (I think
> > it's already robust to that anyway, especially now we using weak ref)
> 
> hmm one option is to just require 'context instanceof Activity' here, like we
do
> for alert dialogs (see canShowAlertDialog() in webview classic)
> It's a bit hacky, but would avoid random exceptions all down the line when
> trying to add views to an inappropriate context type

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:555:
if (client == null) return null;
On 2013/04/13 01:18:35, joth wrote:
> On 2013/04/12 17:56:24, joth wrote:
> > can this reasonably happen? Seems like an error, and returning null just
> likely
> > to trip up the caller
> > 
> > that said...
> > given discussion above about context possibly being non-activity, then it's
> > quite likely in that case we'd get exception here anyway when we attempt to
> > contruct the base View against inappropriate context type.
> > So maybe the answer is to catch any excption in this method, log it, and
> return
> > null, and then make sure the native side can deal with null return. (I think
> > it's already robust to that anyway, especially now we using weak ref)
> 
> hmm one option is to just require 'context instanceof Activity' here, like we
do
> for alert dialogs (see canShowAlertDialog() in webview classic)
> It's a bit hacky, but would avoid random exceptions all down the line when
> trying to add views to an inappropriate context type

Done.

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:555:
if (client == null) return null;
Removed the null check for client.

On 2013/04/12 17:56:24, joth wrote:
> can this reasonably happen? Seems like an error, and returning null just
likely
> to trip up the caller
> 
> that said...
> given discussion above about context possibly being non-activity, then it's
> quite likely in that case we'd get exception here anyway when we attempt to
> contruct the base View against inappropriate context type.
> So maybe the answer is to catch any excption in this method, log it, and
return
> null, and then make sure the native side can deal with null return. (I think
> it's already robust to that anyway, especially now we using weak ref)

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2646:
getContentViewClient().onExternalVideoSurfaceRequested(playerId);
This is used by Google TV, not sure we should move it to ContentVideoViewClient.

On 2013/04/12 17:56:24, joth wrote:
> TODO move this method to ContentVideoClient?

https://codereview.chromium.org/13669003/diff/33001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2651:
return mContentViewClient.getContentVideoViewClient();
I don't understand why it could be prematurely GCed, ContentVideoViewClient will
be passed all the way to the ContentVideoView which hold a strong ref to it. All
this happened before the native side have weak reference to ContentVideoView.


On 2013/04/12 17:56:24, joth wrote:
> as the native side only holds a weak ref, I think you'll need to keep an
> appropriately life-timed  strong ref on the java side to stop it getting
> premeturely GCed.
> Stashing that ref in ContentViewCore, here, would make sense.
> if (mContentViewClient == null) mContentViewClient = 
> mContentViewClient.getContentVideoViewClient();
> return mContentViewClient;
> 
> incude a comment on the member to explain it is required for ensuring correct
> lifetime of the java object
> 
> (mRetainedJavaScriptObjects is doing something similar, on a larger scale)

[joth, 2013-05-15 22:38:57]

Looking good. Sorry this took so long, it fell out of my working set. And you're
going to have some work rebasing this now.... :-/

https://codereview.chromium.org/13669003/diff/49001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/13669003/diff/49001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:368:
}
(note you'll need to move this to the inner ContentViewClient subclass now
https://chromiumcodereview.appspot.com/13136002 landed)

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.cc:45:
j_content_video_view_.reset(new JavaObjectWeakGlobalRef(env,
j_content_video_view_ = JavaObjectWeakGlobalRef(env,
Java_ContentVideoView_createContentVideoView(env, context.obj())

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.cc:48: g_content_video_view = this;
problem: at this point no one has a strong-ref to the java side object, so it
could get GC'd before anyone actually tries to use it / add it to the view tree,
etc.
To solve, rather than construct it here, maybe do it lazily in OpenVideo?
OR perhaps clearer, retain a 'private static ContentVideoView sInstance' on the
java side too, and add comments to indicate the ownership model. ? -
destroyContentVideoView() would null out the static instance pointer.

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.cc:51:
ContentVideoView::~ContentVideoView() {
do we need to check that ExitFullscreen has been done? maybe DCHECK
j_content_video_view_ is now null?

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.cc:103: }
j_content_video_view_.reset() ?

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
File content/browser/android/content_video_view.h (right):

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.h:80:
scoped_ptr<JavaObjectWeakGlobalRef> j_content_video_view_;
no need for scoped_ptr, just have the JavaObjectWeakGlobalRef as a member

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/m...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/m...
content/browser/android/media_player_manager_android.cc:168:
video_view_.reset(new ContentVideoView(content_view_core_impl->GetContext(),
I'm not sure what stops two competing ContentViewCores from both getting here
when trying to enter fullscreen, and the second will then hit the singleton
DCHECK inside the ContentVideoView constructor?

Would it be better to have a ContentVideoView::Create() method, and inside there
we can detect there's already a fullscreen view and tear it down? (THis would
avoid the need for the code you have in AwContentClient to exit fullscreen)

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/m...
File content/browser/android/media_player_manager_android.h (right):

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/m...
content/browser/android/media_player_manager_android.h:90: // The fullscreen
video view object.
or null if... ?

https://codereview.chromium.org/13669003/diff/49001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/13669003/diff/49001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:542:
if (!(context instanceof Activity)) return null;
Log.w the problem  ?

https://codereview.chromium.org/13669003/diff/49001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewClient.java
(right):

https://codereview.chromium.org/13669003/diff/49001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewClient.java:13:
*  and WebView API.
nit: "by browser UI and WebView API." => "by the embedder"

https://codereview.chromium.org/13669003/diff/49001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewClient.java:16:
*  getVideoLoadingProgressView() is optional.
is optional, and may return null if not required.

[michaelbai, 2013-05-22 18:08:39]

PTAL, sync-ed, but it seemed WebRTC change was reverted.

https://codereview.chromium.org/13669003/diff/49001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/13669003/diff/49001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:368:
}
Thanks

On 2013/05/15 22:38:57, joth wrote:
> (note you'll need to move this to the inner ContentViewClient subclass now
> https://chromiumcodereview.appspot.com/13136002 landed)

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.cc:45:
j_content_video_view_.reset(new JavaObjectWeakGlobalRef(env,
On 2013/05/15 22:38:57, joth wrote:
> j_content_video_view_ = JavaObjectWeakGlobalRef(env,
> Java_ContentVideoView_createContentVideoView(env, context.obj())

Done.

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.cc:48: g_content_video_view = this;
In the ctor of ContentVideoView(java side), mClient.onShowCustomView is called,
and Java object is supposed be add to the Activity's view tree, if not, it
should be gc-ed, that is our goal, isn't it?


On 2013/05/15 22:38:57, joth wrote:
> problem: at this point no one has a strong-ref to the java side object, so it
> could get GC'd before anyone actually tries to use it / add it to the view
tree,
> etc.
> To solve, rather than construct it here, maybe do it lazily in OpenVideo?
> OR perhaps clearer, retain a 'private static ContentVideoView sInstance' on
the
> java side too, and add comments to indicate the ownership model. ? -
> destroyContentVideoView() would null out the static instance pointer.

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.cc:51:
ContentVideoView::~ContentVideoView() {
On 2013/05/15 22:38:57, joth wrote:
> do we need to check that ExitFullscreen has been done? maybe DCHECK
> j_content_video_view_ is now null?

Done.

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.cc:103: }
On 2013/05/15 22:38:57, joth wrote:
> j_content_video_view_.reset() ?

Done.

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
File content/browser/android/content_video_view.h (right):

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.h:80:
scoped_ptr<JavaObjectWeakGlobalRef> j_content_video_view_;
On 2013/05/15 22:38:57, joth wrote:
> no need for scoped_ptr, just have the JavaObjectWeakGlobalRef as a member

Done.

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/m...
File content/browser/android/media_player_manager_android.cc (right):

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/m...
content/browser/android/media_player_manager_android.cc:168:
video_view_.reset(new ContentVideoView(content_view_core_impl->GetContext(),
hmm, that's no a common use case, it needs user's action to toggle to fullscreen
video, I just made the second one fail.


On 2013/05/15 22:38:57, joth wrote:
> I'm not sure what stops two competing ContentViewCores from both getting here
> when trying to enter fullscreen, and the second will then hit the singleton
> DCHECK inside the ContentVideoView constructor?
> 
> Would it be better to have a ContentVideoView::Create() method, and inside
there
> we can detect there's already a fullscreen view and tear it down? (THis would
> avoid the need for the code you have in AwContentClient to exit fullscreen)

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/m...
File content/browser/android/media_player_manager_android.h (right):

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/m...
content/browser/android/media_player_manager_android.h:90: // The fullscreen
video view object.
On 2013/05/15 22:38:57, joth wrote:
> or null if... ?

Done.

https://codereview.chromium.org/13669003/diff/49001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/13669003/diff/49001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:542:
if (!(context instanceof Activity)) return null;
On 2013/05/15 22:38:57, joth wrote:
> Log.w the problem  ?

Done.

https://codereview.chromium.org/13669003/diff/49001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewClient.java
(right):

https://codereview.chromium.org/13669003/diff/49001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewClient.java:13:
*  and WebView API.
On 2013/05/15 22:38:57, joth wrote:
> nit: "by browser UI and WebView API." => "by the embedder"

Done.

https://codereview.chromium.org/13669003/diff/49001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoViewClient.java:16:
*  getVideoLoadingProgressView() is optional.
On 2013/05/15 22:38:57, joth wrote:
> is optional, and may return null if not required.

Done.

[joth, 2013-06-03 21:22:04]

lgtm

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
File content/browser/android/content_video_view.cc (right):

https://codereview.chromium.org/13669003/diff/49001/content/browser/android/c...
content/browser/android/content_video_view.cc:48: g_content_video_view = this;
On 2013/05/22 18:08:39, michaelbai wrote:
> In the ctor of ContentVideoView(java side), mClient.onShowCustomView is
called,
> and Java object is supposed be add to the Activity's view tree, if not, it
> should be gc-ed, that is our goal, isn't it?
> 

Yes, you're right - I missed a step there.

> 
> On 2013/05/15 22:38:57, joth wrote:
> > problem: at this point no one has a strong-ref to the java side object, so
it
> > could get GC'd before anyone actually tries to use it / add it to the view
> tree,
> > etc.
> > To solve, rather than construct it here, maybe do it lazily in OpenVideo?
> > OR perhaps clearer, retain a 'private static ContentVideoView sInstance' on
> the
> > java side too, and add comments to indicate the ownership model. ? -
> > destroyContentVideoView() would null out the static instance pointer.
>

https://codereview.chromium.org/13669003/diff/62001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/13669003/diff/62001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:9:
import android.content.Context;
delete this duplicate

https://codereview.chromium.org/13669003/diff/62001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/13669003/diff/62001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:188:
mClient.onShowCustomView(this);
does this call have to happen here, or could it be the last thing in the
function? As a general rule it's best to setup internal state and then call out
to embedding layers, and doubly so in a constructor else they may making calls
on the passed half-constructed ContentVideoView instance.

[michaelbai, 2013-06-04 21:55:00]

Hi Yaron,

Need owner approval for content/shell/android and chrome/android.

https://codereview.chromium.org/13669003/diff/62001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/13669003/diff/62001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:9:
import android.content.Context;
On 2013/06/03 21:22:04, joth wrote:
> delete this duplicate

Done.

https://codereview.chromium.org/13669003/diff/62001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/13669003/diff/62001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:188:
mClient.onShowCustomView(this);
On 2013/06/03 21:22:04, joth wrote:
> does this call have to happen here, or could it be the last thing in the
> function? As a general rule it's best to setup internal state and then call
out
> to embedding layers, and doubly so in a constructor else they may making calls
> on the passed half-constructed ContentVideoView instance.

Done.

[Yaron, 2013-06-04 22:19:39]

lgtm

[commit-bot: I haz the power, 2013-06-04 22:25:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/13669003/69001

[commit-bot: I haz the power, 2013-06-05 14:17:12]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-05 17:00:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaelbai@chromium.org/13669003/90002

[commit-bot: I haz the power, 2013-06-05 23:26:36]

Change committed as 204353