Closed Bug 1158228 Opened 9 years ago Closed 9 years ago

Week 18 / May 1 - -Uplift github version of Readability/JSDOMParser into mozilla-central and aurora/beta


(Toolkit :: Reader Mode, defect)

Not set



40.3 - 11 May
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed


(Reporter: Gijs, Assigned: Margaret)




(1 file)

+++ This bug was initially created as a clone of Bug #1158184 +++

Per request from the sheriffs, one bug per uplift, so here's this week's edition of "let's update Readability.js"!

Margaret, I'll be on PTO, will you be able to take care of merges next week?

Roughly, here's what I do:

cd path/to/fx-team

cp path/to/readability/{R,J}*.js toolkit/components/reader/.

# You need the hg 'record' builtin extension for this:
hg record -m "Bug 1234567 - merge github's readability code into m-c, rs=me"

This is will ask you per-file and per-hunk which changes you want. Take everything apart from the removal of the comments at the top of the files.

# revert those comment removals:
hg revert --no-backup toolkit/components/reader/

And then commit as normal. You can use bzexport to export the topmost commit into a patch on this bug, or you can just copy-paste from the raw-rev item in hgweb into a new attachment.
Flags: qe-verify-
Flags: needinfo?(margaret.leibovic)
Flags: in-testsuite+
Flags: firefox-backlog+
Assignee: nobody → margaret.leibovic
Flags: needinfo?(margaret.leibovic)
Approval Request Comment
[Feature/regressing bug #]: reader mode
[User impact if declined]: can't test reader mode changes
[Describe test coverage new/current, TreeHerder]: has basic tests, stringent tests in github
[Risks and why]: low risk; restricted to reader mode so won't break anything else 
[String/UUID change made/needed]: nope
Attachment #8600491 - Flags: approval-mozilla-beta?
Attachment #8600491 - Flags: approval-mozilla-aurora?
(We don't need this on 38.0, only 38.0.5)
Attachment #8600491 - Attachment is patch: true
It looks like the reader view button isn't appearing for these test pages, which is causing the problem.

I tried generating a testcase in the readability repo for the testcase we use in the browser chrome tests, but that actually returns true for isProbablyReaderable, so that wouldn't have caught this issue.

I tried attaching the debugger to see what's going on, and in that case isProbablyReaderable also returned true. But adding a dump statement showed that it returns false.

Some more debugging revealed that helperIsVisible(node) seems to always be returning false for these nodes (introduced in bug 1153384).

Updating our isProbablyReaderable call in ReaderMode.jsm to not take a helper function parameter makes the tests pass, but a real fix would be figuring out how to make this work properly.
Flags: needinfo?(margaret.leibovic) → needinfo?(gijskruitbosch+bugs)
Here's a try push that just comments this parameter out:
Depends on: 1160775
I filed bug 1160775 to deal with the helper function problem.

Note to releng/sheriffs: if we uplift this, we'll need to uplift both of these changesets.
Flags: needinfo?(gijskruitbosch+bugs)
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Iteration: --- → 40.3 - 11 May
Comment on attachment 8600491 [details] [diff] [review]
Checked-in changes

Approved for uplift to aurora. This needs both changesets in comment 9 to be uplifted.
Attachment #8600491 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8600491 [details] [diff] [review]
Checked-in changes

[Triage Comment]
taking it for 38.0.5
Attachment #8600491 - Flags: approval-mozilla-beta? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.