Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11280284:
onShowCustomView (Closed)
|
Created:
8 years ago by acleung Modified:
7 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptiononShowCustomView
BUG=6295515
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182188
Patch Set 1 #Patch Set 2 : fix resource #Patch Set 3 : actually using it .... #
Total comments: 21
Patch Set 4 : address issues #Patch Set 5 : Address comments #Patch Set 6 : rebase #Patch Set 7 : update resource strings #
Total comments: 20
Patch Set 8 : comments from benm #
Total comments: 6
Patch Set 9 : spacing #Patch Set 10 : rebase #
Total comments: 4
Patch Set 11 : revert WebContentsDelegateAndroid #Patch Set 12 : address comments #
Total comments: 10
Patch Set 13 : compilation error #Patch Set 14 : fix spacing #Patch Set 15 : fix NullContentsClient compilation error #Patch Set 16 : fix NullContentsClient compilation error #2 #
Messages
Total messages: 27 (0 generated)
(Adding Ben for double check on AwResources.) https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java (right): https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:12: import android.widget.FrameLayout; most of these look unneeded? https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:17: import org.chromium.content.R; needed? https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:20: * Uses an exisiting Activity to handle displaying video in full screen. nit: reference to activity here is misleading. Key thing is this forwards the decision on to AwContentsClient, and hence to the WebView embedder. https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:32: mAwContentsClient.onShowCustomView(view); note in WebChromeClient we also need to pass 'int orientation' and a callback to allow the app to indicate this view was dismissed -- I think that would map down to ContentVideoView.destroyNativeView() or surfaceDestroyed() (not obvious to me which makes more sense) also should take care that the callback we send is nulled out when we are about to call onHideCustomView (to ignore any subsequent call an app may erroneously make to it later) https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:35: public void onDestroyContentVideoView() { mAwContentsClient.onHideCustomView(view); ? https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:251: new AwContentVideoViewDelegate(contentsClient, containerView.getContext())); There's one AwContents per WebView, and many can exist at once, so creating a new singleton AwContentVideoViewDelegate instance per webview wired up to the most-recently-created webview is not going to work. Unless we can update ContentVideoView to support one delegate per ContentViewCore, I think we'll need to keep a map of weak references so we can find the correct AwContentClient instance for a given onShowCustomView call (for a given ContentViewCore originating the full screen request) https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:143: public void onShowCustomView(View view) { not needed? https://codereview.chromium.org/11280284/diff/3001/content/components/web_con... File content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/WebContentsDelegateAndroid.java (right): https://codereview.chromium.org/11280284/diff/3001/content/components/web_con... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:81: public void onShowCustomView(View view) { You dont' need this, the new class you add calls straight into AwContentsClient. (all the methods in here are meant as straight analogs of methods in WebContentsDelegate.h hence why they're all CalledByNative)
On 7 December 2012 16:17, <joth@chromium.org> wrote: > (Adding Ben for double check on AwResources.) > > > > > https://codereview.chromium.**org/11280284/diff/3001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java<https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java> > File > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java > (right): > > https://codereview.chromium.**org/11280284/diff/3001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java#newcode12<https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java#newcode12> > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java:12: > import android.widget.FrameLayout; > most of these look unneeded? > > https://codereview.chromium.**org/11280284/diff/3001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java#newcode17<https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java#newcode17> > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java:17: > import org.chromium.content.R; > needed? > > https://codereview.chromium.**org/11280284/diff/3001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java#newcode20<https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java#newcode20> > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java:20: > * Uses an exisiting Activity to handle displaying video in full screen. > nit: reference to activity here is misleading. Key thing is this > forwards the decision on to AwContentsClient, and hence to the WebView > embedder. > > https://codereview.chromium.**org/11280284/diff/3001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java#newcode32<https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java#newcode32> > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java:32: > mAwContentsClient.**onShowCustomView(view); > note in WebChromeClient we also need to pass 'int orientation' and a > callback to allow the app to indicate this view was dismissed -- I think > that would map down to ContentVideoView.**destroyNativeView() or > surfaceDestroyed() (not obvious to me which makes more sense) > also should take care that the callback we send is nulled out when we > are about to call onHideCustomView (to ignore any subsequent call an app > may erroneously make to it later) > +qinmin: any thoughts on this? how should a ContentVideoView embedder programmatically dismiss fullscreen mode? thanks > https://codereview.chromium.**org/11280284/diff/3001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java#newcode35<https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java#newcode35> > android_webview/java/src/org/**chromium/android_webview/** > AwContentVideoViewDelegate.**java:35: > public void onDestroyContentVideoView() { > mAwContentsClient.**onHideCustomView(view); ? > > https://codereview.chromium.**org/11280284/diff/3001/** > android_webview/java/src/org/**chromium/android_webview/**AwContents.java<https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/org/chromium/android_webview/AwContents.java> > File > android_webview/java/src/org/**chromium/android_webview/**AwContents.java > (right): > > https://codereview.chromium.**org/11280284/diff/3001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContents.java#newcode251<https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/org/chromium/android_webview/AwContents.java#newcode251> > android_webview/java/src/org/**chromium/android_webview/** > AwContents.java:251: > new AwContentVideoViewDelegate(**contentsClient, > containerView.getContext())); > There's one AwContents per WebView, and many can exist at once, so > creating a new singleton AwContentVideoViewDelegate instance per webview > wired up to the most-recently-created webview is not going to work. > > Unless we can update ContentVideoView to support one delegate per > ContentViewCore, I think we'll need to keep a map of weak references so > we can find the correct AwContentClient instance for a given > onShowCustomView call (for a given ContentViewCore originating the full > screen request) > > https://codereview.chromium.**org/11280284/diff/3001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java<https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java> > File > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java > (right): > > https://codereview.chromium.**org/11280284/diff/3001/** > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java#**newcode143<https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java#newcode143> > android_webview/java/src/org/**chromium/android_webview/** > AwContentsClient.java:143: > public void onShowCustomView(View view) { > not needed? > > https://codereview.chromium.**org/11280284/diff/3001/** > content/components/web_**contents_delegate_android/** > java/src/org/chromium/content/**components/web_contents_** > delegate_android/**WebContentsDelegateAndroid.**java<https://codereview.chromium.org/11280284/diff/3001/content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/WebContentsDelegateAndroid.java> > File > content/components/web_**contents_delegate_android/** > java/src/org/chromium/content/**components/web_contents_** > delegate_android/**WebContentsDelegateAndroid.**java > (right): > > https://codereview.chromium.**org/11280284/diff/3001/** > content/components/web_**contents_delegate_android/** > java/src/org/chromium/content/**components/web_contents_** > delegate_android/**WebContentsDelegateAndroid.**java#newcode81<https://codereview.chromium.org/11280284/diff/3001/content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/WebContentsDelegateAndroid.java#newcode81> > content/components/web_**contents_delegate_android/** > java/src/org/chromium/content/**components/web_contents_** > delegate_android/**WebContentsDelegateAndroid.**java:81: > public void onShowCustomView(View view) { > You dont' need this, the new class you add calls straight into > AwContentsClient. > > (all the methods in here are meant as straight analogs of methods in > WebContentsDelegate.h hence why they're all CalledByNative) > > https://codereview.chromium.**org/11280284/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwResource.java (right): https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwResource.java:37: public static int STRING_VIDEO_INVALID_PLAYBACK; As these are resources defined in content/, you should be able to wire them up directly in the glue layer (see ResourceProvider.java), and just reference them as org.chromium.content.R.string.xxxxx. This file is only used for resources that webview needs not defined in content/.
https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java (right): https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:12: import android.widget.FrameLayout; On 2012/12/08 00:17:58, joth wrote: > most of these look unneeded? Done. https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:17: import org.chromium.content.R; On 2012/12/08 00:17:58, joth wrote: > needed? Done. https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:20: * Uses an exisiting Activity to handle displaying video in full screen. On 2012/12/08 00:17:58, joth wrote: > nit: reference to activity here is misleading. Key thing is this forwards the > decision on to AwContentsClient, and hence to the WebView embedder. Good point. How do you like the updated version? https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:32: mAwContentsClient.onShowCustomView(view); On 2012/12/08 00:17:58, joth wrote: > note in WebChromeClient we also need to pass 'int orientation' and a callback to > allow the app to indicate this view was dismissed -- I think that would map down > to ContentVideoView.destroyNativeView() or surfaceDestroyed() (not obvious to me > which makes more sense) Make sense. Seems like there are more changes need to be done in the base class. > also should take care that the callback we send is nulled out when we are about > to call onHideCustomView (to ignore any subsequent call an app may erroneously > make to it later) I don't fully follow this case. Once onhidCustomView is called. Subsequence call to that call back would just trigger onHidCustomVIew once more without any harm. Or is that the case we should avoid? https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:35: public void onDestroyContentVideoView() { On 2012/12/08 00:17:58, joth wrote: > mAwContentsClient.onHideCustomView(view); ? Done. https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContents.java:251: new AwContentVideoViewDelegate(contentsClient, containerView.getContext())); On 2012/12/08 00:17:58, joth wrote: > There's one AwContents per WebView, and many can exist at once, so creating a > new singleton AwContentVideoViewDelegate instance per webview wired up to the > most-recently-created webview is not going to work. > > Unless we can update ContentVideoView to support one delegate per > ContentViewCore, I think we'll need to keep a map of weak references so we can > find the correct AwContentClient instance for a given onShowCustomView call (for > a given ContentViewCore originating the full screen request) > Ya. I don't even know what happens if there are multiple fullscreen video on different webview host. In theory, if one goes full screen after the other, it should take over the existing one. I think I will open a sperate issue to track the problem of multiple fullscreen custom views which might require further restructure of content video view. https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:143: public void onShowCustomView(View view) { On 2012/12/08 00:17:58, joth wrote: > not needed? Done. https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwResource.java (right): https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwResource.java:37: public static int STRING_VIDEO_INVALID_PLAYBACK; On 2012/12/10 10:54:35, benm wrote: > As these are resources defined in content/, you should be able to wire them up > directly in the glue layer (see ResourceProvider.java), and just reference them > as org.chromium.content.R.string.xxxxx. > > This file is only used for resources that webview needs not defined in content/. I don't fully follow. I am getting these resource in frameworks/base/core/res/res/values/strings.xml. Or are you suggesting that I add these resource in Chromium? https://codereview.chromium.org/11280284/diff/3001/content/components/web_con... File content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/WebContentsDelegateAndroid.java (right): https://codereview.chromium.org/11280284/diff/3001/content/components/web_con... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:81: public void onShowCustomView(View view) { On 2012/12/08 00:17:58, joth wrote: > You dont' need this, the new class you add calls straight into AwContentsClient. > > (all the methods in here are meant as straight analogs of methods in > WebContentsDelegate.h hence why they're all CalledByNative) Done.
https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwResource.java (right): https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwResource.java:37: public static int STRING_VIDEO_INVALID_PLAYBACK; You're right to get them from the android framework, but this isn't the correct place in android_webview to do the mapping. Constants for these resources are already defined in org.chromium.content.R.string (see ./content/public/android/java/resource_map/org/chromium/content/R.java). So we just need to map those resources to the android framework ones and that's done in ResourceProvider.java in the glue layer. And then wherever you need to reference the string you'd just use org.chromium.content.R.string.XXX instead of getters here.
On 2013/01/08 12:31:51, benm wrote: > https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... > File android_webview/java/src/org/chromium/android_webview/AwResource.java > (right): > > https://codereview.chromium.org/11280284/diff/3001/android_webview/java/src/o... > android_webview/java/src/org/chromium/android_webview/AwResource.java:37: public > static int STRING_VIDEO_INVALID_PLAYBACK; > You're right to get them from the android framework, but this isn't the correct > place in android_webview to do the mapping. Constants for these resources are > already defined in org.chromium.content.R.string (see > ./content/public/android/java/resource_map/org/chromium/content/R.java). So we > just need to map those resources to the android framework ones and that's done > in ResourceProvider.java in the glue layer. And then wherever you need to > reference the string you'd just use org.chromium.content.R.string.XXX instead of > getters here. Done
thanks alan! https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java (right): https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: (c) 2013 https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:3: // found in the LICENSE file. nit: extra newline please https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:11: import org.chromium.android_webview.AwContentsClient; shouldn't need to import stuff in our own package. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:24: this.mContext = context; nit: nix this https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:31: // TODO: we need to invoke destroyContentVideoView() nit: 4 space indent https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:33: // from the delegate. The |view| param for onShowCustomView is actually an instance of ContentVideoView, I wonder if we can just change the param to be that type? https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:37: // Should we request a change in orientation? I don't think so, we should use whatever orientation the device is currently in. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:43: // TODO: require changes to the base class. What changes exactly? The base class is an interface. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:52: org.chromium.content.R.string.media_player_error_text_invalid_progressive_playback); nit: 8 space indent for continuations https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:147: int requestedOrientation, CustomViewCallback cb) { 4 space indent
PTAL https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java (right): https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/04 11:38:46, benm wrote: > nit: (c) 2013 The review and the file actually started near the end of last year. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:3: // found in the LICENSE file. On 2013/02/04 11:38:46, benm wrote: > nit: extra newline please Done. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:11: import org.chromium.android_webview.AwContentsClient; On 2013/02/04 11:38:46, benm wrote: > shouldn't need to import stuff in our own package. Done. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:24: this.mContext = context; On 2013/02/04 11:38:46, benm wrote: > nit: nix this Done. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:31: // TODO: we need to invoke destroyContentVideoView() On 2013/02/04 11:38:46, benm wrote: > nit: 4 space indent Done. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:33: // from the delegate. On 2013/02/04 11:38:46, benm wrote: > The |view| param for onShowCustomView is actually an instance of > ContentVideoView, I wonder if we can just change the param to be that type? I am guessing not. You are right that we only have videos now. However, the name of the function suggests that it is not just videos that can trigger a custom view. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:37: // Should we request a change in orientation? On 2013/02/04 11:38:46, benm wrote: > I don't think so, we should use whatever orientation the device is currently in. Done. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:43: // TODO: require changes to the base class. On 2013/02/04 11:38:46, benm wrote: > What changes exactly? The base class is an interface. It was referring to the ContentVideoView object. There is currently no API for closing it yet. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:52: org.chromium.content.R.string.media_player_error_text_invalid_progressive_playback); On 2013/02/04 11:38:46, benm wrote: > nit: 8 space indent for continuations Done. https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11280284/diff/30001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:147: int requestedOrientation, CustomViewCallback cb) { On 2013/02/04 11:38:46, benm wrote: > 4 space indent Done.
lgtm % nits https://codereview.chromium.org/11280284/diff/35001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java (right): https://codereview.chromium.org/11280284/diff/35001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:37: ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED, nit: indents here are wrong. line length is 100 chars and then indent by 8 spaces if necessary. https://codereview.chromium.org/11280284/diff/35001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:42: // TODO: Requires refactoring of ContentVideoView If I understand right, the comment from onCustomViewHidden should be here and onCustomViewHidden should just invoke this method?
https://codereview.chromium.org/11280284/diff/35001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java (right): https://codereview.chromium.org/11280284/diff/35001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:37: ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED, On 2013/02/05 15:18:12, benm wrote: > nit: indents here are wrong. line length is 100 chars and then indent by 8 > spaces if necessary. Done. https://codereview.chromium.org/11280284/diff/35001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:42: // TODO: Requires refactoring of ContentVideoView On 2013/02/05 15:18:12, benm wrote: > If I understand right, the comment from onCustomViewHidden should be here and > onCustomViewHidden should just invoke this method? Yes.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11280284/43001
Failed to apply patch for
android_webview/java/src/org/chromium/android_webview/AwContents.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
android_webview/java/src/org/chromium/android_webview/AwContents.java
Hunk #1 succeeded at 27 (offset 1 line).
Hunk #2 FAILED at 270.
1 out of 2 hunks FAILED -- saving rejects to file
android_webview/java/src/org/chromium/android_webview/AwContents.java.rej
Patch:
android_webview/java/src/org/chromium/android_webview/AwContents.java
Index: android_webview/java/src/org/chromium/android_webview/AwContents.java
diff --git
a/android_webview/java/src/org/chromium/android_webview/AwContents.java
b/android_webview/java/src/org/chromium/android_webview/AwContents.java
index
4fed78c509bba30c3446391c368ff071bb58dd65..14763bae8041b1c162a9d2a17274b73f7d8336b0
100644
--- a/android_webview/java/src/org/chromium/android_webview/AwContents.java
+++ b/android_webview/java/src/org/chromium/android_webview/AwContents.java
@@ -26,6 +26,7 @@ import org.chromium.base.CalledByNative;
import org.chromium.base.JNINamespace;
import org.chromium.base.ThreadUtils;
import org.chromium.content.browser.ContentSettings;
+import org.chromium.content.browser.ContentVideoView;
import org.chromium.content.browser.ContentViewCore;
import org.chromium.content.browser.LoadUrlParams;
import org.chromium.content.browser.NavigationHistory;
@@ -269,6 +270,9 @@ public class AwContents {
mPossiblyStaleHitTestData = new HitTestData();
nativeDidInitializeContentViewCore(mNativeAwContents,
mContentViewCore.getNativeContentViewCore());
+
+ ContentVideoView.registerContentVideoViewContextDelegate(
+ new AwContentVideoViewDelegate(contentsClient,
containerView.getContext()));
}
public ContentViewCore getContentViewCore() {
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11280284/46001
Presubmit check for 11280284-46001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
components/web_contents_delegate_android
Presubmit checks took 1.2s to calculate.
https://codereview.chromium.org/11280284/diff/3001/content/components/web_con... File content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/WebContentsDelegateAndroid.java (right): https://codereview.chromium.org/11280284/diff/3001/content/components/web_con... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:81: public void onShowCustomView(View view) { On 2013/01/07 23:40:03, acleung wrote: > On 2012/12/08 00:17:58, joth wrote: > > You dont' need this, the new class you add calls straight into > AwContentsClient. > > > > (all the methods in here are meant as straight analogs of methods in > > WebContentsDelegate.h hence why they're all CalledByNative) > > Done. Are you sure? I still see this file edited in the latest patchset: https://chromiumcodereview.appspot.com/11280284/diff/46001/components/web_con... You can revert this file https://codereview.chromium.org/11280284/diff/46001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://codereview.chromium.org/11280284/diff/46001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:148: public void onShowCustomView(View view, you can remove this method (and revert WebContentsDelegateAndroid) as your AwContentVideoViewDelegate calls the outer AwContentsClient.onShowCustomView() directly.
https://chromiumcodereview.appspot.com/11280284/diff/46001/android_webview/ja... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://chromiumcodereview.appspot.com/11280284/diff/46001/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:20: import android.webkit.WebChromeClient.CustomViewCallback; nit: as a general rule rather than import inner classes we tend to use the qualified name (e.g. WebChromeClient.CustomViewCallback) as it generally makes the context much clearer (MyFoo.Delegate etc)
https://codereview.chromium.org/11280284/diff/35001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java (right): https://codereview.chromium.org/11280284/diff/35001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:42: // TODO: Requires refactoring of ContentVideoView I kind of meant for you to move the comment :)
lgtm lgtm % spacing issue. thanks alan! https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... File android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java (right): https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:35: ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED, 8 space indent https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:53: org.chromium.content.R.string.media_player_error_text_unknown); 8 space indent https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:58: org.chromium.content.R.string.media_player_error_button); 8 space indent https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:63: org.chromium.content.R.string.media_player_error_title); 8 space indent https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:68: org.chromium.content.R.string.media_player_loading_video); 8 space indent
https://chromiumcodereview.appspot.com/11280284/diff/3001/content/components/... File content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/WebContentsDelegateAndroid.java (right): https://chromiumcodereview.appspot.com/11280284/diff/3001/content/components/... content/components/web_contents_delegate_android/java/src/org/chromium/content/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:81: public void onShowCustomView(View view) { On 2013/02/06 00:02:29, joth wrote: > On 2013/01/07 23:40:03, acleung wrote: > > On 2012/12/08 00:17:58, joth wrote: > > > You dont' need this, the new class you add calls straight into > > AwContentsClient. > > > > > > (all the methods in here are meant as straight analogs of methods in > > > WebContentsDelegate.h hence why they're all CalledByNative) > > > > Done. > > Are you sure? I still see this file edited in the latest patchset: > https://chromiumcodereview.appspot.com/11280284/diff/46001/components/web_con... > > > You can revert this file Not sure what happend. I reverted the file now. https://chromiumcodereview.appspot.com/11280284/diff/35001/android_webview/ja... File android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java (right): https://chromiumcodereview.appspot.com/11280284/diff/35001/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:42: // TODO: Requires refactoring of ContentVideoView On 2013/02/06 10:34:37, benm wrote: > I kind of meant for you to move the comment :) I got confused a bit. onDestroyContentVideoView is a call from video content view informing us the user click the close button. onCustomViewHidden is invoked when the host application wanting to dismiss the custom view: http://developer.android.com/reference/android/webkit/WebChromeClient.CustomV... I updated the comments. We already have another issue to track closing of custom view so I'll leave the implementations out of this CL. https://chromiumcodereview.appspot.com/11280284/diff/46001/android_webview/ja... File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java (right): https://chromiumcodereview.appspot.com/11280284/diff/46001/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:20: import android.webkit.WebChromeClient.CustomViewCallback; On 2013/02/06 00:04:20, joth wrote: > nit: as a general rule rather than import inner classes we tend to use the > qualified name (e.g. WebChromeClient.CustomViewCallback) as it generally makes > the context much clearer (MyFoo.Delegate etc) Done. https://chromiumcodereview.appspot.com/11280284/diff/46001/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:148: public void onShowCustomView(View view, On 2013/02/06 00:02:29, joth wrote: > you can remove this method (and revert WebContentsDelegateAndroid) as your > AwContentVideoViewDelegate calls the outer AwContentsClient.onShowCustomView() > directly. Done. https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... File android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java (right): https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:35: ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED, On 2013/02/12 10:51:28, benm wrote: > 8 space indent Done. https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:53: org.chromium.content.R.string.media_player_error_text_unknown); On 2013/02/12 10:51:28, benm wrote: > 8 space indent Done. https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:58: org.chromium.content.R.string.media_player_error_button); On 2013/02/12 10:51:28, benm wrote: > 8 space indent Done. https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:63: org.chromium.content.R.string.media_player_error_title); On 2013/02/12 10:51:28, benm wrote: > 8 space indent Done. https://chromiumcodereview.appspot.com/11280284/diff/45003/android_webview/ja... android_webview/java/src/org/chromium/android_webview/AwContentVideoViewDelegate.java:68: org.chromium.content.R.string.media_player_loading_video); On 2013/02/12 10:51:28, benm wrote: > 8 space indent Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11280284/45004
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11280284/45005
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11280284/50004
Retried try job too often on win_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acleung@google.com/11280284/50004
Message was sent while issue was closed.
Change committed as 182188 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
