[IWA] Dynamic feature flags for Isolated Web Apps [chromium/src : main]
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 4 comments![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Commit-Queue | +1 |
4 comments
base::RunLoop run_loop;Zgroza (Luke) Klimek```
base::test::TestFuture<bool> future;
permissions_policy_cache->ObtainManifestAndCache(
iwa_origin, future.GetCallback());
CHECK(future.Take());
```
I do not see a huge difference, but changed.
const auto mapping = permission_policy_to_feature_map.find(entry.feature);Zgroza (Luke) Klimek`base::FindOrNull()`
Done
if (!!isolated_app_policy) {Zgroza (Luke) KlimekThis is not a pointer.
```
if (isolated_app_policy) {
}
```
I know this is not a pointer, I just find this explicit cast to bool more readable. But no strong feelings, changed.
if (isolated_app_policy.empty()) {Zgroza (Luke) KlimekDo we need this early exit? I presume `ParseIsolatedAppPermissionsPolicy()` will do the same thing.
Need? No, we do not. But it's a tiny optimization skipping several unnecessary calls.
But right, a more comfortable place for it is at the beginning of `ParseIsolatedAppPermissionsPolicy()`, moved there.
Related details
- Andrew Rayskiy
- Chris Thompson
- 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 voted Code-Review+1![]( "Open in Gerrit")
- Chris Thompson
- Simon Hangl
- Zgroza (Luke) Klimek
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Chris Thompson (Gerrit)
Chris Thompson voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 1 comment![]( "Open in Gerrit")
@cl...@chromium.org can You please take a look at this, especially on `//third_party/blink/public/mojom/navigation/navigation_params.mojom`?
Related details
- Andrew Rayskiy
- Chris Thompson
- 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 voted Code-Review+1![]( "Open in Gerrit")
- Camille Lamy
- Chris Thompson
- 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. |
Camille Lamy (Gerrit)
Camille Lamy added 8 comments![]( "Open in Gerrit")
Thanks! I haven't looked at the test code yet, as the CL is a bit big. In the meantime, a few questions/comments below.
/*permissions_policy=*/std::nullopt, std::move(policy_container),Is there any call to commit navigation where we need to pass a non-null permission_policy parameter here?
std::optional<std::vector<blink::mojom::IsolatedAppPermissionPolicyEntryPtr>>Considering that this is essentially a wrapper around the ContentBrowserClient call, is there any value to keeping this function around instead of just calling the ContentBrowserClient directly from RenderFrameHost?
struct IsolatedAppPermissionPolicyEntry {I imagine we already have a mojo struct for passing permission policy entries between the browser and the renderer. Could we remove it instead of creating a new struct? Or is is that it is unparsed unlike other permission entries? If so, could you precise this in a comment.
const std::optional<Vector<IsolatedAppPermissionPolicyEntry>>&Nit: It would be better to pass as const Vector<IsolatedAppPermissionPolicyEntry>* or as base::optional_ref<Vector<IsolatedAppPermissionPolicyEntry>>; passing by const std::optional<T>& is prone to "copies by implicit conversions".
permissions_policy_header_ =IIUC, this overwrites any additional PermissionPolicyHeader. Is the header information present in IsolatedAppPolicy?
permissions_policy_header_, /*base_policy=*/std::nullopt, origin);Now that isolated_app_policy is no longer passed to this function, is there still usage of the base_policy argument anywhere else?
Related details
- Chris Thompson
- 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 8 comments![]( "Open in Gerrit")
Thanks! I haven't looked at the test code yet, as the CL is a bit big. In the meantime, a few questions/comments below.
Thanks! Sorry for the size, I have not found any more logical splits that could still be done here.
/*permissions_policy=*/std::nullopt, std::move(policy_container),Is there any call to commit navigation where we need to pass a non-null permission_policy parameter here?
No, there is none. I mentioned in some comment before, this is an unused parameter now and after this CL I plan to create a next one with a cleanup. The only reason why it's not here is because it would add yet another few hundred lines to the delta of this already huge CL.
std::optional<std::vector<blink::mojom::IsolatedAppPermissionPolicyEntryPtr>>Considering that this is essentially a wrapper around the ContentBrowserClient call, is there any value to keeping this function around instead of just calling the ContentBrowserClient directly from RenderFrameHost?
We... in fact may I think! I'm not sure why this was needed even earlier even. Removed.
I imagine we already have a mojo struct for passing permission policy entries between the browser and the renderer. Could we remove it instead of creating a new struct? Or is is that it is unparsed unlike other permission entries? If so, could you precise this in a comment.
Yes, this being unparsed is the main point of this, added a comment above.
const std::optional<Vector<IsolatedAppPermissionPolicyEntry>>&Nit: It would be better to pass as const Vector<IsolatedAppPermissionPolicyEntry>* or as base::optional_ref<Vector<IsolatedAppPermissionPolicyEntry>>; passing by const std::optional<T>& is prone to "copies by implicit conversions".
Done, thanks! I did not know about `base::optional_ref`.
permissions_policy_header_ =IIUC, this overwrites any additional PermissionPolicyHeader. Is the header information present in IsolatedAppPolicy?
Within `isolated_app_policy` not, but `ParseIsolatedAppPermissionsPolicy` function performs parsing and merging the policies with the headers, the output is already the manifest policy constrained by the headers, see chained CL: crrev.com/c/7416778
tl;dr The headers themselves of an Isolated Web App do not take direct effect, the permissions policies that are applied are essentially `intersection(manifest, headers)`.
permissions_policy_header_, /*base_policy=*/std::nullopt, origin);Now that isolated_app_policy is no longer passed to this function, is there still usage of the base_policy argument anywhere else?
No, this is an unused argument now, similarly to [here](https://chromium-review.googlesource.com/c/chromium/src/+/7253350/comment/2a594a6e_9c342603/). This will be cleaned up in a follow-up CL.
Related details
- Camille Lamy
- Chris Thompson
- 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. |
Chris Thompson (Gerrit)
Chris Thompson voted Code-Review+1![]( "Open in Gerrit")
- Camille Lamy
- Simon Hangl
- Zgroza (Luke) Klimek
Simon Hangl (Gerrit)
Simon Hangl voted and added 1 comment![]( "Open in Gerrit")
Votes added by Simon Hangl
| Code-Review | +1 |
Related details
- Camille Lamy
- Zgroza (Luke) Klimek