906230 - [fig] reading list pane displayed on phones that do not offer reading mode button

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Kevin Brosnan [Ex-Mozilla]]

We need to figure out the about:home reading list UX for devices that never offer reading mode due to memory constraints.

bug 792603 - Disable automatic reader mode parsing
bug 852417 - Add a force override preference for Reader Mode availability on low-mem devices

Comment 1

[Lucas Rocha (:lucasr)]

I guess we should simply hide the reading list page on these devices.

Comment 2

[Ian Barlow (:ibarlow)]

(In reply to Lucas Rocha (:lucasr) from comment #1)
> I guess we should simply hide the reading list page on these devices.

+1

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Add HardwareUtils.isLowMemoryPlatform()

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: Hide the Reading List page in about:home on low memory devices

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 792233 [details] [diff] [review]
Add HardwareUtils.isLowMemoryPlatform()

Review of attachment 792233 [details] [diff] [review]:
-----------------------------------------------------------------

Minusing for API level.

::: mobile/android/base/util/HardwareUtils.java
@@ +18,5 @@
>      private static final String LOGTAG = "GeckoHardwareUtils";
>  
> +    // Minimum memory threshold for a device to be considered
> +    // a low memory platform (see sIsLowMemoryPlatform). This value
> +    // has be in sync with Gecko's equivalent threshold and should

Update this comment to reference xpcom/base/nsMemoryImpl.cpp, and update that file to reference this one as well. Otherwise it's likely they will end up getting out of sync at some point. Alternatively, we could have a test that asserts the thresholds are always the same somehow.

@@ +91,5 @@
> +
> +            final MemoryInfo memInfo = new MemoryInfo();
> +            am.getMemoryInfo(memInfo);
> +
> +            sIsLowMemoryPlatform = (memInfo.totalMem / 1024 < LOW_MEMORY_THRESHOLD);

totalMem: Added in API level 16

Comment 6

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #4)
> Created attachment 792234 [details] [diff] [review]
> Hide the Reading List page in about:home on low memory devices

Hiding this on phones will make it have two pages -- and the first focused page will be the second entry and not the first one. With three items, making the middle item default would look good. With two items, making the second item default wouldn't look good. Shall we move the history item to be the second item -- and focus the bookmarks -- still the first item -- on these phones?

basically, on latest 5" quad core CPU phones:
[history] [bookmarks] [reading-list]

on older phones,
[bookmarks] [history]

Comment 7

[Ian Barlow (:ibarlow)]

That's a good point. I agree with Sriram's suggestion. I wonder if this would mean rethinking how our tabs work too in cases like this, since having one in the middle and one cut off on the edge of the screen might look a little weird too...

#scopecreep

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Ian Barlow (:ibarlow) from comment #7)
> That's a good point. I agree with Sriram's suggestion. I wonder if this
> would mean rethinking how our tabs work too in cases like this, since having
> one in the middle and one cut off on the edge of the screen might look a
> little weird too...
> 
> #scopecreep

Good points sriram and ibarlow. Maybe use something similar to the tablet's tabs when you have only 2 pages?

Comment 9

[Ian Barlow (:ibarlow)]

Yeah that's kind of what i was thinking.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 792234 [details] [diff] [review]
Hide the Reading List page in about:home on low memory devices

Review of attachment 792234 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review as UX has to decide on the pages and their order.

Comment 11

[Lucas Rocha (:lucasr)]

Ian, any specific ideas (mockups) you'd like to experiment with here?

Comment 12

[Ian Barlow (:ibarlow)]

Oh. I thought we had already agreed in writing what we were doing.

This is what I was thinking http://cl.ly/image/2x07411J3N3N

Comment 13

[Lucas Rocha (:lucasr)]

The about:home tabs are being rearranged in bug 917394. We'll have 3 tabs (instead of 4) when the reading list panel is not available. This means we can use the same tab strip layout on those devices. Ian, agree?

Comment 14

[Ian Barlow (:ibarlow)]

Yep!

Comment 15

[Lucas Rocha (:lucasr)]

Attachment: Add HardwareUtils.isLowMemoryPlatform()

Read directly from /proc/meminfo

Comment 16

[Lucas Rocha (:lucasr)]

Attachment: Hide the Reading List page in about:home on low memory devices

With the new tabs from new-new-abouthome we can simply hide the reading list page.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 810633 [details] [diff] [review]
Add HardwareUtils.isLowMemoryPlatform()

Review of attachment 810633 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/util/HardwareUtils.java
@@ +21,5 @@
>      private static final String LOGTAG = "GeckoHardwareUtils";
>  
> +    // Minimum memory threshold for a device to be considered
> +    // a low memory platform (see sIsLowMemoryPlatform). This value
> +    // has be in sync with Gecko's equivalent threshold and should

I'd still like this comment to point to xpcom/base/nsMemoryImpl.cpp, and have that code point back here in a comment.

Comment 18

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 810636 [details] [diff] [review]
Hide the Reading List page in about:home on low memory devices

Review of attachment 810636 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 19

[Lucas Rocha (:lucasr)]

Pushed (with comments in HardwareUtils.java and nsMemoryImpl.cpp as suggested)

https://hg.mozilla.org/integration/fx-team/rev/9274dfefb19b
https://hg.mozilla.org/integration/fx-team/rev/c554c6a0c003

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9274dfefb19b
https://hg.mozilla.org/mozilla-central/rev/c554c6a0c003

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 810633 [details] [diff] [review]
Add HardwareUtils.isLowMemoryPlatform()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: if this patch is not uplifted, users using devices with reader mode automatically disabled (due to low memory specs) will still see a useless reading list panel in about:home. 
Testing completed (on m-c, etc.): Landed in m-c, no obvious issues so far. 
Risk to taking this patch (and alternatives if risky): Fairly low, this is just hiding a panel in about:home on low-memory devices.
String or IDL/UUID changes made by this patch: n/a

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 810636 [details] [diff] [review]
Hide the Reading List page in about:home on low memory devices

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862793 (new about:home)
User impact if declined: if this patch is not uplifted, users using devices with reader mode automatically disabled (due to low memory specs) will still see a useless reading list panel in about:home. 
Testing completed (on m-c, etc.): Landed in m-c, no obvious issues so far. 
Risk to taking this patch (and alternatives if risky): Fairly low, this is just hiding a panel in about:home on low-memory devices.
String or IDL/UUID changes made by this patch: n/a

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d4cfa77ea43
https://hg.mozilla.org/releases/mozilla-aurora/rev/adc42c51d961