698836 - Add full screen mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

[:Margaret Leibovic]

We should use the new fullscreen API for this.

(This bug depends on bug 695460, since we would add a context menu item to get to full screen mode.)

Comment 1

[:Margaret Leibovic]

Attachment: wip

This generally works, but it has problems if you leave Fennec while you're in full screen mode, so I need to add something that deals with that case.

Comment 2

[:Margaret Leibovic]

(This patch is written on top of the patch in bug 699667)

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 572132 [details] [diff] [review]
wip

Review of attachment 572132 [details] [diff] [review]:
-----------------------------------------------------------------

mostly good, but hiding chrome whenever we go into fullscreen needs to be fixed for this to land

::: embedding/android/GeckoApp.java
@@ +991,5 @@
> +                                     WindowManager.LayoutParams.FLAG_FULLSCREEN : 0,
> +                                     WindowManager.LayoutParams.FLAG_FULLSCREEN);
> +
> +                // Hide/show the browser toolbar
> +                mBrowserToolbar.setVisibility(fullscreen ? View.GONE : View.VISIBLE);

I don't think we should be hiding chrome here. Instead, browser.js should be listening for the fullscreen event (see https://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4486, http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser.js#330 and mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1671) and that handler should send a "Window:HideChrome" message to java to execute this code.

Comment 4

[:Margaret Leibovic]

Attachment: patch

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 572665 [details] [diff] [review]
patch

there are two actions happening here, let's not get them crossed up.

You're first patch was basically right for handling SetFullscreen on the window, just remove the "mBrowserToolbar.setVisibility(mFullScreen ? View.VISIBLE : View.GONE);" line.

For the fullscreen event on the document (unfortunately, it is confusingly named), this essentially means "hide the chrome". You're second patch essentially handles that correctly, but it should only call "mBrowserToolbar.setVisibility(mFullScreen ? View.VISIBLE : View.GONE);" in that handler. 

Also, to try to limit the confusion, could you name the message sent for the document going fullscreen to "ToggleChrome:Hide/Show" or something along those lines. We loose the context of it being related to the document when it becomes a message sent through this interface.

Comment 6

[:Margaret Leibovic]

Attachment: patch v2

I hope I understand this now!

Comment 7

[:Margaret Leibovic]

https://hg.mozilla.org/projects/birch/rev/bda7e7fb45e8

Comment 8

[Wesley Johnston (:wesj)]

These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547

Comment 10

[Aaron Train [:aaronmt]]

20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)

Comment 11

[Andreea Pod]

Added testcase for Fennec 11.0 Catch-All Test Run and Fennec 12.0 Catch-All Test Run :
- https://litmus.mozilla.org/show_test.cgi?id=44849
- https://litmus.mozilla.org/show_test.cgi?id=44850