Closed Bug 1134443 Opened 9 years ago Closed 9 years ago

Update Readability.js from shared library on github

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

I'm going to write a patch to pull in the current version of our shared library.

I think after that we should try to keep the github version in sync with the one in mozilla-central by landing patches as pull requests are merged.
Blocks: 958735
/r/4147 - Bug 1134443 - Update Readability.js from shared library on github. r=bnicholson

Pull down this commit:

hg pull review -r ae98587574357345b188cc48b6bfecd78bb26612
Attachment #8567731 - Flags: review?(bnicholson)
Brian, what are your thoughts on how to best maintain the Readability.js in mozilla-central going forward? Do you think we should merge in changes as they land in the github repo, or should we start version tagging Readability.js and merging in known good revisions, similarly to what we would do for other external dependencies?
Flags: needinfo?(bnicholson)
Comment on attachment 8567731 [details]
MozReview Request: bz://1134443/margaret

https://reviewboard.mozilla.org/r/4145/#review3323
Attachment #8567731 - Flags: review?(bnicholson) → review+
(In reply to :Margaret Leibovic from comment #2)
> Brian, what are your thoughts on how to best maintain the Readability.js in
> mozilla-central going forward? Do you think we should merge in changes as
> they land in the github repo, or should we start version tagging
> Readability.js and merging in known good revisions, similarly to what we
> would do for other external dependencies?

I'd vote to land them immediately and keep m-c in sync with the master branch. If there are any major incoming changes that will break things, we could do that work on a separate branch and merge it to master when it's ready.

One concern I have is that we'll forget/overlook changes to Readability.js outside of the repo, resulting in code getting lost. A few ideas I can think of:
* Add a big flashy comment to Readability.js warning people not to modify it directly.
* When merging, make sure that the m-c Readability.js is exactly the same as the parent for the incoming changes. If not, that means something landed on m-c without landing in the repo, and we should update the repo accordingly. This could work well since we can easily do the same checks when merging to iOS.
* Use a hook to prevent Readability.js from being changed without special commit flags. A heavier approach for sure, but maybe we already have a list of monitored files we could tack this onto easily.

I'm curious how we handle this with other external dependencies.
Flags: needinfo?(bnicholson)
https://hg.mozilla.org/integration/fx-team/rev/2bbb4ac2a49e

I added a comment at the top of Readability.js pointing to the github repo.

I also agree merging in every commit immediately seems like the best option, especially since changes living in the github repo won't really get much testing themselves.

This morning I was discussing this a bit with rnewman, and I think as long as we make sure a Firefox/toolkit peer signs off on every change to Readability.js itself, it would be fine to automatically merge those back into mozilla-central.
I also updated the github repo README to include a mention of this review policy:
https://github.com/mozilla/readability/commit/93861352c91d86b7933cee184663ad088d529571
https://hg.mozilla.org/mozilla-central/rev/2bbb4ac2a49e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Blocks: 1128757
Attachment #8567731 - Attachment is obsolete: true
Attachment #8619521 - Flags: review+
You need to log in before you can comment on or make changes to this bug.