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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Overlays, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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.

Comment 1

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

I wonder if we should hide it (as suggested here) or just open the tab in the background.

Comment 2

[Richard Newman [:rnewman]]

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.

Comment 3

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Had no idea you can turn reading list off on certain devices. 

I agree, we should hide the action in this case.

Comment 4

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

(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.

Comment 5

[Richard Newman [:rnewman]]

> 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?

Comment 6

[Chris Kitching [:ckitching]]

Yoink.

Richard, do you happen to know a simple way to determine if reading mode is disabled, or shall I liberally apply grep?

Comment 7

[Richard Newman [:rnewman]]

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.

Comment 8

[Chris Kitching [:ckitching]]

Attachment: Hide reading list add button from overlay on low memory devices

*yawn*

Comment 9

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/29c35d8e3d90

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29c35d8e3d90

Comment 11

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

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

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8492508 [details] [diff] [review]
Hide reading list add button from overlay on low memory devices

Beta+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for beta uplift.

Comment 14

[Richard Newman [:rnewman]]

We're very likely not turning the feature on in Beta per Bug 1092409, so clearing the branch request for now.