Strip about:reader urls before passing them into Reading List DB remove method from the menu

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
4 years ago
11 months ago

People

(Reporter: mcomella, Assigned: mking, Mentored)

Tracking

unspecified
Firefox 40
All
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [lang=java][good next bug][see comment 3])

Attachments

(1 attachment)

bug 1127445 comment 80 describes an issue where about:reader urls are in the DB and cannot be removed. This happens because we started stripping the about:reader urls before trying remove them in bug 1142196.

A better solution would be to pass in the already stripped url into the DB add method.

Perhaps add an assertion (or DB check) in the add to reading list DB method to ensure more about:reader urls won't be getting into the DB.
As per bug 1127445 comment 80, this only affects Nightly users so there is no reason to uplift to Aurora (38).
Mentor: michael.l.comella, margaret.leibovic
Whiteboard: [lang=java][good next bug]
We strip the URLs before they go into the DB in org.mozilla.gecko.db.LocalReadingListAccessor.

URLs that we're adding to reading list come from BrowserApp.onOptionsItemSelected.
After talking to Margaret, the task is:
  * Remove the code to strip about:reader from the urls in org.mozilla.gecko.db.LocalReadingListAccessor.removeReadingListItemWithURL - whoever is trying to delete the URL knows what they want to delete
  * Strip about:reader from the URLs when removing them from the Browser toolbar menu button - see BrowserApp.onOptionsItemSelected. Note that we don't care to strip urls when adding them - the DB should do that just fine
Whiteboard: [lang=java][good next bug] → [lang=java][good next bug][see comment 3]
Summary: Strip about:reader urls before passing them into Reading List DB add method → Strip about:reader urls before passing them into Reading List DB remove method from the menu
Assignee

Comment 5

4 years ago
I'd like to take this
(In reply to Matt King from comment #5)
> I'd like to take this

Great! Thanks for posting a patch. In general, it's good to always flag a reviewer for review/feedback on a patch to make sure it doesn't slip through the cracks. I'll do that for you now.
Assignee: nobody → mking
Comment on attachment 8589704 [details] [diff] [review]
Strip about:reader urls before passing them into Reading List DB remove method from the menu -

Let's see if mcomella beats me to providing feedback :)
Attachment #8589704 - Flags: feedback?(michael.l.comella)
Attachment #8589704 - Flags: feedback?(margaret.leibovic)
Assignee

Comment 8

4 years ago
Thanks Margaret, I was unable to find where to set the reviewer flag on this page. I did it on another bug where it would allow me to fill in an email, but it didn't appear on this bug for some reason?
(In reply to Matt King from comment #8)
> Thanks Margaret, I was unable to find where to set the reviewer flag on this
> page. I did it on another bug where it would allow me to fill in an email,
> but it didn't appear on this bug for some reason?

It should be on the page where you attach a patch, or alternately if you click on the "Details" view for a patch that's already attached.

I'll take a look at this today to get your some feedback!
Comment on attachment 8589704 [details] [diff] [review]
Strip about:reader urls before passing them into Reading List DB remove method from the menu -

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

This seems reasonable to me, but we should check the removeReadingListItemWithURL call sites in HomeFragment and ReadingListHelper to make sure we're not going to be passing any about:reader URLs to those as well. Can you try removing reading list items from the "Reading List" panel on the home screen, as well as the toolbar button in reader view to make sure those both still work properly?
Attachment #8589704 - Flags: feedback?(margaret.leibovic) → feedback+
Comment on attachment 8589704 [details] [diff] [review]
Strip about:reader urls before passing them into Reading List DB remove method from the menu -

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

I agree with Margaret.
Attachment #8589704 - Flags: feedback?(michael.l.comella) → feedback+
Assignee

Comment 12

4 years ago
Hi Margaret and Michael,

I verified that I was able to remove about:reader URLs from both the home screen reading list, and from the context menu while in reader mode. Is there anything else I can do?
Comment on attachment 8589704 [details] [diff] [review]
Strip about:reader urls before passing them into Reading List DB remove method from the menu -

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

That sounds good to me! try above ^! "checkin-needed" when it goes green! Thanks, Matt!
Attachment #8589704 - Flags: feedback+ → review+
Assignee

Updated

4 years ago
Attachment #8589704 - Flags: checkin?(michael.l.comella)
Assignee

Comment 15

4 years ago
Thanks Michael, I'm still getting used to using Bugzilla, did I properly set 'checkin-needed'?
(In reply to Matt King from comment #15)
> Thanks Michael, I'm still getting used to using Bugzilla, did I properly set
> 'checkin-needed'?

Sorry, this is not clear at all. We use a "checkin-needed" keyword, not the flag on the patch (although that can be useful if there are multiple patches on a bug).
Keywords: checkin-needed
By the way, you can use the "Need more information from" section to send a notification to the user you want to respond to a question. Fill in the users email (lol, yeah right), or auto-complete it with ":" and the IRC nick of the user (":mcomella" for me, for example). Alternatively, select the user from the dropdown. These "NEEDINFO" flags help prevent questions from getting lost in email. :)

e.g...
Flags: needinfo?(mking)
(For the record, I don't actually need anything)
Attachment #8589704 - Flags: checkin?(michael.l.comella)
https://hg.mozilla.org/integration/fx-team/rev/1598f56887d7
Keywords: checkin-needed
Whiteboard: [lang=java][good next bug][see comment 3] → [lang=java][good next bug][see comment 3][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1598f56887d7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [lang=java][good next bug][see comment 3][fixed-in-fx-team] → [lang=java][good next bug][see comment 3]
Target Milestone: --- → Firefox 40
Assignee

Updated

4 years ago
Flags: needinfo?(mking)

Updated

11 months 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.