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

RESOLVED FIXED in Firefox 38.0.5

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Gijs, Assigned: Margaret)

Tracking

(Blocks 1 bug)

Trunk
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox38 wontfix, firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

4 years ago
+++ 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

4 years ago
Assignee: nobody → margaret.leibovic
Flags: needinfo?(margaret.leibovic)
Assignee

Comment 2

4 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

4 years ago
(We don't need this on 38.0, only 38.0.5)
Assignee

Updated

4 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

4 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

4 years ago
Here's a try push that just comments this parameter out:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=31f083e568c1
Assignee

Updated

4 years ago
Depends on: 1160775
Assignee

Comment 8

4 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)
https://hg.mozilla.org/mozilla-central/rev/6ca86602ed8d
https://hg.mozilla.org/mozilla-central/rev/576b7bb1c844
Status: NEW → RESOLVED
Closed: 4 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.