515768 - Fennec needs a task switch button in titlebar for n900

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch (component)

This patch adds code to switch out of the fullscreen application and launch another task

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch (ui)

This patch adds the UI to switch tasks. It also uses the n900 button images in the toolbar

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2 (ui)

previous patch had a mixup with #ifndef MOZ_PLATFORM_HILDON

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 3 (ui)

ugh, this one has even less #ifdef issues

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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 7

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

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.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/29dd86020bd9
https://hg.mozilla.org/mobile-browser/rev/ebf0f5dae41a

Comment 11

[Aakash Desai [:aakashd]]

verified FIXED On build:

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20090924
Fennec/1.0b4pre

Comment 12

[Aakash Desai [:aakashd]]

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.

Comment 13

[Aakash Desai [:aakashd]]

Litmus testcase https://litmus.mozilla.org/show_test.cgi?id=9758 should take care of regression testing this bug.