Speedreader stylesheet from component updater #6257
Conversation
AndriusA
changed the title
AndriusA
force-pushed
the
speedreader-stylesheet
branch
from
03f730b
to
ceaa5db
Compare
|
Only unrelated |
| @@ -10,29 +10,22 @@ | |||
| #include <utility> | |||
|
|
|||
| #include "base/bind.h" | |||
| #include "base/files/file_path.h" | |||
| if (speedreader::IsWhitelistedForTest(handle->GetURL()) || | ||
| whitelist->IsWhitelisted(handle->GetURL())) { | ||
| rewriter_service->IsWhitelisted(handle->GetURL())) { |
There was a problem hiding this comment.
Now it's not super clear why IsWhitelisted belongs to "rewriter" service. Maybe we could rename to something generic, like SpeedreaderService
There was a problem hiding this comment.
I'd agree, but we already have SpeedreaderService that does a very different function of controlling/tracking when the feature is enabled/disabled. Here it isWhitelisted for rewriting, so made sense to be more specific
Refactors SpeedReader to separate component updater hooks from business logic. Splits SpeedreaderWhitelist into SpeedreaderComponent (for component management) and SpeedreaderRewriterService for the rewriter.
AndriusA
force-pushed
the
speedreader-stylesheet
branch
from
ceaa5db
to
5a30273
Compare
| "//brave/components/resources:static_resources_grit", | ||
| "//brave/components/resources", | ||
| "//brave/components/weekly_storage", |
There was a problem hiding this comment.
previously missed, not picked up by gn check
| // Used in testing/development with custom rule set for auto-reloading | ||
| void OnWhitelistFileReady(const base::FilePath& path, bool error); | ||
|
|
||
| base::ObserverList<Observer> observers_; |
There was a problem hiding this comment.
I think we can simplify by just holding a raw pointer, given that the only observer by design is an actual owner
There was a problem hiding this comment.
You actually can pass this pointer to Component constructor
There was a problem hiding this comment.
thought of that, but went for a solution that better abstracts away the specifics of speedreader and attempts to follow a more decoupled pattern of using component updater.
Happy to couple them up more if you prefer?
Perhaps might be even better to initialise SpeedreaderComponent outside of the SpeedreaderRewriterService altogether and just subscribe the service? Didn't want to add too much to the browser process either
| GetDATFileDataResult result) { | ||
| VLOG(2) << "Speedreader loaded from DAT file"; | ||
| if (result.first) | ||
| speedreader_ = std::move(result.first); |
There was a problem hiding this comment.
given that in any case we load the whole thing from disk into memory, we could move all work with files / DAT stuff to the component and only get final results in this class. But this would make SpeedreaderComponent aware of rust "client", so also doesn't look neat
There was a problem hiding this comment.
it was previously in the component, but this moves it closer to where it is used, and as a next step would allow to more straightforwardly unload both from memory when the feature is disabled independently of component handling
Resolves brave/brave-browser#11004
Partly addresses brave/brave-browser#9652
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
For initial verification that the approach works, need to:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.