Closed Bug 1067543 Opened 5 years ago Closed 5 years ago

Android action handler 'Send tab to': Don't offer 'Reading list' action if it has been turned off (low-memory devices)

Categories

(Firefox for Android :: Overlays, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35
Tracking Status
firefox34 + disabled
firefox35 --- fixed
firefox36 --- fixed
fennec 34+ ---

People

(Reporter: aryx, Assigned: ckitching)

References

Details

Attachments

(1 file)

Firefox Nightly and Aurora 2014-09-15 on Android 4.1.2 (Nexus S)

Opening a page and using the Share button from the context menu offers to send it to other applications etc. Picking 'Add to Aurora/Nightly' offers to add the page to the reading list even if it has been turned off, e.g. on low memory devices. The user can't access the reading list on these.
Component: Reader Mode → Overlays
tracking-fennec: --- → ?
I wonder if we should hide it (as suggested here) or just open the tab in the background.
Flags: needinfo?(ywang)
I imagine background opening is even worse for low mem devices. And of course, we don't have that functionality yet! 

We should just not show the button.
Had no idea you can turn reading list off on certain devices. 

I agree, we should hide the action in this case.
Flags: needinfo?(ywang)
(In reply to Richard Newman [:rnewman] from comment #2)
> I imagine background opening is even worse for low mem devices. And of
> course, we don't have that functionality yet! 

But we have a system in place to handle low-memory devices and opening tabs: Tab Zombies

> We should just not show the button.

I am fine with this for now, but I don't like the Reading List dependency on getting a URL into Firefox when all I want to do is read the URL after scanning the rest of the topics in the external app.
> But we have a system in place to handle low-memory devices and opening tabs:
> Tab Zombies

I was under the impression that we couldn't create those directly, because we need to write into sessionstore, so we'd need to launch Gecko even if we didn't want to load the page directly. Am I mistaken?


> I am fine with this for now, but I don't like the Reading List dependency on
> getting a URL into Firefox…

I'd be inclined to say we'll just tackle that via "Read Soon" (bubble), which we do plan to hook into Share, too.

(Ab)using Reading List for 'read soon' doesn't seem ideal for anyone, really.

> … when all I want to do is read the URL after
> scanning the rest of the topics in the external app.

Do you use RL in this way?
Yoink.

Richard, do you happen to know a simple way to determine if reading mode is disabled, or shall I liberally apply grep?
Assignee: nobody → chriskitching
This is how the panel config does it:

        // We disable reader mode support on low memory devices. Hence the
        // reading list panel should not show up on such devices.
        if (!HardwareUtils.isLowMemoryPlatform()) {
            panelConfigs.add(createBuiltinPanelConfig(mContext, PanelType.READING_LIST));
        }

Copypasta seems acceptable in this case. I don't think we want to exclude the option if the user has (temporarily) manually hidden the RL panel.
tracking-fennec: ? → 34+
Attachment #8492508 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/29c35d8e3d90
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Comment on attachment 8492508 [details] [diff] [review]
Hide reading list add button from overlay on low memory devices

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Better UX on low-memory devices
[Describe test coverage new/current, TBPL]: Working fine in Fx36 and Fx35
[Risks and why]: Low. Small change using well-known code
[String/UUID change made/needed]: None
Attachment #8492508 - Flags: approval-mozilla-beta?
Comment on attachment 8492508 [details] [diff] [review]
Hide reading list add button from overlay on low memory devices

Beta+
Attachment #8492508 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing for beta uplift.
Flags: needinfo?(chriskitching)
We're very likely not turning the feature on in Beta per Bug 1092409, so clearing the branch request for now.
Flags: needinfo?(chriskitching)
You need to log in before you can comment on or make changes to this bug.