793920 - Move android reader mode to a shared place in toolkit

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Michael Anderson]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/536.25 (KHTML, like Gecko) Version/6.0 Safari/536.25




Expected results:

Android reader mode code needs to be copied and ported over to the desktop code base.  Readability.js should be put in a common area accessible by both.

Comment 1

[Chelsea Carr]

Attachment: Created a copy of Readability in toolkit/reader

We haven't started using the functionality yet;  the file has only been copied

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 666223 [details] [diff] [review]
Created a copy of Readability in toolkit/reader

You should use 'hg copy mobile/android/chrome/content/Readability.js toolkit/reader/Readability.js' rather than re-adding the whole file. You'll also need review from a toolkit peer.

Comment 3

[Lucas Rocha (:lucasr)]

You should also copy JSDOMParser.js to the same location as Readability.js depends on it.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 666223 [details] [diff] [review]
Created a copy of Readability in toolkit/reader

Clearing review based on comment #2 and comment #3.

Comment 5

[Chelsea Carr]

Attachment: copied readability.js and JSDOMparser.js using hg copy

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 668981 [details] [diff] [review]
copied readability.js and JSDOMparser.js using hg copy

Looks good to me.

Comment 7

[Dave Townsend [:mossop]]

How much more is there to readability? If it's just these two files (and likely to stay that) then I'd rather they went into toolkit/content rather than creating a whole new directory (and presumably you'll be wanting to add makefiles and jar.mn to ship it).

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Dave Townsend (:Mossop) from comment #7)
> How much more is there to readability? If it's just these two files (and
> likely to stay that) then I'd rather they went into toolkit/content rather
> than creating a whole new directory (and presumably you'll be wanting to add
> makefiles and jar.mn to ship it).

I don't expect the number of files to grow much more. There will likely be one more file to add (readerWorker.js) but that's all. I have a slight preference for keeping the files in a new directory just for the sake of keeping things clearly separated. Not a big deal though.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 668981 [details] [diff] [review]
copied readability.js and JSDOMparser.js using hg copy

># HG changeset patch
># User Chelsea Carr <[e-mail]>
># Date 1349655730 14400
># Node ID e52e460980777457eb4224a6e6d6e7bbba5043e5
># Parent  938dacff2d5caff3d97e3517850ca6f3c3f6b017
>Bug 793920: Moved Readability.js and JSDOMparser.js to reader/toolkit

The summary here is incorrect, it should say:
Bug 793920: Moved Readability.js and JSDOMparser.js to toolkit/reader
Of course, if these files are just moved to /toolkit then we don't need the reader portion.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Jared Wein [:jaws] from comment #9)
> Comment on attachment 668981 [details] [diff] [review]
> copied readability.js and JSDOMparser.js using hg copy
> 
> ># HG changeset patch
> ># User Chelsea Carr <[e-mail]>
> ># Date 1349655730 14400
> ># Node ID e52e460980777457eb4224a6e6d6e7bbba5043e5
> ># Parent  938dacff2d5caff3d97e3517850ca6f3c3f6b017
> >Bug 793920: Moved Readability.js and JSDOMparser.js to reader/toolkit
> 
> The summary here is incorrect, it should say:
> Bug 793920: Moved Readability.js and JSDOMparser.js to toolkit/reader
> Of course, if these files are just moved to /toolkit then we don't need the
> reader portion.

Even more than that, it just copies, doesn't remove the old files. Is that intentional?

Comment 11

[Chelsea Carr]

In one of our Monday meetings, we had talked about just copying the files to this common location for now, so that we don't break reader mode for mobile firefox.  I would think eventually we will delete the ones still in the mobile directory, but we didn't want to risk breaking other features for now.

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 668981 [details] [diff] [review]
copied readability.js and JSDOMparser.js using hg copy

I'm fine with the new location but don't think there is any point in making copies of these files until we're going to actually ship them somewhere. And at that point we should be moving and using the same files for both desktop and mobile.

Comment 13

[:Margaret Leibovic]

I'm going to take this over. I'm going to try to move the reader code in /mobile over to /toolkit, maintaining the current functionality in Fennec, but setting us up to use the same logic in a desktop implementation.

Ideally, we could use the same about:reader page in both desktop and Fennec, and just change the CSS around to suit different screen sizes.

Comment 14

[:Margaret Leibovic]

Hm, it looks like we tried to do something similar over in bug 961994, but the patch there is moving code from metro to toolkit.

Eklavya, could you explain some of the context behind that bug?

