Closed
Bug 698836
Opened 13 years ago
Closed 13 years ago
Add full screen mode
Categories
(Firefox for Android Graveyard :: General, defect, P4)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
(Whiteboard: [QA+])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
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.)
Updated•13 years ago
|
Assignee: nobody → margaret.leibovic
Priority: -- → P4
Assignee | ||
Comment 1•13 years ago
|
||
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•13 years ago
|
||
(This patch is written on top of the patch in bug 699667)
Depends on: 699667
Comment 3•13 years ago
|
||
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•13 years ago
|
||
Attachment #572132 -
Attachment is obsolete: true
Attachment #572665 -
Flags: review?(blassey.bugs)
Comment 5•13 years ago
|
||
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•13 years ago
|
||
I hope I understand this now!
Attachment #572665 -
Attachment is obsolete: true
Attachment #572679 -
Flags: review?(blassey.bugs)
Updated•13 years ago
|
Attachment #572679 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 7•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Flags: in-litmus?(fennec)
Whiteboard: [QA+]
Comment 8•13 years ago
|
||
These patches were backed while investigating Talos failures. Now that tests are green again, we will need to reland.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 9•13 years ago
|
||
backout was backed out https://hg.mozilla.org/projects/birch/rev/6f925b45a547
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Comment 10•13 years ago
|
||
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
Comment 11•13 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+
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•