[Web Install] Define the web-app-installation PermissionPolicy [chromium/src : main]
Kristin Lee (Gerrit)
Message from Kristin Lee![]( "Open in Gerrit")
Set Ready For Review
Related details
- Kristin Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
Hello! Please take a look when you can. Some advice would be much appreciated.
Please note that this is not exposed end to end yet because this is the first CL of many CLs to come for the Web Install project. Because this is not exposed end to end, I'm not sure of a easy way to test nor take a screenshot of the new string that's being added for the Web Install permission prompt. Some advice on how to best approach this would be very helpful. Here are some ideas I had, but of course let me know if there is a better way:
(A) Since screenshots are required, I'm wondering if the new string could be added in a follow-up CL when we are able to test end to end?
(B) If the new string is best included in this CL, is it appropriate to take a screenshot by creating an API that would not be added to the codebase and is purely for invoking the permission prompt?
Related details
- Chromium Permissions Reviews
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
hka...@chromium.org is from context(googleclient/chrome/chromium_gwsq/components/permissions/config.gwsq)
Related details
- Kamila Hasanbega
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
Hi Permission Reviewers,
I see that Kamila Hasanbega was auto added as a reviewer, but I noticed that their status in Gerrit shows that they are OOO until February 13. I'm not sure if this is true and was wondering if someone can help confirm? If Kamila is OOO until next year, can someone help share how to have a different reviewer assigned to this CL? Thanks!
Related details
- Chromium Permissions Reviews
- Kamila Hasanbega
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
el...@chromium.org is from context(googleclient/chrome/chromium_gwsq/components/permissions/config.gwsq)
Related details
- Elias Klim
- Kamila Hasanbega
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Elias Klim (Gerrit)
Elias Klim added 1 comment![]( "Open in Gerrit")
Hi Kristin! I've noticed there was a bit of a delay with the previous reviewer, I'm sorry about that. This message is just a notification that I've seen the CL and working on it. I'm planning to review it EOW.
Related details
- Elias Klim
- Kristin Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
Hi Kristin! I've noticed there was a bit of a delay with the previous reviewer, I'm sorry about that. This message is just a notification that I've seen the CL and working on it. I'm planning to review it EOW.
Hi Elias! Thanks for sending a message. No need to apologize. Based on the previous reviewer's status, I'm not sure if they're OOO until February or not, but if you're available to help review instead, that would be wonderful and I can maybe move them to CC. EoW is perfect. I am actually OOO 8/15-8/19 but will respond to your feedback as soon as I'm back.
I see there's now a merge conflict so I rebased and am currently building locally to be sure it still builds fine.
When you're ready to review, please also take a look at my initial question I had (I think it's rather buried at this point so I'll re-paste here):
"Please note that this is not exposed end to end yet because this is the first CL of many CLs to come for the Web Install project. Because this is not exposed end to end, I'm not sure of an easy way to test nor take a screenshot of the new string that's being added for the Web Install permission prompt. Some advice on how to best approach this would be very helpful. Here are some ideas I had, but of course let me know if there is a better way:
(A) Since screenshots are required, I'm wondering if the new string could be added in a follow-up CL when we are able to test end to end?
(B) If the new string is best included in this CL, is it appropriate to take a screenshot by creating an API that would not be added to the codebase and is purely for invoking the permission prompt?"
Related details
- Elias Klim
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Elias Klim (Gerrit)
Elias Klim added 1 comment![]( "Open in Gerrit")
Kristin LeeHi Kristin! I've noticed there was a bit of a delay with the previous reviewer, I'm sorry about that. This message is just a notification that I've seen the CL and working on it. I'm planning to review it EOW.
Hi Elias! Thanks for sending a message. No need to apologize. Based on the previous reviewer's status, I'm not sure if they're OOO until February or not, but if you're available to help review instead, that would be wonderful and I can maybe move them to CC. EoW is perfect. I am actually OOO 8/15-8/19 but will respond to your feedback as soon as I'm back.
I see there's now a merge conflict so I rebased and am currently building locally to be sure it still builds fine.
When you're ready to review, please also take a look at my initial question I had (I think it's rather buried at this point so I'll re-paste here):
"Please note that this is not exposed end to end yet because this is the first CL of many CLs to come for the Web Install project. Because this is not exposed end to end, I'm not sure of an easy way to test nor take a screenshot of the new string that's being added for the Web Install permission prompt. Some advice on how to best approach this would be very helpful. Here are some ideas I had, but of course let me know if there is a better way:
(A) Since screenshots are required, I'm wondering if the new string could be added in a follow-up CL when we are able to test end to end?
(B) If the new string is best included in this CL, is it appropriate to take a screenshot by creating an API that would not be added to the codebase and is purely for invoking the permission prompt?"
The status is obsolete but Kamila is OOO anyway, so moving to CC is OK.
Regarding strings: 1. If you add new strings, you should add screenshots. The screenshots are used by translators and they don't verify a feature. In other words feel free to simulate a permission prompt and/or any other UI elements and take screenshots. In such situations, I usually
- use another permissions, e.g. Geolocation
- change strings to new one
- make screenshots
- undo/discard changes
- 2. I didn't read the explainer and/or other docs but if strings are not final, as a short-term solution you can hardcode temporary English strings. The whole logic should be behind a disabled by default feature flag. Each such string should be marked with `//TODO(bug)` comment and should be fixed as soon as possible.
Related details
- Kristin Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kristin Lee
| Commit-Queue | +1 |
Kristin LeeHi Kristin! I've noticed there was a bit of a delay with the previous reviewer, I'm sorry about that. This message is just a notification that I've seen the CL and working on it. I'm planning to review it EOW.
Elias KlimHi Elias! Thanks for sending a message. No need to apologize. Based on the previous reviewer's status, I'm not sure if they're OOO until February or not, but if you're available to help review instead, that would be wonderful and I can maybe move them to CC. EoW is perfect. I am actually OOO 8/15-8/19 but will respond to your feedback as soon as I'm back.
I see there's now a merge conflict so I rebased and am currently building locally to be sure it still builds fine.
When you're ready to review, please also take a look at my initial question I had (I think it's rather buried at this point so I'll re-paste here):
"Please note that this is not exposed end to end yet because this is the first CL of many CLs to come for the Web Install project. Because this is not exposed end to end, I'm not sure of an easy way to test nor take a screenshot of the new string that's being added for the Web Install permission prompt. Some advice on how to best approach this would be very helpful. Here are some ideas I had, but of course let me know if there is a better way:
(A) Since screenshots are required, I'm wondering if the new string could be added in a follow-up CL when we are able to test end to end?
(B) If the new string is best included in this CL, is it appropriate to take a screenshot by creating an API that would not be added to the codebase and is purely for invoking the permission prompt?"
The status is obsolete but Kamila is OOO anyway, so moving to CC is OK.
Regarding strings: 1. If you add new strings, you should add screenshots. The screenshots are used by translators and they don't verify a feature. In other words feel free to simulate a permission prompt and/or any other UI elements and take screenshots. In such situations, I usually
- use another permissions, e.g. Geolocation
- change strings to new one
- make screenshots
- undo/discard changes
- 2. I didn't read the explainer and/or other docs but if strings are not final, as a short-term solution you can hardcode temporary English strings. The whole logic should be behind a disabled by default feature flag. Each such string should be marked with `//TODO(bug)` comment and should be fixed as soon as possible.
Hi Elias, thank you so much for your guidance! That helps a lot. From my understanding, I believe the string I'm adding in this CL is finalized, but the rest of the strings you'll see in the doc are still in review and will be added in a follow-up CL that is a work in progress (crrev.com/c/5784580).
I followed your suggestion to simulate it using the Geolocation permission prompt. It seems only a Googler is able to upload screenshots. Would you be able to help with this as well?
Screenshot of IDS_WEB_APP_INSTALLATION_PERMISSION_FRAGMENT ("Ask to install web apps": https://drive.google.com/file/d/1rXxdWnFqUSIN6Qv0nOhGjr9BrHFMQm_R/view?usp=sharing
To be sure I understand #2 of your comment, by hardcoding English strings as a short-term solution are you referring to not adding the strings in their appropriate .grdp files, but directly into the code where those strings are being used/added?
Related details
- Kristin Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Elias Klim (Gerrit)
Elias Klim voted and added 2 comments![]( "Open in Gerrit")
Votes added by Elias Klim
| Code-Review | +1 |
Kristin LeeHi Kristin! I've noticed there was a bit of a delay with the previous reviewer, I'm sorry about that. This message is just a notification that I've seen the CL and working on it. I'm planning to review it EOW.
Elias KlimHi Elias! Thanks for sending a message. No need to apologize. Based on the previous reviewer's status, I'm not sure if they're OOO until February or not, but if you're available to help review instead, that would be wonderful and I can maybe move them to CC. EoW is perfect. I am actually OOO 8/15-8/19 but will respond to your feedback as soon as I'm back.
I see there's now a merge conflict so I rebased and am currently building locally to be sure it still builds fine.
When you're ready to review, please also take a look at my initial question I had (I think it's rather buried at this point so I'll re-paste here):
"Please note that this is not exposed end to end yet because this is the first CL of many CLs to come for the Web Install project. Because this is not exposed end to end, I'm not sure of an easy way to test nor take a screenshot of the new string that's being added for the Web Install permission prompt. Some advice on how to best approach this would be very helpful. Here are some ideas I had, but of course let me know if there is a better way:
(A) Since screenshots are required, I'm wondering if the new string could be added in a follow-up CL when we are able to test end to end?
(B) If the new string is best included in this CL, is it appropriate to take a screenshot by creating an API that would not be added to the codebase and is purely for invoking the permission prompt?"
Kristin LeeThe status is obsolete but Kamila is OOO anyway, so moving to CC is OK.
Regarding strings: 1. If you add new strings, you should add screenshots. The screenshots are used by translators and they don't verify a feature. In other words feel free to simulate a permission prompt and/or any other UI elements and take screenshots. In such situations, I usually
- use another permissions, e.g. Geolocation
- change strings to new one
- make screenshots
- undo/discard changes
- 2. I didn't read the explainer and/or other docs but if strings are not final, as a short-term solution you can hardcode temporary English strings. The whole logic should be behind a disabled by default feature flag. Each such string should be marked with `//TODO(bug)` comment and should be fixed as soon as possible.
Hi Elias, thank you so much for your guidance! That helps a lot. From my understanding, I believe the string I'm adding in this CL is finalized, but the rest of the strings you'll see in the doc are still in review and will be added in a follow-up CL that is a work in progress (crrev.com/c/5784580).
I followed your suggestion to simulate it using the Geolocation permission prompt. It seems only a Googler is able to upload screenshots. Would you be able to help with this as well?
Screenshot of IDS_WEB_APP_INSTALLATION_PERMISSION_FRAGMENT ("Ask to install web apps": https://drive.google.com/file/d/1rXxdWnFqUSIN6Qv0nOhGjr9BrHFMQm_R/view?usp=sharing
To be sure I understand #2 of your comment, by hardcoding English strings as a short-term solution are you referring to not adding the strings in their appropriate .grdp files, but directly into the code where those strings are being used/added?
I've uploaded the screenshot. I didn't want to update the CL myself as then I'll be marked as "uploader" and will not be able to review. Please add
1. In a folder components/permissions_strings_grdp/
2. New file IDS_WEB_APP_INSTALLATION_PERMISSION_FRAGMENT.png.sha1
3. With content 7a1093d83aa002f7891ec64d7ed944f28751d1a5
Regarding the comment #2, yes. This is pretty much an edge case that should be avoided but sometimes, especially if you have a chain of CLs, it is OK to put temp strings in one and then put proper values in another.
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
Sounds good. I've uploaded the new file for the screenshot. Thank you so much, Elias!
Related details
- Kristin Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
Please take a look when you can or please point me in the right direction if you're not the right person. Thanks!
Related details
- Christian Dullweber
- Chromium IPC Reviews
- Peter Beverloo
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: toyo...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): toyo...@chromium.org
Reviewer source(s):
toyo...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Christian Dullweber
- Peter Beverloo
- Takashi Toyoshima
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Christian Dullweber (Gerrit)
Christian Dullweber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Christian Dullweber
| Code-Review | +1 |
1 comment
- Kristin Lee
- Peter Beverloo
- Takashi Toyoshima
Elias Klim (Gerrit)
Elias Klim voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Kristin Lee
- Peter Beverloo
- Takashi Toyoshima
Code-Coverage
Code-Review
Peter Beverloo (Gerrit)
Peter Beverloo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Peter Beverloo
| Code-Review | +1 |
1 comment
return {name: "web-app-installation"};maintainability nit: it'd be ace if we could reduce duplication here by doing something akin to the following:
```
function nameToObject(permissionName) {
switch (permissionName) {
case "clipboard-read-write":
return {name: "clipboard-write", allowWithoutSanitization: true};
case "clipboard-sanitized-write":
return {name: "clipboard-write", allowWithoutSanitization: false};
case "midi-sysex":
return {name: "midi", sysex: true};
case "push-messaging":
return {name: "push", userVisibleOnly: true};
case "accessibility-events":
case "background-fetch":
case "background-sync":
case "captured-surface-control":
case "display-capture":
case "geolocation":
case "midi":
case "nfc":
case "notifications":
case "payment-handler":
case "periodic-background-sync":
case "speaker-selection":
case "web-app-installation":
return {name: permissionName};
default:
throw "Invalid permission name provided";
}
}
```
Related details
- Kristin Lee
- Takashi Toyoshima
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Kristin Lee
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
return {name: "web-app-installation"};Hi Peter, thanks for the suggestion! That would absolutely help reduce duplication and maintainability. It would also be easier to read, but I am inclined to refrain from restructuring any features that involve other permissions policies and would like to just add the necessary code changes for web-app-installation PermissionPolicy in this CL. Perhaps restructuring this function to your example could be done in a follow-up CL by someone? In case the web-app-installation Permission code needs to be reverted or if the restructuring of this function needs to be reverted, only 1 CL would need to be reverted and not affect the other code changes which would be ideal.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kristin Lee
| Commit-Queue | +1 |
1 comment
Hi Alex and Martin, please take a look when you can or please point me in the right direction if you are not the right person. Thanks!
Related details
- Alex Rudenko
- Martin Šrámek
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Rudenko (Gerrit)
Alex Rudenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Rudenko
| Code-Review | +1 |
1 comment
- Kristin Lee
- Martin Šrámek
Martin Šrámek (Gerrit)
Martin Šrámek voted and added 1 comment![]( "Open in Gerrit")
Related details
- Kristin Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
Thank you so much for the review and approval! I had to rebase to resolve merge conflicts and it seems I need 1 person to help re-submit their vote to have a total of 2 votes as I don't have a Chromium committer status according to the code-review requirements:
"If the author is a Chromium committer, code review is required from one other committer (see https://www.chromium.org/getting-involved/become-a-committer/). Otherwise, code review is required from two committers."
Could 1 person here help take another look and re-submit their vote? Thanks!
Related details
- Alex Rudenko
- Christian Dullweber
- Takashi Toyoshima
Elias Klim (Gerrit)
Elias Klim voted Code-Review+1![]( "Open in Gerrit")
- Alex Rudenko
- Christian Dullweber
- Kristin Lee
- Takashi Toyoshima
Code-Coverage
Code-Review
Christian Dullweber (Gerrit)
Christian Dullweber voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Alex Rudenko
- Kristin Lee
- Takashi Toyoshima
Alex Rudenko (Gerrit)
Alex Rudenko voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Kristin Lee
- Takashi Toyoshima
Kristin Lee (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Web Install] Define the web-app-installation PermissionPolicy
The API for Web Install is to be gated behind a permission prompt, associated with the web-app-installation PermissionPolicy. This CL defines this PermissionPolicy and the PermissionContext with which it is associated.
Note: This is not exposed end to end yet. This is the first of many CLs for Web Install.
CL for adding permission UI to chrome://settings/content: 5549155
Explainer: https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/WebInstall/explainer.md
Dev Spec: https://docs.google.com/document/d/12nSXJLm8mW0gWZ_yjlXfrV8r9gwJliVt4WVa-209-KA/edit?usp=sharing
Chrome Status: https://chromestatus.com/feature/5183481574850560
- M android_webview/browser/aw_permission_manager.cc
- M chrome/android/javatests/src/org/chromium/chrome/browser/site_settings/WebsitePermissionsFetcherTest.java
- M chrome/browser/permissions/permission_manager_factory.cc
- M chrome/browser/ui/webui/settings/site_settings_helper.cc
- M components/content_settings/core/browser/content_settings_registry.cc
- M components/content_settings/core/browser/content_settings_uma_util.cc
- M components/content_settings/core/common/content_settings_types.mojom
- M components/permissions/BUILD.gn
- A components/permissions/contexts/web_app_installation_permission_context.cc
- A components/permissions/contexts/web_app_installation_permission_context.h
- M components/permissions/permission_request.cc
- M components/permissions/permission_uma_util.cc
- M components/permissions/permission_uma_util.h
- M components/permissions/permission_util.cc
- M components/permissions/request_type.cc
- M components/permissions/request_type.h
- M components/permissions/test/permission_test_util.cc
- M components/permissions_strings.grdp
- A components/permissions_strings_grdp/IDS_WEB_APP_INSTALLATION_PERMISSION_FRAGMENT.png.sha1
- M content/browser/devtools/protocol/browser_handler.cc
- M content/browser/permissions/permission_controller_impl.cc
- M content/shell/browser/shell_permission_manager.cc
- M third_party/blink/common/permissions/permission_utils.cc
- M third_party/blink/public/common/permissions/permission_utils.h
- M third_party/blink/public/devtools_protocol/browser_protocol.pdl
- M third_party/blink/public/mojom/permissions/permission.mojom
- M third_party/blink/public/mojom/permissions_policy/permissions_policy_feature.mojom
- M third_party/blink/renderer/core/permissions_policy/feature_policy.dict
- M third_party/blink/renderer/core/permissions_policy/permissions_policy_features.json5
- M third_party/blink/renderer/modules/permissions/permission_descriptor.idl
- M third_party/blink/renderer/modules/permissions/permission_utils.cc
- M third_party/blink/web_tests/http/tests/resources/permissions-helper.js
- M third_party/blink/web_tests/webexposed/feature-policy-features-expected.txt
- M tools/metrics/histograms/enums.xml
- M tools/metrics/histograms/metadata/content/histograms.xml
- M tools/metrics/histograms/metadata/histogram_suffixes_list.xml
Code-Review: +1 by Elias Klim, +1 by Alex Rudenko, +1 by Christian Dullweber, +1 by Martin Šrámek
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |