[Speedreader] Update to kuchikiki 0.8.2 #18488

rillian · 2023-05-15T20:40:27Z

Update to our renamed fork to address unmaintained crate warnings about the kuchiki crate. Also cleans up some obsolete code.

NB This should probably wait until the rust build rework lands and be redone with the correct vendoring updates, but I wanted to get the source changes tested to make sure the new version will work for us.

Resolves brave/brave-browser#29079

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

rillian · 2023-05-16T21:20:22Z

CI failures are the SpeedreaderRewriterPagesTest.CheckPages failing, I think because of different attribute ordering.

wknapik · 2023-07-17T12:51:19Z

@rillian is this waiting for something still?

rillian · 2023-07-17T22:15:35Z

It was waiting for the rust built rewrite to land. Now it just needs redoing on top of those changes, and reviewing and updating the tests. I won't be work on it until August, so anyone feel free to take it over.

wknapik · 2023-09-15T11:48:05Z

*bump*

test/data/speedreader/rewriter/pages/news_pages/www.csmonitor.com/distilled.html

test/data/speedreader/rewriter/pages/news_pages/www.nbcdfw.com/distilled.html

test/data/speedreader/rewriter/pages/news_pages/kdvr.com/distilled.html

rillian · 2023-09-19T21:40:10Z

I redid the patch on top of the new rust build integration. Haven't updated html5ever to match the version in kuchikiki's Cargo.toml, but it seems to work without that in local testing.

test/data/speedreader/rewriter/pages/news_pages/www.eonline.com/distilled.html

test/data/speedreader/rewriter/pages/news_pages/www.newsday.com/distilled.html

rillian · 2024-01-20T15:34:46Z

@antonok-edm @bridiver Finally got this to pass CI. Ready for review again.

Remove obsolete benchmark target and fix relative path to the kuchiki dependency so `cargo check` passes in the local crate directory for the rust code. This facilitates local testing and comparison between cargo and the gn builds. For the same reason, add a similar diversion to the ffi crate. Each top-level Cargo.toml needs to declare depencency patches separately.

We renamed our fork of the kuchiki tree-handling crate since upstream is unmaintained. It's easier to build against released crates now that we're vendoring, so move to the new publication crate name. This also addresses a `cargo audit` warning about the unmaintained dependency. Note however that we're still using a brave-specific fork from git here, so the crate itself is still installed from DEPS. This adds a new transitive dependency on `indexmap`, which is used to roundtrip html attributes with stable ordering so unit tests don't need trivial updates whenever we touch something. This dependency was for a while available in upstream chromium, but has since been dropped, so we're using our own copy under brave/third_party/rust. kuchikiki 0.8.2 specifies newer versions of its other dependencies but seems to still work when built against the old releases, so those are left to another commit. This replaces the DEPS entry with a copy of the source from https://github.com/brave/kuchikiki for simpler updating. Note that this is still a patched version, different from the 0.8.2 package published on crates.io.

Attribute ordering in the simplified pages has changed, verified by running `tidy --sort-attributes alpha` on both the new and old versions from the failure report. Otherwise where are no changes to document content or structure. Fixes SpeedreaderRewriterPagesTest.CheckPages unit test failures.

Lint fixes on the new speedreader code.

Document each file as MPL 2.0-licensed, per code policy. Addresses an `npm run presubmit` warning.

The `indexmap` crate tries to detect with a build script whether the `std` library crate is available. This fails on our android integration builds: ACTION //brave/third_party/rust/indexmap/v1:indexmap_lib_v1_build_script_output(//build/toolchain/android:android_clang_x86) error[E0463]: can't find crate for `std` | = note: the `i686-linux-android` target may not be installed However, the check can be overridden by explicitly requesting the feature, and here we do so in our build. This is the default in the newer v2 release of the crate, so this change is likely to remain correct going forward.

rillian · 2024-01-25T20:19:32Z

Replaced DEPS checkout with a vendored copy of the kuchikiki crate source; rebased past adjacent changes.

rillian added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label May 15, 2023

rillian self-assigned this May 15, 2023

rillian requested a review from a team as a code owner May 15, 2023 20:40

rillian requested a review from boocmp May 15, 2023 20:40

