1165481 - Use readability parser from readability github repo

Status: RESOLVED FIXED

(Firefox for iOS :: Reader View, defect)

Description

[Wesley Johnston (:wesj)]

We have this repo:

https://github.com/mozilla/readability

we should just import directly from it.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: PR https://github.com/mozilla/firefox-ios/pull/469

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8606519 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/469

I'm giving this a - because it copies all the Readability project files into our packaged application. Like tests, etc. I think it is fine to Carthage for this but don't drag the whole folder into the project. Instead just drag Readability.js in there.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8606519 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/469

Updated. Does having the files in the project actually build them or anything or do you just not like them being in the proj file?

Comment 4

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Wes, they were actually part of the Copy Files phase. So those files would end up in the Firefox.ipa file. 

If it makes sense to have them in the project file then they should not be part of any targets.

Comment 5

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8606519 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/469

This does not work for me. I did a clean checkout and ran `./checkout.sh`. In the project I now see a `readability` under `Third-Party Source` that has `Readability.js` in it. But, that file is marked red. And when I look at the path for it, it says `/Users/sarentz/Mozilla/firefox-ios/Readability.js`. Not sure what is going on here.

Do you want me to fix this? Just assign it to me.

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I'll pick this one up.

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8606519 [details] [review]
PR https://github.com/mozilla/firefox-ios/pull/469

Obsoleting this PR. Going to create a new one.

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: PR: https://github.com/mozilla/firefox-ios/pull/550

This patch includes https://github.com/mozilla/readability as a Carthage dependency and pulls in the `Readability.js` from that checkout.

Comment 9

[Stephan Leroux [:sleroux]]

Comment on attachment 8614686 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/550

LGTM