Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2154883003:
Make ContentVideoView progress view visible in WebView (Closed)
|
Created:
4 years, 5 months ago by Tima Vaisburd Modified:
4 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake ContentVideoView progress view visible in WebView
The progress view used to be part of ContentVideoView underlay.
In the current Spitzer implementation for WebView FullScreenView
got opaque content which now obstructs the progress bar.
This CL removes the progress bar from ContentVideoView
and makes the ContentVideoViewEmbedder responsible for
its creation and display.
As well, the progress bar remained in WebView only and has been
removed from Chrome.
BUG=628828
NOPRESUBMIT=true
Committed: https://crrev.com/ee5c228c4753bd80e1f6f14e7cbc7ee3c673b00c
Cr-Commit-Position: refs/heads/master@{#410234}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Take into account whether Spitzer is enabled #
Total comments: 4
Patch Set 3 : Moved Java method isUnifiedMediaPipelineEnabled from AwContents to MediaCodecUtil #Patch Set 4 : Moved progress view to WebView embedder #Patch Set 5 : Cleanup #
Total comments: 6
Patch Set 6 : Addressed comments #
Total comments: 6
Patch Set 7 : Removed getVideoLoadingProgressView from the embedder protocol; moved a string to WebView resources. #Patch Set 8 : Fixed compilation, rebased #Patch Set 9 : Fixed unit test #
Total comments: 12
Patch Set 10 : Improved formatting #Patch Set 11 : Fixed Java continuation lines identation #Messages
Total messages: 78 (27 generated)
This is a rather simplistic approach. Maybe we should have a separate class for progress view, unrelated to ContentVideoView. Please take a look anyway. https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:99: return false; The return value shall probably depend on whether Spizter is enabled.
timav@chromium.org changed reviewers: + boliu@chromium.org, sgurun@chromium.org
This is a rather simplistic approach. Maybe we should have a separate class for progress view, unrelated to ContentVideoView. Please take a look anyway.
who's reviewing content? looks like that's where the complexity of this CL is https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:99: return false; On 2016/07/16 01:19:18, Tima Vaisburd wrote: > The return value shall probably depend on whether Spizter is enabled. then do that? sounds like so is the change above too? https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:151: mCustomView.addView(fullscreenView); addView(0, fullscreenView) here? too?
https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:99: return false; On 2016/07/18 21:06:32, boliu wrote: > On 2016/07/16 01:19:18, Tima Vaisburd wrote: > > The return value shall probably depend on whether Spizter is enabled. > > then do that? > > sounds like so is the change above too? Yep, sorry. I rushed to make this last Friday but just figured out I never sent it to any reviewer. Will do.
https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:151: mCustomView.addView(fullscreenView); On 2016/07/18 21:06:32, boliu wrote: > addView(0, fullscreenView) here? too? rather addView(fullscreenView,0)?
timav@chromium.org changed reviewers: + liberato@chromium.org, watk@chromium.org
https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:99: return false; On 2016/07/18 21:10:14, Tima Vaisburd wrote: > On 2016/07/18 21:06:32, boliu wrote: > > On 2016/07/16 01:19:18, Tima Vaisburd wrote: > > > The return value shall probably depend on whether Spizter is enabled. > > > > then do that? > > > > sounds like so is the change above too? > > Yep, sorry. I rushed to make this last Friday but just figured out I never sent > it to any reviewer. Will do. Done. https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:151: mCustomView.addView(fullscreenView); On 2016/07/22 17:11:36, sgurun wrote: > On 2016/07/18 21:06:32, boliu wrote: > > addView(0, fullscreenView) here? too? > > rather addView(fullscreenView,0)? As far as I understand it would not make a difference: at this point |mCustomView| is an empty FrameLayout and contains no views. Conceptually though we want to add FullScreenView at the top, I think. Upon entering full screen video two methods will be called: first, enterFullscreen() (this one), then enterFullscreenVideo(). The enterFullscreen() method adds the FullScreenView, which is transparent and does not show the actual video for old video pipeline, and shows the video for Spitzer. The enterFullscreenVideo() adds ContentVideoView, which is another frame layout, to it: at the bottom for old video pipeline and at the top for Spitzer.
https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:151: mCustomView.addView(fullscreenView); On 2016/07/22 22:48:53, Tima Vaisburd wrote: > On 2016/07/22 17:11:36, sgurun wrote: > > On 2016/07/18 21:06:32, boliu wrote: > > > addView(0, fullscreenView) here? too? > > > > rather addView(fullscreenView,0)? > > As far as I understand it would not make a difference: at this point > |mCustomView| is an empty FrameLayout and contains no views. > Conceptually though we want to add FullScreenView at the top, I think. > > Upon entering full screen video two methods will be called: first, > enterFullscreen() (this one), then enterFullscreenVideo(). How strong is the guarnatee that enterFullscreen is called before enterFullscreenVideo? Is it accidental or intentional ordering, or how likely is it for someone to change it in the future, and forget about this code? > > The enterFullscreen() method adds the FullScreenView, which is transparent and > does not show the actual video for old video pipeline, and shows the video for > Spitzer. > > The enterFullscreenVideo() adds ContentVideoView, which is another frame layout, > to it: at the bottom for old video pipeline and at the top for Spitzer. https://codereview.chromium.org/2154883003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2154883003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1267: public static boolean isUnifiedMediaPipelineEnabled() { going through AwContents makes no sense here can some java class from media/base/android expose this method instead?
https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:151: mCustomView.addView(fullscreenView); On 2016/07/22 22:57:31, boliu wrote: > How strong is the guarnatee that enterFullscreen is called before > enterFullscreenVideo? By looking at the code, enterFullscreen() is originated by Blink, FullscreenController::enterFullScreenForElement() https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Fullscreen... enterFullscreenVideo() is called when we create ContentVideoView in WMPI, which is also originated by Blink, Fullscreen::didEnterFullscreenForElement(Element* element) https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Fulls... I beleve didEnter() should come after? > Is it accidental or intentional ordering There are a couple of comments that indicate intention, or at least prior knowledge: https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an... https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an... Having said that, maybe there is a better approach overall? > or how likely is it for someone to change it in the future, > and forget about this code? Unable to tell! https://codereview.chromium.org/2154883003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2154883003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1267: public static boolean isUnifiedMediaPipelineEnabled() { On 2016/07/22 22:57:31, boliu wrote: > going through AwContents makes no sense here > > can some java class from media/base/android expose this method instead? Thank you, will do.
https://codereview.chromium.org/2154883003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2154883003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1267: public static boolean isUnifiedMediaPipelineEnabled() { On 2016/07/23 00:07:27, Tima Vaisburd wrote: > On 2016/07/22 22:57:31, boliu wrote: > > going through AwContents makes no sense here > > > > can some java class from media/base/android expose this method instead? > > Thank you, will do. Hm, I could not find a good place in media/base/android either. Now put it in MediaCodecUtils. Shall I create a new Java class, e.g. MediaInfo? Chris, Frank, WDYT?
random thoughts, maybe nonsensical. :) thanks -fl https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:151: mCustomView.addView(fullscreenView); On 2016/07/22 22:48:53, Tima Vaisburd wrote: > On 2016/07/22 17:11:36, sgurun wrote: > > On 2016/07/18 21:06:32, boliu wrote: > > > addView(0, fullscreenView) here? too? > > > > rather addView(fullscreenView,0)? > > As far as I understand it would not make a difference: at this point > |mCustomView| is an empty FrameLayout and contains no views. > Conceptually though we want to add FullScreenView at the top, I think. > > Upon entering full screen video two methods will be called: first, > enterFullscreen() (this one), then enterFullscreenVideo(). > > The enterFullscreen() method adds the FullScreenView, which is transparent and > does not show the actual video for old video pipeline, and shows the video for > Spitzer. > > The enterFullscreenVideo() adds ContentVideoView, which is another frame layout, > to it: at the bottom for old video pipeline and at the top for Spitzer. it seems a little complicated that CVV is sometimes at the top and sometimes at the bottom, and that the progress bar sometimes ends up behind the compositor output (without spitzer) and sometimes above. does anything else besides WebView actually care about the progress view? as in, i see that CVV creates its own implementation for all other embedders. does this default UI actually show anything? i've never seen it. it would be much nicer if the progress view were handled entirely by the embedder, and CVV had nothing to do with it. i think that a lot of this gets much simpler. in fact, i think it would continue to work if we just never create a CVV. https://codereview.chromium.org/2154883003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/2154883003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:1267: public static boolean isUnifiedMediaPipelineEnabled() { On 2016/07/23 03:50:58, Tima Vaisburd wrote: > On 2016/07/23 00:07:27, Tima Vaisburd wrote: > > On 2016/07/22 22:57:31, boliu wrote: > > > going through AwContents makes no sense here > > > > > > can some java class from media/base/android expose this method instead? > > > > Thank you, will do. > > Hm, I could not find a good place in media/base/android either. Now put it in > MediaCodecUtils. Shall I create a new Java class, e.g. MediaInfo? > Chris, Frank, WDYT? i think that MediaCodecUtils is fine. if we don't have any more methods similar to this in the future, then there's no problem. otherwise, we can pull them out into a separate class then, when it's clearer what their common theme is.
https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:151: mCustomView.addView(fullscreenView); On 2016/07/25 15:43:28, liberato wrote: > it would be much nicer if the progress view were handled entirely by the > embedder, and CVV had nothing to do with it. i think that a lot of this gets > much simpler. > > in fact, i think it would continue to work if we just never create a CVV. I like that idea.
Thank you for the review! I have a couple of questions. On 2016/07/25 15:43:28, liberato wrote: > it would be much nicer if the progress view were handled entirely by the > embedder, and CVV had nothing to do with it. One thing to solve with this approach is how to signal the embedder that we did load and this progress view has to disappear. Right now it is attached to onVideoSizeChanged: https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... which in Spitzer is initialed by WMPI::EnableOverlay(): https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=0... I guess the reasoning is that if we know the size, we got the initial data. > does anything else besides WebView actually care about the progress view? I thought Chrome does? Consistency? On the other hand I did not see this message in my Chrome testing in full screen.
On 2016/07/25 17:14:42, Tima Vaisburd wrote: > Thank you for the review! > I have a couple of questions. > > On 2016/07/25 15:43:28, liberato wrote: > > it would be much nicer if the progress view were handled entirely by the > > embedder, and CVV had nothing to do with it. > > One thing to solve with this approach is how to signal the embedder that we did > load and this progress view has to disappear. > > Right now it is attached to onVideoSizeChanged: > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > which in Spitzer is initialed by WMPI::EnableOverlay(): > https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=0... > > I guess the reasoning is that if we know the size, we got the initial data. > > > does anything else besides WebView actually care about the progress view? > > I thought Chrome does? Consistency? On the other hand I did not see this message > in my Chrome testing in full screen. > load and this progress view has to disappear. i think that blink should do it via the same general path as enterFullScreen / exitFullScreen. probably would send when it transitions to HAVE_METADATA or similar. i haven't traced it through to see if it's reasonable to actually hook things up that way. in an ideal world, it makes sense that blink owns all of it.
There used to be a visible progress view in chrome, but I found that it was broken a while ago because it was affecting power measurements by animating while invisible :) https://bugs.chromium.org/p/chromium/issues/detail?id=536956 I made no effort to bring it back with Spitzer but left it there in case webview needed it. We would need to change the trigger if we wanted to enable it with spitzer because we immediately call onVideoSizeChanged when entering fullscreen (with either 0x0 or 1x1 if we don't know the size yet).
On 2016/07/25 21:59:14, watk wrote: > There used to be a visible progress view in chrome, but I found that it was > broken a while ago because it was affecting power measurements by animating > while invisible :) https://bugs.chromium.org/p/chromium/issues/detail?id=536956 > > I made no effort to bring it back with Spitzer but left it there in case webview > needed it. We would need to change the trigger if we wanted to enable it with > spitzer because we immediately call onVideoSizeChanged when entering fullscreen > (with either 0x0 or 1x1 if we don't know the size yet). i forgot about that! if it's off and nobody misses it, then i vote that we leave it off for now. if we bring it back, we can clean the code up and avoid having it drain batteries in the background(!) at the same time... that greatly simplifies things.
On 2016/07/25 22:02:45, liberato wrote: > if it's off and nobody misses it, then i vote that we leave it off for now. Please clarify for what cases. Did you mean Chrome app or all cases including WebView?
On 2016/07/25 22:07:04, Tima Vaisburd wrote: > On 2016/07/25 22:02:45, liberato wrote: > > if it's off and nobody misses it, then i vote that we leave it off for now. > > Please clarify for what cases. Did you mean Chrome app or all cases including > WebView? sorry, i meant chrome. watk -- why was it spinning in the background? did it never get turned off when the video loaded?
On 2016/07/25 22:10:25, liberato wrote: > On 2016/07/25 22:07:04, Tima Vaisburd wrote: > > On 2016/07/25 22:02:45, liberato wrote: > > > if it's off and nobody misses it, then i vote that we leave it off for now. > > > > Please clarify for what cases. Did you mean Chrome app or all cases including > > WebView? > > sorry, i meant chrome. > > watk -- why was it spinning in the background? did it never get turned off when > the video loaded? Maybe view.setVisibility(View.GONE) just hides the view, we might want to removeView() instead.
On 2016/07/25 22:10:25, liberato wrote: > On 2016/07/25 22:07:04, Tima Vaisburd wrote: > > On 2016/07/25 22:02:45, liberato wrote: > > > if it's off and nobody misses it, then i vote that we leave it off for now. > > > > Please clarify for what cases. Did you mean Chrome app or all cases including > > WebView? > > sorry, i meant chrome. > > watk -- why was it spinning in the background? did it never get turned off when > the video loaded? I think it was just a bug in our prototype so it shouldn't be affecting anything that matters. Whether we should remove from chrome: IMO probably. The number of cases where users might see it is smaller than ever with Spitzer, because we hide the fullscreen button on the default controls until HAVE_METADATA. And if they're using custom controls they'll fullscreen a div and won't use CVV anyway.
Description was changed from
==========
Make ContentVideoView progress view visible in WebView
In the current Spitzer implementation for WebView the FullScreenView
became opaque and now obstructs ContentVideoView which is placed below.
ContentVideoView is not used for the video playback in WebView, but
holds a progress view which has to be visible.
This CL
1. Adds a new method to ContentVideoViewEmbedder interface
that controls whether ContentVideoView has a SurfaceView child,
2. Removes this SurfaceView child for WebView,
3. Adds ContentVideoView which now has only a progress bar on top
of FullScreenView to make it visible.
BUG=628828
==========
to
==========
Make ContentVideoView progress view visible in WebView
The progress view used to be part of ContentVideoView underlay.
In the current Spitzer implementation for WebView FullScreenView
got opaque content which now obstructs the progress bar.
This CL removes the progress bar from ContentVideoView
and makes the ContentVideoViewEmbedder responsible for
its creation and display.
As well, the progress bar remained in WebView only and has been
removed from Chrome.
BUG=628828
==========
> On 2016/07/25 22:10:25, liberato wrote: > > > > if it's off and nobody misses it, then i vote that we leave it off for > now [...] i meant chrome. Moved the progress bar from ContentVideoView to AwContentViewClient (this is the embedder for WebView) and made AwContentViewClient responsible for displaying it. I modified the embedder interface. However, the progress bar is not completely decoupled from CVV since enteredFullscreenVideo() is called from here. Because of that I still use the frame size as an indicator that the metadata is loaded. On 2016/07/25 22:30:41, watk wrote: > Whether we should remove from chrome: IMO probably. The number of cases where > users might see it is smaller than ever with Spitzer, because we hide the > fullscreen button on the default controls until HAVE_METADATA. And if they're > using custom controls they'll fullscreen a div and won't use CVV anyway. Yes, and I was not able to see this progress bar until I restored the fullscreen button. Then, can we ever see this progress bar in Spitzer? If not, maybe this CL is not required?
On 2016/07/27 02:34:01, Tima Vaisburd wrote: > > On 2016/07/25 22:10:25, liberato wrote: > > > > > if it's off and nobody misses it, then i vote that we leave it off for > > now [...] i meant chrome. > > Moved the progress bar from ContentVideoView to AwContentViewClient (this is > the embedder for WebView) and made AwContentViewClient responsible for > displaying it. I modified the embedder interface. > > However, the progress bar is not completely decoupled from CVV since > enteredFullscreenVideo() is called from here. Because of that I still use the > frame size as an indicator that the metadata is loaded. > > On 2016/07/25 22:30:41, watk wrote: > > > Whether we should remove from chrome: IMO probably. The number of cases where > > users might see it is smaller than ever with Spitzer, because we hide the > > fullscreen button on the default controls until HAVE_METADATA. And if they're > > using custom controls they'll fullscreen a div and won't use CVV anyway. > > Yes, and I was not able to see this progress bar until I restored the fullscreen > button. Then, can we ever see this progress bar in Spitzer? If not, maybe this > CL > is not required? > using custom controls they'll fullscreen a div and won't use CVV anyway. hrm, it's too bad that we don't have any usage stats on this. i'm worried that we might be biased thinking about the chrome use-cases rather than webview. we're only keeping this around because it's supported in the webview api, so whoever's using that api might not be doing what we expect to see in chrome. do we have any idea about how often it's used? i'm guessing no, since we don't have UMA yet. >If not, maybe this CL is not required? i think the outcome of this should either be a working progress bar in webview, or no progress bar anywhere. i.e., fix or remove. either is an improvement :)
On 2016/07/27 15:52:11, liberato wrote: > On 2016/07/27 02:34:01, Tima Vaisburd wrote: > > > On 2016/07/25 22:10:25, liberato wrote: > > > > > > if it's off and nobody misses it, then i vote that we leave it off for > > > now [...] i meant chrome. > > > > Moved the progress bar from ContentVideoView to AwContentViewClient (this is > > the embedder for WebView) and made AwContentViewClient responsible for > > displaying it. I modified the embedder interface. > > > > However, the progress bar is not completely decoupled from CVV since > > enteredFullscreenVideo() is called from here. Because of that I still use the > > frame size as an indicator that the metadata is loaded. > > > > On 2016/07/25 22:30:41, watk wrote: > > > > > Whether we should remove from chrome: IMO probably. The number of cases > where > > > users might see it is smaller than ever with Spitzer, because we hide the > > > fullscreen button on the default controls until HAVE_METADATA. And if > they're > > > using custom controls they'll fullscreen a div and won't use CVV anyway. > > > > Yes, and I was not able to see this progress bar until I restored the > fullscreen > > button. Then, can we ever see this progress bar in Spitzer? If not, maybe this > > CL > > is not required? > > > using custom controls they'll fullscreen a div and won't use CVV anyway. > > hrm, it's too bad that we don't have any usage stats on this. i'm worried that > we might be biased thinking about the chrome use-cases rather than webview. > we're only keeping this around because it's supported in the webview api, so > whoever's using that api might not be doing what we expect to see in chrome. > > do we have any idea about how often it's used? i'm guessing no, since we don't > have UMA yet. > > >If not, maybe this CL is not required? > i think the outcome of this should either be a working progress bar in webview, > or no progress bar anywhere. i.e., fix or remove. either is an improvement :) WMPI modified the semantics of hasVideo() method that controls whether we show the fullscreen button or not. In WMPA it tried to figure out the type of the media, explicitly saying it wanted to support enterfullscreen (https://cs.chromium.org/chromium/src/content/renderer/media/android/webmediap...). In WMPI it returns true only after metadata is loaded. My attempt to go full screen programmatically with JS fullscreen API failed because it requires a user gesture: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Fulls... I conclude we need to either restore the semantics of WMPI::hasVideo() or remove WebChromeClient.getVideoLoadingProgressView(). Bo, Selim, what do you think?
On 2016/07/27 20:39:33, Tima Vaisburd wrote: > On 2016/07/27 15:52:11, liberato wrote: > > On 2016/07/27 02:34:01, Tima Vaisburd wrote: > > > > On 2016/07/25 22:10:25, liberato wrote: > > > > > > > if it's off and nobody misses it, then i vote that we leave it off > for > > > > now [...] i meant chrome. > > > > > > Moved the progress bar from ContentVideoView to AwContentViewClient (this is > > > the embedder for WebView) and made AwContentViewClient responsible for > > > displaying it. I modified the embedder interface. > > > > > > However, the progress bar is not completely decoupled from CVV since > > > enteredFullscreenVideo() is called from here. Because of that I still use > the > > > frame size as an indicator that the metadata is loaded. > > > > > > On 2016/07/25 22:30:41, watk wrote: > > > > > > > Whether we should remove from chrome: IMO probably. The number of cases > > where > > > > users might see it is smaller than ever with Spitzer, because we hide the > > > > fullscreen button on the default controls until HAVE_METADATA. And if > > they're > > > > using custom controls they'll fullscreen a div and won't use CVV anyway. > > > > > > Yes, and I was not able to see this progress bar until I restored the > > fullscreen > > > button. Then, can we ever see this progress bar in Spitzer? If not, maybe > this > > > CL > > > is not required? > > > > > using custom controls they'll fullscreen a div and won't use CVV anyway. > > > > hrm, it's too bad that we don't have any usage stats on this. i'm worried > that > > we might be biased thinking about the chrome use-cases rather than webview. > > we're only keeping this around because it's supported in the webview api, so > > whoever's using that api might not be doing what we expect to see in chrome. > > > > do we have any idea about how often it's used? i'm guessing no, since we > don't > > have UMA yet. > > > > >If not, maybe this CL is not required? > > i think the outcome of this should either be a working progress bar in > webview, > > or no progress bar anywhere. i.e., fix or remove. either is an improvement > :) > > WMPI modified the semantics of hasVideo() method that controls whether we show > the fullscreen > button or not. In WMPA it tried to figure out the type of the media, explicitly > saying > it wanted to support enterfullscreen > (https://cs.chromium.org/chromium/src/content/renderer/media/android/webmediap...). > In WMPI it returns true only after metadata is loaded. > My attempt to go full screen programmatically with JS fullscreen API failed > because > it requires a user gesture: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Fulls... > > I conclude we need to either restore the semantics of WMPI::hasVideo() or remove > WebChromeClient.getVideoLoadingProgressView(). > > Bo, Selim, what do you think? I vote for killing getVideoLoadingProgressView.
On 2016/07/27 20:40:57, boliu wrote: > On 2016/07/27 20:39:33, Tima Vaisburd wrote: > > On 2016/07/27 15:52:11, liberato wrote: > > > On 2016/07/27 02:34:01, Tima Vaisburd wrote: > > > > > On 2016/07/25 22:10:25, liberato wrote: > > > > > > > > if it's off and nobody misses it, then i vote that we leave it off > > for > > > > > now [...] i meant chrome. > > > > > > > > Moved the progress bar from ContentVideoView to AwContentViewClient (this > is > > > > the embedder for WebView) and made AwContentViewClient responsible for > > > > displaying it. I modified the embedder interface. > > > > > > > > However, the progress bar is not completely decoupled from CVV since > > > > enteredFullscreenVideo() is called from here. Because of that I still use > > the > > > > frame size as an indicator that the metadata is loaded. > > > > > > > > On 2016/07/25 22:30:41, watk wrote: > > > > > > > > > Whether we should remove from chrome: IMO probably. The number of cases > > > where > > > > > users might see it is smaller than ever with Spitzer, because we hide > the > > > > > fullscreen button on the default controls until HAVE_METADATA. And if > > > they're > > > > > using custom controls they'll fullscreen a div and won't use CVV anyway. > > > > > > > > Yes, and I was not able to see this progress bar until I restored the > > > fullscreen > > > > button. Then, can we ever see this progress bar in Spitzer? If not, maybe > > this > > > > CL > > > > is not required? > > > > > > > using custom controls they'll fullscreen a div and won't use CVV anyway. > > > > > > hrm, it's too bad that we don't have any usage stats on this. i'm worried > > that > > > we might be biased thinking about the chrome use-cases rather than webview. > > > we're only keeping this around because it's supported in the webview api, so > > > whoever's using that api might not be doing what we expect to see in chrome. > > > > > > do we have any idea about how often it's used? i'm guessing no, since we > > don't > > > have UMA yet. > > > > > > >If not, maybe this CL is not required? > > > i think the outcome of this should either be a working progress bar in > > webview, > > > or no progress bar anywhere. i.e., fix or remove. either is an improvement > > :) > > > > WMPI modified the semantics of hasVideo() method that controls whether we show > > the fullscreen > > button or not. In WMPA it tried to figure out the type of the media, > explicitly > > saying > > it wanted to support enterfullscreen > > > (https://cs.chromium.org/chromium/src/content/renderer/media/android/webmediap...). > > In WMPI it returns true only after metadata is loaded. > > My attempt to go full screen programmatically with JS fullscreen API failed > > because > > it requires a user gesture: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Fulls... > > > > I conclude we need to either restore the semantics of WMPI::hasVideo() or > remove > > WebChromeClient.getVideoLoadingProgressView(). > > > > Bo, Selim, what do you think? > > I vote for killing getVideoLoadingProgressView. I like it.
On 2016/07/27 20:43:32, Tima Vaisburd wrote: > On 2016/07/27 20:40:57, boliu wrote: > > On 2016/07/27 20:39:33, Tima Vaisburd wrote: > > > On 2016/07/27 15:52:11, liberato wrote: > > > > On 2016/07/27 02:34:01, Tima Vaisburd wrote: > > > > > > On 2016/07/25 22:10:25, liberato wrote: > > > > > > > > > if it's off and nobody misses it, then i vote that we leave it > off > > > for > > > > > > now [...] i meant chrome. > > > > > > > > > > Moved the progress bar from ContentVideoView to AwContentViewClient > (this > > is > > > > > the embedder for WebView) and made AwContentViewClient responsible for > > > > > displaying it. I modified the embedder interface. > > > > > > > > > > However, the progress bar is not completely decoupled from CVV since > > > > > enteredFullscreenVideo() is called from here. Because of that I still > use > > > the > > > > > frame size as an indicator that the metadata is loaded. > > > > > > > > > > On 2016/07/25 22:30:41, watk wrote: > > > > > > > > > > > Whether we should remove from chrome: IMO probably. The number of > cases > > > > where > > > > > > users might see it is smaller than ever with Spitzer, because we hide > > the > > > > > > fullscreen button on the default controls until HAVE_METADATA. And if > > > > they're > > > > > > using custom controls they'll fullscreen a div and won't use CVV > anyway. > > > > > > > > > > Yes, and I was not able to see this progress bar until I restored the > > > > fullscreen > > > > > button. Then, can we ever see this progress bar in Spitzer? If not, > maybe > > > this > > > > > CL > > > > > is not required? > > > > > > > > > using custom controls they'll fullscreen a div and won't use CVV anyway. > > > > > > > > hrm, it's too bad that we don't have any usage stats on this. i'm worried > > > that > > > > we might be biased thinking about the chrome use-cases rather than > webview. > > > > we're only keeping this around because it's supported in the webview api, > so > > > > whoever's using that api might not be doing what we expect to see in > chrome. > > > > > > > > do we have any idea about how often it's used? i'm guessing no, since we > > > don't > > > > have UMA yet. > > > > > > > > >If not, maybe this CL is not required? > > > > i think the outcome of this should either be a working progress bar in > > > webview, > > > > or no progress bar anywhere. i.e., fix or remove. either is an > improvement > > > :) > > > > > > WMPI modified the semantics of hasVideo() method that controls whether we > show > > > the fullscreen > > > button or not. In WMPA it tried to figure out the type of the media, > > explicitly > > > saying > > > it wanted to support enterfullscreen > > > > > > (https://cs.chromium.org/chromium/src/content/renderer/media/android/webmediap...). > > > In WMPI it returns true only after metadata is loaded. > > > My attempt to go full screen programmatically with JS fullscreen API failed > > > because > > > it requires a user gesture: > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Fulls... > > > > > > I conclude we need to either restore the semantics of WMPI::hasVideo() or > > remove > > > WebChromeClient.getVideoLoadingProgressView(). > > > > > > Bo, Selim, what do you think? > > > > I vote for killing getVideoLoadingProgressView. > > I like it. normally, this means deprecating the API and such things happen in major android releases. I did not read the whole thread though.
On 2016/07/28 21:05:58, sgurun wrote: > On 2016/07/27 20:43:32, Tima Vaisburd wrote: > > On 2016/07/27 20:40:57, boliu wrote: > > > On 2016/07/27 20:39:33, Tima Vaisburd wrote: > > > > On 2016/07/27 15:52:11, liberato wrote: > > > > > On 2016/07/27 02:34:01, Tima Vaisburd wrote: > > > > > > > On 2016/07/25 22:10:25, liberato wrote: > > > > > > > > > > if it's off and nobody misses it, then i vote that we leave it > > off > > > > for > > > > > > > now [...] i meant chrome. > > > > > > > > > > > > Moved the progress bar from ContentVideoView to AwContentViewClient > > (this > > > is > > > > > > the embedder for WebView) and made AwContentViewClient responsible for > > > > > > displaying it. I modified the embedder interface. > > > > > > > > > > > > However, the progress bar is not completely decoupled from CVV since > > > > > > enteredFullscreenVideo() is called from here. Because of that I still > > use > > > > the > > > > > > frame size as an indicator that the metadata is loaded. > > > > > > > > > > > > On 2016/07/25 22:30:41, watk wrote: > > > > > > > > > > > > > Whether we should remove from chrome: IMO probably. The number of > > cases > > > > > where > > > > > > > users might see it is smaller than ever with Spitzer, because we > hide > > > the > > > > > > > fullscreen button on the default controls until HAVE_METADATA. And > if > > > > > they're > > > > > > > using custom controls they'll fullscreen a div and won't use CVV > > anyway. > > > > > > > > > > > > Yes, and I was not able to see this progress bar until I restored the > > > > > fullscreen > > > > > > button. Then, can we ever see this progress bar in Spitzer? If not, > > maybe > > > > this > > > > > > CL > > > > > > is not required? > > > > > > > > > > > using custom controls they'll fullscreen a div and won't use CVV > anyway. > > > > > > > > > > hrm, it's too bad that we don't have any usage stats on this. i'm > worried > > > > that > > > > > we might be biased thinking about the chrome use-cases rather than > > webview. > > > > > we're only keeping this around because it's supported in the webview > api, > > so > > > > > whoever's using that api might not be doing what we expect to see in > > chrome. > > > > > > > > > > do we have any idea about how often it's used? i'm guessing no, since > we > > > > don't > > > > > have UMA yet. > > > > > > > > > > >If not, maybe this CL is not required? > > > > > i think the outcome of this should either be a working progress bar in > > > > webview, > > > > > or no progress bar anywhere. i.e., fix or remove. either is an > > improvement > > > > :) > > > > > > > > WMPI modified the semantics of hasVideo() method that controls whether we > > show > > > > the fullscreen > > > > button or not. In WMPA it tried to figure out the type of the media, > > > explicitly > > > > saying > > > > it wanted to support enterfullscreen > > > > > > > > > > (https://cs.chromium.org/chromium/src/content/renderer/media/android/webmediap...). > > > > In WMPI it returns true only after metadata is loaded. > > > > My attempt to go full screen programmatically with JS fullscreen API > failed > > > > because > > > > it requires a user gesture: > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Fulls... > > > > > > > > I conclude we need to either restore the semantics of WMPI::hasVideo() or > > > remove > > > > WebChromeClient.getVideoLoadingProgressView(). > > > > > > > > Bo, Selim, what do you think? > > > > > > I vote for killing getVideoLoadingProgressView. > > > > I like it. > > normally, this means deprecating the API and such things happen in major android > releases. I did not read the whole thread though. TL;DR is that recent changes to chrome's media stack might cause the progress bar to (almost?) never show up anyway; user-provided progress views won't be used (much?) regardless of any other action we take here. I don't know if we can fix this number at exactly zero or not.
On 2016/07/28 21:24:54, liberato wrote: > On 2016/07/28 21:05:58, sgurun wrote: > > On 2016/07/27 20:43:32, Tima Vaisburd wrote: > > > On 2016/07/27 20:40:57, boliu wrote: > > > > On 2016/07/27 20:39:33, Tima Vaisburd wrote: > > > > > On 2016/07/27 15:52:11, liberato wrote: > > > > > > On 2016/07/27 02:34:01, Tima Vaisburd wrote: > > > > > > > > On 2016/07/25 22:10:25, liberato wrote: > > > > > > > > > > > if it's off and nobody misses it, then i vote that we leave > it > > > off > > > > > for > > > > > > > > now [...] i meant chrome. > > > > > > > > > > > > > > Moved the progress bar from ContentVideoView to AwContentViewClient > > > (this > > > > is > > > > > > > the embedder for WebView) and made AwContentViewClient responsible > for > > > > > > > displaying it. I modified the embedder interface. > > > > > > > > > > > > > > However, the progress bar is not completely decoupled from CVV since > > > > > > > enteredFullscreenVideo() is called from here. Because of that I > still > > > use > > > > > the > > > > > > > frame size as an indicator that the metadata is loaded. > > > > > > > > > > > > > > On 2016/07/25 22:30:41, watk wrote: > > > > > > > > > > > > > > > Whether we should remove from chrome: IMO probably. The number of > > > cases > > > > > > where > > > > > > > > users might see it is smaller than ever with Spitzer, because we > > hide > > > > the > > > > > > > > fullscreen button on the default controls until HAVE_METADATA. And > > if > > > > > > they're > > > > > > > > using custom controls they'll fullscreen a div and won't use CVV > > > anyway. > > > > > > > > > > > > > > Yes, and I was not able to see this progress bar until I restored > the > > > > > > fullscreen > > > > > > > button. Then, can we ever see this progress bar in Spitzer? If not, > > > maybe > > > > > this > > > > > > > CL > > > > > > > is not required? > > > > > > > > > > > > > using custom controls they'll fullscreen a div and won't use CVV > > anyway. > > > > > > > > > > > > hrm, it's too bad that we don't have any usage stats on this. i'm > > worried > > > > > that > > > > > > we might be biased thinking about the chrome use-cases rather than > > > webview. > > > > > > we're only keeping this around because it's supported in the webview > > api, > > > so > > > > > > whoever's using that api might not be doing what we expect to see in > > > chrome. > > > > > > > > > > > > do we have any idea about how often it's used? i'm guessing no, since > > we > > > > > don't > > > > > > have UMA yet. > > > > > > > > > > > > >If not, maybe this CL is not required? > > > > > > i think the outcome of this should either be a working progress bar in > > > > > webview, > > > > > > or no progress bar anywhere. i.e., fix or remove. either is an > > > improvement > > > > > :) > > > > > > > > > > WMPI modified the semantics of hasVideo() method that controls whether > we > > > show > > > > > the fullscreen > > > > > button or not. In WMPA it tried to figure out the type of the media, > > > > explicitly > > > > > saying > > > > > it wanted to support enterfullscreen > > > > > > > > > > > > > > > (https://cs.chromium.org/chromium/src/content/renderer/media/android/webmediap...). > > > > > In WMPI it returns true only after metadata is loaded. > > > > > My attempt to go full screen programmatically with JS fullscreen API > > failed > > > > > because > > > > > it requires a user gesture: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Fulls... > > > > > > > > > > I conclude we need to either restore the semantics of WMPI::hasVideo() > or > > > > remove > > > > > WebChromeClient.getVideoLoadingProgressView(). > > > > > > > > > > Bo, Selim, what do you think? > > > > > > > > I vote for killing getVideoLoadingProgressView. > > > > > > I like it. > > > > normally, this means deprecating the API and such things happen in major > android > > releases. I did not read the whole thread though. > > TL;DR is that recent changes to chrome's media stack might cause the progress > bar to (almost?) never show up anyway; user-provided progress views won't be > used (much?) regardless of any other action we take here. I don't know if we > can fix this number at exactly zero or not. The question is whether this has been broken in webview for as long as chrome (probably yes?). If so, no one has been able to use this for ~ a year. If no, then spitzer effectively breaks this api (in a way that's fairly easily fixable, by plumbing the HAVE_METADATA event). The use case for this API is pretty limited. You have to fullscreen a video element before it has metadata, which isn't possible through the default controls, but is possible with custom controls.
> You have to fullscreen a video > element before it has metadata, which isn't possible through the default > controls, but is possible with custom controls. How to do it exactly? I like to try. It should not be <div> that contains that <video>. > spitzer effectively breaks this api IMO not showing the fullscreen button until the metadata arrives is an improvement in the video element behavior even if it formally broke the existing WebView API.
On 2016/07/28 22:31:30, Tima Vaisburd wrote: > > You have to fullscreen a video > > element before it has metadata, which isn't possible through the default > > controls, but is possible with custom controls. > > How to do it exactly? I like to try. It should not be <div> that contains that > <video>. Try this one http://output.jsbin.com/mifetenugi > > spitzer effectively breaks this api > > IMO not showing the fullscreen button until the metadata arrives is an > improvement > in the video element behavior even if it formally broke the existing WebView > API. Agreed. Actually I wasn't referring to that. Rather when we create a CVV with spitzer we immediately set the size (to a dummy value if we don't yet know it) which hides the progress view https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
On 2016/07/28 22:39:06, watk wrote: > Actually I wasn't referring to that. Rather when we create a CVV with > spitzer we immediately set the size (to a dummy value if we don't yet know it) > which hides the progress view > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... I know. As far as I understood, we pass the natural video size to create surface even it is not initialized: https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=0... and then we call onVideoSizeChanged() with this size right after creation: https://cs.chromium.org/chromium/src/content/browser/media/android/browser_su... > Try this one http://output.jsbin.com/mifetenugi Thank you! Then this CL suddenly becomes relevant? PS5 changes the way we pass and analyse the video size. Please take a look?
lgtm https://codereview.chromium.org/2154883003/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java (right): https://codereview.chromium.org/2154883003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:130: mIsVideoLoaded = videoWidth > 0 && videoHeight > 0; it might be nice to document somewhere that CVV considers the video as not loaded until the size is > 0x0 https://codereview.chromium.org/2154883003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:292: if (mVideoSurfaceView != null) { Do we need the null checks any more? https://codereview.chromium.org/2154883003/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewEmbedder.java (right): https://codereview.chromium.org/2154883003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewEmbedder.java:16: * {@link #needsVideoSurfaceView} must be implemented, needsVideoSurfaceView is not here?
this is not removing the progress view altogether? there are cases it's still needed with spitzer? (didn't read through whole discussion)
boliu@> this is not removing the progress view altogether? boliu@> there are cases it's still needed with spitzer? Chris pointed out that full screen can still be launched with JS, e.g. by fullscreen button as in http://jsbin.com/mifetenugi. That means there can be a case in Spitzer (although probably rare) that video element can go fullscreen before the video content loads. This CL restores a progress view in this case for WebView, but (intentionally) not for Chrome. I thought we might want to have this change while we are deprecating getVideoLoadingProgressView(). https://codereview.chromium.org/2154883003/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java (right): https://codereview.chromium.org/2154883003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:130: mIsVideoLoaded = videoWidth > 0 && videoHeight > 0; On 2016/08/01 19:26:53, watk wrote: > it might be nice to document somewhere that CVV considers the video as not > loaded until the size is > 0x0 Added comments in createContentVideoView() and onVideoSizeChanged(). https://codereview.chromium.org/2154883003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:292: if (mVideoSurfaceView != null) { On 2016/08/01 19:26:53, watk wrote: > Do we need the null checks any more? My bad, removed these checks. https://codereview.chromium.org/2154883003/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewEmbedder.java (right): https://codereview.chromium.org/2154883003/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewEmbedder.java:16: * {@link #needsVideoSurfaceView} must be implemented, On 2016/08/01 19:26:53, watk wrote: > needsVideoSurfaceView is not here? Removed this line.
https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:120: public View getVideoLoadingProgressView() { does this still need to be public and override? https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:202: context.getString(org.chromium.content.R.string.media_player_loading_video); move this resource to android_webview
https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:120: public View getVideoLoadingProgressView() { On 2016/08/03 05:33:21, boliu wrote: > does this still need to be public and override? It was public and override because it was part of ConventVideoViewEmbedder interface. But now we do not call it, and it looks like it makes no sense any more because now the implementor of this interface should create and display the progress view. Removed this method from the interface. https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:202: context.getString(org.chromium.content.R.string.media_player_loading_video); On 2016/08/03 05:33:21, boliu wrote: > move this resource to android_webview I moved the English string, do you know what to do with translations?
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % string question https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:202: context.getString(org.chromium.content.R.string.media_player_loading_video); On 2016/08/03 21:47:23, Tima Vaisburd wrote: > On 2016/08/03 05:33:21, boliu wrote: > > move this resource to android_webview > > I moved the English string, do you know what to do with translations? I don't know actually. This page only says things about adding strings, not moving them: http://www.chromium.org/developers/design-documents/ui-localization Maybe should move all other languages as well? Or ask amineer who handles translations I think
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:202: context.getString(org.chromium.content.R.string.media_player_loading_video); On 2016/08/03 21:52:54, boliu wrote: > On 2016/08/03 21:47:23, Tima Vaisburd wrote: > > On 2016/08/03 05:33:21, boliu wrote: > > > move this resource to android_webview > > > > I moved the English string, do you know what to do with translations? > > I don't know actually. This page only says things about adding strings, not > moving them: http://www.chromium.org/developers/design-documents/ui-localization > Maybe should move all other languages as well? > > Or ask amineer who handles translations I think In *.grd it is referenced by a string, and in translations by some id number, i.e. <message name="IDS_LICENSE_ACTIVITY_TITLE" ... > (English) <translation id="3572484393913897457">System-WebView-Lizenzen</translation> (German) That stopped me. I'll try to figure out.
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/03 22:03:35, Tima Vaisburd wrote: > https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... > File > android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java > (right): > > https://codereview.chromium.org/2154883003/diff/100001/android_webview/java/s... > android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:202: > context.getString(org.chromium.content.R.string.media_player_loading_video); > On 2016/08/03 21:52:54, boliu wrote: > > On 2016/08/03 21:47:23, Tima Vaisburd wrote: > > > On 2016/08/03 05:33:21, boliu wrote: > > > > move this resource to android_webview > > > > > > I moved the English string, do you know what to do with translations? > > > > I don't know actually. This page only says things about adding strings, not > > moving them: > http://www.chromium.org/developers/design-documents/ui-localization > > Maybe should move all other languages as well? > > > > Or ask amineer who handles translations I think > > In *.grd it is referenced by a string, and in translations by some id number, > i.e. > <message name="IDS_LICENSE_ACTIVITY_TITLE" ... > (English) > <translation id="3572484393913897457">System-WebView-Lizenzen</translation> > (German) > > That stopped me. I'll try to figure out. According to amineer@ it will be done automatically by translation scripts later, but needs manual verification
timav@chromium.org changed reviewers: + tedchoc@chromium.org
Ted, owner review, please.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
On 2016/08/04 01:09:56, Tima Vaisburd wrote: > Ted, owner review, please. I meant "Could you please make an owner review?". Sorry, the phrase above looks pretty rude.
lgtm...not blocking on my api comment and the other things are trivial nits https://codereview.chromium.org/2154883003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java (right): https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:135: mEmbedder.enterFullscreenVideo(this, mIsVideoLoaded); I find this api slightly odd. Is the thinking that we pass mIsVideoLoaded here to avoid clients from inflating their progress views and doing extra work? Is that really a big issue? I'm just wondering why we don't just leave it as enterFullscreenVideo and then rely on the onVideoSizeChanged call that happens right after this to trigger fullscreenVideoLoaded if need be. The worst I see is that you're potentially unnecessarily inflating a view, adding it to the view hierarchy and then detaching it immediately. I don't feel terribly strongly here, but it does seem unfortunate that you've got the same information passed via two different mechanisms. https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:157: this.addView( you shouldn't need "this." here https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:159: ViewGroup.LayoutParams.WRAP_CONTENT, Gravity.CENTER)); indenting here should be 8 from the start of the previous line. https://codereview.chromium.org/2154883003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewEmbedder.java (right): https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewEmbedder.java:27: * When it is false, embedder might show the progress view. If you wrap on javadocs, align it with the sentence above (in this case "The flag")
https://codereview.chromium.org/2154883003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java (right): https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:135: mEmbedder.enterFullscreenVideo(this, mIsVideoLoaded); On 2016/08/05 00:19:05, Ted C wrote: > Is the thinking that we pass mIsVideoLoaded here to avoid clients from inflating > their progress views and doing extra work? Is that really a big issue? I'm not sure that I completely understood you; the idea here was that this progress view should appear for a short period of time after the user pressed Play till the moment some video actually appears. It is noticeable only on slow connections. This situation can only happen in the beginning, if a user went to fullscreen before the start. If they went to fullscreen in the middle of the playback, we should not show anything (if we did show, there would be no event that could trigger the hiding). The parameter |mIsVideoLoaded| tells whether the video is already loaded at the moment the user enters fullscreen. If it is not, we show the progress bar and rely on subsequent onVideoSizeChanged() to hide it. If it is already loaded, we do not show. > I'm just wondering why we don't just leave it as enterFullscreenVideo and then > rely on the onVideoSizeChanged call that happens right after this to trigger > fullscreenVideoLoaded if need be. That's what it does. fullscreenVideoLoaded is triggered by onVideoSizeChanged, exactly as you said, and hides the progress view. In other words, it is an attempt at show/hide protocol. https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:157: this.addView( On 2016/08/05 00:19:05, Ted C wrote: > you shouldn't need "this." here Done. https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:159: ViewGroup.LayoutParams.WRAP_CONTENT, Gravity.CENTER)); On 2016/08/05 00:19:05, Ted C wrote: > indenting here should be 8 from the start of the previous line. "git cl format" insists on this formatting (slightly improved after removing |this|). I do not know how to make it cooperate. https://codereview.chromium.org/2154883003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewEmbedder.java (right): https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewEmbedder.java:27: * When it is false, embedder might show the progress view. On 2016/08/05 00:19:05, Ted C wrote: > If you wrap on javadocs, align it with the sentence above (in this case "The > flag") Done.
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2154883003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java (right): https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:135: mEmbedder.enterFullscreenVideo(this, mIsVideoLoaded); On 2016/08/05 01:24:01, Tima Vaisburd wrote: > On 2016/08/05 00:19:05, Ted C wrote: > > Is the thinking that we pass mIsVideoLoaded here to avoid clients from > inflating > > their progress views and doing extra work? Is that really a big issue? > > I'm not sure that I completely understood you; the idea here was that this > progress view should appear for a short period of time after the user pressed > Play till the moment some video actually appears. It is noticeable only on slow > connections. > > This situation can only happen in the beginning, if a user went to fullscreen > before the start. If they went to fullscreen in the middle of the playback, we > should not show anything (if we did show, there would be no event that could > trigger the hiding). > > The parameter |mIsVideoLoaded| tells whether the video is already loaded at the > moment the user enters fullscreen. If it is not, we show the progress bar and > rely on subsequent onVideoSizeChanged() to hide it. If it is already loaded, we > do not show. > > > I'm just wondering why we don't just leave it as enterFullscreenVideo and then > > rely on the onVideoSizeChanged call that happens right after this to trigger > > fullscreenVideoLoaded if need be. > > That's what it does. fullscreenVideoLoaded is triggered by onVideoSizeChanged, > exactly as you said, and hides the progress view. > > In other words, it is an attempt at show/hide protocol. The parameter is an optimization as I see it. If you were to remove the parameter, a loaded video would trigger for the embedder: enterFullscreenVideo followed immediately by fullscreenVideoLoaded. They would happen in the same block of execution, so you might be potenitally doing work in enterFullscreenVideo (building a progress view, attaching it to the view hierarchy, etc...) that is immediately discarded when you get fullscreenVideoLoaded. The flag is just an optimization to avoid the work in enterFullscreenVideo that will be cleared by fullscreenVideoLoaded. I think it's fine, but I find it to be a bit weird and feels somewhat like an early optimization. With that said, I'm fine with your decision here...it's just my opinion. https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:159: ViewGroup.LayoutParams.WRAP_CONTENT, Gravity.CENTER)); On 2016/08/05 01:24:01, Tima Vaisburd wrote: > On 2016/08/05 00:19:05, Ted C wrote: > > indenting here should be 8 from the start of the previous line. > > "git cl format" insists on this formatting (slightly improved after removing > |this|). I do not know how to make it cooperate. git cl format doesn't work for java files reliably
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2154883003/diff/160001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java (right): https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:135: mEmbedder.enterFullscreenVideo(this, mIsVideoLoaded); On 2016/08/05 17:36:31, Ted C wrote: > If you were to remove the parameter, a loaded video would trigger for the > embedder: > > enterFullscreenVideo followed immediately by fullscreenVideoLoaded. Yes, you are right, onVideoSizeChanged() is always called after fullscreen video creation (https://cs.chromium.org/chromium/src/content/browser/media/android/browser_su...). > With that said, I'm fine with your decision here...it's just my opinion. Thank you, I would prefer to keep it as it is to avoid potential blinking. https://codereview.chromium.org/2154883003/diff/160001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:159: ViewGroup.LayoutParams.WRAP_CONTENT, Gravity.CENTER)); On 2016/08/05 17:36:31, Ted C wrote: > On 2016/08/05 01:24:01, Tima Vaisburd wrote: > > On 2016/08/05 00:19:05, Ted C wrote: > > > indenting here should be 8 from the start of the previous line. > > > > "git cl format" insists on this formatting (slightly improved after removing > > |this|). I do not know how to make it cooperate. > > git cl format doesn't work for java files reliably Ted, how can I bypass the presubmit warning then? I also get a warning "Avoid android.app.AlertDialog", do you know how to bypass it too, or is it possible to use android.support.v7.app.AlertDialog in WebView (see https://codereview.chromium.org/1804293002/) ?
On 2016/08/05 20:13:57, Tima Vaisburd wrote: > https://codereview.chromium.org/2154883003/diff/160001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java > (right): > > https://codereview.chromium.org/2154883003/diff/160001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:135: > mEmbedder.enterFullscreenVideo(this, mIsVideoLoaded); > On 2016/08/05 17:36:31, Ted C wrote: > > If you were to remove the parameter, a loaded video would trigger for the > > embedder: > > > > enterFullscreenVideo followed immediately by fullscreenVideoLoaded. > > Yes, you are right, onVideoSizeChanged() is always called after fullscreen video > creation > (https://cs.chromium.org/chromium/src/content/browser/media/android/browser_su...). > > > > With that said, I'm fine with your decision here...it's just my opinion. > > Thank you, I would prefer to keep it as it is to avoid potential blinking. > > https://codereview.chromium.org/2154883003/diff/160001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:159: > ViewGroup.LayoutParams.WRAP_CONTENT, Gravity.CENTER)); > On 2016/08/05 17:36:31, Ted C wrote: > > On 2016/08/05 01:24:01, Tima Vaisburd wrote: > > > On 2016/08/05 00:19:05, Ted C wrote: > > > > indenting here should be 8 from the start of the previous line. > > > > > > "git cl format" insists on this formatting (slightly improved after removing > > > |this|). I do not know how to make it cooperate. > > > > git cl format doesn't work for java files reliably > > Ted, how can I bypass the presubmit warning then? > I also get a warning "Avoid android.app.AlertDialog", do you know how to bypass > it too, or is it possible to use android.support.v7.app.AlertDialog in WebView > (see https://codereview.chromium.org/1804293002/) ? I'm pretty sure if you indented as I suggested it wouldn't warn you about that. I think our checkstyle rules are more conforming than git cl format. In android, you should "always" indent 8 from the start of the previous line. I've never seen it complain otherwise. I honestly don't know about using the support library in WebView. Based on my greps of android.support.* it doesn't seem to be used in content or webview, so I suspect you can't. If you get warnings about presubmit, then you can ignore them. If they prevent you from uploading, git cl upload --bypass-hooks
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, boliu@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2154883003/#ps200001 (title: "Fixed Java continuation lines identation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
On 2016/08/05 20:34:57, Ted C wrote: > If you get warnings about presubmit, then you can ignore them. If they > prevent you from uploading, git cl upload --bypass-hooks Uploading went ok, but presubmit step failed on commit stage. The only response I see in the log is the warning about the Dialog. Do you have any idea how to make the commit going?
Description was changed from ========== Make ContentVideoView progress view visible in WebView The progress view used to be part of ContentVideoView underlay. In the current Spitzer implementation for WebView FullScreenView got opaque content which now obstructs the progress bar. This CL removes the progress bar from ContentVideoView and makes the ContentVideoViewEmbedder responsible for its creation and display. As well, the progress bar remained in WebView only and has been removed from Chrome. BUG=628828 ========== to ========== Make ContentVideoView progress view visible in WebView The progress view used to be part of ContentVideoView underlay. In the current Spitzer implementation for WebView FullScreenView got opaque content which now obstructs the progress bar. This CL removes the progress bar from ContentVideoView and makes the ContentVideoViewEmbedder responsible for its creation and display. As well, the progress bar remained in WebView only and has been removed from Chrome. BUG=628828 NOPRESUBMIT=true ==========
The CQ bit was checked by timav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make ContentVideoView progress view visible in WebView The progress view used to be part of ContentVideoView underlay. In the current Spitzer implementation for WebView FullScreenView got opaque content which now obstructs the progress bar. This CL removes the progress bar from ContentVideoView and makes the ContentVideoViewEmbedder responsible for its creation and display. As well, the progress bar remained in WebView only and has been removed from Chrome. BUG=628828 NOPRESUBMIT=true ========== to ========== Make ContentVideoView progress view visible in WebView The progress view used to be part of ContentVideoView underlay. In the current Spitzer implementation for WebView FullScreenView got opaque content which now obstructs the progress bar. This CL removes the progress bar from ContentVideoView and makes the ContentVideoViewEmbedder responsible for its creation and display. As well, the progress bar remained in WebView only and has been removed from Chrome. BUG=628828 NOPRESUBMIT=true ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Make ContentVideoView progress view visible in WebView The progress view used to be part of ContentVideoView underlay. In the current Spitzer implementation for WebView FullScreenView got opaque content which now obstructs the progress bar. This CL removes the progress bar from ContentVideoView and makes the ContentVideoViewEmbedder responsible for its creation and display. As well, the progress bar remained in WebView only and has been removed from Chrome. BUG=628828 NOPRESUBMIT=true ========== to ========== Make ContentVideoView progress view visible in WebView The progress view used to be part of ContentVideoView underlay. In the current Spitzer implementation for WebView FullScreenView got opaque content which now obstructs the progress bar. This CL removes the progress bar from ContentVideoView and makes the ContentVideoViewEmbedder responsible for its creation and display. As well, the progress bar remained in WebView only and has been removed from Chrome. BUG=628828 NOPRESUBMIT=true Committed: https://crrev.com/ee5c228c4753bd80e1f6f14e7cbc7ee3c673b00c Cr-Commit-Position: refs/heads/master@{#410234} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ee5c228c4753bd80e1f6f14e7cbc7ee3c673b00c Cr-Commit-Position: refs/heads/master@{#410234} |