Comment 15

[:Margaret Leibovic]

Attachment: Move android reader mode to a shared place in toolkit

This patch moves our shareable reader mode code into toolkit, while maintaining the current reader mode functionality on Android.

I left Reader.js in /mobile for now, since this would need some significant re-working to be generic enough to share. We will probably want to keep our Reader.js for things like our page actions icon, but then move the shareable code for things like downloading a page into some new module.

I also realized that we're going to need to update aboutReader.js, since it does contain some Fennec-specific code, mostly around interacting with the reading list.

I loosely based the structure here on what I saw in some of the patches in bug 961994 and bug 927124. This will definitely conflict with those patches, but at this point those are way out of date and would need a lot of work to be updated, so I think the best path forward is to move existing code over to toolkit, then iterate on it there.

I also still need to move over the locale and theme files, but I wanted to get some feedback here before continuing forward.

Comment 16

[Eklavya Mirani]

Hi Margaret. I started porting the Android Reader Mode to Metro, and then it was decided to move it over to desktop as well and both of these would share as much code as possible. So inherently, Metro Copied code from Android, then from metro it was moved to /toolkit so both Metro Firefox and Firefox Desktop could share the same codebase apart from some of the UI elements.

Hope this clears it.

Comment 17

[:Margaret Leibovic]

