Closed
Bug 828124
Opened 11 years ago
Closed 11 years ago
Adjust low memory bar for Reader Mode
Categories
(Firefox for Android Graveyard :: Reader View, defect)
Tracking
(firefox18 unaffected, firefox19+ fixed, firefox20+ fixed, firefox21 fixed)
RESOLVED
FIXED
Firefox 21
People
(Reporter: kbrosnan, Assigned: kats)
References
Details
(Keywords: regression)
Attachments
(2 files)
2.06 KB,
text/html
|
Details | |
714 bytes,
patch
|
blassey
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Reader mode is not displayed on my G2 running Android 2.3.7. Document consists of html header info then <p><p>Some text</p> <p>second paragraph</p> The regression range for this is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20ec9014f220&tochange=d8e4f06198dc
Comment 1•11 years ago
|
||
Is this really a regression? Also, we need to check to see if the document is a "valid" read-able document. I don't think it is by the look of it.
Component: General → Reader Mode
Comment 2•11 years ago
|
||
I have not changed anything in reader mode for some time but I've heard reports from some users that reader mode is being offered at all in Nightly but works fine in, say, Beta.
Comment 3•11 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #2) > I have not changed anything in reader mode for some time but I've heard > reports from some users that reader mode is being offered at all in Nightly > but works fine in, say, Beta. This might be happening due to the "isLowMemoryPlatform" check we do here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3449 For example, this check stops Reader Mode, for all situations, on my Nexus S device.
Comment 4•11 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #3) > (In reply to Lucas Rocha (:lucasr) from comment #2) > > I have not changed anything in reader mode for some time but I've heard > > reports from some users that reader mode is being offered at all in Nightly > > but works fine in, say, Beta. > > This might be happening due to the "isLowMemoryPlatform" check we do here: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ > browser.js#3449 > > For example, this check stops Reader Mode, for all situations, on my Nexus S > device. Is the Nexus S supposed to be considered a "low memory" device? I'd expect it to run fine with reader mode.
Assignee | ||
Comment 5•11 years ago
|
||
The "low memory" bar is set to 512 MiB as reported by the MemTotal value in /proc/meminfo. Most devices that are supposedly 512-meg devices report a value that is smaller than 512 in their MemTotal (presumably because some is taken up by radio firmware and/or graphics) and so they are considered "low memory". For example mfinkle's Nexus S reports 406052 kB even though it's a 512-meg device. I can lower the "low mem" threshold to 384 or so to account for this. Actual 384-meg devices tend to report just under 300 as their MemTotal so they would still be "low memory"
Comment 6•11 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #5) > The "low memory" bar is set to 512 MiB as reported by the MemTotal value in > /proc/meminfo. Most devices that are supposedly 512-meg devices report a > value that is smaller than 512 in their MemTotal (presumably because some is > taken up by radio firmware and/or graphics) and so they are considered "low > memory". For example mfinkle's Nexus S reports 406052 kB even though it's a > 512-meg device. I can lower the "low mem" threshold to 384 or so to account > for this. Actual 384-meg devices tend to report just under 300 as their > MemTotal so they would still be "low memory" Makes sense to lower the threshold then.
Updated•11 years ago
|
Summary: Reader mode not displayed for a simple document → Adjust low memory bar for Reader Mode
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #699831 -
Flags: review?(blassey.bugs)
Updated•11 years ago
|
Attachment #699831 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 9•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/281657268a89
Assignee: nobody → bugmail.mozilla
Comment 10•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/281657268a89
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Comment 11•11 years ago
|
||
Uplift potential?
Assignee | ||
Comment 12•11 years ago
|
||
Yeah, we can request uplift to aurora and beta.
Assignee | ||
Comment 13•11 years ago
|
||
Comment on attachment 699831 [details] [diff] [review] Adjust low-memory threshold [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 801818, bug 808772 User impact if declined: Some "512 meg" devices get treated as low-memory devices, and don't get offered reader mode. They will also be subject to more aggressive tab expiration. Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): low risk, android only String or UUID changes made by this patch: none
Attachment #699831 -
Flags: approval-mozilla-beta?
Attachment #699831 -
Flags: approval-mozilla-aurora?
Comment 14•11 years ago
|
||
Be aware that the reported memory is no constant: On a Nexus S with Android 4.1.2 stock, I can see no Reader icon or menu items with Nightly 20130110 and 'Manage apps' shows 343 MB RAM.
Assignee | ||
Comment 15•11 years ago
|
||
The 'Manage apps' and any other view of memory in the Android UI is highly inconsistent. The memory reading we take is what the kernel provides as being visible, which should be the same across all instances of a given hardware model.
Comment 16•11 years ago
|
||
Unfortunately, running |adb shell| and calling /proc/meminfo throws a 'Permission denied.' on a stock rooted Nexus S and if you run |adb root|, it will tell you that it can't do that on production builds. (The pie memory chart in ddms seems to have even lower memory reports).
Comment 17•11 years ago
|
||
(In reply to Archaeopteryx [:aryx] from comment #16) > Unfortunately, running |adb shell| and calling /proc/meminfo throws a > 'Permission denied.' on a stock rooted Nexus S and if you run |adb root|, it > will tell you that it can't do that on production builds. (The pie memory > chart in ddms seems to have even lower memory reports). `cat /proc/meminfo`
Comment 18•11 years ago
|
||
Thank you. MemTotal: 351420 kB Everything: MemTotal: 351420 kB MemFree: 11868 kB Buffers: 2220 kB Cached: 70056 kB SwapCached: 0 kB Active: 202632 kB Inactive: 51500 kB Active(anon): 182272 kB Inactive(anon): 22696 kB Active(file): 20360 kB Inactive(file): 28804 kB Unevictable: 332 kB Mlocked: 0 kB SwapTotal: 0 kB SwapFree: 0 kB Dirty: 0 kB Writeback: 0 kB AnonPages: 182200 kB Mapped: 77488 kB Shmem: 22780 kB Slab: 19432 kB SReclaimable: 3356 kB SUnreclaim: 16076 kB KernelStack: 5544 kB PageTables: 6880 kB NFS_Unstable: 0 kB Bounce: 0 kB WritebackTmp: 0 kB CommitLimit: 175708 kB Committed_AS: 3475088 kB VmallocTotal: 327680 kB VmallocUsed: 67704 kB VmallocChunk: 226308 kB
Assignee | ||
Comment 19•11 years ago
|
||
Huh, your MemTotal value is lower than what mfinkle's Nexus S reported. I wasn't expecting that. Do you know the model number of your Nexus S? Could be a slightly different hardware configuration.
Comment 20•11 years ago
|
||
Product name - herring HW version - rev 11 Bootloader version - i9020xxlc2 Baseband version - i9020xxki1 Carrier info - TMB Lock state - unlocked Model number - Nexus S Android version - 4.1.2 Kernel version - 3.0.31-g5894150 android-build@vpbs1 ) #1 PREEMPT Mon Sept 10 14:10:13 PDT 2012 Build number JZO54K
Comment 21•11 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #19) > Huh, your MemTotal value is lower than what mfinkle's Nexus S reported. I > wasn't expecting that. Do you know the model number of your Nexus S? Could > be a slightly different hardware configuration. We have to draw the line somewhere though. 384 seems like a good line.
Reporter | ||
Comment 22•11 years ago
|
||
This does not need to be checked into beta. My regression range is valid and contains bug 792603.
Blocks: 792603
status-firefox18:
--- → unaffected
status-firefox19:
--- → unaffected
status-firefox20:
--- → affected
Assignee | ||
Comment 23•11 years ago
|
||
The reader mode issue isn't in beta, agreed, but I'd like the definition of a "low memory platform" to be consistent on all versions. Other features like the tab expiration also depend on this definition.
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #699831 -
Flags: approval-mozilla-beta?
Attachment #699831 -
Flags: approval-mozilla-beta+
Attachment #699831 -
Flags: approval-mozilla-aurora?
Attachment #699831 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 24•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/86652d19a3c3 https://hg.mozilla.org/releases/mozilla-beta/rev/e12b5af23eda
Updated•11 years ago
|
Comment 25•11 years ago
|
||
Preferrably this should be mentioned in SUMO? Especially this article: https://support.mozilla.org/en-US/kb/How%20to%20enable%20Reader%20Mode%20in%20Firefox%20for%20Android?esab=a&s=reader+mode&r=0&as=s It confused me because I used to see the reader mode button. Some other folks have seen this problem too: https://support.mozilla.org/en-US/questions/955736?esab=a&s=reader+mode&r=1&as=s
Assignee | ||
Comment 26•11 years ago
|
||
Good point, I will start a discussion thread on that article. Thanks for the tip!
Updated•10 years ago
|
tracking-fennec: ? → ---
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•