Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

speedreader: Add --speedreader-backend switch #8363

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

kkuehlz
Copy link
Contributor

@kkuehlz kkuehlz commented Mar 25, 2021

Resolves brave/brave-browser#14959

This is so @pes10k and @rebron can test the changes we make on nightly. Not particularly relevant until #8349 is merged because the current heuristics backend in master is very incomplete.

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
  • 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, npm run lint, 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:

@kkuehlz kkuehlz requested a review from iefremov as a code owner March 25, 2021 18:40
@kkuehlz kkuehlz self-assigned this Mar 25, 2021
@kkuehlz kkuehlz requested a review from pes10k March 25, 2021 18:40
Copy link
Contributor

@pes10k pes10k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, thanks for clarifying. Could you change the names to something more descriptive then?

Maybe list, classify (and if there is such an option, list-and-classify)?

@kkuehlz
Copy link
Contributor Author

kkuehlz commented Mar 26, 2021

Done. Since we only have two backends right now, any option that isn't classify is implied to be list, including no cli option at all.

@kkuehlz kkuehlz requested a review from pes10k March 26, 2021 22:28
@pes10k
Copy link
Contributor

pes10k commented Mar 26, 2021

great, thanks! LGTM!

@kkuehlz kkuehlz force-pushed the speedreader_specify_backend branch from d55b7c6 to e63c6fe Compare March 30, 2021 17:08
@kkuehlz
Copy link
Contributor Author

kkuehlz commented Mar 30, 2021

iOS failing for python reasons, CI passes on macOS, didn't touch anything in vendor/brave-ios. Checked with ios folks and said it should be good to ignore. pre-init failing because of python linting, so also unrelated.

@kkuehlz kkuehlz merged commit 49a7335 into master Mar 30, 2021
@kkuehlz kkuehlz deleted the speedreader_specify_backend branch March 30, 2021 21:43
@kkuehlz kkuehlz added this to the 1.24.x - Nightly milestone Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow user to specify speedreader backend
3 participants