Closed
Bug 1134443
Opened 10 years ago
Closed 10 years ago
Update Readability.js from shared library on github
Categories
(Toolkit :: Reader Mode, defect)
Toolkit
Reader Mode
Tracking
()
RESOLVED
FIXED
mozilla39
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.
Assignee | ||
Comment 1•10 years ago
|
||
/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)
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
Comment on attachment 8567731 [details]
MozReview Request: bz://1134443/margaret
https://reviewboard.mozilla.org/r/4145/#review3323
Attachment #8567731 -
Flags: review?(bnicholson) → review+
Comment 4•10 years ago
|
||
(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)
Assignee | ||
Comment 5•10 years ago
|
||
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.
Assignee | ||
Comment 6•10 years ago
|
||
I also updated the github repo README to include a mention of this review policy:
https://github.com/mozilla/readability/commit/93861352c91d86b7933cee184663ad088d529571
Comment 7•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Updated•10 years ago
|
Blocks: desktop-reader
Assignee | ||
Updated•10 years ago
|
status-firefox38:
--- → affected
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8567731 -
Attachment is obsolete: true
Attachment #8619521 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•