Closed Bug 632649 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set

Tracking

(fennec2.0b5+)

VERIFIED FIXED
Tracking Status
fennec 2.0b5+ ---

People

(Reporter: pavlov, Assigned: blassey)

References

Details

(Whiteboard: [has-patch])

Attachments

(3 files, 1 obsolete file)

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."
tracking-fennec: --- → 2.0b5+
Assignee: nobody → blassey.bugs
Adding Madhava for string inpupt
OS: All → Android
Attached patch patchSplinter Review
Attachment #511165 - Flags: review?(doug.turner)
Attached image screenshot
Attachment #511166 - Flags: ui-review?(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?
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.
(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.
Whiteboard: [has-patch]
Comment on attachment 511166 [details]
screenshot

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

r+ with the string change
Attachment #511166 - Flags: ui-review?(madhava) → ui-review+
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.
Attachment #511165 - Flags: review?(doug.turner) → review+
(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
Could we add a link to https://www.mozilla.com/en-US/mobile/platforms/ so users can find more info?
(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
pushed http://hg.mozilla.org/mozilla-central/rev/1f86b3c908cf
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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).
Blocks: 633649
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">
(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)
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
(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.
(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.
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).
Is there nothing on the java side that give you the "MemTotal" from /proc/meminfo?
also, this should not block.
(In reply to comment #23)
> also, this should not block.

I disagree, we want this for b5
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.
Attached patch patch to use /proc/meminfo (obsolete) — Splinter Review
Attachment #512052 - Flags: review?(doug.turner)
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.
Attachment #512052 - Flags: review?(doug.turner) → review-
Attachment #512052 - Attachment is obsolete: true
Attachment #512085 - Flags: review?(doug.turner)
Comment on attachment 512085 [details] [diff] [review]
patch to use /proc/meminfo

remove |propNames|
Attachment #512085 - Flags: review?(doug.turner) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/385abb827ff3
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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?
Also confirmed fixed, using today's nightly Fennec build on:
Device: Motorola Milestone
Android: 2.2.1
Status: RESOLVED → VERIFIED
Blocks: 649841
You need to log in before you can comment on or make changes to this bug.