Upstream FullscreenManager

Upstream FullscreenManager

And upstream logics related to FullscreenManager from ChromeTab to Tab.

BUG=440613

[Jaekyun Seok (inactive), 2014-12-15 23:39:42]

jaekyun@chromium.org changed reviewers:
+ changwan@chromium.org, dtrainor@chromium.org, tedchoc@chromium.org

[Jaekyun Seok (inactive), 2014-12-15 23:39:42]

Ted, David and Changwan, please review this change.

Thanks,

[Ted C, 2014-12-16 23:04:30]

Awesome!  Looking forward to this being upstream.  A few little comments, but
overall looks good.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:170: protected
FullscreenManager mFullscreenManager;
I'd rather have a private variable and a protected getter.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:313: :
mFullscreenManager.getPersistentFullscreenMode();
This is c++ style indenting, so this should be 8 from the start of the previous
line.

I would vote for putting the ? and everything after on the second line actually.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1321: protected
void hideControlsInFullscreen() {
I "think" this is for clearing the fullscreen state when navigating right (i.e.
didStartPageLoad).

If so, this name isn't actually what this function does.

It just removes the persistent show token for a hung renderer and allows the
controls to move based on other constraints.  This could be just as easily
called "clearHungRendererState" or something better.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1387: protected
void updateTopControlsState(int constraints, int current, boolean animate) {}
I think the native side of this could easily go upstream now too

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
File chrome/android/java/src/org/chromium/chrome/browser/widget/TextBubble.java
(right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widget/TextBubble.java:239:
// Not supported
I would remove this comment as it's not actually accurate (it's more like not
needed than not supported, but both are unnecessary).

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java:5:
package org.chromium.chrome.browser.widgetcontroller.fullscreen;
let's drop widget controller and just do browser.fullscreen.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java:46:
private static final String TAG = "FullscreenManager";
can we update to ChromeFullscreenManager since you're here :-)

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java:473:
/*
can you add an extra * to make this a javadoc comment.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/FullscreenManager.java
(right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/FullscreenManager.java:100:
/*
add a * here to make it javadoc.

Same for isOverlayVideoMode below.

[David Trainor- moved to gerrit, 2014-12-16 23:13:55]

+1.  Looks fine pending Ted's comments.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/res/anim...
File chrome/android/java/res/anim/fullscreen_notification_in.xml (right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/res/anim...
chrome/android/java/res/anim/fullscreen_notification_in.xml:2: <!-- Copyright
(c) 2013 The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java:1:
// Copyright 2012 The Chromium Authors. All rights reserved.
2014.  Same with other files.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java:46:
private static final String TAG = "FullscreenManager";
On 2014/12/16 23:04:29, Ted C wrote:
> can we update to ChromeFullscreenManager since you're here :-)

Does anything use this variable?  If not just erase it IMO.

[Jaekyun Seok (inactive), 2014-12-17 01:21:09]

PTAL.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/res/anim...
File chrome/android/java/res/anim/fullscreen_notification_in.xml (right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/res/anim...
chrome/android/java/res/anim/fullscreen_notification_in.xml:2: <!-- Copyright
(c) 2013 The Chromium Authors. All rights reserved.
On 2014/12/16 23:13:54, David Trainor wrote:
> 2014

Done.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:170: protected
FullscreenManager mFullscreenManager;
On 2014/12/16 23:04:29, Ted C wrote:
> I'd rather have a private variable and a protected getter.

Done.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:313: :
mFullscreenManager.getPersistentFullscreenMode();
On 2014/12/16 23:04:29, Ted C wrote:
> This is c++ style indenting, so this should be 8 from the start of the
previous
> line.
> 
> I would vote for putting the ? and everything after on the second line
actually.

Done.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1321: protected
void hideControlsInFullscreen() {
On 2014/12/16 23:04:29, Ted C wrote:
> I "think" this is for clearing the fullscreen state when navigating right
(i.e.
> didStartPageLoad).
> 
> If so, this name isn't actually what this function does.
> 
> It just removes the persistent show token for a hung renderer and allows the
> controls to move based on other constraints.  This could be just as easily
> called "clearHungRendererState" or something better.

Done.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1387: protected
void updateTopControlsState(int constraints, int current, boolean animate) {}
On 2014/12/16 23:04:29, Ted C wrote:
> I think the native side of this could easily go upstream now too

Done.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
File chrome/android/java/src/org/chromium/chrome/browser/widget/TextBubble.java
(right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widget/TextBubble.java:239:
// Not supported
On 2014/12/16 23:04:29, Ted C wrote:
> I would remove this comment as it's not actually accurate (it's more like not
> needed than not supported, but both are unnecessary).

Done.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java:1:
// Copyright 2012 The Chromium Authors. All rights reserved.
On 2014/12/16 23:13:55, David Trainor wrote:
> 2014.  Same with other files.

Done.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java:5:
package org.chromium.chrome.browser.widgetcontroller.fullscreen;
On 2014/12/16 23:04:29, Ted C wrote:
> let's drop widget controller and just do browser.fullscreen.

Done.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java:46:
private static final String TAG = "FullscreenManager";
On 2014/12/16 23:13:55, David Trainor wrote:
> On 2014/12/16 23:04:29, Ted C wrote:
> > can we update to ChromeFullscreenManager since you're here :-)
> 
> Does anything use this variable?  If not just erase it IMO.

I will remove it.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/ChromeFullscreenManager.java:473:
/*
On 2014/12/16 23:04:30, Ted C wrote:
> can you add an extra * to make this a javadoc comment.

Done.

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/FullscreenManager.java
(right):

https://codereview.chromium.org/807683002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/widgetcontroller/fullscreen/FullscreenManager.java:100:
/*
On 2014/12/16 23:04:30, Ted C wrote:
> add a * here to make it javadoc.
> 
> Same for isOverlayVideoMode below.

Done.

[Jaekyun Seok (inactive), 2014-12-17 01:34:49]

I found some style errors in my latest patch.

BTW, I mistakenly removed my working branch. So I tried to cherry-pick this CL,
but I failed with the following error.

fatal: git diff header lacks filename information when removing 0 leading
pathname components (line 30)
Failed to apply the patch

Does anyone know how to fix this issue?

https://codereview.chromium.org/807683002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenManager.java
(right):

https://codereview.chromium.org/807683002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenManager.java:108:
/*
Need '*'

https://codereview.chromium.org/807683002/diff/20001/chrome/browser/android/t...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/807683002/diff/20001/chrome/browser/android/t...
chrome/browser/android/tab_android.cc:729: }
Need a new line.

[Ted C, 2014-12-17 01:49:21]

On 2014/12/17 01:34:49, Jaekyun Seok wrote:
> I found some style errors in my latest patch.
> 
> BTW, I mistakenly removed my working branch. So I tried to cherry-pick this
CL,
> but I failed with the following error.
> 
> fatal: git diff header lacks filename information when removing 0 leading
> pathname components (line 30)
> Failed to apply the patch
> 
> Does anyone know how to fix this issue?
> 
>
https://codereview.chromium.org/807683002/diff/20001/chrome/android/java/src/...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenManager.java
> (right):
> 
>
https://codereview.chromium.org/807683002/diff/20001/chrome/android/java/src/...
>
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenManager.java:108:
> /*
> Need '*'
> 
>
https://codereview.chromium.org/807683002/diff/20001/chrome/browser/android/t...
> File chrome/browser/android/tab_android.cc (right):
> 
>
https://codereview.chromium.org/807683002/diff/20001/chrome/browser/android/t...
> chrome/browser/android/tab_android.cc:729: }
> Need a new line.

Sadly, I'm seeing the same.

-v to git cl patch did not provide any more useful info for me to help here.

[Ted C, 2014-12-17 01:51:02]

On 2014/12/17 01:49:21, Ted C wrote:
> On 2014/12/17 01:34:49, Jaekyun Seok wrote:
> > I found some style errors in my latest patch.
> > 
> > BTW, I mistakenly removed my working branch. So I tried to cherry-pick this
> CL,
> > but I failed with the following error.
> > 
> > fatal: git diff header lacks filename information when removing 0 leading
> > pathname components (line 30)
> > Failed to apply the patch
> > 
> > Does anyone know how to fix this issue?
> > 
> >
>
https://codereview.chromium.org/807683002/diff/20001/chrome/android/java/src/...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenManager.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/807683002/diff/20001/chrome/android/java/src/...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenManager.java:108:
> > /*
> > Need '*'
> > 
> >
>
https://codereview.chromium.org/807683002/diff/20001/chrome/browser/android/t...
> > File chrome/browser/android/tab_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/807683002/diff/20001/chrome/browser/android/t...
> > chrome/browser/android/tab_android.cc:729: }
> > Need a new line.
> 
> Sadly, I'm seeing the same.
> 
> -v to git cl patch did not provide any more useful info for me to help here.

With newt@'s help, it turns out this is from having binary files in the patch.

You can download the raw patch (from the web ui) and delete the binary bits at
the top and do the regular git commands for applying a patch file.

[Jaekyun Seok (inactive), 2014-12-17 04:04:46]

> With newt@'s help, it turns out this is from having binary files in the patch.
>
> You can download the raw patch (from the web ui) and delete the binary bits at
> the top and do the regular git commands for applying a patch file.

I was able to apply the raw patch, but a new issue was created when I uploaded
it.

So please review https://codereview.chromium.org/810853003/ instead of this CL.

[Ted C, 2014-12-17 17:24:17]

On 2014/12/17 04:04:46, Jaekyun Seok wrote:
> > With newt@'s help, it turns out this is from having binary files in the
patch.
> >
> > You can download the raw patch (from the web ui) and delete the binary bits
at
> > the top and do the regular git commands for applying a patch file.
> 
> I was able to apply the raw patch, but a new issue was created when I uploaded
> it.
> 
> So please review https://codereview.chromium.org/810853003/ instead of this
CL.

I'm surprised:
git cl issue 807683002
wouldn't have worked and let you upload to this patch again after even the raw
patch.

Oh well, I'll review the other one.

[Jaekyun Seok (inactive), 2014-12-17 20:24:26]

On 2014/12/17 17:24:17, Ted C wrote:
> I'm surprised:
> git cl issue 807683002
> wouldn't have worked and let you upload to this patch again after even the raw
> patch.

I didn't know this. I will use it next time.

> 
> Oh well, I'll review the other one.

Thanks.