Status

()

defect
P4
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

unspecified
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [QA+])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.)
Assignee: nobody → margaret.leibovic
Priority: -- → P4
(Assignee)

Comment 1

8 years ago
Posted patch wip (obsolete) — Splinter Review
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.
Attachment #572132 - Flags: feedback?(blassey.bugs)
(Assignee)

Comment 2

8 years ago
(This patch is written on top of the patch in bug 699667)
Depends on: 699667
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.
Attachment #572132 - Flags: feedback?(blassey.bugs) → feedback-
(Assignee)

Comment 4

8 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #572132 - Attachment is obsolete: true
Attachment #572665 - Flags: review?(blassey.bugs)
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.
Attachment #572665 - Flags: review?(blassey.bugs) → review-
(Assignee)

Comment 6

8 years ago
Posted patch patch v2Splinter Review
I hope I understand this now!
Attachment #572665 - Attachment is obsolete: true
Attachment #572679 - Flags: review?(blassey.bugs)
Attachment #572679 - Flags: review?(blassey.bugs) → review+
(Assignee)

Updated

8 years ago
Blocks: 688082
(Assignee)

Comment 7

8 years ago
https://hg.mozilla.org/projects/birch/rev/bda7e7fb45e8
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Flags: in-litmus?(fennec)
Whiteboard: [QA+]
These patches were backed while investigating Talos failures.  Now that tests are green again, we will need to reland.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED

Comment 11

7 years ago
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
Flags: in-litmus?(fennec) → in-litmus+
Depends on: 719557
You need to log in before you can comment on or make changes to this bug.