Partial revert of "Restart fullscreen video playback when switching back from background"

Partial revert of "Restart fullscreen video playback when switching back from background"

This CL partially reverts r242570 and r243346.

The functionality added in those patches has been removed while some refactoring part
has been kept because those make sense and also because some code came later would have
to be moved otherwise.

The suspend/resume fullscreen video logic does not work well when the browser
activity is destroyed when it is in the background. Unfortunately I could not
find a way to make it work for that case yet so it's better remove it.

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

[kbalazs, 2014-02-11 23:05:39]

PTAL

[kbalazs, 2014-02-11 23:06:12]

On 2014/02/11 23:05:39, kbalazs wrote:
> PTAL

Look at my monologues at https://codereview.chromium.org/103583005/ for context
:)

[Ted C, 2014-02-13 03:55:07]

lgtm

[bulach, 2014-02-13 12:02:43]

thanks! I think my comments are mostly about the description rather than the
patch per se, but anyways, thanks for the cleanup!

https://codereview.chromium.org/157783004/diff/1/content/browser/android/cont...
File content/browser/android/content_video_view.h (left):

https://codereview.chromium.org/157783004/diff/1/content/browser/android/cont...
content/browser/android/content_video_view.h:33: BrowserMediaPlayerManager*
manager);
hmm... in the patches referenced in the description, the changes were to add the
following constructor, plus changing "bool HasContentVideoView()" with
"GetInstance()"..

I'm fine with keeping these changes, however I'd update the description to say
something like "Clear up after patches x and y" rather than "Revert ..."
the "revert" is normally associated with a straightforwad revert, normally
automatically by drover.

https://codereview.chromium.org/157783004/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/157783004/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:501:
video_view_.reset(new ContentVideoView(this));
ditto..

https://codereview.chromium.org/157783004/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.h (left):

https://codereview.chromium.org/157783004/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.h:52:
ContentViewCoreImpl* GetContentViewCore() const;
nit: ditto..

[kbalazs, 2014-02-13 18:11:15]

On 2014/02/13 12:02:43, bulach wrote:
> thanks! I think my comments are mostly about the description rather than the
> patch per se, but anyways, thanks for the cleanup!
> 
>
https://codereview.chromium.org/157783004/diff/1/content/browser/android/cont...
> File content/browser/android/content_video_view.h (left):
> 
>
https://codereview.chromium.org/157783004/diff/1/content/browser/android/cont...
> content/browser/android/content_video_view.h:33: BrowserMediaPlayerManager*
> manager);
> hmm... in the patches referenced in the description, the changes were to add
the
> following constructor, plus changing "bool HasContentVideoView()" with
> "GetInstance()"..
> 
> I'm fine with keeping these changes, however I'd update the description to say
> something like "Clear up after patches x and y" rather than "Revert ..."
> the "revert" is normally associated with a straightforwad revert, normally
> automatically by drover.

Right, I changed the description. I think the constructor looks nicer with the
refactor so I kept it. The HasContentVideoView() -> GetInstance() change is kind
of neutral imho but I would need the latter in case I will be able at some point
to reland a fixed version of the original change.

[kbalazs, 2014-02-13 18:11:58]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-02-13 18:13:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/157783004/1

[commit-bot: I haz the power, 2014-02-13 18:13:32]

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

[commit-bot: I haz the power, 2014-02-13 18:13:33]

Failed to apply patch for content/browser/android/content_video_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/android/content_video_view.cc
  Hunk #1 succeeded at 45 (offset 1 line).
  Hunk #2 FAILED at 160.
  1 out of 2 hunks FAILED -- saving rejects to file
content/browser/android/content_video_view.cc.rej

