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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reading List, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

As per bug 1127445 comment 80, this only affects Nightly users so there is no reason to uplift to Aurora (38).

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 4

[u535116]

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

Comment 5

[u535116]

I'd like to take this

Comment 6

[:Margaret Leibovic]

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

Comment 7

[:Margaret Leibovic]

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 :)

Comment 8

[u535116]

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?

Comment 9

[:Margaret Leibovic]

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

[:Margaret Leibovic]

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?

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 12

[u535116]

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 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=24b8ad4a34e7

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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!

Comment 15

[u535116]

Thanks Michael, I'm still getting used to using Bugzilla, did I properly set 'checkin-needed'?

Comment 16

[:Margaret Leibovic]

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

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(For the record, I don't actually need anything)

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1598f56887d7

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/1598f56887d7

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1598f56887d7