Closed
Bug 1078355
Opened 10 years ago
Closed 10 years ago
Move reader mode code out of BrowserApp
Categories
(Firefox for Android Graveyard :: Reader View, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: wesj, Assigned: capella, Mentored)
References
Details
(Whiteboard: [lang=java])
Attachments
(1 file, 1 obsolete file)
21.25 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
There's a bunch of reader mode code in BrowserApp that should be in its own class: private void handleReaderListStatusRequest(final String url) private void handleReaderAdded(int result, final ContentValues values) private ContentValues messageToReadingListContentValues(JSONObject message) private void handleReaderRemoved(final String url)
Comment 1•10 years ago
|
||
+10000
Component: General → Reader Mode
OS: Linux → Android
Hardware: x86_64 → All
Updated•10 years ago
|
Mentor: rnewman, wjohnston
Whiteboard: [lang=java]
Assignee | ||
Comment 2•10 years ago
|
||
Would it make sense to add it to ReaderModeUtils ?
Attachment #8505583 -
Flags: review?(wjohnston)
Reporter | ||
Comment 3•10 years ago
|
||
Comment on attachment 8505583 [details] [diff] [review] bug1078355.diff This is lucas' world. I'd prefer her reviewed this.
Attachment #8505583 -
Flags: review?(wjohnston) → review?(lucasr.at.mozilla)
Comment 4•10 years ago
|
||
Comment on attachment 8505583 [details] [diff] [review] bug1078355.diff Review of attachment 8505583 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/BrowserApp.java @@ +1430,5 @@ > removeAddonMenuItem(id); > } > }); > > } else if ("Reader:ListStatusRequest".equals(event)) { Drive-by: Instead of listing for the messages here, can we create some other class that handles listening for the messages and then dealing with them? Moving these handleReader* methods out of BrowserApp is certainly a win, but it would be really nice if BrowserApp didn't know about reader mode at all.
Updated•10 years ago
|
Assignee: nobody → markcapella
Comment 5•10 years ago
|
||
> Drive-by: Instead of listing for the messages here, can we create some other
> class that handles listening for the messages and then dealing with them?
And when you do, use switch (event) rather than chained `if` blocks.
Comment 6•10 years ago
|
||
FYI, I just filed bug 1084100, which is similar, and will probably depend on whatever approach we take here.
Blocks: readerv2
Assignee | ||
Comment 7•10 years ago
|
||
:-) In version 2, we clean-up BrowserApp, but leave ReaderModeUtils alone ...
Attachment #8505583 -
Attachment is obsolete: true
Attachment #8505583 -
Flags: review?(lucasr.at.mozilla)
Attachment #8506687 -
Flags: review?(margaret.leibovic)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 8•10 years ago
|
||
Comment on attachment 8506687 [details] [diff] [review] bug1078355.diff Review of attachment 8506687 [details] [diff] [review]: ----------------------------------------------------------------- Looking good, just a few small comments. ::: mobile/android/base/ReaderModeHelper.java @@ +33,5 @@ > + > + protected final Context context; > + > + > + public ReaderModeHelper(Context context) { Other than the favicon request (which I want to try to get rid of), everything else in here has to do with reading list, not reader mode. So I think we could call this class ReadingListHelper instead. @@ +49,5 @@ > + EventDispatcher.getInstance().registerGeckoThreadListener((NativeEventListener) this, > + "Reader:ListStatusRequest", "Reader:Removed"); > + } > + > + // GeckoEventListener implementation You don't need to include these comments. @@ +118,5 @@ > + /** > + * Gecko (ReaderMode) requests the page favicon to append to the > + * document head for display. > + */ > + private void handleReaderFaviconRequest(final String url) { Out of scope for this bug, but I feel like the reader JS code should just figure out how to keep the icon for the original page. Or doesn't our favicons code already special-case about:reader URLs to show the favicon for the original page? If that's the case, maybe we could get rid of this. But in a separate bug :) @@ +200,5 @@ > + /** > + * Quick helper method to return the Contentresolver. > + */ > + private ContentResolver getContentResolver() { > + return context.getContentResolver(); I don't think we need this helper method. `context.getContentResolver()` isn't too verbose :)
Attachment #8506687 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Push to try, (cause sheriffs are our friends ;D ) https://tbpl.mozilla.org/?tree=Try&rev=8d2a95a4ff28
Assignee | ||
Comment 10•10 years ago
|
||
Caught on the warning track ... Fixed a duplicated registerGeckoThreadListener() to be a register/unregister pair https://tbpl.mozilla.org/?tree=Try&rev=92b1f4d89b0a
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/37e5f630049b
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/37e5f630049b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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
•