CRX Telemetry - Cookies APIs signal fixes. [chromium/src : main]

Richard Chen (Gerrit)

unread,

Mar 29, 2023, 3:36:31 PM3/29/23

to Adam Psarouthakis, Anunoy Ghosh, Zack Han, andysjl...@chromium.org, chromium-a...@chromium.org, druber...@chromium.org, extension...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention is currently required from: Adam Psarouthakis, Anunoy Ghosh, Zack Han.

Richard Chen would like Adam Psarouthakis, Anunoy Ghosh and Zack Han to review this change.

View Change

CRX Telemetry - Cookies APIs signal fixes.

- Report getAll() secure/is_session boolean fields correctly when empty.
- Add comma separators when creating unique args id to ensure uniqueness between argument sets.

Bug:1428680
Change-Id: Id817f1ca65772c08b57662436202bae0830c435e
---
M chrome/browser/extensions/api/cookies/cookies_api.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal.h
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal_processor.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal_processor_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_signal.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_signal_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service_browsertest.cc
9 files changed, 62 insertions(+), 32 deletions(-)

To view, visit change 4381471. To unsubscribe, or for help writing mail filters, visit settings.

Richard Chen (Gerrit)

unread,

Mar 29, 2023, 3:36:34 PM3/29/23

to andysjl...@chromium.org, chromium-a...@chromium.org, druber...@chromium.org, extension...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org, Anunoy Ghosh, Adam Psarouthakis, Zack Han, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Adam Psarouthakis, Anunoy Ghosh, Zack Han.

View Change

To view, visit change 4381471. To unsubscribe, or for help writing mail filters, visit settings.

Anunoy Ghosh (Gerrit)

unread,

Mar 30, 2023, 7:33:17 AM3/30/23

to Richard Chen, andysjl...@chromium.org, chromium-a...@chromium.org, druber...@chromium.org, extension...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org, Adam Psarouthakis, Zack Han, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Adam Psarouthakis, Richard Chen, Zack Han.

View Change

2 comments:

Commit Message:
- Patch Set #4, Line 10: Add comma separators when creating unique args id to ensure uniqueness between argument sets
  Why do we need the comma separators? Is that for the empty args list case to avoid an empty key for the args store?
Patchset:
- Patch Set #4:
  Thanks Richard! Just one question - see comments.

To view, visit change 4381471. To unsubscribe, or for help writing mail filters, visit settings.

Richard Chen (Gerrit)

unread,

Mar 30, 2023, 11:23:28 AM3/30/23

to andysjl...@chromium.org, chromium-a...@chromium.org, druber...@chromium.org, extension...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org, Anunoy Ghosh, Adam Psarouthakis, Zack Han, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Adam Psarouthakis, Anunoy Ghosh, Zack Han.

View Change

1 comment:

Commit Message:

- Patch Set #4, Line 10: Add comma separators when creating unique args id to ensure uniqueness between argument sets

- Why do we need the comma separators? Is that for the empty args list case to avoid an empty key for […]
  Adding comma separators is to ensure the uniqueness of each argument set. Previously, it's possible for 2 different argument sets to have the same argument id. Then, whichever argument set appears first would be recorded, and the other argument would just increment the counter instead of reported as its own individual argument set.
  For example, the following argument sets would return the same unique id.
  - name = "google", secure = "true"
  - name = "google", is_session = "true"
  If no other fields are populated, the unique id for both would be "googletrue". Adding the comma separators keep track of which value appeared in which field position.

To view, visit change 4381471. To unsubscribe, or for help writing mail filters, visit settings.

Anunoy Ghosh (Gerrit)

unread,

Mar 30, 2023, 11:58:15 AM3/30/23

to Richard Chen, andysjl...@chromium.org, chromium-a...@chromium.org, druber...@chromium.org, extension...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org, Adam Psarouthakis, Zack Han, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Adam Psarouthakis, Richard Chen, Zack Han.

