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

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[:Margaret Leibovic]

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.

Comment 1

[:Margaret Leibovic]

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

Comment 2

[: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

Comment 3

[:Gijs (he/him)]

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).

Comment 4

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1143844/margaret

/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

Comment 5

[:Gijs (he/him)]

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

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

Ship It!

Comment 6

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/1dbf52164ffe

Comment 7

[:Margaret Leibovic]

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

Comment 8

[:Margaret Leibovic]

Attachment: Update readerModeArticle.html test case to have more paragrpahs

Tests pass locally for me with this change.

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/6b02cd43cc20
https://hg.mozilla.org/integration/fx-team/rev/911e01832a4a

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6b02cd43cc20
https://hg.mozilla.org/mozilla-central/rev/911e01832a4a

Comment 11

[Andrei Vaida [:avaida]]

Margaret, is there anything manual QA can verify here? If so, please provide any information you think might help us on the matter.

Comment 12

[: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.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6f97fe51dff7
https://hg.mozilla.org/releases/mozilla-aurora/rev/28912ec5344f

Comment 14

[Andrei Vaida [:avaida]]

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.

Comment 15

[Andrei Vaida [:avaida]]

Marking this as verified since all of the new bugs were filed in GitHub and are being tracked separately.

Comment 17

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1143844 - Check document for readerable content to determine whether or not to show reader button. r=Gijs

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Manual testing for this was already done in Firefox 38, and I don't think this work needs to be repeated for Firefox 39.