Closed Bug 1467742 Opened 6 years ago Closed 6 years ago

Update reader mode to github tip (rev bf64b58d909d716770a2dd650d78286097ba797b )

Categories

(Toolkit :: Reader Mode, enhancement)

Product:
Toolkit
Component:
Reader Mode
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(3 files)

Git log
5.16 KB, text/plain
Details
Bug 1467742 - update readability from github rev 8fec62d246f563090f5711c02907e2bea7ce96f8,
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1467742 - update reader mode code and tests for readability updates,
59 bytes, text/x-review-board-request
jaws
: review+
Details
Attached file Git log — 
A few nice bug fixes have landed in github and we should take advantage of them:

Last sync was bug 1444082, which was 8525c6af36d3badbe27c4672a6f2dd99ddb4097f. The git log is a bit long so I've moved it into the attachment.

Pull requests:
https://github.com/mozilla/readability/pull/428 (change parameter name to be more accurate)
https://github.com/mozilla/readability/pull/430 (fix a bug in <br>-to-<p> conversion code)
https://github.com/mozilla/readability/pull/431 (remove <aside> tags as they're usually not relevant)
https://github.com/mozilla/readability/pull/438 (fixes a NodeJS-related issue)
https://github.com/mozilla/readability/pull/439 and https://github.com/mozilla/readability/pull/445 (now Readability no longer requires a URI parameter (!))
https://github.com/mozilla/readability/pull/442 and https://github.com/mozilla/readability/pull/448 (fix the testcase generation for the tests in the github repo)
https://github.com/mozilla/readability/pull/443 (avoid extracting the root node)
https://github.com/mozilla/readability/pull/447 (avoid "readability-styled" elements by moving some nodes into paragraphs)
https://github.com/mozilla/readability/pull/449 (don't put non-phrasing content into paragraphs)
https://github.com/mozilla/readability/pull/285 (removes inline-style "display: none" and "hidden" elements)
https://github.com/mozilla/readability/pull/454 (minor title/heading detection fixes)
https://github.com/mozilla/readability/pull/455 (Stop nesting <p> tags, which isn't valid HTML)
Comment on attachment 8984400 [details]
Bug 1467742 - update readability from github rev 8fec62d246f563090f5711c02907e2bea7ce96f8,

https://reviewboard.mozilla.org/r/250228/#review256704

::: toolkit/components/reader/JSDOMParser.js:1091
(Diff revision 1)
> +        var endChar = this.html.indexOf("]]>", this.currentChar);
> +        if (endChar === -1) {
> +          this.error("unclosed CDATA section");
> +          return null;
> +        }
> +        var text = new Text();

Hm, this is an odd type of review since I'm really just reviewing the merge, but...

Can this share the textNode object declared above? Or at least assign textNode to a new Text() instance?
Attachment #8984400 - Flags: review?(jaws) → review+
Comment on attachment 8984401 [details]
Bug 1467742 - update reader mode code and tests for readability updates,

https://reviewboard.mozilla.org/r/250230/#review256706

::: commit-message-3d361:3
(Diff revision 1)
> +Bug 1467742 - update reader mode code and tests for readability updates, r?jaws
> +
> +The phrasing content change means that the contents of <div id="foo> were being put

Missing a closing double quotation mark here, where after inspecting the code it doesn't seem to be intended. From first reading the comment I thought the missing double quotation mark was important to the issue.
Attachment #8984401 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Comment on attachment 8984400 [details]
> Bug 1467742 - update readability from github rev
> bf64b58d909d716770a2dd650d78286097ba797b,
> 
> https://reviewboard.mozilla.org/r/250228/#review256704
> 
> ::: toolkit/components/reader/JSDOMParser.js:1091
> (Diff revision 1)
> > +        var endChar = this.html.indexOf("]]>", this.currentChar);
> > +        if (endChar === -1) {
> > +          this.error("unclosed CDATA section");
> > +          return null;
> > +        }
> > +        var text = new Text();
> 
> Hm, this is an odd type of review since I'm really just reviewing the merge,
> but...
> 
> Can this share the textNode object declared above? Or at least assign
> textNode to a new Text() instance?

Sure. I created https://github.com/mozilla/readability/pull/459 for this. If you review that + https://github.com/mozilla/readability/pull/458 I'll include both of those before merging. :-)

(Pinging here because ISTR that you don't get pinged/emailed for github review requests...)
Flags: needinfo?(jaws)
I have configured my email client to star and mark as important github and phabricator review requests but it is still very easy for them to go unseen (I make heavy use of the nag emails from Bugzilla as well as the Requests button in the header on b.m.o), so thank you for the needinfo :)

I've granted review to the two pull requests on Github.
Flags: needinfo?(jaws)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/349625d94832
update readability from github rev 8fec62d246f563090f5711c02907e2bea7ce96f8, r=jaws
https://hg.mozilla.org/integration/autoland/rev/21bc698a4d9c
update reader mode code and tests for readability updates, r=jaws
https://hg.mozilla.org/mozilla-central/rev/349625d94832
https://hg.mozilla.org/mozilla-central/rev/21bc698a4d9c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Blocks: 1394941
Depends on: 1430493
Blocks: 800165
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: