Closed Bug 1143844 Opened 6 years ago Closed 5 years ago

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

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla39
Tracking Status
firefox38 --- verified
firefox39 --- fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.
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: nobody → margaret.leibovic
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
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).
/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 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+
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
Tests pass locally for me with this change.
Attachment #8580242 - Flags: review?(gijskruitbosch+bugs)
Attachment #8580242 - Flags: review?(gijskruitbosch+bugs) → review+
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)
(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
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
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.