[WebInstallAPI] Rewire 0 param installs through manifest URL path [chromium/src : main]
Lia Hiscock (Gerrit)
Lia Hiscock voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Lu Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lu Huang (Gerrit)
Lu Huang added 4 comments![]( "Open in Gerrit")
CHECK(provider);Since the AreWebAppsUserInstallable()/TriggerInstallNotSupportedDialog() guard now runs later in the flow, can GetForWebApps(provider_profile) return null here? The legacy path reached its CHECK(provider) only after that guard, while the manifest-URL path handles a null provider gracefully. Should this do the same?
// Manifest was successfully parsed and meets all web install requirements.Does this manifest actually meet all install requirements? GetPrimaryPageFirstSpecifiedManifest() does not enforce a valid specified start_url or name like ParseManifestFromStringJob does. Should that required-field validation be shared here, or tracked as a follow-up?
const webapps::AppId* app_id =
web_app::WebAppTabHelper::GetAppId(web_contents);I think WATH::GetAppId uses `FindBestAppWithUrlInScope(url, ...)`.
Should we do this instead for a more precise match using `manifest->id`?
```C++
auto* provider = WebAppProvider::GetForWebContents(web_contents);
CHECK(provider);
std::optional<webapps::AppId> app_id =
IsAppInstalled(*provider, last_committed_url_,
std::make_optional(manifest->id));
```
web_app::WebAppTabHelper::GetAppId(web_contents);Ditto?
Related details
- Lia Hiscock
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. |
Lu Huang (Gerrit)
Lu Huang added 1 comment![]( "Open in Gerrit")
EXPECT_TRUE(tester.IsFulfilled());Could we lock in that a successful zero-argument install no longer exposes manifestId? There does not appear to be a successful-install WPT covering the result shape.
```
ASSERT_TRUE(tester.IsFulfilled());
DummyExceptionStateForTesting conversion_exception_state;
WebInstallResult* result =
NativeValueTraits<WebInstallResult>::NativeValue(
GetScriptState()->GetIsolate(), tester.Value().V8Value(),
conversion_exception_state);
ASSERT_FALSE(conversion_exception_state.HadException());
ASSERT_TRUE(result);
EXPECT_FALSE(result->hasManifestId());
```
Lu Huang (Gerrit)
Lu Huang added 1 comment![]( "Open in Gerrit")
web_app::WebAppTabHelper::GetAppId(web_contents);Ditto?
Actually, manifest->id is not available here and this should be removed soon anyway.
Lu Huang (Gerrit)
Lu Huang voted and added 1 comment![]( "Open in Gerrit")
Related details
- Lia Hiscock
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. |
Lu Huang (Gerrit)
Lu Huang added 2 comments![]( "Open in Gerrit")
EXPECT_TRUE(future.Wait());
return future.Get();Nit:
```suggestion
return future.Take();
```
EXPECT_TRUE(future.Wait());
return future.Get();Nit:
```suggestion
return future.Take();
```
Lu Huang (Gerrit)
Lu Huang added 1 comment![]( "Open in Gerrit")
EXPECT_TRUE(future.Wait());
return future.Get();Nit:
```suggestion
return future.Take();
```
Take includes a CHECK for success -
Take() → TakeTuple() → Wait() → CHECK(success)
Lia Hiscock (Gerrit)
Lia Hiscock added 9 comments![]( "Open in Gerrit")
Since the AreWebAppsUserInstallable()/TriggerInstallNotSupportedDialog() guard now runs later in the flow, can GetForWebApps(provider_profile) return null here? The legacy path reached its CHECK(provider) only after that guard, while the manifest-URL path handles a null provider gracefully. Should this do the same?
yeah...this should only be an issue if web apps are somehow not enabled in both the original and the calling profiles...according to copilot "some system, kiosk, or unusual embedder-created profiles"...still unclear on those other profiles, but yeah i switched to an abort error
// Manifest was successfully parsed and meets all web install requirements.Does this manifest actually meet all install requirements? GetPrimaryPageFirstSpecifiedManifest() does not enforce a valid specified start_url or name like ParseManifestFromStringJob does. Should that required-field validation be shared here, or tracked as a follow-up?
Thanks for the callout. I ended up switching `GetPrimaryPageFirstSpecifiedManifest` to `CheckInstallabilityAndRetrieveManifest`, as that's what's eventually used by `FetchManifestAndInstallCommand`, so it'll get us the closest parity for data errors, as it's not just missing/mismatched manifest id.
e.g. A manifest without a valid icon should also produce a data error in incognito, since that's what would happen in a regular profile. Added `WebInstallPrivacyInvariantTest` coverage for these data errors.
LMK (or @krist...@microsoft.com i guess) if there are any other things you can think that we should specifically be pre-validating here!
const webapps::AppId* app_id =
web_app::WebAppTabHelper::GetAppId(web_contents);I think WATH::GetAppId uses `FindBestAppWithUrlInScope(url, ...)`.
Should we do this instead for a more precise match using `manifest->id`?
```C++
auto* provider = WebAppProvider::GetForWebContents(web_contents);
CHECK(provider);std::optional<webapps::AppId> app_id =
IsAppInstalled(*provider, last_committed_url_,
std::make_optional(manifest->id));
```
ah yes good point. then we can also plumb `app_id` through to the actual launch dialog command, instetad of recomputing it again
void WebInstallServiceImpl::OnAppInstalledFromManifest(@krist...@microsoft.com - i realized i never actually copied over all the logic here that was in `OnAppInstalled` - apparently it wasn't that critical to any tests, but JFYI that this logic wasn't here before...
Lu HuangDitto?
Actually, manifest->id is not available here and this should be removed soon anyway.
Acknowledged - yes `TryInstallCurrentDocument` will be removed
*app_id, web_app::WebAppFilter::InstalledInChrome())) {@krist...@microsoft.com - can you double check why we used `InstalledInChrome` here instead of `LaunchableFromInstallApi`? Can't remember if I just hadn't landed `LaunchableFromInstallApi` yet or there was an intentional reason...
It's relevant for the new function I added - `RecheckInstalledAppMaybeLaunch` - which I chose `LaunchableFromInstallApi` for.
Either way, my initial feeling was to use `LaunchableFromInstallApi`, since that's what's being used for the initial "is this app installed" check, so it doesn't really make sense to diverge.
Lu HuangNit:
```suggestion
return future.Take();
```
Take includes a CHECK for success -
Take() → TakeTuple() → Wait() → CHECK(success)
Done
EXPECT_TRUE(future.Wait());
return future.Get();Lia HiscockNit:
```suggestion
return future.Take();
```
Done
EXPECT_TRUE(tester.IsFulfilled());Could we lock in that a successful zero-argument install no longer exposes manifestId? There does not appear to be a successful-install WPT covering the result shape.
```
ASSERT_TRUE(tester.IsFulfilled());DummyExceptionStateForTesting conversion_exception_state;
WebInstallResult* result =
NativeValueTraits<WebInstallResult>::NativeValue(
GetScriptState()->GetIsolate(), tester.Value().V8Value(),
conversion_exception_state);ASSERT_FALSE(conversion_exception_state.HadException());
ASSERT_TRUE(result);
EXPECT_FALSE(result->hasManifestId());
```
you're correct, we don't have any WPTs that cover the result shape - I think this is because i assumed we could only test the initial invocation of `n.i()`, and return value validation would require some kind of response? @krist...@microsoft.com can you confirm whether a result-shape WPT is possible?
re. this comment though - yes, added the manifestId validation here.
Related details
- Lu Huang
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. |
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
NestedScopeInstallsManifestApp) {@krist...@microsoft.com - this test, and the one below, were added by my agent to ensure our `FindInstalledAppByManifestId` correctly handles nested app scopes - I didn't get a chance to think through the general logic here though - if you can, please just confirm these are actually testing what we want
cc @lu...@microsoft.com - these are new since you reviewed I think
Related details
- Kristin Lee
- Lu Huang
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. |