boocmp approved these changes May 16, 2023

View reviewed changes

rillian changed the title ~~Speedreader] Update to kuchikiki 0.8.2~~ [Speedreader] Update to kuchikiki 0.8.2 May 17, 2023

rillian requested a review from iefremov as a code owner May 17, 2023 21:55

rillian force-pushed the speedreader/kuchiki-0.8.2 branch from a332fcb to 3d19bda Compare September 19, 2023 21:34

github-actions bot reviewed Sep 19, 2023

View reviewed changes

github-actions bot added the needs-security-review label Sep 19, 2023

github-actions bot assigned fmarier and thypon Sep 19, 2023

rillian force-pushed the speedreader/kuchiki-0.8.2 branch from 3d19bda to 760ab8f Compare September 19, 2023 23:07

rillian requested review from a team and bridiver as code owners September 19, 2023 23:07

github-actions bot reviewed Sep 19, 2023

View reviewed changes

test/data/speedreader/rewriter/pages/news_pages/www.eonline.com/distilled.html Show resolved Hide resolved

test/data/speedreader/rewriter/pages/news_pages/www.newsday.com/distilled.html Show resolved Hide resolved

fmarier removed the needs-security-review label Sep 20, 2023

fmarier unassigned fmarier and thypon Sep 20, 2023

rillian force-pushed the speedreader/kuchiki-0.8.2 branch from c74e8cd to 35f4522 Compare November 15, 2023 00:52

rillian force-pushed the speedreader/kuchiki-0.8.2 branch from 35f4522 to e5419cb Compare November 28, 2023 04:48

rillian force-pushed the speedreader/kuchiki-0.8.2 branch from e5419cb to 9f63d8c Compare December 5, 2023 22:54

rillian force-pushed the speedreader/kuchiki-0.8.2 branch 2 times, most recently from 0d7325f to 2887779 Compare January 9, 2024 00:54

rillian force-pushed the speedreader/kuchiki-0.8.2 branch from 2887779 to c972204 Compare January 9, 2024 15:35

rillian force-pushed the speedreader/kuchiki-0.8.2 branch 2 times, most recently from 321e16d to a91e8a4 Compare January 18, 2024 18:02

rillian added 6 commits January 25, 2024 11:20

speedreader: Add semicolons to return lines

31a1ba4

Lint fixes on the new speedreader code.

speedreader: Add license headers

b7b3517

Document each file as MPL 2.0-licensed, per code policy. Addresses an `npm run presubmit` warning.

rillian force-pushed the speedreader/kuchiki-0.8.2 branch from 653cc98 to 2ecf050 Compare January 25, 2024 19:56

bridiver approved these changes Jan 30, 2024

View reviewed changes

rillian merged commit a7a102b into master Jan 30, 2024
19 checks passed

rillian deleted the speedreader/kuchiki-0.8.2 branch January 30, 2024 22:19

github-actions bot added this to the 1.64.x - Nightly milestone Jan 30, 2024

rillian mentioned this pull request Jan 30, 2024

kuchikiki version inconsistencies brave/brave-browser#35708

Closed

bridiver mentioned this pull request Jan 31, 2024

Revert "[Speedreader] Update to kuchikiki 0.8.2" #21832

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Speedreader] Update to kuchikiki 0.8.2 #18488

[Speedreader] Update to kuchikiki 0.8.2 #18488

rillian commented May 15, 2023 •

edited

Loading

rillian commented May 16, 2023

wknapik commented Jul 17, 2023

rillian commented Jul 17, 2023

wknapik commented Sep 15, 2023

rillian commented Sep 19, 2023

rillian commented Jan 20, 2024

rillian commented Jan 25, 2024

[Speedreader] Update to kuchikiki 0.8.2 #18488

[Speedreader] Update to kuchikiki 0.8.2 #18488

Conversation

rillian commented May 15, 2023 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

rillian commented May 16, 2023

wknapik commented Jul 17, 2023

rillian commented Jul 17, 2023

wknapik commented Sep 15, 2023

rillian commented Sep 19, 2023

rillian commented Jan 20, 2024

rillian commented Jan 25, 2024

rillian commented May 15, 2023 •

edited

Loading