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
Categories
(Toolkit :: Reader Mode, defect)
Toolkit
Reader Mode
Tracking
()
People
(Reporter: Gijs, Assigned: Margaret)
References
Details
Attachments
(1 file)
14.06 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
+++ 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 | ||
Updated•9 years ago
|
Assignee: nobody → margaret.leibovic
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 2•9 years ago
|
||
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?
Assignee | ||
Comment 3•9 years ago
|
||
(We don't need this on 38.0, only 38.0.5)
Assignee | ||
Updated•9 years ago
|
Attachment #8600491 -
Attachment is patch: true
I had to back this out for breaking bc1 tests on desktop: https://hg.mozilla.org/integration/fx-team/rev/fd9158e97d81 https://treeherder.mozilla.org/logviewer.html#?job_id=2911810&repo=fx-team
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 5•9 years ago
|
||
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)
Assignee | ||
Comment 6•9 years ago
|
||
Here's a try push that just comments this parameter out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31f083e568c1
https://hg.mozilla.org/integration/fx-team/rev/6ca86602ed8d https://hg.mozilla.org/integration/fx-team/rev/576b7bb1c844
Assignee | ||
Comment 8•9 years ago
|
||
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)
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6ca86602ed8d https://hg.mozilla.org/mozilla-central/rev/576b7bb1c844
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
Iteration: --- → 40.3 - 11 May
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1f1699b0cec1 https://hg.mozilla.org/releases/mozilla-aurora/rev/5de0834fe8a9
Comment 12•9 years ago
|
||
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+
Updated•9 years ago
|
Comment 13•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-release/rev/503f9aa61c25 https://hg.mozilla.org/releases/mozilla-release/rev/46b968653f4d
You need to log in
before you can comment on or make changes to this bug.
Description
•