Site search: Init Search Engine component [chromium/src : main]
Kay Lin (Gerrit)
Kay Lin has uploaded the change for review![]( "Open in Gerrit")
Commit message
Site search: Init Search Engine component
Change diff
Change information
- M chrome/browser/search_engines/android/BUILD.gn
- A chrome/browser/search_engines/android/java/res/layout/custom_search_engine_item.xml
- A chrome/browser/search_engines/android/java/res/layout/custom_search_engine_list_preference.xml
- A chrome/browser/search_engines/android/java/res/xml/custom_search_engine_preferences.xml
- M chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/SiteSearchSettings.java
- A chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/custom_search_engine/CustomSearchEngineListCoordinator.java
- A chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/custom_search_engine/CustomSearchEngineListMediator.java
- A chrome/browser/search_engines/android/java/src/org/chromium/chrome/browser/search_engines/settings/custom_search_engine/CustomSearchEngineListPreference.java
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keigo Oka (Gerrit)
Keigo Oka added 7 comments![]( "Open in Gerrit")
private final LargeIconBridge mLargeIconBridge;I think the owner of the bridge is a mediator.
private final TemplateUrlService mTemplateUrlService;same here
private final int mFaviconSize;same here (looks like it's needed for mediator to retrieve favicon)
private void updateListItems(List<TemplateUrl> urls) {Coordinator shouldn't directly update the view. I think we should define our model and bind it to the view. We could refer to https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/safety_check/android/java/src/org/chromium/chrome/browser/safety_check/SafetyCheckViewBinder.java;l=211;drc=69560b66256608765d8611974a452019d262de89 as an example.
I'm not really sure whether we should use PropertyModel or ModelList. ModelList might leads to a cleaner code as the UI becomes complex, but maybe we can start our impl from PropertyModel for simplicity.
mLargeIconBridge.getLargeIconForUrl(This should be something mediator should do. The mediator should retrieve the icons and set them to the model
View menuButton = itemView.findViewById(R.id.overflow_menu_button);Overflow might be natively supported by PreferenceCategory. Could you try the initialExpandedChildrenCount attribute https://developer.android.com/develop/ui/views/components/settings/components-and-attributes#preferencecategory_attributes
mCoordinator.onBindViewHolder(holder.itemView);The preference shouldn't be aware of coordinator. It should be the SiteSearchSettings that creates a coordinator.
Related details
- Kay Lin
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. |
Kay Lin (Gerrit)
Kay Lin added 7 comments![]( "Open in Gerrit")
private final LargeIconBridge mLargeIconBridge;I think the owner of the bridge is a mediator.
Done
private final TemplateUrlService mTemplateUrlService;Kay Linsame here
Done
private final int mFaviconSize;same here (looks like it's needed for mediator to retrieve favicon)
Done
private void updateListItems(List<TemplateUrl> urls) {Coordinator shouldn't directly update the view. I think we should define our model and bind it to the view. We could refer to https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/safety_check/android/java/src/org/chromium/chrome/browser/safety_check/SafetyCheckViewBinder.java;l=211;drc=69560b66256608765d8611974a452019d262de89 as an example.
I'm not really sure whether we should use PropertyModel or ModelList. ModelList might leads to a cleaner code as the UI becomes complex, but maybe we can start our impl from PropertyModel for simplicity.
SG! PTAL
mLargeIconBridge.getLargeIconForUrl(This should be something mediator should do. The mediator should retrieve the icons and set them to the model
Done
View menuButton = itemView.findViewById(R.id.overflow_menu_button);Overflow might be natively supported by PreferenceCategory. Could you try the initialExpandedChildrenCount attribute https://developer.android.com/develop/ui/views/components/settings/components-and-attributes#preferencecategory_attributes
This overflow_menu_button is for editing the rows CRUD. Is that different from what you had in mind?
mCoordinator.onBindViewHolder(holder.itemView);The preference shouldn't be aware of coordinator. It should be the SiteSearchSettings that creates a coordinator.
If the preference doesn't own the coordinator, then it means SiteSearchSettings needs to delegate the onBindViewHolder to Preference?
Related details
- Keigo Oka
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. |
Keigo Oka (Gerrit)
Keigo Oka added 1 comment![]( "Open in Gerrit")
<androidx.recyclerview.widget.RecyclerViewI conceptually like the design, but I'm not sure whether this is allowed to add a RecyclerView inside a PreferenceFragmentCompat, which also uses RecyclerView internally IIUC. Doesn't it result in a brittle UX (like a scrollable inside a scrollable)? Would you mind splitting this CL to the view and the others, where the view is implemented on top of fake data, and send the view one to review?
Related details
- Kay Lin
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. |
Kay Lin (Gerrit)
Kay Lin abandoned this change.
Kay Lin abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |