Closed
Bug 1143844
Opened 9 years ago
Closed 9 years ago
Be smarter about whether or not to show the reader toolbar button
Categories
(Toolkit :: Reader Mode, defect)
Toolkit
Reader Mode
Tracking
()
VERIFIED
FIXED
mozilla39
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.
Assignee | ||
Comment 1•9 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•9 years ago
|
Assignee: nobody → margaret.leibovic
Assignee | ||
Comment 2•9 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•9 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•9 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•9 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 6•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1dbf52164ffe
Assignee | ||
Comment 7•9 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•9 years ago
|
||
Tests pass locally for me with this change.
Attachment #8580242 -
Flags: review?(gijskruitbosch+bugs)
Updated•9 years ago
|
Attachment #8580242 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/6b02cd43cc20 https://hg.mozilla.org/integration/fx-team/rev/911e01832a4a
https://hg.mozilla.org/mozilla-central/rev/6b02cd43cc20 https://hg.mozilla.org/mozilla-central/rev/911e01832a4a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 11•9 years ago
|
||
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•9 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)
Comment 13•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/6f97fe51dff7 https://hg.mozilla.org/releases/mozilla-aurora/rev/28912ec5344f
status-firefox38:
--- → fixed
Updated•9 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Attachment #8580143 -
Attachment is obsolete: true
Attachment #8619766 -
Flags: review+
Assignee | ||
Comment 17•9 years ago
|
||
Comment 18•9 years ago
|
||
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.
Description
•