Closed
Bug 999488
Opened 10 years ago
Closed 10 years ago
Move reader mode related listener registration from GeckoApp to BrowserApp
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 32
People
(Reporter: Margaret, Assigned: marcosadp)
Details
(Whiteboard: [mentor=margaret][lang=java][good first bug])
Attachments
(1 file, 1 obsolete file)
6.31 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
We already handle most Reader:* events in BrowserApp, so we should start by registering/unregistering them in BrowserApp instead of GeckoApp. The one Reader:* event we do handle in GeckoApp in Reader:FaviconRequest, but it seems like that could also go in BrowserApp. See these files: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java
Assignee | ||
Comment 1•10 years ago
|
||
I'll take this one. Will see if I get a chance to work on it during the weekend.
Reporter | ||
Comment 2•10 years ago
|
||
Great! As always, let me know if you have any questions :)
Assignee: nobody → marcosadp
Assignee | ||
Comment 3•10 years ago
|
||
Sure. If you feel there is something I should know about this bug before diving into it just let me know. Otherwise I'm just going to assume that this is simply a re-factoring task and I just need to move the handling logic from one file to the other and register the handles in BrowserApp.java instead of GeckoApp.java
Assignee | ||
Comment 4•10 years ago
|
||
The only question I might have regarding this is about testing. How do I go about testing that the changes I've done didn't break anything?
Reporter | ||
Comment 5•10 years ago
|
||
You're correct, this is just a refactoring task. To make sure you haven't broken anything, I would test activating reader mode and making sure that all the functionality still works. You can look at the code to get a sense for what different features are handled by these different events. AFAIK, we don't have automated reader mode tests. We do have a testReaderMode file in the tree, but the test is disabled. A good follow-up bug could be to try to get some working test coverage for this feature.
Assignee | ||
Comment 6•10 years ago
|
||
Move reader mode related listener registration from GeckoApp to BrowserApp. It also moves the Reader:FaviconRequest handler from GeckoApp to BrowserApp.
Attachment #8413376 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 7•10 years ago
|
||
Based on the following event listeners, the following testing took place: registerEventListener("Reader:ListStatusRequest"); - Not really sure what to test here registerEventListener("Reader:Added"); - Opened a NY times article and opened reader mode. registerEventListener("Reader:Removed"); - Removed reader mode from the NY times article. registerEventListener("Reader:Share"); - Tried to share the NY times article in reader mode. registerEventListener("Reader:FaviconRequest"); - Checked favicon was being displayed on the NY times article in reader mode. Everything looked to be working fine.
Reporter | ||
Comment 8•10 years ago
|
||
Comment on attachment 8413376 [details] [diff] [review] patch.diff Review of attachment 8413376 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good, I just have a few suggestions about where to place some of these changes in BrowserApp. ::: mobile/android/base/BrowserApp.java @@ +1258,5 @@ > EventDispatcher.sendError(message, response); > } > + } else if (event.equals("Reader:FaviconRequest")) { > + final String url = message.getString("url"); > + handleFaviconRequest(url); You should put this up higher, under the other "Reader:Foo" if statements, so that all the reader-related logic is kept together. @@ +2720,5 @@ > appLocale, > previousSession); > } > + > + void handleFaviconRequest(final String url) { This can be private. I would also rename this to handleReaderFaviconRequest, to make it less ambiguous. I realize you're just moving this over from GeckoApp, but I think we can make it better while we do that :) I also think this should go up higher in the class, below the handleReaderRemoved method declaration.
Attachment #8413376 -
Flags: review?(margaret.leibovic) → feedback+
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Marcos A. Di Pietro from comment #7) > Based on the following event listeners, the following testing took place: > > registerEventListener("Reader:ListStatusRequest"); > - Not really sure what to test here This is used to update the reader mode toolbar. So when you open an article in reader mode, if it's in the reading list you'll see a remove icon (a trash can), and if it's not in reader mode you'll see an add-to-reading-list icon (a little book with a +). > registerEventListener("Reader:Added"); > - Opened a NY times article and opened reader mode. This is actually used when the user tries to add an article to the reading list, not when an article is opened in reader mode. > registerEventListener("Reader:Removed"); > - Removed reader mode from the NY times article. Inverse is true here. This means the user tried to remove the article from the reading list.
Assignee | ||
Comment 10•10 years ago
|
||
Takes into account Margaret's suggestions.
Attachment #8413376 -
Attachment is obsolete: true
Attachment #8414102 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 11•10 years ago
|
||
Thanks for the feedback Margaret. The new patch has your suggestions. Testing was also adjusted per your clarifications. Everything seems to be working as expected.
Reporter | ||
Comment 12•10 years ago
|
||
Comment on attachment 8414102 [details] [diff] [review] patch_v2.diff Review of attachment 8414102 [details] [diff] [review]: ----------------------------------------------------------------- Excellent, thanks!
Attachment #8414102 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c4cdaf3bd2d4
Keywords: checkin-needed
Whiteboard: [mentor=margaret][lang=java][good first bug] → [mentor=margaret][lang=java][good first bug][fixed-in-fx-team]
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c4cdaf3bd2d4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=margaret][lang=java][good first bug][fixed-in-fx-team] → [mentor=margaret][lang=java][good first bug]
Target Milestone: --- → Firefox 32
Updated•3 years 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.
Description
•