Allow fullscreen manager and control container to be null

Allow fullscreen manager and control container to be null

Also fixes webapp top controls flashing issue which sprang back after this change.

BUG=430635, 463957
Committed: https://crrev.com/5e148eb4f7679c85daa8e0d314dfd636f739b0d8
Cr-Commit-Position: refs/heads/master@{#321253}

[Changwan Ryu, 2015-03-09 06:01:37]

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

[David Trainor- moved to gerrit, 2015-03-09 17:59:30]

lgtm

[Ted C, 2015-03-09 19:59:15]

not lgtm

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

https://codereview.chromium.org/986963002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:231:
mControlContainer = controlContainer;
There are a ton of places in this file that assume this is non-null.  I don't
think we want to allow this.

For example, html5 fullscreen will probably crash with this change.

[Changwan Ryu, 2015-03-13 08:35:52]

PTAL

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

https://codereview.chromium.org/986963002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:231:
mControlContainer = controlContainer;
On 2015/03/09 19:59:15, Ted C wrote:
> There are a ton of places in this file that assume this is non-null.  I don't
> think we want to allow this.
> 
> For example, html5 fullscreen will probably crash with this change.

Since we disable fullscreen manager when we set control container, those places
should never be reached. Verified that html5 fullscreen does not crash when I
change embedcontentviewactivity's URL to webapp example URL. But I agree that it
is not very obvious in the code flow now. Removed setControlContainer() function
and added some more checks . Please take another look.

[Ted C, 2015-03-16 18:29:27]

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

https://codereview.chromium.org/986963002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:210:
assert !enabled || controlContainer != null;
I still don't like this.  This class just shouldn't be used in cases where
enabled = false (and in fact, no one has set this boolean to false in a long
time).

enabled and persistentFullscreenSupported should both be deleted as params and
this class should only be used where fullscreen is full enabled.

Downstream, why can't we fully support a null chrome fullscreen manager instead?
 It seems like most places already handle null correctly, so maybe we can just
flush that out?

[Changwan Ryu, 2015-03-17 10:06:21]

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

https://codereview.chromium.org/986963002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:210:
assert !enabled || controlContainer != null;
On 2015/03/16 18:29:27, Ted C wrote:
> I still don't like this.  This class just shouldn't be used in cases where
> enabled = false (and in fact, no one has set this boolean to false in a long
> time).
> 
> enabled and persistentFullscreenSupported should both be deleted as params and
> this class should only be used where fullscreen is full enabled.
> 
> Downstream, why can't we fully support a null chrome fullscreen manager
instead?
>  It seems like most places already handle null correctly, so maybe we can just
> flush that out?

Done.

[Ted C, 2015-03-17 23:50:39]

need to update the commit message now.  But this seems much better to me.

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (left):

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2413: if
(mFullscreenManager == null) return TopControlsState.SHOWN;
why remove this bit?

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(left):

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:196:
public ChromeFullscreenManager(Activity activity, View controlContainer, boolean
enabled,
probably should keep this constructor around if it will allow you to commit this
change without rolling downstream at the same time.

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:276:
tab.updateTopControlsState(TopControlsState.BOTH, true);
after exiting fullscreen, we want to show the top controls, and I think this
changes that behavior.

[Changwan Ryu, 2015-03-18 01:11:03]

PTAL

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (left):

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2413: if
(mFullscreenManager == null) return TopControlsState.SHOWN;
On 2015/03/17 23:50:38, Ted C wrote:
> why remove this bit?

With webapp and embed content, top controls may be hidden even when full screen
is disabled.

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(left):

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:196:
public ChromeFullscreenManager(Activity activity, View controlContainer, boolean
enabled,
On 2015/03/17 23:50:38, Ted C wrote:
> probably should keep this constructor around if it will allow you to commit
this
> change without rolling downstream at the same time.

Done.

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
(right):

https://codereview.chromium.org/986963002/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:276:
tab.updateTopControlsState(TopControlsState.BOTH, true);
On 2015/03/17 23:50:38, Ted C wrote:
> after exiting fullscreen, we want to show the top controls, and I think this
> changes that behavior.

Changed back to SHOWN.

[Ted C, 2015-03-18 17:47:50]

lgtm

[Changwan Ryu, 2015-03-19 00:03:43]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2015-03-19 00:03:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/986963002/#ps80001 (title:
"addressed more comments from tedchoc")

[commit-bot: I haz the power, 2015-03-19 00:05:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/986963002/80001

[commit-bot: I haz the power, 2015-03-19 00:34:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-19 00:35:31]

Patchset 5 (id:??) landed as
https://crrev.com/5e148eb4f7679c85daa8e0d314dfd636f739b0d8
Cr-Commit-Position: refs/heads/master@{#321253}