[Search] Observe TemplateURLService from search engines handler [chromium/src : main]
Amelie Schneider (Gerrit)
Amelie Schneider voted and added 1 comment![]( "Open in Gerrit")
Votes added by Amelie Schneider
| Commit-Queue | +0 |
1 comment
Hi Nicolas, could you please take a look? Thanks!
Related details
- Nicolas Dossou-Gbété
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas Dossou-Gbété (Gerrit)
Nicolas Dossou-Gbété added 4 comments![]( "Open in Gerrit")
base::WeakPtrFactory<SearchEnginesHandler> weak_ptr_factory_{this};unused? we should remove it
// TemplateURLServiceObserver notification.nit: convention for grouping overridden methods
```suggestion
// TemplateURLServiceObserver implementation.
```
pref_change_registrar_.Init(profile_->GetPrefs());looks unused, we should be able to remove it
list_controller_.table_model()->SetObserver(nullptr);is this the right thing to do? Doesn't it mean that it could still be updating and firing events even if the handler is not supposed to be active? I see a lot of other settings handlers wiring up their observers in `OnJavascriptAllowed` and unregistering them in `OnJavascriptDisallowed`. I think we should follow that pattern. [WebUI docs](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/webui/webui_explainer.md#webuimessagehandler_allowjavascript) also mention it.
IDK how relevant it is in modern WebUI, but If we want to diverge from it, we should check with dpapad@
In the meantime, since we have `profile_`, we should be able to rewire the observers easily
Related details
- Amelie Schneider
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |