Last Comment Bug 784674 - Improve byline fetching in Readability.js
: Improve byline fetching in Readability.js
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Reader View (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: Firefox 17
Assigned To: Lucas Rocha (:lucasr)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-22 07:38 PDT by Lucas Rocha (:lucasr)
Modified: 2016-07-29 14:29 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Better byline fetching in Readability (3.22 KB, patch)
2012-08-22 07:38 PDT, Lucas Rocha (:lucasr)
bnicholson: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Lucas Rocha (:lucasr) 2012-08-22 07:38:21 PDT
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) 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

Note You need to log in before you can comment on or make changes to this bug.