Enable actor UI interface to build on Android [chromium/src : main]
David Bokan (Gerrit)
David Bokan added 1 comment![]( "Open in Gerrit")
if (!is_android) {Would it make sense to replace guards like this with something like `if(!skip_android_unmigrated_actor_files)` which is set to true in the Android build. That way we can ensure the migration is complete when that flag is removed vs. missing an `is_android` flag which could be easy to do in a more generic buildfile. (though IDK, I guess that just moves that onus from then to now...)
Related details
- Abe Boujane
- Kevin Graney
- Siddhartha S
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. |
Min Qin (Gerrit)
Min Qin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Abe Boujane
- Kevin Graney
- Siddhartha S
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. |
Kevin Graney (Gerrit)
Kevin Graney added 2 comments![]( "Open in Gerrit")
if (!skip_android_unmigrated_actor_files) {
sources += [
"actor_border_view_controller.cc",
"actor_border_view_controller.h",
"actor_overlay_handler.cc",
"actor_overlay_handler.h",
"actor_overlay_ui.cc",
"actor_overlay_ui.h",
"actor_overlay_web_view.cc",
"actor_overlay_web_view.h",
"actor_ui_state_manager_prefs.cc",
"actor_ui_state_manager_prefs.h",
"actor_ui_tab_controller.cc",
"actor_ui_tab_controller.h",
"actor_ui_tab_controller_interface.cc",
"actor_ui_tab_controller_interface.h",
"actor_ui_window_controller.cc",
"actor_ui_window_controller.h",
"handoff_button_controller.cc",
"handoff_button_controller.h",
]
deps += [
"//chrome/app/vector_icons",
"//chrome/browser/actor/resources:browser_resources",
"//chrome/browser/actor/ui:ui_event_utils",
"//chrome/browser/profiles:profile",
"//chrome/browser/resources/actor_overlay:resources",
"//chrome/browser/ui/browser_window",
"//chrome/browser/ui/tabs:tab_strip",
"//chrome/browser/ui/tabs:tabs_public",
"//chrome/browser/ui/toasts",
"//chrome/browser/ui/toasts/api:toasts",
"//chrome/browser/ui/views/frame:immersive_mode_controller",
"//chrome/browser/ui/views/interaction",
"//chrome/browser/ui/webui:webui_util",
"//chrome/common",
"//content/public/browser",
"//third_party/abseil-cpp:absl",
"//ui/base/unowned_user_data",
"//ui/views",
"//ui/views/controls/webview",
"//ui/webui",
]
# TODO(crbug.com/40031409): Fix code that adds exit-time destructors and
# enable the diagnostic by removing this line.
configs += [ "//build/config/compiler:no_exit_time_destructors" ]
if (enable_glic) {
deps += [ "//chrome/browser/glic" ]
}
}Maybe split these into different targets? One for the base UI controller logic and another for the UI implementation on desktop platforms?
You can come up with better names, but something like:
- //chrome/browser/actor/ui:ui
- //chrome/browser/actor/ui:ui_desktop
Related details
- Abe Boujane
- Christine Ying
- Siddhartha S
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. |
Siddhartha S (Gerrit)
Siddhartha S added 2 comments![]( "Open in Gerrit")
Would it make sense to replace guards like this with something like `if(!skip_android_unmigrated_actor_files)` which is set to true in the Android build. That way we can ensure the migration is complete when that flag is removed vs. missing an `is_android` flag which could be easy to do in a more generic buildfile. (though IDK, I guess that just moves that onus from then to now...)
Done
there is still some ongoing discussion on how much ui we can reuse. for example we might reuse the overlay implementation on android as well. so its not desktop_ui permanently. there are files in few different states:
- files we can just reuse - interface, metrics
- files we know we need, not migrated, may need more work to split logic - states, pref
- files we are unsure if we want to use - overlay web ui, button controller too
- files we know we cant use - bubbles, views etc.
It feels more appropriate to have them marked as unmigrated now and then once we decide, split the targets to desktop and android. This helps track the status of the migration and work needed. Else we would be going down the rabbit hole of deciding each file's final target?
cc @haile...@google.com
I anticipate that once we have the overlay implementation decided, we would migrate these files related. then split the logic and implementation and create targets
Related details
- Abe Boujane
- Christine Ying
- David Bokan
- Hailey Wang
- Kevin Graney
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. |
Christine Ying (Gerrit)
Christine Ying added 1 comment![]( "Open in Gerrit")
for (const auto& tab : GetTabs(task_id)) {
if (auto* tab_controller = ActorUiTabControllerInterface::From(tab)) {
tab_controller->OnUiTabStateChange(ui_tab_state,
base::BindOnce(&LogUiChangeError));
}
}since the tab controller is marked unmigrated for now, should we also add the BUILDFLAG check here? and everywhere else ActorUiTabControllerInterface is used.
Related details
- Abe Boujane
- David Bokan
- Hailey Wang
- Kevin Graney
- Siddhartha S
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. |
David Bokan (Gerrit)
David Bokan added 1 comment![]( "Open in Gerrit")
Seems reasonable to me but kmg@, boujane@, chstne@ would be the experts here so will defer the rest to them.
Related details
- Abe Boujane
- Hailey Wang
- Kevin Graney
- Siddhartha S
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. |
Kevin Graney (Gerrit)
Kevin Graney added 1 comment![]( "Open in Gerrit")
Do you need to know your implementation details to make progress on dividing up this build target? It seems possible to draw a few component boundaries and separate those into different targets. There's a boundary between UiEventDispatcher and ActorUiStateManager, for example. You could probably also pull the overlay out into its own component. That seems more productive than adding temporary branches to the BUILD rules.
Related details
- Abe Boujane
- Hailey Wang
- Siddhartha S
Siddhartha S (Gerrit)
Siddhartha S added 1 comment![]( "Open in Gerrit")
for (const auto& tab : GetTabs(task_id)) {
if (auto* tab_controller = ActorUiTabControllerInterface::From(tab)) {
tab_controller->OnUiTabStateChange(ui_tab_state,
base::BindOnce(&LogUiChangeError));
}
}since the tab controller is marked unmigrated for now, should we also add the BUILDFLAG check here? and everywhere else ActorUiTabControllerInterface is used.
thanks. Maybe we could incldue the interface also in the build. didn't notice the calls. But i am going OOO so will let Linda take over and fix this.
Related details
- Abe Boujane
- Christine Ying
- Hailey Wang
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. |
Linda Wang (Gerrit)
Linda Wang has uploaded the change for review![]( "Open in Gerrit")
Linda Wang would like Kevin Graney, Salvador Guerrero Ramos, Abe Boujane, Christine Ying and Min Qin to review this change.
Commit message
Enable actor UI interface to build on Android
Change diff
Change information
- M chrome/browser/actor/BUILD.gn
- M chrome/browser/actor/ui/BUILD.gn
- M chrome/browser/actor/ui/actor_ui_state_manager.cc
- M chrome/browser/actor/ui/actor_ui_state_manager.h
- M chrome/browser/actor/ui/actor_ui_state_manager_interface.h
Related details
- Abe Boujane
- Christine Ying
- Kevin Graney
- Min Qin
- Salvador Guerrero Ramos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hailey Wang (Gerrit)
Hailey Wang added 1 comment![]( "Open in Gerrit")
Quick update on this about the overlay impl; we decided to reuse the desktop overlay structure/files. We are hoping it should work out of the box + some extra detail polish/changes. FYI @linda...@google.com
Related details
- Abe Boujane
- Linda Wang
- Salvador Guerrero Ramos
- Siddhartha S
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. |