SAA: Always call RequestPermission [chromium/src : main]
Christian Dullweber (Gerrit)
Christian Dullweber uploaded patch set #3 to this change.
SAA: Always call RequestPermission
Alternative proposal to https://crrev.com/c/4588709
Need to add tests and see if existing tests pass. Might change some metric collections slightly?
Change-Id: I17773a845c4e37a358fb8fc164fd6be1f01a594f
---
M chrome/browser/storage_access_api/storage_access_grant_permission_context.cc
M third_party/blink/renderer/core/dom/document.cc
2 files changed, 4 insertions(+), 9 deletions(-)
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Chris Fredrickson (Gerrit)
Attention is currently required from: Christian Dullweber.
1 comment:
File third_party/blink/renderer/core/dom/document.cc:
Patch Set #3, Line 6494:
GetPermissionService(ExecutionContext::From(resolver->GetScriptState()))I may be missing something here. If the previous status is `DENIED`, can't we skip the RequestPermission call? It seems like an unnecessary round trip, since it will be denied anyway.
And same if the previous status is `GRANTED` - why request permission again?
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Christian Dullweber (Gerrit)
Christian Dullweber uploaded patch set #5 to this change.
SAA: Always call RequestPermission
In order to always let the browser process know that StorageAccess was
granted to a frame, we will remove the shortcut to resolve the
StorageAccess request immediately when a permission was previously
granted.
Change-Id: I17773a845c4e37a358fb8fc164fd6be1f01a594f
---
M chrome/browser/storage_access_api/api_browsertest.cc
M chrome/browser/storage_access_api/storage_access_grant_permission_context.cc
M third_party/blink/renderer/core/dom/document.cc
3 files changed, 94 insertions(+), 13 deletions(-)
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Christian Dullweber (Gerrit)
Attention is currently required from: Chris Fredrickson.
Patch set 5:Commit-Queue +1
1 comment:
File third_party/blink/renderer/core/dom/document.cc:
Patch Set #3, Line 6494:
GetPermissionService(ExecutionContext::From(resolver->GetScriptState()))
I may be missing something here. […]
As discussed, I need to know in the browser process that a StorageAccess permission was granted to a frame. I added extensive tests. As far as I can tell based on tests, the only histogram that was changed is that we will additionally record a ReusedPreviousDecision entry
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Chris Fredrickson (Gerrit)
Attention is currently required from: Christian Dullweber.
2 comments:
File chrome/browser/storage_access_api/api_browsertest.cc:
Patch Set #5, Line 538:
EXPECT_FALSE(storage::test::RequestAndCheckStorageAccessForFrame(GetFrame()));This helper returns false if either step fails, so this line doesn't guarantee that the `requestStorageAccess` call failed. I think we should just call the JS directly instead:
```suggestion
EXPECT_FALSE(content::ExecJs(GetFrame(), "document.requestStorageAccess()"));
```Same comment below.
File third_party/blink/renderer/core/dom/document.cc:
Patch Set #3, Line 6494:
GetPermissionService(ExecutionContext::From(resolver->GetScriptState()))
As discussed, I need to know in the browser process that a StorageAccess permission was granted to a […]
Can you add a TODO to remove the call to `HasPermission`? I'm pretty sure we can use `has_user_gesture` to infer whether permission was denied due to missing user gesture vs pre-existing denial, so we ought not to need the `HasPermission` call.
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Christian Dullweber (Gerrit)
Attention is currently required from: Christian Dullweber.
Christian Dullweber uploaded patch set #6 to this change.
SAA: Always call RequestPermission
In order to always let the browser process know that StorageAccess was
granted to a frame, we will remove the shortcut to resolve the
StorageAccess request immediately when a permission was previously
granted.
Bug: 1433644
Change-Id: I17773a845c4e37a358fb8fc164fd6be1f01a594f
---
M chrome/browser/storage_access_api/api_browsertest.cc
M chrome/browser/storage_access_api/storage_access_grant_permission_context.cc
M third_party/blink/renderer/core/dom/document.cc
3 files changed, 94 insertions(+), 13 deletions(-)
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Christian Dullweber (Gerrit)
Attention is currently required from: Chris Fredrickson.
Christian Dullweber would like Chris Fredrickson to review this change.
SAA: Always call RequestPermission
In order to always let the browser process know that StorageAccess was
granted to a frame, we will remove the shortcut to resolve the
StorageAccess request immediately when a permission was previously
granted.
Bug: 1433644
Change-Id: I17773a845c4e37a358fb8fc164fd6be1f01a594f
---
M chrome/browser/storage_access_api/api_browsertest.cc
M chrome/browser/storage_access_api/storage_access_grant_permission_context.cc
M third_party/blink/renderer/core/dom/document.cc
3 files changed, 97 insertions(+), 14 deletions(-)
Christian Dullweber (Gerrit)
Attention is currently required from: Chris Fredrickson.
Patch Set #5, Line 538:
EXPECT_FALSE(storage::test::RequestAndCheckStorageAccessForFrame(GetFrame()));
This helper returns false if either step fails, so this line doesn't guarantee that the `requestStor […]
I saw that you are already fixing this in https://crrev.com/c/4598448
I guess no more work is needed form my side?
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Christian Dullweber (Gerrit)
Attention is currently required from: Chris Fredrickson.
Patch Set #3, Line 6494:
GetPermissionService(ExecutionContext::From(resolver->GetScriptState()))
Can you add a TODO to remove the call to `HasPermission`? I'm pretty sure we can use `has_user_gestu […]
Done
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Chris Fredrickson (Gerrit)
Attention is currently required from: Christian Dullweber.
Patch set 7:Code-Review +1
1 comment:
File chrome/browser/storage_access_api/api_browsertest.cc:
Patch Set #5, Line 538:
EXPECT_FALSE(storage::test::RequestAndCheckStorageAccessForFrame(GetFrame()));
I saw that you are already fixing this in https://crrev.com/c/4598448 […]
Yeah, this is fine - I can fix any new instances in that CL.
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Christian Dullweber (Gerrit)
Attention is currently required from: Joey Arhar.
Christian Dullweber would like Joey Arhar to review this change.
SAA: Always call RequestPermission
In order to always let the browser process know that StorageAccess was
granted to a frame, we will remove the shortcut to resolve the
StorageAccess request immediately when a permission was previously
granted.
Bug: 1433644
Change-Id: I17773a845c4e37a358fb8fc164fd6be1f01a594f
---
M chrome/browser/storage_access_api/api_browsertest.cc
M chrome/browser/storage_access_api/storage_access_grant_permission_context.cc
M third_party/blink/renderer/core/dom/document.cc
3 files changed, 97 insertions(+), 14 deletions(-)
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Christian Dullweber (Gerrit)
Attention is currently required from: Joey Arhar.
To view, visit change 4588535. To unsubscribe, or for help writing mail filters, visit settings.
Joey Arhar (Gerrit)
Christian Dullweber (Gerrit)
Patch set 7:Commit-Queue +2
Christian Dullweber (Gerrit)
Attention is currently required from: Christian Dullweber.
Patch set 8:Commit-Queue +2
Christian Dullweber (Gerrit)
Patch set 9:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/storage_access_api/api_browsertest.cc
Insertions: 3, Deletions: 2.
The diff is too large to show. Please review the diff.
```
SAA: Always call RequestPermission
In order to always let the browser process know that StorageAccess was
granted to a frame, we will remove the shortcut to resolve the
StorageAccess request immediately when a permission was previously
granted.
Bug: 1433644
Change-Id: I17773a845c4e37a358fb8fc164fd6be1f01a594f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4588535
Commit-Queue: Christian Dullweber <dull...@chromium.org>
Reviewed-by: Chris Fredrickson <cfre...@chromium.org>
Reviewed-by: Joey Arhar <jar...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1157521}
---
M chrome/browser/storage_access_api/api_browsertest.cc
M chrome/browser/storage_access_api/storage_access_grant_permission_context.cc
M third_party/blink/renderer/core/dom/document.cc
3 files changed, 98 insertions(+), 14 deletions(-)