Be smarter about whether or not to show the reader toolbar button

VERIFIED FIXED in Firefox 38

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

(Depends on 1 bug)

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 verified, firefox39 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
With my patch from bug 1139678, we're not doing background parsing anymore to determine whether or not to show the reader button, and instead erring on the side of showing it most of the time (eliminating the obvious things like about: URLs).

However, in the future, it would be nice to be smarter about this and only show the button on pages that will actually return readable articles.
(Assignee)

Comment 1

4 years ago
Maybe we can try using some of the old Android logic we had for a Readability check fast-path:
http://hg.mozilla.org/mozilla-central/rev/4ed72e80519c
(Assignee)

Updated

4 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 2

4 years ago
I started working on a simple algorithm for checking to see if a page is likely readerable:

* Grab all the <div>s and <p>s
* Filter out the "unlikely candidates" (but not the "ok maybe it's a candidate")
* Filter out elements with that don't have a certain amount of text content
* If we have more than a certain number of elements left, show the button

Before trying to optimize the correctness of this (it actually does seem to do an alright job of hiding the button in some obvious cases), I made some try pushes to see how this impacts performance:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=60164801ceea
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32fdcd77ee54

Comment 3

4 years ago
The latest trypush:

http://compare-talos.mattn.ca/?oldRevs=60164801ceea&newRev=dba7ba51ed22&server=graphs.mozilla.org&submit=true

(now with results for ubuntu and winxp, win7 retriggers seem to be slower atm)

suggests that the latest patch:

https://hg.mozilla.org/try/rev/778c7079f100

has basically so little impact that talos can't reasonably detect it (in particular, it thinks it reduced paint times... which I'm pretty sure is not logically possible).
(Assignee)

Comment 4

4 years ago
/r/5727 - Bug 1143844 - Check document for readerable content to determine whether or not to show reader button. r=Gijs

Pull down this commit:

hg pull review -r bb719f59ab850517449e08cb3f86c06ff727692d
Attachment #8580143 - Flags: review?(gijskruitbosch+bugs)

Comment 5

4 years ago
Comment on attachment 8580143 [details]
MozReview Request: bz://1143844/margaret

https://reviewboard.mozilla.org/r/5725/#review4679

Ship It!
Attachment #8580143 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 7

4 years ago
Sigh, this caused test failures, likely because we aren't showing the button on our test pages now. I backed this out, but I think a fix will just involve making sure our test page has a bunch of big enough paragraphs to match our algorithm.

https://hg.mozilla.org/integration/fx-team/rev/80ade5ec1d31
(Assignee)

Comment 8

4 years ago
Tests pass locally for me with this change.
Attachment #8580242 - Flags: review?(gijskruitbosch+bugs)

Updated

4 years ago
Attachment #8580242 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/6b02cd43cc20
https://hg.mozilla.org/mozilla-central/rev/911e01832a4a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Margaret, is there anything manual QA can verify here? If so, please provide any information you think might help us on the matter.
Flags: qe-verify?
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 12

4 years ago
(In reply to Andrei Vaida, QA [:avaida] from comment #11)
> Margaret, is there anything manual QA can verify here? If so, please provide
> any information you think might help us on the matter.

The main thing to look out for here is whether or not the button appears when we think it should/shouldn't. If you find any pages where the page looks like an article, but the button doesn't appear, you should report that. Alternately, if you ever click the button and the page just reloads, you should report that as well.
Flags: needinfo?(margaret.leibovic)
Depends on: 1147487
(Assignee)

Updated

4 years ago
Depends on: 1147626
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
A quick update here: this is currently being covered by the tests conducted on Readability.js. The current progress is available in the following etherpad - https://etherpad.mozilla.org/Fx38-Readability. 

As of this moment, issues have been filed with the Readability GitHub Repository and will be tracked there in the future as well, for each of the websites available in the list from Bug 1139165 Comment 1.
Marking this as verified since all of the new bugs were filed in GitHub and are being tracked separately.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 16

4 years ago
Attachment #8580143 - Attachment is obsolete: true
Attachment #8619766 - Flags: review+
Manual testing for this was already done in Firefox 38, and I don't think this work needs to be repeated for Firefox 39.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.