Patch set 4:Code-Review +1

View Change

1 comment:

Commit Message:

- Patch Set #4, Line 10: Add comma separators when creating unique args id to ensure uniqueness between argument sets

- Adding comma separators is to ensure the uniqueness of each argument set. […]
  Makes sense. Thanks for the explanation!

To view, visit change 4381471. To unsubscribe, or for help writing mail filters, visit settings.

Richard Chen (Gerrit)

unread,

Mar 30, 2023, 1:02:50 PM3/30/23

to David Bertoni, andysjl...@chromium.org, chromium-a...@chromium.org, druber...@chromium.org, extension...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org, Anunoy Ghosh, Adam Psarouthakis, Zack Han

Attention is currently required from: Adam Psarouthakis, David Bertoni, Zack Han.

Richard Chen would like David Bertoni to review this change.

View Change

CRX Telemetry - Cookies APIs signal fixes.

- Report getAll() secure/is_session boolean fields correctly when empty.
- Add comma separators when creating unique args id to ensure uniqueness between argument sets.

Bug:1428680
Change-Id: Id817f1ca65772c08b57662436202bae0830c435e
---
M chrome/browser/extensions/api/cookies/cookies_api.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal.h
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal_processor.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal_processor_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_signal.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_signal_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service_browsertest.cc
9 files changed, 62 insertions(+), 32 deletions(-)

To view, visit change 4381471. To unsubscribe, or for help writing mail filters, visit settings.

David Bertoni (Gerrit)

unread,

Mar 30, 2023, 1:33:32 PM3/30/23

to Richard Chen, andysjl...@chromium.org, chromium-a...@chromium.org, druber...@chromium.org, extension...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org, Anunoy Ghosh, Adam Psarouthakis, Zack Han, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Adam Psarouthakis, Richard Chen, Zack Han.

Patch set 4:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #4:
  lgtm, thanks!

To view, visit change 4381471. To unsubscribe, or for help writing mail filters, visit settings.

Richard Chen (Gerrit)

unread,

Mar 30, 2023, 1:34:36 PM3/30/23

to andysjl...@chromium.org, chromium-a...@chromium.org, druber...@chromium.org, extension...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org, David Bertoni, Anunoy Ghosh, Adam Psarouthakis, Zack Han, Chromium LUCI CQ, chromium...@chromium.org

Attention is currently required from: Adam Psarouthakis, Richard Chen, Zack Han.

Patch set 4:Commit-Queue +2

View Change

To view, visit change 4381471. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Mar 30, 2023, 3:33:13 PM3/30/23

to Richard Chen, andysjl...@chromium.org, chromium-a...@chromium.org, druber...@chromium.org, extension...@chromium.org, nwoked...@chromium.org, vakh+safe_br...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org, David Bertoni, Anunoy Ghosh, Adam Psarouthakis, Zack Han, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  David Bertoni: Looks good to me
  Anunoy Ghosh: Looks good to me
  Richard Chen: Commit

CRX Telemetry - Cookies APIs signal fixes.

- Report getAll() secure/is_session boolean fields correctly when empty.
- Add comma separators when creating unique args id to ensure uniqueness between argument sets.

Bug: 1428680
Change-Id: Id817f1ca65772c08b57662436202bae0830c435e

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4381471
Reviewed-by: Anunoy Ghosh <anu...@chromium.org>
Reviewed-by: David Bertoni <dber...@chromium.org>
Commit-Queue: Richard Chen <ric...@google.com>
Cr-Commit-Position: refs/heads/main@{#1124362}


---
M chrome/browser/extensions/api/cookies/cookies_api.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal.h
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal_processor.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal_processor_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_all_signal_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_signal.cc
M chrome/browser/safe_browsing/extension_telemetry/cookies_get_signal_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service_browsertest.cc
9 files changed, 62 insertions(+), 32 deletions(-)

To view, visit change 4381471. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward