10907: Fix threading for certain adblock service methods. #6250
Conversation
Fix brave/brave-browser#10907 Some functionality of adblock service must be used only on certain TaskRunner, so this fixes a couple of methods that were previously called from UI thread. It also required making the corresponding extension shields API functions async. I also did some small code style improvements.
| @@ -54,60 +54,90 @@ BraveShieldsUrlCosmeticResourcesFunction::Run() { | |||
| std::unique_ptr<brave_shields::UrlCosmeticResources::Params> params( | |||
| brave_shields::UrlCosmeticResources::Params::Create(*args_)); | |||
| EXTENSION_FUNCTION_VALIDATE(params.get()); | |||
| g_brave_browser_process->ad_block_service()->GetTaskRunner() | |||
There was a problem hiding this comment.
@antonok-edm This is not super illustrative example of async code - here it is implicitly asserted that all services run on the same task runner exposed by any of them. It would be better for these services to expose async api accepting callbacks for passing resulted values, dealing with task runners inside them.
There was a problem hiding this comment.
@bridiver could you please elaborate
| base::Optional<base::Value> resources = g_brave_browser_process-> | ||
| ad_block_service()->UrlCosmeticResources(params->url); | ||
| ad_block_service()->UrlCosmeticResources(url); |
There was a problem hiding this comment.
I think a better fix here would be to have UrlCosmeticResources take a callback so the threading is handled in the service instead of the caller
There was a problem hiding this comment.
As discussed in slack, I suggest to keep it as is for now, since there are less changes and thus less risks. Also this is aligned with other methods of adblock service (there is no single method taking a callback actually).
I want to refactor the service and improve threading work while fixing brave/brave-browser#10765
Fix brave/brave-browser#10907
Some functionality of adblock service must be used only on
certain TaskRunner, so this fixes a couple of methods that
were previously called from UI thread. It also required
making the corresponding extension shields API functions
async. I also did some small code style improvements.
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.