IWA: Add IWA permissions policy cache [chromium/src : main]
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Commit-Queue | +1 |
1 comment
@green...@google.com @sim...@chromium.org can You take a look? I have not yet rebased + chained the second CL but You two know the context anyway.
Related details
- Andrew Rayskiy
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Commit-Queue | +1 |
1 comment
@green...@google.com @sim...@chromium.org can You take a look? I have not yet rebased + chained the second CL but You two know the context anyway.
Sorry, I meant @sim...@google.com, Gerrit autocorrect failed me.
Related details
- Andrew Rayskiy
- Simon Hangl
- Zgroza (Luke) Klimek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Commit-Queue | +1 |
1 comment
This is not yet used in this commit, just populated (see chained CL).(not yet, soon)
Related details
- Andrew Rayskiy
- Simon Hangl
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
Please fix this WARNING reported by Large Change: This change adds 325 lines of production code. Consider splitting this change in...
This change adds 325 lines of production code. Consider splitting this change into smaller ones.
Small changes get reviewed faster and more thoroughly, are less likely to introduce bugs, and are easier to roll back if needed.
You can bypass this warning by adding 'BYPASS_LARGE_CHANGE_WARNING: <reason>' to the change description. (Google-internal: See go/large-change-nudge)
Related details
- Simon Hangl
- Zgroza (Luke) Klimek
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
auto resource_request = std::make_unique<network::ResourceRequest>();As discussed offline, I don't think that the throttle itself is the right place for the parsing code itself -- it's supposed to be a thin proxy that just defers the navigation on a set of external conditions (provider ready, component ready, policy cache entry ready) while avoiding unnecessary bloating. Please move this logic to the PPCache.
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
if (!url_info.has_value() ||You can use the presence of this web contents marker to skip the manifest parsing for installation-like events (update probing, update applying). This will also be much-much clearer to understand.
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 2 comments![]( "Open in Gerrit")
Please fix this WARNING reported by Large Change: This change adds 325 lines of production code. Consider splitting this change in...
This change adds 325 lines of production code. Consider splitting this change into smaller ones.
Small changes get reviewed faster and more thoroughly, are less likely to introduce bugs, and are easier to roll back if needed.
You can bypass this warning by adding 'BYPASS_LARGE_CHANGE_WARNING: <reason>' to the change description. (Google-internal: See go/large-change-nudge)
Not done yet, will do soon.
auto resource_request = std::make_unique<network::ResourceRequest>();As discussed offline, I don't think that the throttle itself is the right place for the parsing code itself -- it's supposed to be a thin proxy that just defers the navigation on a set of external conditions (provider ready, component ready, policy cache entry ready) while avoiding unnecessary bloating. Please move this logic to the PPCache.
Done. Also, removed the dependency on Mojo from this CL and related changes.
Related details
- Andrew Rayskiy
- Simon Hangl
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 9 comments![]( "Open in Gerrit")
: public BrowserContextKeyedServiceFactory {Use `IsolatedWebAppBrowserContextServiceFactory`; see [this](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/isolated_web_apps/window_management/isolated_web_apps_window_open_permission_service_factory.cc). You don't need the getbrowsercontext... override
std::map<url::Origin, CacheEntry> cache_;base::flat_map
explicit IwaPermissionsPolicyCache(Profile* profile);Create from a `WebAppProvider*`; passing `Profile*` here is an antipattern (although an unavoidable one at times)
std::string feature;I'm 95% sure that a struct that simple doesn't need any constructors/operators.
// WebAppInstallManager is initialized withinThis doesn't guarantee anything about the provider initialization (i.e. `on_ready()`). Yes, the `provider_` ptr will be valid, but that's just it.
auto it = cache_.find(iwa_origin);`base::FindOrNull()`
std::optional<base::Value> json_value =Move the parsing to a static func in an anon namespace 😊
cache_.clear();You should rather set the `provider_` to nullptr here to avoid being hit by the dangling ptr checker -- there's not much sense in clearing the cache here (as it will be immediately destroyed anyway).
ManifestBuilder& ClearPermissionsPolicy();I'd rather have a different constructor for the ManifestBuilder (or just add an extra `include_coi_pp` boolean)
Related details
- Simon Hangl
- Zgroza (Luke) Klimek
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
return PROCEED;I'd rather keep this piece of code -- this saves one important callback hop on pretty much every navigation.
Related details
- Simon Hangl
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 1 comment![]( "Open in Gerrit")
// WebAppInstallManager is initialized withinThis doesn't guarantee anything about the provider initialization (i.e. `on_ready()`). Yes, the `provider_` ptr will be valid, but that's just it.
As I wrote in the comment, this does not only create the pointer but also calls `Start()` (literally, the next line of code after `make_unique`) which creates the `WebAppInstallManager`. See initialization [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider_factory.cc;drc=e94ce3f01b473c02658753bdd8502ad039ce1748;l=69) -> [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=165) -> [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=437) and creation in the constructor of the provider itself [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=157) -> [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=378).
Unless the provider can be created somewhere outside of `WebAppProviderFactory::BuildServiceInstanceForBrowserContext` this should assure that the install manager is initialized and well. And even if somehow created from somewhere entirely else, install manager will be created in the constructor itself, so the pointer will be valid.
Related details
- Andrew Rayskiy
- Simon Hangl
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 2 comments![]( "Open in Gerrit")
This is not yet used in this commit, just populated (see chained CL).Zgroza (Luke) Klimek(not yet, soon)
Done
return PROCEED;I'd rather keep this piece of code -- this saves one important callback hop on pretty much every navigation.
Resolving as this is split to another CL. Added comment there: https://chromium-review.googlesource.com/c/chromium/src/+/7280970/comment/f0f98492_8a07809d/
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 10 comments![]( "Open in Gerrit")
Zgroza (Luke) KlimekPlease fix this WARNING reported by Large Change: This change adds 325 lines of production code. Consider splitting this change in...
This change adds 325 lines of production code. Consider splitting this change into smaller ones.
Small changes get reviewed faster and more thoroughly, are less likely to introduce bugs, and are easier to roll back if needed.
You can bypass this warning by adding 'BYPASS_LARGE_CHANGE_WARNING: <reason>' to the change description. (Google-internal: See go/large-change-nudge)
Not done yet, will do soon.
Done
You can use the presence of this web contents marker to skip the manifest parsing for installation-like events (update probing, update applying). This will also be much-much clearer to understand.
Moved to the chained CL, resolving here: https://chromium-review.googlesource.com/c/chromium/src/+/7280970/comment/564f5959_4e95c847/
: public BrowserContextKeyedServiceFactory {Use `IsolatedWebAppBrowserContextServiceFactory`; see [this](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/isolated_web_apps/window_management/isolated_web_apps_window_open_permission_service_factory.cc). You don't need the getbrowsercontext... override
Uh, useful. Done.
std::map<url::Origin, CacheEntry> cache_;Zgroza (Luke) Klimekbase::flat_map
Done
explicit IwaPermissionsPolicyCache(Profile* profile);Create from a `WebAppProvider*`; passing `Profile*` here is an antipattern (although an unavoidable one at times)
Done
std::string feature;Zgroza (Luke) KlimekI'm 95% sure that a struct that simple doesn't need any constructors/operators.
Alas, `error: [chromium-style] Complex class/struct needs an explicit out-of-line constructor.`. I also do not like it, would not do this unless necessary :(
auto it = cache_.find(iwa_origin);Zgroza (Luke) Klimek`base::FindOrNull()`
Oh, that's useful! Done.
std::optional<base::Value> json_value =Move the parsing to a static func in an anon namespace 😊
Done
cache_.clear();You should rather set the `provider_` to nullptr here to avoid being hit by the dangling ptr checker -- there's not much sense in clearing the cache here (as it will be immediately destroyed anyway).
Done
ManifestBuilder& ClearPermissionsPolicy();Zgroza (Luke) KlimekI'd rather have a different constructor for the ManifestBuilder (or just add an extra `include_coi_pp` boolean)
Done
Related details
- Andrew Rayskiy
- Simon Hangl
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 5 comments![]( "Open in Gerrit")
base::flat_map<url::Origin, CacheEntry> cache_;I'd rather have a map from a `SignedWebBundleId` for type guarantees 😊
// WebAppInstallManager is initialized withinZgroza (Luke) KlimekThis doesn't guarantee anything about the provider initialization (i.e. `on_ready()`). Yes, the `provider_` ptr will be valid, but that's just it.
As I wrote in the comment, this does not only create the pointer but also calls `Start()` (literally, the next line of code after `make_unique`) which creates the `WebAppInstallManager`. See initialization [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider_factory.cc;drc=e94ce3f01b473c02658753bdd8502ad039ce1748;l=69) -> [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=165) -> [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=437) and creation in the constructor of the provider itself [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=157) -> [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=378).
Unless the provider can be created somewhere outside of `WebAppProviderFactory::BuildServiceInstanceForBrowserContext` this should assure that the install manager is initialized and well. And even if somehow created from somewhere entirely else, install manager will be created in the constructor itself, so the pointer will be valid.
The road roller will crush you [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=203). As I said, please add a hop via `on_registry_ready()`.
bool IwaPermissionsPolicyCache::ParseManifestAndSetPolicy(Cross-referencing it here. Can't we just have a `ObtainManifestAndCache(origin, callback)` method and hide the URL request impl here?
permissions_policy.push_back({key, std::move(allowed_origins)});Please fix this WARNING reported by ClangTidy: check: modernize-use-emplace
use emplace_back instead of push_back (https://cla...
check: modernize-use-emplace
use emplace_back instead of push_back (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-emplace` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
bool IwaPermissionsPolicyCache::ParseManifestAndSetPolicy(Ideally I'd prefer a `base::expected<>` here. But that's more of a nit
Related details
- Simon Hangl
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 4 comments![]( "Open in Gerrit")
// WebAppInstallManager is initialized withinZgroza (Luke) KlimekThis doesn't guarantee anything about the provider initialization (i.e. `on_ready()`). Yes, the `provider_` ptr will be valid, but that's just it.
Andrew RayskiyAs I wrote in the comment, this does not only create the pointer but also calls `Start()` (literally, the next line of code after `make_unique`) which creates the `WebAppInstallManager`. See initialization [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider_factory.cc;drc=e94ce3f01b473c02658753bdd8502ad039ce1748;l=69) -> [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=165) -> [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=437) and creation in the constructor of the provider itself [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=157) -> [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=378).
Unless the provider can be created somewhere outside of `WebAppProviderFactory::BuildServiceInstanceForBrowserContext` this should assure that the install manager is initialized and well. And even if somehow created from somewhere entirely else, install manager will be created in the constructor itself, so the pointer will be valid.
The road roller will crush you [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=203). As I said, please add a hop via `on_registry_ready()`.
No, it will not. I just provided You with the exact call path that also sets `connected_` [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=460). It is impossible to have `connected_` as `false` if both services were created using their factories.
bool IwaPermissionsPolicyCache::ParseManifestAndSetPolicy(Cross-referencing it here. Can't we just have a `ObtainManifestAndCache(origin, callback)` method and hide the URL request impl here?
I wanted to keep it out of here. But maybe that's alright. Though this is bulky, will add this in the next CL, this one is for parsing and is already big enough.
permissions_policy.push_back({key, std::move(allowed_origins)});Please fix this WARNING reported by ClangTidy: check: modernize-use-emplace
use emplace_back instead of push_back (https://cla...
check: modernize-use-emplace
use emplace_back instead of push_back (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-emplace` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Already fixed as of the time when You wrote this comment.
bool IwaPermissionsPolicyCache::ParseManifestAndSetPolicy(Ideally I'd prefer a `base::expected<>` here. But that's more of a nit
I though of this too. But:
- There's no actual information that this returns, so nothing to put in `<>`.
- There's nothing really that could be easily done with more details on the error code here.
- This being `false` is a corner case that should absolutely never happen, the only scenarios that I can come up with is IWA being corrupted on disk or dev proxy case. I even wondered whether not to make this a `CHECK`, but thought that killing the browser might be too much in case there is some corner case unknown to me.
- So, in short, I'd rather just keep this as `bool`.
Related details
- Andrew Rayskiy
- Simon Hangl
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 1 comment![]( "Open in Gerrit")
base::flat_map<url::Origin, CacheEntry> cache_;I'd rather have a map from a `SignedWebBundleId` for type guarantees 😊
This would add some irritating conversions. But it's already guaranteed in the next CL when I left only one public method for setting this, and this method does everything E2E, so also downloads the manifest. Thus, it's impossible to set anything other than an IWA there. Would that be acceptable?
Related details
- Andrew Rayskiy
- Simon Hangl
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. |
Robbie McElrath (Gerrit)
Robbie McElrath added 2 comments![]( "Open in Gerrit")
void SetPolicy(const url::Origin& iwa_origin, CacheEntry policy);Does this need to be public?
bool include_cross_origin_isolated_permissions_policy = true);Is this used anywhere?
Related details
- Andrew Rayskiy
- Simon Hangl
- Zgroza (Luke) Klimek
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted and added 5 comments![]( "Open in Gerrit")
Votes added by Andrew Rayskiy
| Code-Review | +1 |
| Owners-Override | +1 |
5 comments
base::flat_map<url::Origin, CacheEntry> cache_;Zgroza (Luke) KlimekI'd rather have a map from a `SignedWebBundleId` for type guarantees 😊
This would add some irritating conversions. But it's already guaranteed in the next CL when I left only one public method for setting this, and this method does everything E2E, so also downloads the manifest. Thus, it's impossible to set anything other than an IWA there. Would that be acceptable?
You might also use a map of `<IwaOrigin, CacheEntry` if you feel strongly. [this](https://source.chromium.org/chromium/chromium/src/+/main:components/webapps/isolated_web_apps/types/iwa_origin.h;l=21?q=-f:%5Eout%2F%20-f:%5Egen%2F%20-f:%5Esrc%2F%20case:auto%20IwaOrigin)
std::string feature;I'm 95% sure that a struct that simple doesn't need any constructors/operators.
Alas, `error: [chromium-style] Complex class/struct needs an explicit out-of-line constructor.`. I also do not like it, would not do this unless necessary :(
Yeah, but I'm 99.9% sure that it doesn't force you to provide copy and the defaulted ==.
// WebAppInstallManager is initialized withinZgroza (Luke) KlimekThis doesn't guarantee anything about the provider initialization (i.e. `on_ready()`). Yes, the `provider_` ptr will be valid, but that's just it.
Andrew RayskiyAs I wrote in the comment, this does not only create the pointer but also calls `Start()` (literally, the next line of code after `make_unique`) which creates the `WebAppInstallManager`. See initialization [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider_factory.cc;drc=e94ce3f01b473c02658753bdd8502ad039ce1748;l=69) -> [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=165) -> [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=437) and creation in the constructor of the provider itself [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=157) -> [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=378).
Unless the provider can be created somewhere outside of `WebAppProviderFactory::BuildServiceInstanceForBrowserContext` this should assure that the install manager is initialized and well. And even if somehow created from somewhere entirely else, install manager will be created in the constructor itself, so the pointer will be valid.
Zgroza (Luke) KlimekThe road roller will crush you [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=203). As I said, please add a hop via `on_registry_ready()`.
No, it will not. I just provided You with the exact call path that also sets `connected_` [here](https://crsrc.org/c/chrome/browser/web_applications/web_app_provider.cc;drc=f0821f47fc30afab8eeae78ea1d689480e0d5f6a;l=460). It is impossible to have `connected_` as `false` if both services were created using their factories.
Sheesh. You really got me there...
permissions_policy.push_back({key, std::move(allowed_origins)});Zgroza (Luke) KlimekPlease fix this WARNING reported by ClangTidy: check: modernize-use-emplace
use emplace_back instead of push_back (https://cla...
check: modernize-use-emplace
use emplace_back instead of push_back (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-emplace.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-emplace` footer to the CL description to skip the check)
(Lint observed on `linux-clang-tidy-rel`, but not on `android-clang-tidy-rel`)
Already fixed as of the time when You wrote this comment.
No? Still `push_back`
void IwaPermissionsPolicyCache::OnWebAppManifestUpdated(nit: space
Related details
- Simon Hangl
- Zgroza (Luke) Klimek
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. |