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

VERIFIED FIXED in Firefox 32

Status

()

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: capella, Assigned: liuche)

Tracking

({regression})

unspecified
Firefox 33
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 unaffected, firefox32+ verified, firefox33+ verified, fennec32+)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Who can be assigned to this?
Blocks: 1011714
tracking-fennec: --- → ?
Keywords: regression
Summary: Regression is ReaderMode - Removing Item from Readinglist by tapping banner icon fails → Regression: Removing Item from Reading List by tapping banner icon fails
Assignee: nobody → liuche
Posted patch Patch: Re-add Reader:Removed (obsolete) — Splinter Review
Attachment #8437238 - Flags: review?(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.
Attachment #8437238 - Flags: review?(margaret.leibovic) → review-
What about just backing this out of aurora, and then landing a different patch on nightly, all part of this bug?
Attachment #8437863 - Flags: review?(margaret.leibovic)
I can move this to a new bug if you'd like, and split out the backout changes.
Attachment #8437238 - Attachment is obsolete: true
Attachment #8437911 - Flags: review?(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.
Attachment #8437863 - Flags: review?(margaret.leibovic) → review+
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.
Attachment #8437911 - Flags: review?(margaret.leibovic) → review+
Attachment #8437863 - Attachment description: Patch: Backout of bug 1011714 → Part 1: Backout of bug 1011714
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.
Attachment #8437911 - Attachment is obsolete: true
Attachment #8438004 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8901849f95bc
https://hg.mozilla.org/mozilla-central/rev/93530f5caa97
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
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
Attachment #8437863 - Flags: approval-mozilla-aurora?
tracking-fennec: ? → 32+
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)
Regression, tracking.
Attachment #8437863 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Re: Aurora
Flags: needinfo?(liuche)
Thanks Aaron! I missed the a+ in my bugmail.

https://hg.mozilla.org/releases/mozilla-aurora/rev/304ea70beed9
Flags: needinfo?(liuche)
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.