784674 - Improve byline fetching in Readability.js

Status: RESOLVED FIXED

Description

[Lucas Rocha (:lucasr)]

Created attachment 654202 [details] [diff] [review]
Better byline fetching in Readability

Somehow missed Brian's suggestion to remove byline node to avoid duplicate bylines in reader. Also, we need to match both id and class. Using textContent instead of stripping tags should be enough.

Comment 1

[Lucas Rocha (:lucasr)]

Comment on attachment 654202 [details] [diff] [review]
Better byline fetching in Readability

[Approval Request Comment]
User impact if declined: Some articles contain byline info inside main content DIV and we'll show duplicate byline for those.
Testing completed (on m-c, etc.): A lot of local testing, no regressions.
Risk to taking this patch (and alternatives if risky): Low, should only affect pages with byline in main content DIV, which is not that common.
String or UUID changes made by this patch: None.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Is this going to inbound/central first?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08b790ce10cd

Comment 4

[Lucas Rocha (:lucasr)]

(In reply to Lukas Blakk [:lsblakk] from comment #2)
> Is this going to inbound/central first?

Sorry, forgot to post the inbound push link. Just landed in m-c.

Comment 5

[microrffr]

Comment on attachment 654202 [details] [diff] [review]
Better byline fetching in Readability

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

::: mobile/android/chrome/content/Readability.js
@@ +437,5 @@
>            continue;
>  
> +        let matchString = node.className + node.id;
> +        if (matchString.search(this.REGEXPS.byline) !== -1 && !this._articleByline) {
> +          this._articleByline = node.textContent;

This later flows to _creditsElement.innerHTML, which would be weird if there were any < and > in the byline.

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to microrffr from comment #5)
> Comment on attachment 654202 [details] [diff] [review]
> Better byline fetching in Readability
> 
> Review of attachment 654202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/Readability.js
> @@ +437,5 @@
> >            continue;
> >  
> > +        let matchString = node.className + node.id;
> > +        if (matchString.search(this.REGEXPS.byline) !== -1 && !this._articleByline) {
> > +          this._articleByline = node.textContent;
> 
> This later flows to _creditsElement.innerHTML, which would be weird if there
> were any < and > in the byline.

Good point. Filed bug 785549 to track this.

Comment 7

[Lucas Rocha (:lucasr)]

Pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/f3a2b3f6cd8b