632649 - Warn users on devices with 256mb of RAM or less that they are on an unsupported device

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Stuart Parmenter]

Can we in our Java code pop up a warning the first time a user runs that tells them something along the lines of:

"Firefox does not currently support this device and you may experience problems."

Comment 1

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

Adding Madhava for string inpupt

Comment 2

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

Attachment: patch

Comment 3

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

Attachment: screenshot

Comment 4

[Madhava Enros [:madhava]]

What will actually happen if they try to run it?

Also - will we likely ever support it?  If not, we should probably drop to:

"Firefox does not support this device. You may experience problems."

Now, it'd be great if we could be more specific or helpful about what a user is going to encounter, to help them make a real decision about whether to proceed.

Do we know what these problems are?

Comment 5

[Madhava Enros [:madhava]]

Also - if the user does opt to "Continue", is he or she going to see this prompt every time on startup? If so, we should have a "[x] Show this on start up" in there.

Comment 6

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

(In reply to comment #4)
> What will actually happen if they try to run it?
hangs and crashes
> 
> Also - will we likely ever support it?  If not, we should probably drop to:
> 
nope
> "Firefox does not support this device. You may experience problems."
ok
> 
> Now, it'd be great if we could be more specific or helpful about what a user is
> going to encounter, to help them make a real decision about whether to proceed.
> Do we know what these problems are?
nothing damaging to their phone... hangs, crashes and slowness. Just setting expectations


(In reply to comment #5)
> Also - if the user does opt to "Continue", is he or she going to see this
> prompt every time on startup? If so, we should have a "[x] Show this on start
> up" in there.

just after the first install and not again after that.

Comment 7

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

Comment on attachment 511166 [details]
screenshot

"Firefox does not support this device. You may experience problems."

r+ with the string change

Comment 8

[Doug Turner (:dougt)]

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


>             .setCancelable(false)
>-            .setPositiveButton(getResources().getString(R.string.exit_label),
>+            .setPositiveButton(R.string.exit_label,

I do not understand why we use to call getResources().getString().


>                                new DialogInterface.OnClickListener() {
>                                    public void onClick(DialogInterface dialog,
>                                                        int id)
>                                    {
>                                        GeckoApp.this.finish();
>+                                       System.exit(0);


comment why we are calling exit() here instead of just calling finish().  Also in the other exit() call.

Comment 9

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

(In reply to comment #8)
> Comment on attachment 511165 [details] [diff] [review]
> patch
> 
> 
> >             .setCancelable(false)
> >-            .setPositiveButton(getResources().getString(R.string.exit_label),
> >+            .setPositiveButton(R.string.exit_label,
> 
> I do not understand why we use to call getResources().getString().
You can either pass a resource id or a string. No need to get the string ourselves when the system can do it.
> 
> >                                new DialogInterface.OnClickListener() {
> >                                    public void onClick(DialogInterface dialog,
> >                                                        int id)
> >                                    {
> >                                        GeckoApp.this.finish();
> >+                                       System.exit(0);
> 
> 
> comment why we are calling exit() here instead of just calling finish().  Also
> in the other exit() call.

We didn't need the exit() call prior to the network watcher landing, now we do

Comment 10

[Thomas Arend [:tarend]]

Could we add a link to https://www.mozilla.com/en-US/mobile/platforms/ so users can find more info?

Comment 11

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

(In reply to comment #10)
> Could we add a link to https://www.mozilla.com/en-US/mobile/platforms/ so users
> can find more info?

Out of scope for this bug

Comment 12

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

pushed http://hg.mozilla.org/mozilla-central/rev/1f86b3c908cf

Comment 13

[Matt Brubeck (:mbrubeck)]

(In reply to comment #10)
> Could we add a link to https://www.mozilla.com/en-US/mobile/platforms/ so users
> can find more info?

Thomas, can you file a followup bug to add this?  I would like to see this in a future release (maybe even 4.0 if we can do it without new localizable strings).

Comment 14

[Thomas Arend [:tarend]]

Done: see Bug 633649

Comment 15

[Tony Chung [:tchung]]

Aaron, can you please verify this fix against the Motorola Droid/Milestone that you have? looks like it alerts on firstrun.

  2.12 +<!ENTITY  incompatable_device "&brandShortName; does not support this device and you may experience
    2.13 +problems.">
    2.14 +<!ENTITY  continue_label "Continue">

Comment 16

[Tony Chung [:tchung]]

(In reply to comment #15)
> Aaron, can you please verify this fix against the Motorola Droid/Milestone that
> you have? looks like it alerts on firstrun.
> 
>   2.12 +<!ENTITY  incompatable_device "&brandShortName; does not support this
> device and you may experience
>     2.13 +problems.">
>     2.14 +<!ENTITY  continue_label "Continue">

Also,

mbrubeck: tchung: only on first run, or after uninstall/reinstall or "Clear Data" (in the Android settings, not the Firefox prefs)

Comment 17

[Aaron Train [:aaronmt]]

I am not seeing this warning notification. 

1. I updated over 20110211, cleared data, started Fennec and saw nothing.
2. Ran a complete uninstall and clean install of 20110212, started Fennec and saw nothing.

Mozilla/5.0 (Android; Linux armv7l; rv:2.0b12pre) Gecko/20110212 Firefox/4.0b12pre Fennec/4.0b5pre

Device: Motorola Milestone
Android: 2.2.1

Comment 18

[Matt Brubeck (:mbrubeck)]

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

>+        // We don't currently support devices with less than 256Mb of RAM, warn on first run
>+        if (Runtime.getRuntime().totalMemory() <= 262144L && !new File(sGREDir, "application.ini").exists()) {

The documentation says that totalMemory returns a value in bytes, so (a) the threshold should be 268435456L, and (b) why isn't this triggering on all devices right now?

Comment 19

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

(In reply to comment #18)
> Comment on attachment 511165 [details] [diff] [review]
> patch
> 
> >+        // We don't currently support devices with less than 256Mb of RAM, warn on first run
> >+        if (Runtime.getRuntime().totalMemory() <= 262144L && !new File(sGREDir, "application.ini").exists()) {
> 
> The documentation says that totalMemory returns a value in bytes, so (a) the
> threshold should be 268435456L, and (b) why isn't this triggering on all
> devices right now?

huh? coulda sworn I read that it returned memory in kb. as for #b, good question.

Comment 20

[Matt Brubeck (:mbrubeck)]

(In reply to comment #18)
> The documentation says that totalMemory returns a value in bytes, so (a) the
> threshold should be 268435456L, and (b) why isn't this triggering on all
> devices right now?

Actually, ignore part (b) - I was thinking backwards for some reason.

But it's true that totalMemory returns memory in bytes.  I also don't think that totalMemory is what we want here.  It's not the total physical memory of the host, but rather "the total amount of memory in the Java virtual machine. The value returned by this method may vary over time, depending on the host environment."

In a small test app on my G2 (512 MB of physical RAM), it returns 2953200, or about 2.8 MB.

Comment 21

[Matt Brubeck (:mbrubeck)]

We could also implement this in C++ or JavaScript using PR_GetPhysicalMemorySize or nsSystemInfo (if that's any simpler than grepping /proc/meminfo in Java).

Comment 22

[Doug Turner (:dougt)]

Is there nothing on the java side that give you the "MemTotal" from /proc/meminfo?

Comment 23

[Doug Turner (:dougt)]

also, this should not block.

Comment 24

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

(In reply to comment #23)
> also, this should not block.

I disagree, we want this for b5

Comment 25

[Doug Turner (:dougt)]

who is we?

it is some little protection on a set of devices that we dont support.  polish.  like to have, but i find it hard to say id block all of the other b5 changes on it.

Comment 26

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

Attachment: patch to use /proc/meminfo

Comment 27

[Doug Turner (:dougt)]

Comment on attachment 512052 [details] [diff] [review]
patch to use /proc/meminfo

Would you mind creating a new method called |IsSupportedDevice()| and move the logic to test for info there.  This will allow us to just add other stuff (like testing for ARM6 or something like that) without having to refactor.

So, then you can do something like:

   if (!IsSupportedDevice())
      show_the_dialog...

And the try and its indentation can be in that other method.

Could you also add a comment why 256 is our limit?  Doesn't have to be anything elborate, but it is pretty vague now.  256 is total system memory shared frame buffers, system, file cache... so not all 256mb devices will act the same.

what is |propNames| for?

Otherwise looks good.

Comment 28

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

Attachment: patch to use /proc/meminfo

Comment 29

[Doug Turner (:dougt)]

Comment on attachment 512085 [details] [diff] [review]
patch to use /proc/meminfo

remove |propNames|

Comment 30

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

pushed http://hg.mozilla.org/mozilla-central/rev/385abb827ff3

Comment 31

[Aaron Train [:aaronmt]]

Mozilla/5.0 (Android; Linux armv7l; rv:2.0b12pre) Gecko/20110214
Firefox/4.0b12pre Fennec/4.0b5pre

Verifying that I now see this with http://hg.mozilla.org/mozilla-central/rev/385abb827ff3

Device: Motorola Milestone
Android: 2.2.1

Anyone else want to confirm on another currently unsupported device?

Comment 32

[Martijn Wargers (zombie)]

Also confirmed fixed, using today's nightly Fennec build on:
Device: Motorola Milestone
Android: 2.2.1