Patch:       content/browser/android/content_video_view.cc
Index: content/browser/android/content_video_view.cc
diff --git a/content/browser/android/content_video_view.cc
b/content/browser/android/content_video_view.cc
index
af3067a2094b4d2dc014125e7bd987948fa25dd1..d774a4e5dc5af3955d1bf488965b48e94aef1cfb
100644
--- a/content/browser/android/content_video_view.cc
+++ b/content/browser/android/content_video_view.cc
@@ -44,7 +44,6 @@ ContentVideoView* ContentVideoView::GetInstance() {
 ContentVideoView::ContentVideoView(
     BrowserMediaPlayerManager* manager)
     : manager_(manager),
-      fullscreen_state_(ENTERED),
       weak_factory_(this) {
   DCHECK(!g_content_video_view);
   j_content_video_view_ = CreateJavaObject();
@@ -161,40 +160,14 @@ void ContentVideoView::Pause(JNIEnv*, jobject obj) {
 
 void ContentVideoView::ExitFullscreen(
     JNIEnv*, jobject, jboolean release_media_player) {
-  if (fullscreen_state_ == SUSPENDED)
-    return;
   j_content_video_view_.reset();
   manager_->ExitFullscreen(release_media_player);
 }
 
-void ContentVideoView::SuspendFullscreen() {
-  if (fullscreen_state_ != ENTERED)
-    return;
-  fullscreen_state_ = SUSPENDED;
-  DestroyContentVideoView(false);
-  manager_->SuspendFullscreen();
-}
-
-void ContentVideoView::ResumeFullscreenIfSuspended() {
-  if (fullscreen_state_ != SUSPENDED)
-    return;
-  JNIEnv* env = AttachCurrentThread();
-  DCHECK(!GetJavaObject(env).obj());
-  fullscreen_state_ = RESUME;
-  j_content_video_view_ = CreateJavaObject();
-}
-
 void ContentVideoView::SetSurface(JNIEnv* env, jobject obj,
                                   jobject surface) {
-  gfx::ScopedJavaSurface scoped_surface =
-      gfx::ScopedJavaSurface::AcquireExternalSurface(surface);
-  if (fullscreen_state_ == RESUME) {
-    DCHECK(surface);
-    manager_->ResumeFullscreen(scoped_surface.Pass());
-    fullscreen_state_ = ENTERED;
-  } else {
-    manager_->SetVideoSurface(scoped_surface.Pass());
-  }
+  manager_->SetVideoSurface(
+      gfx::ScopedJavaSurface::AcquireExternalSurface(surface));
 }
 
 void ContentVideoView::RequestMediaMetadata(JNIEnv* env, jobject obj) {

[kbalazs, 2014-02-13 22:25:43]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-02-13 22:32:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/157783004/130001

[commit-bot: I haz the power, 2014-02-13 23:06:06]

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

[commit-bot: I haz the power, 2014-02-13 23:06:06]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[kbalazs, 2014-02-14 00:07:48]

@bulach: needs an lgtm from you :)

[bulach, 2014-02-14 09:24:11]

lgtm, thanks for updating the description! I agree with keeping some of the
fixes...

[kbalazs, 2014-02-14 17:17:31]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-02-14 17:19:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/157783004/130001

[commit-bot: I haz the power, 2014-02-14 17:44:02]

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

[commit-bot: I haz the power, 2014-02-14 17:44:03]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[kbalazs, 2014-02-14 18:39:57]

Could someone rubber-stamp it please? Still missing an owner review from
content/browser/media.

[kbalazs, 2014-02-14 18:42:19]

On 2014/02/14 18:39:57, kbalazs wrote:
> Could someone rubber-stamp it please? Still missing an owner review from
> content/browser/media.

Btw, shouldn't Ted be an owner of content/browser/media/android? :)

[Ami GONE FROM CHROMIUM, 2014-02-14 19:03:08]

c/b/m rubberstamp lgtm % please update the CL description to name the svn
revisions being partially reverted here, instead of using git sha1s which are
less canonical / easily findable.

[kbalazs, 2014-02-14 19:33:07]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-02-14 19:34:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/157783004/130001

[commit-bot: I haz the power, 2014-02-15 00:33:28]

Change committed as 251455