Speedreader repository moved into brave core #5142
Conversation
|
|
AndriusA
force-pushed
the
speedreader-rust
branch
2 times, most recently
from
a64b3fc
to
2c4a28f
Compare
|
|
||
| if (is_mac || is_ios ) { | ||
| libs += [ | ||
| "Security.framework" |
There was a problem hiding this comment.
is this actually needed or was it just copied from another repo?
There was a problem hiding this comment.
copied, not noticed issues without, removed now
| speedreader_flags = "" | ||
| } | ||
|
|
||
| speedreader_shared_lib_install_name = "" |
There was a problem hiding this comment.
this stuff was never meant to be duplicated in other repos, in a follow up we need to reorganize this stuff
There was a problem hiding this comment.
reorganised some if it here, still need to resolve a linking issue on linux, can you take another look?
| "src/errors.rs", | ||
| "src/lib.rs", | ||
| "src/speedreader.rs", | ||
| "../src/src/whitelist.rs", |
There was a problem hiding this comment.
there seem to be some missing inputs here like lolhtml.rs all inputs files must be listed or changes to them won't trigger a rebuild
There was a problem hiding this comment.
listed all ../lib/src files that should trigger a rebuild (examples, benches and tests not used in the browser build)
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| * You can obtain one at http://mozilla.org/MPL/2.0/. */''' | ||
| include_guard = "BRAVE_COMPONENTS_SPEEDREADER_RUST_FFI_INCLUDE_SPEEDREADER_FFI_H_" |
There was a problem hiding this comment.
this config controls how components/speedreader/rust/ffi/includes/speedreader_ffi.h is auto-generated (besides the auto-generated file having to be in output dir)
include_guard just specifies the full guard as per chromium's naming convention, since cbindgen doesn' know about the rest of the project structure
|
|
||
| cbindgen::generate(crate_dir) | ||
| .expect("Unable to generate bindings") | ||
| .write_to_file("include/speedreader_ffi.h"); |
There was a problem hiding this comment.
I'm confused, is this a generated file? If so we need to change the way this works because generated files need to go in the output dir and they shouldn't be committed in the repo
There was a problem hiding this comment.
it is, will try to figure out a way of controlling output path via gn
There was a problem hiding this comment.
introduced a new action template globally to handle this (and other crates). Bindings header does not strictly need to be auto-generated, but in all of our cases seem to be, only committed to source control instead of generating from source on-demand
| "src/errors.rs", | ||
| "src/lib.rs", | ||
| "src/speedreader.rs", | ||
| "../src/src/whitelist.rs", |
There was a problem hiding this comment.
also why is there a bunch of stuff in src/src instead of just src? Most of the files are also missing from the list of inputs
There was a problem hiding this comment.
wanting to keep the FFI part of the library separate from the logic, we discussed having brave/components/speedreader/rust/src and brave/components/speedreader/rust/ffi and Cargo package convention puts sources within src.
Happy to rename (src -> lib, so it would be lib/src?), but doesn't seem useful to just merge them
| bool deserialize(const char* data, size_t data_size); | ||
|
|
||
| /// Checks if the provided URL matches whitelisted readable URLs. | ||
| bool ReadableURL(const std::string& url); |
There was a problem hiding this comment.
Why this class is responsible for both whitelist and rewriting? Is it possible to split?
If we do, we can relax about creating multiple speedreaders inside throttles
There was a problem hiding this comment.
suggested rename IsReadableURL
There was a problem hiding this comment.
discussed reason for joint responsibility, method renamed
| void*), | ||
| void* output_sink_user_data); | ||
|
|
||
| static std::string TakeLastError(); |
There was a problem hiding this comment.
just curious - is it needed? is it thread-safe?
There was a problem hiding this comment.
ends up useful for logging and a baby step towards better error handling across the FFI boundary.
but yes, it is thread-safe in the sense that the error if any is stored in thread-local storage
AndriusA
force-pushed
the
speedreader-rust
branch
2 times, most recently
from
a023307
to
c3e4097
Compare
| return engine_->matches(url.spec(), url.host(), url.host(), | ||
| false, "sub_frame", &explicit_cancel, | ||
| &saved_from_exception, &redirect); | ||
| std::unique_ptr<Rewriter> SpeedreaderWhitelist::MakeRewriter(const GURL& url) { |
There was a problem hiding this comment.
for followup - seems like this method belongs in the SpeedreaderService along with IsWhitelisted/IsReadableURL
| @@ -8,29 +8,54 @@ | |||
| #include <utility> | |||
There was a problem hiding this comment.
for followup the name/class of this file should really be SpeedreaderWhitelistInstaller
| @@ -23,8 +25,8 @@ class SpeedReaderThrottle : public blink::URLLoaderThrottle { | |||
| // |task_runner| is used to bind the right task runner for handling incoming | |||
| // IPC in SpeedReaderLoader. |task_runner| is supposed to be bound to the | |||
| // current sequence. | |||
| explicit SpeedReaderThrottle( | |||
| scoped_refptr<base::SingleThreadTaskRunner> task_runner); | |||
| SpeedReaderThrottle(SpeedreaderWhitelist* whitelist, | |||
There was a problem hiding this comment.
for followup this should use the SpeadreaderService
| @@ -7,6 +7,8 @@ | |||
| #define BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_THROTTLE_H_ | |||
|
|
|||
| #include "base/memory/weak_ptr.h" | |||
| #include "brave/components/speedreader/speedreader_switches.h" | |||
There was a problem hiding this comment.
this doesn't appear to be used here
| @@ -7,6 +7,8 @@ | |||
| #define BRAVE_COMPONENTS_SPEEDREADER_SPEEDREADER_THROTTLE_H_ | |||
|
|
|||
| #include "base/memory/weak_ptr.h" | |||
| #include "brave/components/speedreader/speedreader_switches.h" | |||
| #include "brave/components/speedreader/speedreader_whitelist.h" | |||
There was a problem hiding this comment.
this should be a forward declaration
|
unrelated travis npm security failure |
|
Added missing milestone |
Not needed after #5142 Fixes brave/brave-browser#9650
Resolves brave/brave-browser#8978
Resolves brave/brave-browser#7995
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.