(In reply to Eklavya Mirani from comment #16)
> Hi Margaret. I started porting the Android Reader Mode to Metro, and then it
> was decided to move it over to desktop as well and both of these would share
> as much code as possible. So inherently, Metro Copied code from Android,
> then from metro it was moved to /toolkit so both Metro Firefox and Firefox
> Desktop could share the same codebase apart from some of the UI elements.
> 
> Hope this clears it.

Thanks for the clarification! I took a look at the patch that's in bug 927124, and it's fallen very out of date (if you look at the current Android reader code, a lot has changed since then).

So I think the best path forward is to just start with moving the latest code into a shared place, and then we can iterate on the UI in each product separately. Unfortunately, this means you would need to rebase your patch for bug 927124 to add support for metro, but hopefully it would make that patch easier to write.

Comment 18

[Eklavya Mirani]

Yes, I agree with you here. It has indeed changed a lot.

I am not sure how much of the code I have already written is still usable now, but it seems to me that we can reuse most of it since it didn't really use much of the reader mode apis. (it was mostly UI work)

I'd be happy to collaborate with you on this, however you want me to. :)

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8534727 [details] [diff] [review]
Move android reader mode to a shared place in toolkit

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

Rather than create a new chrome namespace make it exist at chrome://global/content/reader/...

Who will own this code?

Comment 20

[:Margaret Leibovic]

(In reply to Dave Townsend [:mossop] (Out till December 1st) from comment #19)
> Comment on attachment 8534727 [details] [diff] [review]
> Move android reader mode to a shared place in toolkit
> 
> Review of attachment 8534727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Rather than create a new chrome namespace make it exist at
> chrome://global/content/reader/...

Sounds good. I was basing this off of what I saw in the other bugs, but that makes more sense to me.

> Who will own this code?

Well in the near-to-medium term, I'll be continuing to work on this, and bnicholson and rnewman are both familiar with this code as well. I imagine that at least one desktop engineer will become familiar with it as we work to create the desktop UI.

But to answer your question, if you're looking for someone to be responsible, I can volunteer.

Comment 21

[Brian Nicholson (:bnicholson)]

Comment on attachment 8534727 [details] [diff] [review]
Move android reader mode to a shared place in toolkit

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

Makes sense to me so far.

Comment 23

[:Margaret Leibovic]

Attachment: (Part 1) Move majority of Android reader mode code to a shared place in /toolkit

I decided to leave the existing aboutReader.css in /mobile and make it override an empty default theme in /toolkit, since there are a lot of mobile-specific images used in there, and I didn't feel like moving them all if we might be modifying the theme for desktop anyway. I figure we can consolidate into a single theme and/or move over shared styles in a future bug where we're actually working on the about:reader UI for desktop.

Comment 24

[:Margaret Leibovic]

Attachment: (Part 2) Split up Reader.js to create shared ReaderMode.jsm in /toolkit

Sorry, Brian, this patch is a bit more complicated. However, I did an hg mv from Reader.js to create ReaderMode.jsm, so hopefully it should be easier to review than if I had just created a new file.

Basically this patch splits up Reader.js such that the Fennec-specific bits stay in Reader.js, while the things we can share end up in ReaderMode.jsm. I had to make a few changes to the logic in ReaderMode.jsm to account for the fact that it's now a JS module, rather than a subscript loaded in the global window.

Next up will be figuring out how to split the Fennec-specific logic out of aboutReader.js, but I think I'll do that in another bug, since there's already enough going on here. Also, I think refactoring aboutReader.js will be a good time to start thinking about how to e10s-ify this code.

Comment 25

[:Margaret Leibovic]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e8b953eefe7f

Comment 26

[Brian Nicholson (:bnicholson)]

Comment on attachment 8535915 [details] [diff] [review]
(Part 1) Move majority of Android reader mode code to a shared place in /toolkit

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

Looks OK to me.

Comment 27

[Brian Nicholson (:bnicholson)]

Comment on attachment 8535917 [details] [diff] [review]
(Part 2) Split up Reader.js to create shared ReaderMode.jsm in /toolkit

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

::: toolkit/components/reader/ReaderMode.jsm
@@ +203,5 @@
>    },
>  
>    _downloadDocument: function (url) {
>      return new Promise((resolve, reject) => {
> +      // XXX: This doesn't follow meta redirects.

File a bug for this? I'm not sure how we'd even do this using an XHR. I'm also curious how much it matters -- how many articles actually use meta redirects?

@@ +204,5 @@
>  
>    _downloadDocument: function (url) {
>      return new Promise((resolve, reject) => {
> +      // XXX: This doesn't follow meta redirects.
> +      let xhr = new XMLHttpRequest();

This change concerns me a bit. I don't remember the reasons we used a raw <browser> over XHR to begin with, but I feel like it was for more than just meta redirects. That said, if this works, it's definitely preferable over the <browser> hack. Can you flag QA once this lands so they can browse through some random articles to look for any regressions?

Comment 28

[:Margaret Leibovic]

I decided that instead of changing the download logic to use an XHR instead of a browser, we can just leave that logic in Reader.js, and deal with moving it over later when we actually need it on desktop. As it is, this logic is only used on Fennec if you directly open a URL in reader mode that wasn't previously loaded in the tab or stored in the cache. So basically you would have to explicitly type a URL, or try to open an item that was added to the reading list from the share overlay (bug 1093172 will rely on this working properly).

(In reply to Brian Nicholson (:bnicholson) from comment #27)
> Comment on attachment 8535917 [details] [diff] [review]
> (Part 2) Split up Reader.js to create shared ReaderMode.jsm in /toolkit
> 
> Review of attachment 8535917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/reader/ReaderMode.jsm
> @@ +203,5 @@
> >    },
> >  
> >    _downloadDocument: function (url) {
> >      return new Promise((resolve, reject) => {
> > +      // XXX: This doesn't follow meta redirects.
> 
> File a bug for this? I'm not sure how we'd even do this using an XHR. I'm
> also curious how much it matters -- how many articles actually use meta
> redirects?

My insidious redirect testcase is the URLs in the pocket hits feed, which do two different kinds of redirects before resolving to the final URL.

http://pocket.co/sBYec
http://getpocket.com/s/BYec
http://well.blogs.nytimes.com/2014/12/03/run-to-stay-young

I'll omit this change for now, but we can use bug 1107588 to investigate this.

> @@ +204,5 @@
> >  
> >    _downloadDocument: function (url) {
> >      return new Promise((resolve, reject) => {
> > +      // XXX: This doesn't follow meta redirects.
> > +      let xhr = new XMLHttpRequest();
> 
> This change concerns me a bit. I don't remember the reasons we used a raw
> <browser> over XHR to begin with, but I feel like it was for more than just
> meta redirects. That said, if this works, it's definitely preferable over
> the <browser> hack. Can you flag QA once this lands so they can browse
> through some random articles to look for any regressions?

As I mentioned above, this code is rarely hit, so it would be hard to spot regressions, so I'll just punt on this part of the patch for now.

I'll post an updated patch.

Comment 29

[:Margaret Leibovic]

Attachment: (Part 2 v2) Split up Reader.js to create shared ReaderMode.jsm in /toolkit

I updated the patch to keep the document downloading bit in Reader.js. I did some manual testing to verify there were no regressions, and testReadingListCache passes locally.

Comment 30

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/1050ea09ba3e
https://hg.mozilla.org/integration/fx-team/rev/59b37fbb306a

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1050ea09ba3e
https://hg.mozilla.org/mozilla-central/rev/59b37fbb306a