Integrate --focus flag to focus existing browser content [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
#if !BUILDFLAG(IS_ANDROID)
#include "chrome/browser/ui/startup/focus/focus_handler.h"
#endifHelmut Januschkastyle: avoid putting conditional includes inside the main blocks. This can be moved to line 150
Done
if (focus_result.status == focus::FocusStatus::kFocused ||Helmut Januschkacan you please add a comment explaining the intended behavior here? e.g., why do we early-return if there's no match and no command line args?
Done
Related details
- Kaan Alsan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaan Alsan (Gerrit)
Kaan Alsan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kaan Alsan
| Code-Review | +1 |
1 comment
LGTM! I'll comment here once we get launch approval.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
@al...@chromium.org
during review, there where some changes added, so this will require a re-review.
- the `app:` syntax has been removed, and we should use pwa's `manifest_id` (which is just a url TIL), see: (https://chromium.slack.com/archives/CGK77JY3W/p1759251128002739) to my question: (https://chromium.slack.com/archives/CGK77JY3W/p1759247741835889), this way we can simplify all the code alot!
Related details
- Kaan Alsan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaan Alsan (Gerrit)
Kaan Alsan added 1 comment![]( "Open in Gerrit")
Kaan AlsanThis feature is currently undergoing a formal launch review (http://launch/4425960, for internal folks).
Please hold off on submitting this until the launch is approved (I'll let you know when!)
Kaan AlsanFYI, I'll be OOO from Sep 25th to Oct 5th. I'll try to keep the launch review moving and will provide updates as needed, but won't be active for CL reviews.
We got security and privacy approval. One more approval needed and then I'll +1 this again.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Kaan AlsanThis feature is currently undergoing a formal launch review (http://launch/4425960, for internal folks).
Please hold off on submitting this until the launch is approved (I'll let you know when!)
Kaan AlsanFYI, I'll be OOO from Sep 25th to Oct 5th. I'll try to keep the launch review moving and will provide updates as needed, but won't be active for CL reviews.
We got security and privacy approval. One more approval needed and then I'll +1 this again.
- Kaan Alsan
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. |
Kaan Alsan (Gerrit)
Kaan Alsan added 1 comment![]( "Open in Gerrit")
Kaan AlsanThis feature is currently undergoing a formal launch review (http://launch/4425960, for internal folks).
Please hold off on submitting this until the launch is approved (I'll let you know when!)
Kaan AlsanFYI, I'll be OOO from Sep 25th to Oct 5th. I'll try to keep the launch review moving and will provide updates as needed, but won't be active for CL reviews.
Helmut JanuschkaWe got security and privacy approval. One more approval needed and then I'll +1 this again.
anything i can do from my side?
The last approval came in last night, so we are good to launch!
I'll do another pass over this CL and give the +1 shortly
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaan Alsan (Gerrit)
Kaan Alsan voted and added 5 comments![]( "Open in Gerrit")
Votes added by Kaan Alsan
| Code-Review | +1 |
5 comments
// Get the WebAppProvider to access the registrar for app matchingstyle: comment should end with punctuation
web_app::WebAppProvider* provider =
web_app::WebAppProvider::GetForWebApps(&profile);
web_app::WebAppRegistrar* registrar =
provider ? &provider->registrar_unsafe() : nullptr;I'm not familiar enough with this code to be sure if this is the correct usage. Adding @dmu...@chromium.org to review this section.
// between apps and regular tabs with similar URLsstyle: comments should have a period at the end. Same for other comments below.
web_app::AppBrowserController* app_controller =
browser_window.GetAppBrowserController();
if (app_controller) {
const webapps::AppId& browser_app_id = app_controller->app_id();
const web_app::WebApp* web_app = registrar->GetAppById(browser_app_id);
if (web_app) {
const webapps::ManifestId& manifest_id = web_app->manifest_id();
GURL canonicalized_manifest_id = CanonicalizeUrl(manifest_id);
if (selector.type == SelectorType::kUrlExact) {
is_match = (canonicalized_manifest_id == canonicalized_selector_url);
} else if (selector.type == SelectorType::kUrlPrefix) {
std::string manifest_id_string = canonicalized_manifest_id.spec();
std::string selector_url_string = canonicalized_selector_url.spec();
is_match = base::StartsWith(manifest_id_string, selector_url_string,
base::CompareCase::SENSITIVE);
}
if (is_match) {
matched_app_id = manifest_id.spec();
}
}
}
// For app windows, return early - don't match by tab URL
if (!is_match) {
return std::nullopt;
}this logic is sufficiently complex that a helper function would be beneficial here.
Related details
- Daniel Murphy
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
// Get the WebAppProvider to access the registrar for app matchingstyle: comment should end with punctuation
Done
style: comments should have a period at the end. Same for other comments below.
Done
web_app::AppBrowserController* app_controller =
browser_window.GetAppBrowserController();
if (app_controller) {
const webapps::AppId& browser_app_id = app_controller->app_id();
const web_app::WebApp* web_app = registrar->GetAppById(browser_app_id);
if (web_app) {
const webapps::ManifestId& manifest_id = web_app->manifest_id();
GURL canonicalized_manifest_id = CanonicalizeUrl(manifest_id);
if (selector.type == SelectorType::kUrlExact) {
is_match = (canonicalized_manifest_id == canonicalized_selector_url);
} else if (selector.type == SelectorType::kUrlPrefix) {
std::string manifest_id_string = canonicalized_manifest_id.spec();
std::string selector_url_string = canonicalized_selector_url.spec();
is_match = base::StartsWith(manifest_id_string, selector_url_string,
base::CompareCase::SENSITIVE);
}
if (is_match) {
matched_app_id = manifest_id.spec();
}
}
}
// For app windows, return early - don't match by tab URL
if (!is_match) {
return std::nullopt;
}this logic is sufficiently complex that a helper function would be beneficial here.
Done
Related details
- Daniel Murphy
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. |
Kaan Alsan (Gerrit)
Kaan Alsan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kaan Alsan
| Code-Review | +1 |
1 comment
std::optional<std::string> MatchAppByManifestId(If this is only used within the .cc file, this should be in the anonymous namespace
Related details
- Daniel Murphy
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
If this is only used within the .cc file, this should be in the anonymous namespace
Done
Related details
- Daniel Murphy
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. |
Daniel Murphy (Gerrit)
Daniel Murphy voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniel Murphy
| Code-Review | +1 |
3 comments
web_app::WebAppProvider* provider =
web_app::WebAppProvider::GetForWebApps(&profile);
web_app::WebAppRegistrar* registrar =
provider ? &provider->registrar_unsafe() : nullptr;I'm not familiar enough with this code to be sure if this is the correct usage. Adding @dmu...@chromium.org to review this section.
This is... _ok_. I think the main caveat here is that the app might be uninstalling or having some other in-progress operation. However, given that we are seeing browser windows that [seem to filter out any actively closing ones](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_window/internal/browser_window_interface_iterator_non_android.cc;l=37?q=ForEachCurrentBrowserWindowInterfaceOrderedByActivation&ss=chromium), this is likely safe.
Maybe add
```
// This is safe access for our use-case, as we are only using then when
// encountering a browser with an AppBrowserController, which means that an app
// is installed here, at least for now.
```
IN_PROC_BROWSER_TEST_F(FocusHandlerWebAppBrowserTest,you should add a test that triggers a browser close asynchronously, and then tries to do the focus. The code that
}we also need to make sure this is a WebAppBrowserController - you can use the `AsWebAppBrowserController()` method. If it returns a nullptr, then skip this browser.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 4 comments![]( "Open in Gerrit")
Thanks for the feedback, @dmu...@chromium.org! uploaded a new PS addressing all your comments — could you give it a quick look when you get a chance (even though you already gave a CR+1)?
web_app::WebAppProvider* provider =
web_app::WebAppProvider::GetForWebApps(&profile);
web_app::WebAppRegistrar* registrar =
provider ? &provider->registrar_unsafe() : nullptr;Daniel MurphyI'm not familiar enough with this code to be sure if this is the correct usage. Adding @dmu...@chromium.org to review this section.
This is... _ok_. I think the main caveat here is that the app might be uninstalling or having some other in-progress operation. However, given that we are seeing browser windows that [seem to filter out any actively closing ones](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_window/internal/browser_window_interface_iterator_non_android.cc;l=37?q=ForEachCurrentBrowserWindowInterfaceOrderedByActivation&ss=chromium), this is likely safe.
Maybe add
```
// This is safe access for our use-case, as we are only using then when
// encountering a browser with an AppBrowserController, which means that an app
// is installed here, at least for now.
```
Done
you should add a test that triggers a browser close asynchronously, and then tries to do the focus. The code that
Done
we also need to make sure this is a WebAppBrowserController - you can use the `AsWebAppBrowserController()` method. If it returns a nullptr, then skip this browser.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |