Closed Bug 1011714 Opened 6 years ago Closed 6 years ago

Clean up removal of Reading List items

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 32
Tracking Status
firefox32 --- wontfix
firefox33 --- fixed

People

(Reporter: liuche, Assigned: alexandru.deaconu)

References

Details

(Whiteboard: [mentor=liuche][lang=java])

Attachments

(1 file, 3 obsolete files)

Removing reading list items is currently messy and slightly incorrect. We shouldn't need to wait for Gecko to remove the reading list item from the cache before displaying a notification in Java that the item has been removed.
If you're not planning to work on this yourself, do you want to make it a mentor bug?
Flags: needinfo?(liuche)
Sure!
Flags: needinfo?(liuche)
Whiteboard: [mentor=liuche][lang=java]
To be more specific, we dan
To be more specific, we don't need to wait for Gecko to clear the reading list cache before displaying the the toast that in item has been removed.

This means we should remove the code in BrowserApp that waits for "Reader:Removed" before showing the toast. (We should also remove the Reader:ListCountUpdated as part of this cleanup too.) In fact, it doesn't look like we won't be using the Reader:Removed message anymore at all, so that should just be removed.
Assignee: nobody → eedens
Hi, I've build Fennec and fixed some issues. I'm currently looking for a 'good second bug' and I'd like to work on this issue.

Would it be ok, or does it require a greater experience on the project?
Flags: needinfo?(liuche)
Nope, this is great for a second bug! Let me give you some background on what this bug is about.

In bug 913457, je changed how we handled "remove" for context menu items, and the result is this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeFragment.java#315

You can see that we send a Reader:Remove message to Gecko there, because reading list is managed by Gecko. You'll also see that after Gecko handles Reader:Remove, it sends another message back to called Reader:Removed. Basically, we don't need to know about Gecko removing these reading list items, so you should remove that message ("Reader:Removed") and everything that depends on.

This should be a pretty straightforward bug, and just requires code removal.

Let me know if you need more help or information, and feel free to ping me in #mobile!
Flags: needinfo?(liuche)
Comment on attachment 8428589 [details] [diff] [review]
Removed 'Reader:Remove' message and deleted its usage from 'HomeFragment' and 'BrowserApp'

Don't forget to set the review/feedback flag for patches you submit.
Attachment #8428589 - Flags: feedback?(liuche)
Comment on attachment 8428589 [details] [diff] [review]
Removed 'Reader:Remove' message and deleted its usage from 'HomeFragment' and 'BrowserApp'

I've removed 'Reader:Remove' message usage. If there are any further changes required just let me know :)
Attachment #8428589 - Flags: review?(liuche)
Oops, I realized that Eric Eedens already grabbed this bug - Eric, since Alex already added a patch, do you mind if I reassign it to him? We'll find you something else, sorry!

I'll do a quick review of the patch anyways though.
Comment on attachment 8428589 [details] [diff] [review]
Removed 'Reader:Remove' message and deleted its usage from 'HomeFragment' and 'BrowserApp'

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

Reader:Removed is also present in mobile/android/chrome/content/browser.js, so you'll want to remove any code that uses Reader:Removed there as well.

I noticed you were removing code a little overzealously around Reader:Removed, so be a little more careful in browser.js. Basically, we're using Reader:Remove to tell Gecko (in the JS component of the code) to remove a reading list item, and Gecko is sending back the Reader:Removed message - we need to keep the former (Reader:Remove) but we don't actually need to do anything with the Reader:Removed message, so we're want to remove that code.

Also, Alex, if you're interested in fixing more reading list bugs, take a look at the meta-bug bug 917884 - there may be some bugs there that you could be interested in. (See the list of blocking bugs, and view them as a tree to see all the related bugs at once).

::: mobile/android/base/home/HomeFragment.java
@@ -335,5 @@
>              BrowserDB.removeBookmarksWithURL(cr, mUrl);
>              BrowserDB.removeHistoryEntry(cr, mUrl);
>  
>              BrowserDB.removeReadingListItemWithURL(cr, mUrl);
> -            GeckoEvent e = GeckoEvent.createBroadcastEvent("Reader:Remove", mUrl);

This is actually a different message, and we definitely want to keep it (Reader:Remove vs Reader:Removed).
Attachment #8428589 - Flags: review?(liuche)
Attachment #8428589 - Flags: feedback?(liuche)
Attachment #8428589 - Flags: feedback+
Attachment #8428589 - Attachment is obsolete: true
Comment on attachment 8429164 [details] [diff] [review]
Removed 'Reader:Removed' message handling

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

Sorry for submitting the patch on already working issue, it's my bad, I've subbmitted a new patch but Eric if you can just mark it as obosolete.

Regarding the message logic, I have a better sight of now how it works. I have removed the 'Reader:Removed' handling from http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1242

The 'Reader:Remove' message is beeing send as before, the only changes were made in the 'BrowserApp' class.

