[<install> Element] Build a prototype. [chromium/src : main]
Mike West (Gerrit)
Mike West voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike West
| Commit-Queue | +1 |
1 comment
Hey Thomas, I have a ~working prototype of `<install>` that I'd appreciate some feedback on. I still want to clean a few things up that I might split out into distinct CLs (the shared test infrastructure first and foremost), but I'll ask for your guidance now just to make sure that I'm going in even vaguely the right direction.
Thanks!
Related details
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
HeapMojoRemote<mojom::blink::WebInstallService>& GetService();nit: Per the Blink Style Guide: Naming - "use bare words for getters". Please consider renaming `GetService()` to `Service()` as it is a getter for the `service_` member and does not conflict with a type name.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Mike West
- Thomas Nguyen
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. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 9 comments![]( "Open in Gerrit")
I've left some comments regarding the permission codes in general.
interface HTMLPermissionElement : HTMLElementI might be missing something, do we need to expose it to WebView, as we are not currently supporting Android Webview?
If we are going to enable the Webview support, this should be addressed in a separate ticket, and all Android-related items should be reviewed.
base::BindRepeating(&PermissionElementTestPermissionService::BindHandle,nits: use blink instead of base::Bind*
base::BindRepeating(&PermissionElementTestPermissionService::BindHandle,Ditto
permission_text_span()->setInnerText(inner_text);FYI: We had a problem with `setInnerText`, causing some layout DCHECKs. It might be not a problem here, but worth checking manually, playing around with the page contains the element.
The DHECK is similar to https://issues.chromium.org/issues/40871466, currently we put `permission_text_span()->setInnerText(inner_text)` under `Posttask` to fix it.
// TODO: HTMLPermissionElement should shift to `InPagePermissionMixin`.nit: I've created a task for that, please add this bug to the comment, bug num: 455783637
// `UpdateText` is called via `PostTask`, so `innerText` is checked within a
// `PostTask` to ensure it's updated.We only need posttask if the permission_text_span()->setInnerText(inner_text) is run in Posttask.
So, in the previous note, if we have no problem with the SetInnerText, we can remove the posttask and runloop here.
// - Single permission: geolocation, camera, microphone.nit: update this
PermissionElementTestPermissionService::That makes a lot of senses, thanks for the refactoring the tests into *helper.
Related details
- Mike West
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. |
Mike West (Gerrit)
Mike West added 10 comments![]( "Open in Gerrit")
Thanks, Thomas! I've addressed your feedback inline below. Would you mind taking another look?
Assuming that there are no showstoppers, I'll add some additional reviewers for relevant files you don't own:
tkent@: Would you mind evaluating the VirtualTestSuite change (which neither adds nor removes tests, but simply adds a feature flag).
arthursonzogni@: Could you give `RenderFrameHostImpl::GetCachedPermissionStatuses()` your review from an owners' perspective?
peter@: Could you review the addition of this flag to the collection of similar things that are currently unsupported on WebView?
Thanks, all!
I might be missing something, do we need to expose it to WebView, as we are not currently supporting Android Webview?
If we are going to enable the Webview support, this should be addressed in a separate ticket, and all Android-related items should be reviewed.
Done
base::BindRepeating(&PermissionElementTestPermissionService::BindHandle,nits: use blink instead of base::Bind*
Done
base::BindRepeating(&PermissionElementTestPermissionService::BindHandle,Mike WestDitto
Done
HeapMojoRemote<mojom::blink::WebInstallService>& GetService();nit: Per the Blink Style Guide: Naming - "use bare words for getters". Please consider renaming `GetService()` to `Service()` as it is a getter for the `service_` member and does not conflict with a type name.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
FYI: We had a problem with `setInnerText`, causing some layout DCHECKs. It might be not a problem here, but worth checking manually, playing around with the page contains the element.
The DHECK is similar to https://issues.chromium.org/issues/40871466, currently we put `permission_text_span()->setInnerText(inner_text)` under `Posttask` to fix it.
I'll follow along with this implementation, then. I can certainly imagine rendering/layout making some assumptions about the tree that might not be true in the middle of this processing step.
// TODO: HTMLPermissionElement should shift to `InPagePermissionMixin`.nit: I've created a task for that, please add this bug to the comment, bug num: 455783637
Done
// `UpdateText` is called via `PostTask`, so `innerText` is checked within a
// `PostTask` to ensure it's updated.We only need posttask if the permission_text_span()->setInnerText(inner_text) is run in Posttask.
So, in the previous note, if we have no problem with the SetInnerText, we can remove the posttask and runloop here.
Erring on the side of caution, I'll follow the example HTMLGeolocationElement set. It feels like we could extract this checking code into something shared between the two test files, which I might try to do as a followup.
// - Single permission: geolocation, camera, microphone.Mike Westnit: update this
Done
client_->OnEmbeddedPermissionControlRegistered(/*allowed=*/true,Please fix this WARNING reported by ClangTidy: check: bugprone-argument-comment
argument name 'allowed' in comment does not ma...
check: bugprone-argument-comment
argument name 'allowed' in comment does not match parameter name 'allow' (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html)
(Note: You can add `Skip-Clang-Tidy-Checks: bugprone-argument-comment` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
Related details
- Arthur Sonzogni
- Kent Tamura
- Peter Beverloo
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kent Tamura
| Code-Review | +1 |
1 comment
Thanks, Thomas! I've addressed your feedback inline below. Would you mind taking another look?
Assuming that there are no showstoppers, I'll add some additional reviewers for relevant files you don't own:
tkent@: Would you mind evaluating the VirtualTestSuite change (which neither adds nor removes tests, but simply adds a feature flag).
arthursonzogni@: Could you give `RenderFrameHostImpl::GetCachedPermissionStatuses()` your review from an owners' perspective?
peter@: Could you review the addition of this flag to the collection of similar things that are currently unsupported on WebView?
Thanks, all!
- Arthur Sonzogni
- Mike West
- Peter Beverloo
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Arthur Sonzogni
- Mike West
- Peter Beverloo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
1 comment
RenderFrameHostImpl::GetCachedPermissionStatuses() {arthursonzogni@: Could you give RenderFrameHostImpl::GetCachedPermissionStatuses() your review from an owners' perspective?
I am not deeply familiar with this specific mechanism, I've spent some time reading the [Design doc](https://docs.google.com/document/d/1pfuiJ7BIdX0dxMzboYIOvPV1mFKQ3HE_11VQNeBJSCc/edit?resourcekey=0-9el74tCe5opJXh3CIPyPGA&tab=t.0#heading=h.8zmwced28dbp) and the previous [patch](https://chromium-review.googlesource.com/c/chromium/src/+/5905369) to get up to speed.
---
My understanding is that Blink needs this to style the new CSS selector:
```
permission[type="install"]:granted {...}
```
Because this information lives in the browser process, we must send an initial state with the CommitNavigation IPC to avoid flicker on the initial render. I don't like cache, but this all a lot of sense to me.
I still have a couple of questions about the overall design, which I'm hoping you can clarify:
* The state appears to be snapshotted from the *current* document and used for the *next*. Could you explain the rationale for this? I'm missing why this is the correct state to snapshot and why it isn't considered a potential information leak.
* What's the motivation for sending only a *subset* of permissions? Is there a reason not to send them all?
The change itself (adding `WEB_APP_INSTALLATION`) seems non-controversial, so **LGTM on that**.
I'd appreciate the clarification on the design questions when you have a moment. It would help improve my own understanding of this.
Related details
- Mike West
- Peter Beverloo
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. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
RenderFrameHostImpl::GetCachedPermissionStatuses() {arthursonzogni@: Could you give RenderFrameHostImpl::GetCachedPermissionStatuses() your review from an owners' perspective?
I am not deeply familiar with this specific mechanism, I've spent some time reading the [Design doc](https://docs.google.com/document/d/1pfuiJ7BIdX0dxMzboYIOvPV1mFKQ3HE_11VQNeBJSCc/edit?resourcekey=0-9el74tCe5opJXh3CIPyPGA&tab=t.0#heading=h.8zmwced28dbp) and the previous [patch](https://chromium-review.googlesource.com/c/chromium/src/+/5905369) to get up to speed.
---
My understanding is that Blink needs this to style the new CSS selector:
```
permission[type="install"]:granted {...}
```Because this information lives in the browser process, we must send an initial state with the CommitNavigation IPC to avoid flicker on the initial render. I don't like cache, but this all a lot of sense to me.
I still have a couple of questions about the overall design, which I'm hoping you can clarify:
* The state appears to be snapshotted from the *current* document and used for the *next*. Could you explain the rationale for this? I'm missing why this is the correct state to snapshot and why it isn't considered a potential information leak.
* What's the motivation for sending only a *subset* of permissions? Is there a reason not to send them all?The change itself (adding `WEB_APP_INSTALLATION`) seems non-controversial, so **LGTM on that**.
I'd appreciate the clarification on the design questions when you have a moment. It would help improve my own understanding of this.
Thanks, Arthur!
I'll defer to Thomas on the broader architecture questions, though I believe that a) permissions are origin-scoped, and this should only be read from cache for same-origin navigation, b) the answer to the subset question is that there's cost to monitoring these permissions, and they've decided to limit the checks to those which are supported in the current implementation of permission elements.
Related details
- Arthur Sonzogni
- Peter Beverloo
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. |
Thomas Nguyen (Gerrit)
Thomas Nguyen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Nguyen
| Code-Review | +1 |
1 comment
RenderFrameHostImpl::GetCachedPermissionStatuses() {Mike Westarthursonzogni@: Could you give RenderFrameHostImpl::GetCachedPermissionStatuses() your review from an owners' perspective?
I am not deeply familiar with this specific mechanism, I've spent some time reading the [Design doc](https://docs.google.com/document/d/1pfuiJ7BIdX0dxMzboYIOvPV1mFKQ3HE_11VQNeBJSCc/edit?resourcekey=0-9el74tCe5opJXh3CIPyPGA&tab=t.0#heading=h.8zmwced28dbp) and the previous [patch](https://chromium-review.googlesource.com/c/chromium/src/+/5905369) to get up to speed.
---
My understanding is that Blink needs this to style the new CSS selector:
```
permission[type="install"]:granted {...}
```Because this information lives in the browser process, we must send an initial state with the CommitNavigation IPC to avoid flicker on the initial render. I don't like cache, but this all a lot of sense to me.
I still have a couple of questions about the overall design, which I'm hoping you can clarify:
* The state appears to be snapshotted from the *current* document and used for the *next*. Could you explain the rationale for this? I'm missing why this is the correct state to snapshot and why it isn't considered a potential information leak.
* What's the motivation for sending only a *subset* of permissions? Is there a reason not to send them all?The change itself (adding `WEB_APP_INSTALLATION`) seems non-controversial, so **LGTM on that**.
I'd appreciate the clarification on the design questions when you have a moment. It would help improve my own understanding of this.
Thanks, Arthur!
I'll defer to Thomas on the broader architecture questions, though I believe that a) permissions are origin-scoped, and this should only be read from cache for same-origin navigation, b) the answer to the subset question is that there's cost to monitoring these permissions, and they've decided to limit the checks to those which are supported in the current implementation of permission elements.
>>> The state appears to be snapshotted from the current document and used for the next. Could you explain the rationale for this? I'm missing why this is the correct state to snapshot and why it isn't considered a potential information leak.
My understanding of the question may be incomplete, so correct me if I might be missing. Basically the cache are initialized by an initial set of permission statuses passed down in CommitNavigationParams and it should happen at commit time, not really before.
The existing logic for reusing windows still remains, permission statuses will be passed along with other parameters if the window is reused. I believe the reusing case should be very limited (such as same origin) and we have guaranteed it’s not the cross-leak.
>>> What's the motivation for sending only a subset of permissions? Is there a reason not to send them all?
I don't think we have a limit on the set of of permissions. But basically there's no motivation to use permissions from the cache early like our cases. Adding more permissions is acceptable.
Related details
- Arthur Sonzogni
- Mike West
- Peter Beverloo
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
1 comment
See [line](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/navigation_request.cc;l=1452;drc=ee500ed0974f48252c721fa53f41758b71309178;bpv=1;bpt=1) where the `CachedPermissionStatues` of the current document are plumbed into the navigation.
Then the navigation will commit, we will create a **New** RenderFrameHost and call `RenderFrameHostImpl::CommitNavigation()` to commit the navigation in blink.
I would have expected this field to be left empty until:
`RenderFrameHostImpl::SendCommitNavigation(..)`, where I would have called [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_frame_host_impl.cc;l=16627;drc=ee500ed0974f48252c721fa53f41758b71309178;bpv=0;bpt=1)
```
commit_params->initial_permission_statuses = GetCachedPermissionStatuses()
```
WDYT?
Related details
- Mike West
- Peter Beverloo
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
Friendly ping, peter@. :)
Thomas: Perhaps it would be worth filing a separate bug for Arthur's comment? It does look strange to me as well, but I don't know the history of it's current implementation.
Related details
- Peter Beverloo
- Thomas Nguyen
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
I apologize, I got sick and might have a slight delay in checking this. We can file a separate bug to revisit the old implementation, rather than blocking the progress here.
Related details
- Mike West
- Peter Beverloo
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
RenderFrameHostImpl::GetCachedPermissionStatuses() {Related details
- Mike West
- Peter Beverloo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
boliu@: Would you mind stamping the change to `//android_webview` to disable the flag added in this CL, consistent with the other flags for similar elements that are equally unsupported in that environment?
Related details
- Bo Liu
- Peter Beverloo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu voted Code-Review+1![]( "Open in Gerrit")
- Mike West
- Peter Beverloo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West voted Commit-Queue+2![]( "Open in Gerrit")
- Peter Beverloo
Robert Paveza (Gerrit)
Robert Paveza added 1 comment![]( "Open in Gerrit")
GetLocale().QueryString(message_id, String(url.Host().ToString()));I think this is a fundamental shortcoming with the approach that you had prototyped in the explainer, in that this sticks specifically to the hostname. While that might feasible for some applications, one origin or host may serve many applications.
From a practical standpoint, I think that rendering a button will necessarily require that we fetch the manifest as part of presenting the button so that we can obtain the application's title, and we should also fetch the localized title conditioned on the browser's configured version (something that @akyer...@microsoft.com is currently working on).
Related details
- Alexander Kyereboah
- Mike West
- Peter Beverloo
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. |
Mike West (Gerrit)
Mike West voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike West
| Commit-Queue | +2 |
1 comment
GetLocale().QueryString(message_id, String(url.Host().ToString()));I think this is a fundamental shortcoming with the approach that you had prototyped in the explainer, in that this sticks specifically to the hostname. While that might feasible for some applications, one origin or host may serve many applications.
From a practical standpoint, I think that rendering a button will necessarily require that we fetch the manifest as part of presenting the button so that we can obtain the application's title, and we should also fetch the localized title conditioned on the browser's configured version (something that @akyer...@microsoft.com is currently working on).
Thanks! As I mentioned elsewhere, I think there are downsides to rendering the title, but I agree with you that rendering the hostname might not allow enough differentiation for origins with many apps. That's something we'll certainly need to discuss a bit more to find a reasonable approach!
Related details
- Alexander Kyereboah
- Peter Beverloo
- Robert Paveza
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[<install> Element] Build a prototype.
This CL sketches out a prototype of the `<install>` element proposal at
https://github.com/mikewest/pepc-install. To do so, it:
1. Introduces an `HTMLInstallElement` inheriting from
`HTMLPermissionElement` that relies on the `WEB_APP_INSTALLATION`
permission, and hooks into the `WebInstallService` to trigger the
same behavior as the Web Install API on user interaction. This isn't
quite the same behavior as described in the explainer linked above,
but it sets up scaffolding that we can use later if we decide to go
this route. In particular, the explainer proposes specifying the
manifest URL directly, while the API allows any url in scope of the
app. We'll want to reconcile that in the future.
2. It pulls some testing code out of `HTMLGeolocationElementTest`, into
a new helper file shared between `<geolocation>` and `<install>`,
and adds some light tests to verify the simplest things possible.
We will certainly want more, but locking in too much behavior right
now probably doesn't make sense, as I expect a few things about the
design to change.
3. It adjusts `PermissionControllerImpl` and `PermissionServiceImpl` to
support non-device permissions.
- M android_webview/browser/aw_field_trials.cc
- M chrome/browser/chrome_browser_interface_binders.cc
- M content/browser/permissions/permission_controller_impl.cc
- M content/browser/permissions/permission_service_impl.cc
- M content/browser/renderer_host/render_frame_host_impl.cc
- M third_party/blink/DEPS
- M third_party/blink/public/blink_resources.grd
- M third_party/blink/public/strings/permission_element_strings.grd
- A third_party/blink/public/strings/permission_element_strings_grd/IDS_PERMISSION_REQUEST_INSTALL.png.sha1
- A third_party/blink/public/strings/permission_element_strings_grd/IDS_PERMISSION_REQUEST_INSTALL_WITH_HOST.png.sha1
- M third_party/blink/renderer/bindings/generated_in_core.gni
- M third_party/blink/renderer/bindings/idl_in_core.gni
- M third_party/blink/renderer/core/html/build.gni
- M third_party/blink/renderer/core/html/html_geolocation_element_test.cc
- A third_party/blink/renderer/core/html/html_install_element.cc
- A third_party/blink/renderer/core/html/html_install_element.h
- A third_party/blink/renderer/core/html/html_install_element.idl
- A third_party/blink/renderer/core/html/html_install_element_test.cc
- M third_party/blink/renderer/core/html/html_permission_element.cc
- M third_party/blink/renderer/core/html/html_permission_element.h
- M third_party/blink/renderer/core/html/html_permission_element_test.cc
- A third_party/blink/renderer/core/html/html_permission_element_test_helper.cc
- A third_party/blink/renderer/core/html/html_permission_element_test_helper.h
- M third_party/blink/renderer/core/html/html_permission_icon_element.cc
- M third_party/blink/renderer/core/html/html_tag_names.json5
- A third_party/blink/renderer/core/html/resources/images/permission_icon_install.svg
- M third_party/blink/renderer/core/html/resources/permission.css
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/VirtualTestSuites
- M third_party/blink/web_tests/webexposed/element-instance-property-listing-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
Code-Review: +1 by Kent Tamura, +1 by Thomas Nguyen, +1 by Bo Liu, +1 by Arthur Sonzogni
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |