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)

ARM
Android
defect
Not set
normal

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)

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
I'll take this one. Will see if I get a chance to work on it during the weekend.
Great! As always, let me know if you have any questions :)
Assignee: nobody → marcosadp
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
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?
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.
Attached patch patch.diff (obsolete) — Splinter Review
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)
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.
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+
(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.
Attached patch patch_v2.diffSplinter Review
Takes into account Margaret's suggestions.
Attachment #8413376 - Attachment is obsolete: true
Attachment #8414102 - Flags: review?(margaret.leibovic)
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.
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+
Keywords: checkin-needed
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]
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.