I have marked the old patch as obsolete and submitted a new patch. I didn't see any issues when bookmarking/unbookmarking a page, but I don't know if I should look on other features also
Attachment #8429164 - Flags: review?(liuche)
Assignee: eedens → alexandru.deaconu
Comment on attachment 8429164 [details] [diff] [review]
Removed 'Reader:Removed' message handling

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

Looks like you missed one of my previous pieces of feedback - you'll want to look in browser.js where you'll also want to delete the caller of Reader:Removed, since no one consumes that message anymore.

A good resource to use for finding all instances of code is http://mxr.mozilla.org/ . You can search for Reader:Removed and take a look at all the instances in mobile/android (the source directory of all our mobile code), and see what else you need to remove.
Attachment #8429164 - Flags: review?(liuche)
Hi Alex, wanted to answer the questions you asked on IRC.

The changes you proposed are correct, you'll want to remove that the if statement that sends the Reader:Removed message.

In the future, you should just upload a new patch with changes and obsolete the old one, and flag me for review; that way, I can make comments in the context of the patch - it's easier and better than referencing line numbers because the code base is constantly changing, so line numbers will change and aren't reliable.

Also, no need to worry that the sheriffs will land your patches on the tree unexpectedly - they definitely won't merge patches that haven't been reviewed with an r+, and you also need to explicitly set a whiteboard flag "check-in needed" to alert them that a patch needs to be landed.

Let me know if you have any more questions!
Also, for this bug, the changes are small enough that you should combine the changes into one patch.

Sometimes for more complex bugs, you might need to split work into multiple patches (to make things easier to review in chunks) - in those cases, I just preface the attachment description with "Part 1: ..." and "Part 2: ..." so it's clear what the ordering of the patches should be, so the sheriffs will know how to order the patches for landing.
Attachment #8429164 - Attachment is obsolete: true
Attachment #8430625 - Flags: review?(liuche)
Comment on attachment 8430625 [details] [diff] [review]
Bug 1011714 - Clean up removal of Reading List items. r=liuche

Looks great!

A couple of last things - you should update the patch message to match the bug message exactly, and also append me as a reviewer, and upload the new version. So the patch message should be:

Bug 1011714 - Clean up removal of Reading List items. r=liuche

Then add checkin-needed to the keywords (if you don't have bugzilla permissions, let me know and then I can do it) so the sheriffs know to land this patch.

Thanks!
Attachment #8430625 - Flags: review?(liuche) → review+
Attachment #8430625 - Attachment description: Removed 'Reader:Removed' message handling from 'BrowserApp.java' and 'browser.js' → Bug 1011714 - Clean up removal of Reading List items. r=liuche
Keywords: checkin-needed
Hi Alex, thanks for the patch! One more request, we ask that patches get run through the Tryserver before setting checkin-needed to hopefully avoid introducing new test failures when the patch lands. If you need some assistance doing so, Chenxia should be able to help.

Thanks!
Keywords: checkin-needed
Thanks Ryan, I didn't know that!

https://tbpl.mozilla.org/?tree=Try&rev=af26afedfecf

Alex, I noticed that you changed the patch name on bugzilla, but didn't actually change it in the patch.

You'll want to apply the patch, then "hg qref -e", and then update the message to match what you've done for the bugzilla patch name. You can check this by "hg qser -sv", or by reading the patch itself.
Flags: needinfo?(alexandru.deaconu)
Attachment #8430625 - Attachment is obsolete: true
Thnaks for the feedback. 

I've updated the patch message. I can see it modified in the patch itself however in buzilla the rewiewer part 'r=liuche' does not appear. Do I have to edit the message as well in bugzilla (I don't really know the exact procedure here). 

It's the first time I'm using the Tryserver. I've seen there is a test for this issue. I have a couple of questions :) :
1. Does it require my interrraction in any way?
2. Do I have to re-run the test after updating the message, and if so what steps should I follow?
Flags: needinfo?(alexandru.deaconu) → needinfo?(liuche)
Attachment #8432365 - Flags: review+
Thanks for sticking through this, Alex!

For the patch, the r="reviewer" is so that when something lands in the tree, we can make sure that the patch was reviewed. For people fixing future bugs or trying to understand the code, this gives also gives them another person to ask about a particular piece of code (this being the patch writer and the reviewer). However, the name of the bug on bugzilla should remain the same - no need to list the reviewer.

Just updating the patch message doesn't need another try run - it's not changing the code in any way.

I do see some reading list tests, but this change is a higher level one that doesn't change the test behavior, so there's no need to update the test.

I'm adding the checkin-needed keyword, but in the future, if you have an r+ and see that the try run is all green (see the link in comment #20), you can update that flag yourself.
Flags: needinfo?(liuche)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/4c3e918570c0
Keywords: checkin-needed
Whiteboard: [mentor=liuche][lang=java] → [mentor=liuche][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4c3e918570c0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=liuche][lang=java][fixed-in-fx-team] → [mentor=liuche][lang=java]
Target Milestone: --- → Firefox 32
Depends on: 1022238
This was backed out from Fx32 in bug 1022238.
You need to log in before you can comment on or make changes to this bug.