Closed
Bug 785145
Opened 12 years ago
Closed 12 years ago
Correctly mark Ps created from text-only DIVs for scoring
Categories
(Firefox for Android Graveyard :: Reader View, defect, P1)
Tracking
(firefox16 unaffected, firefox17 verified, firefox18 verified)
VERIFIED
FIXED
Firefox 17
Tracking | Status | |
---|---|---|
firefox16 | --- | unaffected |
firefox17 | --- | verified |
firefox18 | --- | verified |
People
(Reporter: lucasr, Assigned: lucasr)
Details
Attachments
(1 file)
1.14 KB,
patch
|
bnicholson
:
review-
|
Details | Diff | Splinter Review |
Right now it seems we're doing it wrong add queuing a replaced node for scoring:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js#465
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #654740 -
Flags: review?(bnicholson)
Comment 2•12 years ago
|
||
Comment on attachment 654740 [details] [diff] [review]
Correctly mark Ps created from text-only DIVs for scoring
This change causes this page to fail:
http://articles.latimes.com/2012/mar/27/health/la-he-green-coffee-weight-loss-20120328
Even though the node is no longer in the document, maybe adding it to nodesToScore was intentional? Maybe the former node's parent is still useful for the algorithm.
Also, nodeIndex is decremented here. This means the index is checked again, which would cause newNode to be looked at twice. Perhaps this is contributing to the failure for the article I linked to? Maybe you could try removing the "nodeIndex -= 1;" line, but we might just want to leave this alone unless we have good reason to change it.
Attachment #654740 -
Flags: review?(bnicholson) → review-
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #2)
> Comment on attachment 654740 [details] [diff] [review]
> Correctly mark Ps created from text-only DIVs for scoring
>
> This change causes this page to fail:
> http://articles.latimes.com/2012/mar/27/health/la-he-green-coffee-weight-
> loss-20120328
Interesting, it's not failing for me. Are you testing this on your reader test app or on device?
> Even though the node is no longer in the document, maybe adding it to
> nodesToScore was intentional? Maybe the former node's parent is still useful
> for the algorithm.
It doesn't make any sense to add the replaced to node to nodesToScore array. Once we call node.parentNode.replaceChild(newNode, node), the DIV that got replaced by the P will have a null parentNode pointer. This means this node will be simply discarded once through all the nodesToScore items. See:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js#497
Note that the point of converting these DIVs to Ps is because the whole algorithm is driven by how much content each node has. This why we convert double BRs to Ps and why we convert certain DIVs to Ps. Paragraph nodes increment score for their parent and grandparent nodes:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js#530
Hence, considering a parentless node for scoring is pointless for the algorithm.
> Also, nodeIndex is decremented here. This means the index is checked again,
> which would cause newNode to be looked at twice. Perhaps this is
> contributing to the failure for the article I linked to? Maybe you could try
> removing the "nodeIndex -= 1;" line, but we might just want to leave this
> alone unless we have good reason to change it.
Good catch. This is intentional. Once the loop visits the same index again, it will now be a P and the newNode will be added to nodesToScore array (mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js#530). With this change, we don't need to do this anymore as we're adding the newNode straight away.
I think I'll combine the patch in this bug with the patch in bug 774914 as they're dealing with the same part of the algorithm. Better get these changes all at once.
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Priority: -- → P1
Comment 5•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Assignee | ||
Comment 6•12 years ago
|
||
Pushed to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/233683cf491e
Updated•12 years ago
|
status-firefox17:
--- → fixed
Comment 7•12 years ago
|
||
http://articles.latimes.com/2012/mar/27/health/la-he-green-coffee-weight-loss-20120328 doesn't fail anymore on the latest Nightly, Aurora and Beta builds. It seems that the issue was fixed. Closing bug as verified fixed.
--
Device: Galaxy Note
OS: Android 4.0.4
Comment 8•12 years ago
|
||
I guess it was a regression in 17.0.
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•