1022238 - Regression: Removing Item from Reading List by tapping banner icon fails

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Mark Capella [:capella]]

Bug 1011714 removed code that wasn't actually obsolete.

Toggling the readerList icon in the banner while a page is displayed in readermode triggers:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js?rev=e563583ad137&mark=316-319#285

So now when we tap the icon, the article is removed from the cache, but left in the readerMode list (in the DB), and no toast "page removed" is displayed.

Comment 1

[Aaron Train [:aaronmt]]

Who can be assigned to this?

Comment 2

[Chenxia Liu [:liuche]]

Attachment: Patch: Re-add Reader:Removed

Comment 3

[:Margaret Leibovic]

Comment on attachment 8437238 [details] [diff] [review]
Patch: Re-add Reader:Removed

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

r- because this isn't the most low-risk patch we could write for uplift.

This patch effectively backs out the patch from bug 1011714, then makes some more changes on top of that. Instead of doing this, I think we should just back out the patch from bug 1011714 and uplift that to aurora. Then we can write a new patch that does what you're doing here, either in bug 1011714 or in a separate follow-up.

::: mobile/android/chrome/content/browser.js
@@ +7540,5 @@
> +        let args = JSON.parse(aData);
> +        let notify = false;
> +        let url;
> +
> +        if ('url' in args) {

Nit: Use double quotes.

@@ +7544,5 @@
> +        if ('url' in args) {
> +          url = args.url;
> +        } else {
> +          throw new Error("Reader:Remove requires URL as an argument");
> +        }

I feel like I would prefer just doing a check and throwing, then using args.url directly down below.

@@ +7548,5 @@
> +        }
> +
> +        if ('notify' in args) {
> +          notify = args.notify;
> +        }

Instead of declaring the variable up above and then doing this check, you could just do:

let notify = !!args.notify;

Or you could even just use args.notify directly down below.

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Part 1: Backout of bug 1011714

What about just backing this out of aurora, and then landing a different patch on nightly, all part of this bug?

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Nightly patch: re-add Reader:Removed and handling

I can move this to a new bug if you'd like, and split out the backout changes.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8437863 [details] [diff] [review]
Part 1: Backout of bug 1011714

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

Cool, let's land this, and we can request uplift for this to aurora.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8437911 [details] [diff] [review]
Nightly patch: re-add Reader:Removed and handling

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

I was expecting this to just be a patch on top of the backout, but sure, this is good to land on Nightly.

::: mobile/android/base/home/HomeFragment.java
@@ +342,5 @@
> +
> +            final JSONObject json = new JSONObject();
> +            try {
> +                json.put("url", mUrl);
> +                json.put("notify", false);

Does this mean that right now we show two toasts if you remove a reading list item? Is that new in 32? If so, maybe we should split this into a separate patch from the backout, and then request uplift for it as well after it bakes on Nightly for a bit.

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Part 2: Handle removal of reading list items correctly.

Split this patch out from the previous version, so there is Part 1 and Part 2.

> ::: mobile/android/base/home/HomeFragment.java
> @@ +342,5 @@
> > +
> > +            final JSONObject json = new JSONObject();
> > +            try {
> > +                json.put("url", mUrl);
> > +                json.put("notify", false);
> 
> Does this mean that right now we show two toasts if you remove a reading
> list item? Is that new in 32? If so, maybe we should split this into a
> separate patch from the backout, and then request uplift for it as well
> after it bakes on Nightly for a bit.

Nope, this patch adds in the notify property, so when Java requests a removal, we can specify that we don't want the notification of "Reader:Removed" to be fired.

Comment 9

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/8901849f95bc
https://hg.mozilla.org/integration/fx-team/rev/93530f5caa97

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8901849f95bc
https://hg.mozilla.org/mozilla-central/rev/93530f5caa97

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8437863 [details] [diff] [review]
Part 1: Backout of bug 1011714

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1011714
User impact if declined: No notification displayed when removing a reading list item from the in-content reading list banner
Testing completed (on m-c, etc.): local testing, baking on nightly
Risk to taking this patch (and alternatives if risky): very low, backout of patch 1011714
String or IDL/UUID changes made by this patch: none

Comment 12

[Teodora Vermesan (:TeoVermesan)]

After tapping the garbage icon, the article is removed from the reader mode list and a toast "Page removed" is displayed. So, verified fixed on:
Device: Alcatel One Touch
OS: Android 4.1.2
Build: Firefox for Android 33.0a1 (2014-06-15)

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Regression, tracking.

Comment 14

[Aaron Train [:aaronmt]]

Re: Aurora

Comment 15

[Chenxia Liu [:liuche]]

Thanks Aaron! I missed the a+ in my bugmail.

https://hg.mozilla.org/releases/mozilla-aurora/rev/304ea70beed9

Comment 16

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in
Build: Firefox for Android 32.0a2 (2014-06-26)
Device: Asus Transformer Pad TF300T (Android 4.2.1) and Motorola Razr (Android 4.0.4)

Based on Teodora's comment 12 I will mark this as verified fixed.