Fall back to legacy geolocation on pre-1903 Windows releases [chromium/src : main]

[Chris Davis (Gerrit)]

New activity on the change

Open in Gerrit

Related details

Attention set is empty

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Iec6124e9eefc27366fb35830cf1fb8e5bb245755

Gerrit-Change-Number: 8173044

Gerrit-PatchSet: 3

Gerrit-Owner: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Chris Davis <chrd...@microsoft.com>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Permissions Reviews <permissio...@chromium.org>

Gerrit-CC: chromium...@chromium.org

Gerrit-Comment-Date: Thu, 30 Jul 2026 19:56:55 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

[Alvin Ji (Gerrit)]

Attention needed from Balazs Engedy, Chris Davis, Greg Thompson and Marc Treib

Alvin Ji added 1 comment

File services/device/public/cpp/device_features_win_unittest.cc

Line 43, Patchset 3 (Latest): base::test::ScopedOSInfoOverride os_override(
base::test::ScopedOSInfoOverride::Type::kWin11Pro);

Alvin Ji . unresolved

I wonder should Win11 Pro be supported for the feature?

Open in Gerrit

Related details

Attention is currently required from:

• Balazs Engedy
• Chris Davis
• Greg Thompson
• Marc Treib

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Iec6124e9eefc27366fb35830cf1fb8e5bb245755

Gerrit-Change-Number: 8173044

Gerrit-PatchSet: 3

Gerrit-Owner: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Alvin Ji <alv...@chromium.org>

Gerrit-Reviewer: Balazs Engedy <eng...@chromium.org>

Gerrit-Reviewer: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Greg Thompson <g...@chromium.org>

Gerrit-Reviewer: Marc Treib <tr...@chromium.org>

Gerrit-Reviewer: Matt Reynolds <mattre...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Permissions Reviews <permissio...@chromium.org>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Chris Davis <chrd...@microsoft.com>

Gerrit-Attention: Marc Treib <tr...@chromium.org>

Gerrit-Attention: Greg Thompson <g...@chromium.org>

Gerrit-Attention: Balazs Engedy <eng...@chromium.org>

Gerrit-Comment-Date: Thu, 30 Jul 2026 20:16:28 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Chris Davis (Gerrit)]

Attention needed from Alvin Ji, Balazs Engedy, Greg Thompson and Marc Treib

Chris Davis added 1 comment

File services/device/public/cpp/device_features_win_unittest.cc

Line 43, Patchset 3 (Latest): base::test::ScopedOSInfoOverride os_override(
base::test::ScopedOSInfoOverride::Type::kWin11Pro);

Alvin Ji . resolved

I wonder should Win11 Pro be supported for the feature?

Chris Davis

Yes. But this test verifies the kWinSystemLocationPermission disabled state.

Open in Gerrit

Related details

Attention is currently required from:

• Alvin Ji
• Balazs Engedy
• Greg Thompson
• Marc Treib

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Iec6124e9eefc27366fb35830cf1fb8e5bb245755

Gerrit-Change-Number: 8173044

Gerrit-PatchSet: 3

Gerrit-Owner: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Alvin Ji <alv...@chromium.org>

Gerrit-Reviewer: Balazs Engedy <eng...@chromium.org>

Gerrit-Reviewer: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Greg Thompson <g...@chromium.org>

Gerrit-Reviewer: Marc Treib <tr...@chromium.org>

Gerrit-Reviewer: Matt Reynolds <mattre...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Permissions Reviews <permissio...@chromium.org>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Marc Treib <tr...@chromium.org>

Gerrit-Attention: Alvin Ji <alv...@chromium.org>

Gerrit-Attention: Greg Thompson <g...@chromium.org>

Gerrit-Attention: Balazs Engedy <eng...@chromium.org>

Gerrit-Comment-Date: Thu, 30 Jul 2026 20:33:15 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Alvin Ji <alv...@chromium.org>

[Alvin Ji (Gerrit)]

Attention needed from Balazs Engedy, Chris Davis, Greg Thompson and Marc Treib

Alvin Ji voted and added 1 comment

Votes added by Alvin Ji

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Alvin Ji . resolved

LGTM for Geolocation

Open in Gerrit

Related details

Attention is currently required from:

• Balazs Engedy
• Chris Davis
• Greg Thompson
• Marc Treib

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Iec6124e9eefc27366fb35830cf1fb8e5bb245755

Gerrit-Change-Number: 8173044

Gerrit-PatchSet: 3

Gerrit-Owner: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Alvin Ji <alv...@chromium.org>

Gerrit-Reviewer: Balazs Engedy <eng...@chromium.org>

Gerrit-Reviewer: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Greg Thompson <g...@chromium.org>

Gerrit-Reviewer: Marc Treib <tr...@chromium.org>

