[permissions] Correctly reuse existing DENIED/GRANTED permission [chromium/src : main]

0 views

Skip to first unread message

Antonio Sartori (Gerrit)

unread,

2:26 AM (6 hours ago) 2:26 AM

to Judith Hemp, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, nator...@chromium.org, rayanka...@chromium.org, storage...@chromium.org

Attention needed from Judith Hemp

Antonio Sartori added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Antonio Sartori . resolved

Judith could you review?

Open in Gerrit

Related details

Attention is currently required from:

Judith Hemp

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Judith Hemp (Gerrit)

unread,

5:01 AM (4 hours ago) 5:01 AM

to Antonio Sartori, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Permissions Reviews, Peter Beverloo, Rijubrata Bhaumik, Simon Hangl, cfredri...@chromium.org, chfreme...@chromium.org, dibyapal+wa...@chromium.org, feature-me...@chromium.org, feature-v...@chromium.org, iclella...@chromium.org, mattreyno...@chromium.org, nator...@chromium.org, rayanka...@chromium.org, storage...@chromium.org

Attention needed from Antonio Sartori

Judith Hemp added 3 comments

File chrome/browser/permissions/permission_request_manager_browsertest.cc

Line 2312, Patchset 4 (Latest): RequestApproximateGeolocation) {

Judith Hemp . unresolved

The test name is not very helpful - this test is somehow testing more that only requesting approximate geolocation permission.

As I understand this, we
1. request (and grant) approx location (prompt)
2. re-request approx location (no prompt)
3. request (and grant) precise location (prompt) -> "full" location permissions
4. re-request approx location (no prompt)

So, its more like "RequestFullGeolocation", maybe?
nit: Since its a long text, can you maybe document this as a docstring in the beginning of the test, so readers see in one go what this test is doing? Not sure this makes sense and comment-styles are highly subjective, so feel free to ignore, you already comment in this test which is fine.

Line 2341, Patchset 4 (Latest):

            approx_only_permission_result);
  EXPECT_EQ(permission_controller->GetPermissionResultForCurrentDocument(
                kPreciseGeolocationDescriptor, GetActiveMainFrame()),
            content::PermissionResult(
                blink::mojom::PermissionStatus::ASK,
                content::PermissionStatusSource::UNSPECIFIED,
                GeolocationSetting({.approximate = PermissionOption::kAllowed,
                                    .precise = PermissionOption::kAsk})));

Judith Hemp . unresolved

Pls remove, I think this is equivalent to the cmd before, maybe some refactoring leftover?

File components/permissions/permission_prompt_decision.h

Line 28, Patchset 4 (Latest): PromptOptions prompt_options = std::monostate();

Judith Hemp . resolved

Again what learned! I did not know about this idiom.

Open in Gerrit

Related details

Attention is currently required from:

Antonio Sartori

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Antonio Sartori (Gerrit)

unread,

7:20 AM (1 hour ago) 7:20 AM

Attention needed from Judith Hemp

Antonio Sartori added 2 comments

File chrome/browser/permissions/permission_request_manager_browsertest.cc

Line 2312, Patchset 4 (Latest): RequestApproximateGeolocation) {

Judith Hemp . resolved

The test name is not very helpful - this test is somehow testing more that only requesting approximate geolocation permission.
As I understand this, we
1. request (and grant) approx location (prompt)
2. re-request approx location (no prompt)
3. request (and grant) precise location (prompt) -> "full" location permissions
4. re-request approx location (no prompt)
So, its more like "RequestFullGeolocation", maybe?
nit: Since its a long text, can you maybe document this as a docstring in the beginning of the test, so readers see in one go what this test is doing? Not sure this makes sense and comment-styles are highly subjective, so feel free to ignore, you already comment in this test which is fine.

Antonio Sartori

Changed the name of the test. As for the docstring, I just know it will go out-of-sync, so I think relying on the actual content of the test being readable is preferable.

Line 2341, Patchset 4 (Latest):

            approx_only_permission_result);
  EXPECT_EQ(permission_controller->GetPermissionResultForCurrentDocument(
                kPreciseGeolocationDescriptor, GetActiveMainFrame()),
            content::PermissionResult(
                blink::mojom::PermissionStatus::ASK,
                content::PermissionStatusSource::UNSPECIFIED,
                GeolocationSetting({.approximate = PermissionOption::kAllowed,
                                    .precise = PermissionOption::kAsk})));

Judith Hemp . unresolved

Pls remove, I think this is equivalent to the cmd before, maybe some refactoring leftover?

Antonio Sartori

Sorry, I don't follow. To what is this equivalent?

Open in Gerrit

Related details

Attention is currently required from:

Judith Hemp

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages