Comment 1 Lucas Rocha (:lucasr) 2012-08-23 09:14:39 PDT

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 2012-08-23 15:12:16 PDT

Is this going to inbound/central first?

Comment 3 Ryan VanderMeulen [:RyanVM] 2012-08-23 19:20:03 PDT

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

Comment 4 Lucas Rocha (:lucasr) 2012-08-24 02:45:51 PDT

(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 2012-08-24 10:58:35 PDT

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) 2012-08-24 16:23:28 PDT

(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) 2012-08-24 16:28:55 PDT

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