Gerrit-Reviewer: Matt Reynolds <mattre...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Permissions Reviews <permissio...@chromium.org>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Chris Davis <chrd...@microsoft.com>

Gerrit-Attention: Marc Treib <tr...@chromium.org>

Gerrit-Attention: Greg Thompson <g...@chromium.org>

Gerrit-Attention: Balazs Engedy <eng...@chromium.org>

Gerrit-Comment-Date: Thu, 30 Jul 2026 21:26:45 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Greg Thompson (Gerrit)]

Attention needed from Balazs Engedy, Chris Davis and Marc Treib

Greg Thompson added 1 comment

File chrome/browser/chrome_browser_main_win.cc

Line 634, Patchset 3 (Latest): CreateGeolocationSystemPermissionManager());

Greg Thompson . unresolved

it seems cleaner to me if we put this OS check inside `CreateGeolocationSystemPermissionManager()` so that consumers of `GeolocationSystemPermissionManager` don't need their own branching at each point of use. can we have a `SystemGeolocationSource` implementation for the "OS support is missing" case that returns the proper things when it is called?

@alv...@chromium.org: `kWinSystemLocationPermission` launched long ago. can we remove it from the codebase? if so, i think that makes the case of putting this new conditional within `geolocation` even more appealing.

Open in Gerrit

Related details

Attention is currently required from:

• Balazs Engedy
• Chris Davis
• Marc Treib

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Iec6124e9eefc27366fb35830cf1fb8e5bb245755

Gerrit-Change-Number: 8173044

Gerrit-PatchSet: 3

Gerrit-Owner: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Alvin Ji <alv...@chromium.org>

Gerrit-Reviewer: Balazs Engedy <eng...@chromium.org>

Gerrit-Reviewer: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Greg Thompson <g...@chromium.org>

Gerrit-Reviewer: Marc Treib <tr...@chromium.org>

Gerrit-Reviewer: Matt Reynolds <mattre...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Permissions Reviews <permissio...@chromium.org>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Chris Davis <chrd...@microsoft.com>

Gerrit-Attention: Marc Treib <tr...@chromium.org>

Gerrit-Attention: Balazs Engedy <eng...@chromium.org>

Gerrit-Comment-Date: Fri, 31 Jul 2026 06:47:30 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Balazs Engedy (Gerrit)]

Attention needed from Chris Davis and Marc Treib

Balazs Engedy voted and added 1 comment

Votes added by Balazs Engedy

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Balazs Engedy . resolved

chrome/browser/permissions/system/system_permission_settings_win.cc LGTM

Open in Gerrit

Related details

Attention is currently required from:

• Chris Davis
• Marc Treib

Gerrit-Comment-Date: Fri, 31 Jul 2026 09:56:06 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Chris Davis (Gerrit)]

Attention needed from Alvin Ji, Balazs Engedy, Greg Thompson and Marc Treib

Chris Davis added 1 comment

File chrome/browser/chrome_browser_main_win.cc

Line 634, Patchset 3: CreateGeolocationSystemPermissionManager());

Greg Thompson . resolved

it seems cleaner to me if we put this OS check inside `CreateGeolocationSystemPermissionManager()` so that consumers of `GeolocationSystemPermissionManager` don't need their own branching at each point of use. can we have a `SystemGeolocationSource` implementation for the "OS support is missing" case that returns the proper things when it is called?

@alv...@chromium.org: `kWinSystemLocationPermission` launched long ago. can we remove it from the codebase? if so, i think that makes the case of putting this new conditional within `geolocation` even more appealing.

Chris Davis

Done

Open in Gerrit

Related details

Attention is currently required from:

• Alvin Ji
• Balazs Engedy
• Greg Thompson

• Marc Treib

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

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: Iec6124e9eefc27366fb35830cf1fb8e5bb245755

Gerrit-Change-Number: 8173044

Gerrit-PatchSet: 4

Gerrit-Owner: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Alvin Ji <alv...@chromium.org>

Gerrit-Reviewer: Balazs Engedy <eng...@chromium.org>

Gerrit-Reviewer: Chris Davis <chrd...@microsoft.com>

Gerrit-Reviewer: Greg Thompson <g...@chromium.org>

Gerrit-Reviewer: Marc Treib <tr...@chromium.org>

Gerrit-Reviewer: Matt Reynolds <mattre...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Permissions Reviews <permissio...@chromium.org>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Marc Treib <tr...@chromium.org>

Gerrit-Attention: Alvin Ji <alv...@chromium.org>

Gerrit-Attention: Greg Thompson <g...@chromium.org>

Gerrit-Attention: Balazs Engedy <eng...@chromium.org>

Gerrit-Comment-Date: Sat, 01 Aug 2026 23:30:43 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Greg Thompson <g...@chromium.org>