Closed Bug 515768 Opened 15 years ago Closed 15 years ago

Fennec needs a task switch button in titlebar for n900

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Maemo
defect
Not set
normal

Tracking

(fennec1.0b4+)

VERIFIED FIXED
fennec1.0b4
Tracking Status
fennec 1.0b4+ ---

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(2 files, 2 obsolete files)

When Fennec is fullscreen on n900 we have no way to reduce screen size and no way to switch apps. There does not appear to be a hardbutton for fullscreen toggle as there is on the n810.

See:
http://people.mozilla.com/~mfinkle/fennec/screenshots/fennec-n900-rightside.png
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0+
tracking-fennec: 1.0+ → 1.0b4+
This patch adds code to switch out of the fullscreen application and launch another task
Assignee: nobody → mark.finkle
Attached patch patch (ui) (obsolete) — Splinter Review
This patch adds the UI to switch tasks. It also uses the n900 button images in the toolbar
Attached patch patch 2 (ui) (obsolete) — Splinter Review
previous patch had a mixup with #ifndef MOZ_PLATFORM_HILDON
Attachment #402300 - Attachment is obsolete: true
Attached patch patch 3 (ui)Splinter Review
ugh, this one has even less #ifdef issues
Attachment #402302 - Attachment is obsolete: true
Attachment #402303 - Flags: review?(gavin.sharp)
Attachment #402299 - Flags: review?(bugmail)
OK, I tested this on my n900 and it works. I had a component registration issue, but that was my fault: I didn't force the component to be registered since I was just copying it to the device
Attachment #402303 - Flags: review?(gavin.sharp) → review+
Comment on attachment 402303 [details] [diff] [review]
patch 3 (ui)

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+  switchTask: function switchTask() {
>+    let phone = Cc["@mozilla.org/phone/support;1"].createInstance(Ci.nsIPhoneSupport);
>+    phone.switchTask();
>+  },

Should this be ifdefed MOZ_PLATFORM_HILDON too? nsIPhoneSupport seems like a weird place to add this, not sure I like that idea.

>diff --git a/themes/hildon/browser.css b/themes/hildon/browser.css

>+#tool-app-switch {

>+  min-height: 0 !important;
>+  min-width: 0 !important;

These are already in platform.css since http://hg.mozilla.org/mobile-browser/rev/6b89b88cca17 , right?

>diff --git a/themes/hildon/jar.mn b/themes/hildon/jar.mn

>   images/fullscreen-up-40.png          (images/fullscreen-up-40.png)

unused now?
Comment on attachment 402299 [details] [diff] [review]
patch (component)

>+ 	ifdef MOZ_ENABLE_DBUS
this should probably be MOZ_PLATFORM_HILDON since that's what you're testing for in the source code.

>+#ifdef WINCE
> #include <windows.h>
>
> 
>+#ifdef WINCE
>   typedef LONG (*__PhoneMakeCall)(PHONEMAKECALLINFO *ppmci);

use WINCE_WINDOWS_MOBILE


>+  if(hPhoneDLL) {
>+    __PhoneMakeCall MakeCall = (__PhoneMakeCall) 

>+    if(MakeCall) {

fix spacing

>+#endif
>+  return NS_OK;
> }

  return NS_OK;
#else
  return NS_ERROR_NOT_IMPLEMENTED;
#endif
}

and please file a follow up bug to name this component something more appropriate.
(In reply to comment #6)
> (From update of attachment 402303 [details] [diff] [review])
> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >+  switchTask: function switchTask() {
> >+    let phone = Cc["@mozilla.org/phone/support;1"].createInstance(Ci.nsIPhoneSupport);
> >+    phone.switchTask();
> >+  },
> 
> Should this be ifdefed MOZ_PLATFORM_HILDON too? nsIPhoneSupport seems like a
> weird place to add this, not sure I like that idea.

I wrapped it in a try/catch for now. Windows Mobile will probably need the same support. nsIPhoneSupport was the simplest place to put it for now. We can move it later.

> >+  min-height: 0 !important;
> >+  min-width: 0 !important;
> 
> These are already in platform.css since
> http://hg.mozilla.org/mobile-browser/rev/6b89b88cca17 , right?

Yep, removed them and from the other "task" buttons

> >   images/fullscreen-up-40.png          (images/fullscreen-up-40.png)
> 
> unused now?

Yep, removed
(In reply to comment #7)
> (From update of attachment 402299 [details] [diff] [review])
> >+ 	ifdef MOZ_ENABLE_DBUS
> this should probably be MOZ_PLATFORM_HILDON since that's what you're testing
> for in the source code.

Done

> >+#ifdef WINCE
> > #include <windows.h>
> > 
> >+#ifdef WINCE
> >   typedef LONG (*__PhoneMakeCall)(PHONEMAKECALLINFO *ppmci);
> 
> use WINCE_WINDOWS_MOBILE

Done

> >+  if(hPhoneDLL) {
> >+    __PhoneMakeCall MakeCall = (__PhoneMakeCall) 
> 
> >+    if(MakeCall) {
> 
> fix spacing

Done

> >+#endif
> >+  return NS_OK;
> > }
> 
>   return NS_OK;
> #else
>   return NS_ERROR_NOT_IMPLEMENTED;
> #endif
> }

Done

> and please file a follow up bug to name this component something more
> appropriate.

Filed bug 518375
pushed:
https://hg.mozilla.org/mobile-browser/rev/29dd86020bd9
https://hg.mozilla.org/mobile-browser/rev/ebf0f5dae41a
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → B4
verified FIXED On build:

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20090924
Fennec/1.0b4pre
Status: RESOLVED → VERIFIED
Attachment #402299 - Flags: review?(bugmail) → review+
We need to include a n900 subgroup for the maemo device tests testgroup within our Fennec 1.0 test run and start including testcases for that device.
Flags: in-litmus?
Litmus testcase https://litmus.mozilla.org/show_test.cgi?id=9758 should take care of regression testing this bug.
Flags: in-litmus? → in-litmus+
Component: Linux/Maemo → General
OS: Mac OS X → Linux (embedded)
QA Contact: maemo-linux → general
Hardware: x86 → ARM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: