Deprecate SharedMemoryRegion memory(), replace it with data() and spans [chromium/src : main]
0 views
Skip to first unread message
danakj (Gerrit)
Jul 25, 2024, 3:09:46 PM (2 days ago) Jul 25
to Jonathan Ross, James Cook, Avi Drissman, Alex Moshchuk, Khushal Sagar, Łukasz Anforowicz, Piotr Bialecki, Reilly Grant, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Alex Moshchuk, Avi Drissman, Brandon Jones, James Cook, Jonathan Ross, Ken Rockot, Piotr Bialecki and Łukasz Anforowicz
danakj added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 10:
danakj . resolved
danakjavi: chrome
alexmos: content
avi: ui as well please
Related details
Attention is currently required from:
- Alex Moshchuk
- Avi Drissman
- Brandon Jones
- James Cook
- Jonathan Ross
- Ken Rockot
- Piotr Bialecki
- Łukasz Anforowicz
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Jul 25, 2024, 3:18:32 PM (2 days ago) Jul 25
to danakj, Jonathan Ross, James Cook, Avi Drissman, Alex Moshchuk, Łukasz Anforowicz, Piotr Bialecki, Reilly Grant, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Alex Moshchuk, Avi Drissman, Brandon Jones, James Cook, Jonathan Ross, Ken Rockot, Piotr Bialecki, danakj and Łukasz Anforowicz
Khushal Sagar voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Alex Moshchuk
- Avi Drissman
- Brandon Jones
- James Cook
- Jonathan Ross
- Ken Rockot
- Piotr Bialecki
- danakj
- Łukasz Anforowicz
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
Jul 25, 2024, 3:29:07 PM (2 days ago) Jul 25
to danakj, Khushal Sagar, Jonathan Ross, Avi Drissman, Alex Moshchuk, Łukasz Anforowicz, Piotr Bialecki, Reilly Grant, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Alex Moshchuk, Avi Drissman, Brandon Jones, Jonathan Ross, Ken Rockot, Piotr Bialecki, danakj and Łukasz Anforowicz
James Cook voted and added 1 comment![]( "Open in Gerrit")
Attention is currently required from:
- Alex Moshchuk
- Avi Drissman
- Brandon Jones
Alex Moshchuk (Gerrit)
Jul 25, 2024, 4:23:29 PM (2 days ago) Jul 25
to danakj, James Cook, Khushal Sagar, Jonathan Ross, Avi Drissman, Łukasz Anforowicz, Piotr Bialecki, Reilly Grant, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Avi Drissman, Brandon Jones, Jonathan Ross, Ken Rockot, Piotr Bialecki, danakj and Łukasz Anforowicz
Alex Moshchuk voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Moshchuk
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Avi Drissman
- Brandon Jones
- Jonathan Ross
- Ken Rockot
- Piotr Bialecki
- danakj
- Łukasz Anforowicz
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jonathan Ross (Gerrit)
Jul 25, 2024, 4:41:43 PM (2 days ago) Jul 25
to danakj, Alex Moshchuk, James Cook, Khushal Sagar, Avi Drissman, Łukasz Anforowicz, Piotr Bialecki, Reilly Grant, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Avi Drissman, Brandon Jones, Ken Rockot, Piotr Bialecki, danakj and Łukasz Anforowicz
Jonathan Ross voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jonathan Ross
| Code-Review | +1 |
1 comment
Patchset-level comments
Jonathan Ross . resolved
Exo was reverted but files I own are LGTM
cc/layers/heads_up_display_layer_impl.cc
cc/resources/cross_thread_shared_bitmap.h
cc/test/pixel_test.cc
content/browser/renderer_host/render_process_host_impl.cc
services/test/echo/echo_service.cc
third_party/blink/renderer/platform/graphics/canvas_resource.cc
Related details
Attention is currently required from:
- Avi Drissman
- Brandon Jones
Avi Drissman (Gerrit)
Jul 25, 2024, 4:53:19 PM (2 days ago) Jul 25
to danakj, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Łukasz Anforowicz, Piotr Bialecki, Reilly Grant, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Ken Rockot, Piotr Bialecki, danakj and Łukasz Anforowicz
Avi Drissman voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
danakj (Gerrit)
Jul 26, 2024, 11:03:56 AM (21 hours ago) Jul 26
to Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Łukasz Anforowicz, Piotr Bialecki, Reilly Grant, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Ken Rockot, Piotr Bialecki and Łukasz Anforowicz
danakj voted and added 1 comment![]( "Open in Gerrit")
Votes added by danakj
| Commit-Queue | +1 |
1 comment
File services/device/public/cpp/test/fake_sensor_and_provider.cc
Line 343, Patchset 16:
buffers[GetSensorReadingSharedBufferOffset(type)];danakj . resolved
A woopsie in the SensorReadingSharedBuffer code made spans crash on OOB. The GetSensorReadingSharedBufferOffset() gives an index into the SensorReadingSharedBuffer array, but it does it in number of bytes, not in the number of SensorReadingSharedBuffers. So we have to divide by the sizeof(SensorReadingSharedBuffer).
I thought about changing GetSensorReadingSharedBufferOffset() to return the array offset instead of the byte offset, but it gets stored in a bunch of places that I did not change in this CL.
Related details
Attention is currently required from:
- Brandon Jones
- Ken Rockot
- Piotr Bialecki
- Łukasz Anforowicz
Łukasz Anforowicz (Gerrit)
Jul 26, 2024, 12:45:21 PM (20 hours ago) Jul 26
to danakj, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Reilly Grant, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Ken Rockot, Piotr Bialecki and danakj
Łukasz Anforowicz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Łukasz Anforowicz
| Code-Review | +1 |
2 comments
Patchset-level comments
File-level comment, Patchset 18 (Latest):
Łukasz Anforowicz . unresolved
Still LGTM, but I wonder if `GetMemoryAsSpan<char>` could be replaced by something else in some places. `base::as_writable_byte_span(...)`? Or `base::as_string_view(...)`? This is probably optional, but I think it may be useful for teaching future code readers about the generic `span` utilities in `//base` (rather than using a one-off/specific API of shared memory).
File mojo/public/cpp/base/shared_memory_version.cc
Line 21, Patchset 9 (Parent):
new (mapped_region_.mapping.memory()) SharedVersionType;Łukasz Anforowicz . resolved
danakjIs the removal of this placement `new` intentional? IIUC after removal the `GetMemoryAs` below will return uninitialized/non-constructed bytes, which seems wrong?
Łukasz AnforowiczSharedVersionType is an atomic primitive type which is trivially constructed and trivially copyable. So the compiler sees a valid object here. Its value is not important for doing store().
If the placement new were important then we would also need to go through the returned pointer to use it (which we were not).
Acknowledged. Thank you for explaining.
Related details
Attention is currently required from:
- Brandon Jones
- Ken Rockot
- Piotr Bialecki
- danakj
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reilly Grant (Gerrit)
Jul 26, 2024, 1:24:55 PM (19 hours ago) Jul 26
to danakj, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Ken Rockot, Piotr Bialecki and danakj
Reilly Grant voted and added 1 comment![]( "Open in Gerrit")
Votes added by Reilly Grant
| Code-Review | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Brandon Jones
- Ken Rockot
- Piotr Bialecki
- danakj
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Jul 26, 2024, 1:56:01 PM (18 hours ago) Jul 26
to danakj, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Ken Rockot, Piotr Bialecki and danakj
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
danakj (Gerrit)
Jul 26, 2024, 1:58:58 PM (18 hours ago) Jul 26
to findit...@appspot.gserviceaccount.com, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Ken Rockot, Brandon Jones, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Daniel Cheng, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Ken Rockot and Piotr Bialecki
danakj added 1 comment![]( "Open in Gerrit")
Patchset-level comments
danakj . resolved
rockot and bajones are OOO oops
Related details
Attention is currently required from:
- Brandon Jones
- Ken Rockot
- Piotr Bialecki
danakj (Gerrit)
Jul 26, 2024, 2:00:26 PM (18 hours ago) Jul 26
to Daniel Cheng, Ken Rockot, Brandon Jones, findit...@appspot.gserviceaccount.com, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Daniel Cheng, Ken Rockot and Piotr Bialecki
danakj added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Attention is currently required from:
- Brandon Jones
- Daniel Cheng
- Ken Rockot
- Piotr Bialecki
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
danakj (Gerrit)
Jul 26, 2024, 2:01:43 PM (18 hours ago) Jul 26
to Daniel Cheng, Ken Rockot, Brandon Jones, findit...@appspot.gserviceaccount.com, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Daniel Cheng, Ken Rockot and Piotr Bialecki
danakj added 1 comment![]( "Open in Gerrit")
Patchset-level comments
danakj . resolved
The remaining 5 services/ files are trivial / just api change so I think we can OO those ones
Daniel Cheng (Gerrit)
Jul 26, 2024, 2:04:49 PM (18 hours ago) Jul 26
to danakj, Daniel Cheng, Ken Rockot, Brandon Jones, findit...@appspot.gserviceaccount.com, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Ken Rockot, Piotr Bialecki and danakj
Daniel Cheng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
| Owners-Override | +1 |
2 comments
File mojo/core/ipcz_driver/shared_buffer_mapping.cc
Line 31, Patchset 19 (Latest):
// TODO(rockot): This function should give back a span instead of an unboundedDaniel Cheng . unresolved
Ideally this would be a crbug.
File mojo/public/cpp/base/shared_memory_version.cc
Line 21, Patchset 19 (Parent):
new (mapped_region_.mapping.memory()) SharedVersionType;Daniel Cheng . unresolved
I guess as long as MSan doesn't complain... it's a bit of a weird situation though. Technically we need to new it to begin its lifetime, don't we?
Related details
Attention is currently required from:
- Brandon Jones
- Ken Rockot
- Piotr Bialecki
- danakj
danakj (Gerrit)
Jul 26, 2024, 2:14:22 PM (18 hours ago) Jul 26
to Daniel Cheng, Ken Rockot, Brandon Jones, findit...@appspot.gserviceaccount.com, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Ken Rockot and Piotr Bialecki
danakj added 1 comment![]( "Open in Gerrit")
File mojo/public/cpp/base/shared_memory_version.cc
Line 21, Patchset 19 (Parent):
new (mapped_region_.mapping.memory()) SharedVersionType;Daniel Cheng . resolved
I guess as long as MSan doesn't complain... it's a bit of a weird situation though. Technically we need to new it to begin its lifetime, don't we?
danakj
Sorta but not. We should be laundering in the GetMemoryAs() method to introduce a providence barrier.
The objects in SharedMemory come pre-formed generally, we don't (and can't) call constructors on objects already formed in SharedMemory.
As they are trivially copyable they are understood to be valid objects by the compiler (modulo launder).
Related details
Attention is currently required from:
- Brandon Jones
- Ken Rockot
- Piotr Bialecki
danakj (Gerrit)
Jul 26, 2024, 2:28:24 PM (18 hours ago) Jul 26
to Daniel Cheng, Ken Rockot, Brandon Jones, findit...@appspot.gserviceaccount.com, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Ken Rockot and Piotr Bialecki
danakj added 2 comments![]( "Open in Gerrit")
Patchset-level comments
Still LGTM, but I wonder if `GetMemoryAsSpan<char>` could be replaced by something else in some places. `base::as_writable_byte_span(...)`? Or `base::as_string_view(...)`? This is probably optional, but I think it may be useful for teaching future code readers about the generic `span` utilities in `//base` (rather than using a one-off/specific API of shared memory).
danakj
Done
File mojo/core/ipcz_driver/shared_buffer_mapping.cc
Line 31, Patchset 19 (Latest):
// TODO(rockot): This function should give back a span instead of an unboundedDaniel Cheng . resolved
Ideally this would be a crbug.
danakj
Done
Related details
Attention is currently required from:
- Brandon Jones
- Ken Rockot
- Piotr Bialecki
Submit Requirements:
Code-Coverage
Code-Review
danakj (Gerrit)
Jul 26, 2024, 2:32:20 PM (18 hours ago) Jul 26
to Daniel Cheng, Ken Rockot, Brandon Jones, findit...@appspot.gserviceaccount.com, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Attention needed from Brandon Jones, Ken Rockot and Piotr Bialecki
danakj voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention is currently required from:
- Brandon Jones
- Ken Rockot
- Piotr Bialecki
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
findit-for-me@appspot.gserviceaccount.com (Gerrit)
Jul 26, 2024, 4:09:29 PM (16 hours ago) Jul 26
to danakj, Daniel Cheng, Ken Rockot, Brandon Jones, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, David Benjamin, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
findit...@appspot.gserviceaccount.com voted Code-Coverage+1![]( "Open in Gerrit")
This change meets the code coverage requirements.
| Code-Coverage | +1 |
Related details
Attention set is empty
Chromium LUCI CQ (Gerrit)
Jul 26, 2024, 4:31:10 PM (16 hours ago) Jul 26
to danakj, Daniel Cheng, Ken Rockot, Brandon Jones, findit...@appspot.gserviceaccount.com, Reilly Grant, Łukasz Anforowicz, Avi Drissman, Jonathan Ross, Alex Moshchuk, James Cook, Khushal Sagar, Piotr Bialecki, Rijubrata Bhaumik, Wanming Lin, Dirk Schulze, Raphael Kubo Da Costa, Stephen Chenney, AyeAye, chromium...@chromium.org, David Benjamin, Tom Sepez, ozone-...@chromium.org, max+watc...@igalia.com, nickdiego+wa...@igalia.com, cc-...@chromium.org, rginda...@chromium.org, dpr-eng+c...@google.com, chromeos-ca...@google.com, chromium-a...@chromium.org, extension...@chromium.org, filesapp...@chromium.org, pasko...@chromium.org, nyquis...@chromium.org, torne...@chromium.org, lizeb...@chromium.org, yfriedm...@chromium.org, agriev...@chromium.org, kinuko...@chromium.org, crostin...@chromium.org, creis...@chromium.org, blink-reviews-p...@chromium.org, marinacio...@chromium.org, fserb...@chromium.org, navigation...@chromium.org, blink-...@chromium.org, devtools...@chromium.org, yhanada+...@chromium.org, tracing...@chromium.org, zhangwen...@google.com, wfh+...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, alexmo...@chromium.org, blundell+...@chromium.org, olka+...@chromium.org, mattreyno...@chromium.org, spang...@chromium.org, feature-v...@chromium.org, gavinp...@chromium.org, security-...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
19 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/enterprise/connectors/analysis/content_analysis_delegate_unittest.cc
Insertions: 2, Deletions: 1.
@@ -4,6 +4,7 @@
#include "chrome/browser/enterprise/connectors/analysis/content_analysis_delegate.h"
+#include <algorithm>
#include <map>
#include <set>
#include <string>
@@ -107,7 +108,7 @@
base::ReadOnlySharedMemoryRegion create_page(size_t size) {
base::MappedReadOnlyRegion page =
base::ReadOnlySharedMemoryRegion::Create(size);
- std::ranges::fill(page.mapping.GetMemoryAsSpan<char>(), 'a');
+ std::ranges::fill(base::span(page.mapping), 'a');
return std::move(page.region);
}
```
```
The name of the file: chrome/browser/enterprise/connectors/analysis/content_analysis_delegate_browsertest.cc
Insertions: 1, Deletions: 1.
@@ -1811,7 +1811,7 @@
constexpr int64_t kLargeSize = 51 * 1024 * 1024;
base::MappedReadOnlyRegion page =
base::ReadOnlySharedMemoryRegion::Create(kLargeSize);
- std::ranges::fill(page.mapping.GetMemoryAsSpan<char>(), 'a');
+ std::ranges::fill(base::span(page.mapping), 'a');
data.page = std::move(page.region);
ASSERT_TRUE(ContentAnalysisDelegate::IsEnabled(browser()->profile(),
```
```
The name of the file: mojo/core/platform_wrapper_unittest.cc
Insertions: 2, Deletions: 2.
@@ -134,7 +134,7 @@
auto region = base::UnsafeSharedMemoryRegion::Create(kMessage.size());
base::WritableSharedMemoryMapping buffer = region.Map();
CHECK(buffer.IsValid());
- buffer.GetMemoryAsSpan<char>().copy_from(kMessage);
+ base::as_writable_chars(base::span(buffer)).copy_from(kMessage);
RunTestClient("ReadPlatformSharedBuffer", [&](MojoHandle h) {
// Wrap the shared memory handle and send it to the child along with the
@@ -229,7 +229,7 @@
ASSERT_TRUE(region.IsValid());
base::WritableSharedMemoryMapping mapping = region.Map();
- EXPECT_EQ(base::span(message), mapping.GetMemoryAsSpan<char>());
+ EXPECT_EQ(base::as_byte_span(message), base::span(mapping));
// Verify that the received buffer's internal GUID was preserved in transit.
EXPECT_EQ(MOJO_RESULT_OK, WaitForSignals(h, MOJO_HANDLE_SIGNAL_READABLE));
```
```
The name of the file: chrome/browser/ash/extensions/file_manager/image_loader_private_api.cc
Insertions: 1, Deletions: 1.
@@ -245,7 +245,7 @@
Respond(Error("Failed allocate memory for PDF file"));
return;
}
- pdf_region.mapping.GetMemoryAsSpan<char>().copy_from(content);
+ base::as_writable_chars(base::span(pdf_region.mapping)).copy_from(content);
DCHECK(!pdf_thumbnailer_.is_bound());
GetPdfService()->BindPdfThumbnailer(
pdf_thumbnailer_.BindNewPipeAndPassReceiver());
```
```
The name of the file: mojo/core/ipcz_driver/shared_buffer_mapping.cc
Insertions: 2, Deletions: 2.
@@ -28,8 +28,8 @@
if (!m.IsValid()) {
return nullptr;
}
- // TODO(rockot): This function should give back a span instead of an unbounded
- // pointer.
+ // TODO(crbug.com/355607629): This function should give back a span instead of
+ // an unbounded pointer.
*memory = const_cast<uint8_t*>(m.data());
return std::make_unique<typename RegionType::MappingType>(std::move(m));
}
```
```
The name of the file: mojo/public/cpp/base/shared_memory_unittest.cc
Insertions: 9, Deletions: 6.
@@ -13,14 +13,15 @@
TEST(SharedMemoryMojomTest, ReadOnly) {
auto region = base::ReadOnlySharedMemoryRegion::Create(64);
const std::string kTestData = "Hello, world!";
- region.mapping.GetMemoryAsSpan<char>().copy_prefix_from(kTestData);
+ base::as_writable_chars(base::span(region.mapping))
+ .copy_prefix_from(kTestData);
base::ReadOnlySharedMemoryRegion read_only_out;
ASSERT_TRUE(mojo::test::SerializeAndDeserialize<
mojo_base::mojom::ReadOnlySharedMemoryRegion>(region.region,
read_only_out));
base::ReadOnlySharedMemoryMapping mapping = read_only_out.Map();
- EXPECT_EQ(region.mapping.GetMemoryAsSpan<char>().first(kTestData.size()),
+ EXPECT_EQ(base::as_chars(base::span(region.mapping)).first(kTestData.size()),
kTestData);
}
@@ -29,7 +30,7 @@
auto region = base::WritableSharedMemoryRegion::Create(64);
auto mapping = region.Map();
const std::string kTestData = "Hello, world!";
- mapping.GetMemoryAsSpan<char>().copy_prefix_from(kTestData);
+ base::as_writable_chars(base::span(mapping)).copy_prefix_from(kTestData);
base::WritableSharedMemoryRegion writable_out;
ASSERT_TRUE(
@@ -37,7 +38,8 @@
mojo_base::mojom::WritableSharedMemoryRegion>(region, writable_out));
mapping = writable_out.Map();
- EXPECT_EQ(mapping.GetMemoryAsSpan<char>().first(kTestData.size()), kTestData);
+ EXPECT_EQ(base::as_chars(base::span(mapping)).first(kTestData.size()),
+ kTestData);
}
#endif // BUILDFLAG(USE_BLINK)
@@ -45,12 +47,13 @@
auto region = base::UnsafeSharedMemoryRegion::Create(64);
auto mapping = region.Map();
const std::string kTestData = "Hello, world!";
- mapping.GetMemoryAsSpan<char>().copy_prefix_from(kTestData);
+ base::as_writable_chars(base::span(mapping)).copy_prefix_from(kTestData);
base::UnsafeSharedMemoryRegion unsafe_out;
ASSERT_TRUE(mojo::test::SerializeAndDeserialize<
mojo_base::mojom::UnsafeSharedMemoryRegion>(region, unsafe_out));
mapping = unsafe_out.Map();
- EXPECT_EQ(mapping.GetMemoryAsSpan<char>().first(kTestData.size()), kTestData);
+ EXPECT_EQ(base::as_chars(base::span(mapping)).first(kTestData.size()),
+ kTestData);
}
```
```
The name of the file: mojo/public/cpp/platform/tests/platform_handle_unittest.cc
Insertions: 2, Deletions: 2.
@@ -158,7 +158,7 @@
PlatformHandle SetUpSharedMemory() {
auto region = base::UnsafeSharedMemoryRegion::Create(kTestData.size());
auto mapping = region.Map();
- mapping.GetMemoryAsSpan<char>().copy_from(kTestData);
+ base::as_writable_chars(base::span(mapping)).copy_from(kTestData);
auto generic_region =
base::UnsafeSharedMemoryRegion::TakeHandleForSerialization(
std::move(region));
@@ -184,7 +184,7 @@
auto region =
base::UnsafeSharedMemoryRegion::Deserialize(std::move(generic_region));
auto mapping = region.Map();
- std::string contents(base::as_string_view(mapping.GetMemoryAsSpan<char>()));
+ std::string contents(base::as_string_view(mapping));
// Let |handle| retain ownership.
generic_region = base::UnsafeSharedMemoryRegion::TakeHandleForSerialization(
```
Change information
Commit message:
Deprecate SharedMemoryRegion memory(), replace it with data() and spans
The accessor returns an unbounded void* which is dangerous to use.
Callers should instead use GetMemoryAsSpan() or GetMemoryAs(). To ease
this we introduce `uint8_t* data()` which allows SharedMemoryMappings
to convert implicitly or explicitly to base::span<uint8_t>.
We enable unsafe-buffer-usage warning in the shared memory unit tests,
which mostly required changing tests to use span apis instead of
working with the unbounded pointer accessor. The span apis
return the same pointer but with a length attached.
Other code is changed to span(mapping), GetMemoryAsSpan() or
GetMemoryAs(). These require the types being pulled out of shared
memory are trivially copyable. However a couple classes used in this
way in devices were _not_ trivially copyable. This can cause UB.
These classes wanted to be trivially copyable but could not be
because of the out-of-line ctor requirements of the chromium clang
plugin. So we template these and use a type alias to avoid rewriting
1000 LOC with useless template arguments. This works around the clang
plugin for now.
R=luk...@chromium.org
Bug: 40284755, 355003174
Change-Id: Id7fd4c9bcf0b86b8109134c18740453bb24aa5de
Reviewed-by: James Cook <jame...@chromium.org>
Reviewed-by: Łukasz Anforowicz <luk...@chromium.org>
Reviewed-by: Khushal Sagar <khusha...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Alex Moshchuk <ale...@chromium.org>
Reviewed-by: Avi Drissman <a...@chromium.org>
Owners-Override: Daniel Cheng <dch...@chromium.org>
Commit-Queue: danakj <dan...@chromium.org>
Reviewed-by: Reilly Grant <rei...@chromium.org>
Reviewed-by: Jonathan Ross <jon...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1333755}
Files:
- M ash/webui/camera_app_ui/camera_app_helper_impl.cc
- M base/android/library_loader/library_prefetcher_unittest.cc
- M base/memory/discardable_shared_memory.cc
- M base/memory/platform_shared_memory_region_unittest.cc
- M base/memory/read_only_shared_memory_region.cc
- M base/memory/shared_memory_mapping.h
- M base/memory/shared_memory_region_unittest.cc
- M base/trace_event/process_memory_dump_unittest.cc
- M cc/layers/heads_up_display_layer_impl.cc
- M cc/resources/cross_thread_shared_bitmap.h
- M cc/test/pixel_test.cc
- M chrome/browser/ash/app_list/search/local_image_search/image_content_annotator.cc
- M chrome/browser/ash/extensions/file_manager/image_loader_private_api.cc
- M chrome/browser/devtools/devtools_eye_dropper.cc
- M chrome/browser/enterprise/connectors/analysis/content_analysis_delegate_browsertest.cc
- M chrome/browser/enterprise/connectors/analysis/content_analysis_delegate_unittest.cc
- M content/browser/android/synchronous_compositor_host.cc
- M content/browser/devtools/devtools_video_consumer.cc
- M content/browser/font_access/font_enumeration_cache.cc
- M content/browser/font_unique_name_lookup/font_unique_name_lookup.cc
- M content/browser/renderer_host/render_process_host_impl.cc
- M content/renderer/pepper/ppb_buffer_impl.cc
- M content/renderer/pepper/ppb_image_data_impl.cc
- M device/gamepad/public/cpp/gamepad.cc
- M device/gamepad/public/cpp/gamepad.h
- M device/gamepad/public/mojom/gamepad_hardware_buffer.h
- M device/vr/orientation/orientation_device_provider_unittest.cc
- M device/vr/orientation/orientation_device_unittest.cc
- M mojo/core/ipcz_driver/shared_buffer_mapping.cc
- M mojo/core/platform_wrapper_unittest.cc
- M mojo/public/cpp/base/shared_memory_unittest.cc
- M mojo/public/cpp/base/shared_memory_version.cc
- M mojo/public/cpp/platform/tests/platform_handle_unittest.cc
- M services/audio/sync_reader.cc
- M services/audio/sync_reader_unittest.cc
- M services/device/generic_sensor/platform_sensor.h
- M services/device/generic_sensor/platform_sensor_and_provider_unittest.cc
- M services/device/generic_sensor/platform_sensor_and_provider_unittest_linux.cc
- M services/device/generic_sensor/platform_sensor_and_provider_unittest_win.cc
- M services/device/generic_sensor/platform_sensor_provider.cc
- M services/device/generic_sensor/sensor_provider_impl.cc
- M services/device/public/cpp/generic_sensor/sensor_reading_shared_buffer.cc
- M services/device/public/cpp/generic_sensor/sensor_reading_shared_buffer.h
- M services/device/public/cpp/generic_sensor/sensor_reading_shared_buffer_reader.cc
- M services/device/public/cpp/generic_sensor/sensor_reading_shared_buffer_reader.h
- M services/device/public/cpp/test/fake_sensor_and_provider.cc
- M services/device/public/cpp/test/fake_sensor_and_provider.h
- M services/test/echo/echo_service.cc
- M services/tracing/public/cpp/trace_startup.cc
- M services/tracing/public/cpp/trace_startup_config.cc
- M third_party/blink/common/font_unique_name_lookup/font_table_matcher.cc
- M third_party/blink/common/font_unique_name_lookup/font_table_persistence.cc
- M third_party/blink/renderer/modules/font_access/font_access.cc
- M third_party/blink/renderer/modules/gamepad/gamepad_dispatcher.h
- M third_party/blink/renderer/modules/gamepad/gamepad_listener.h
- M third_party/blink/renderer/modules/gamepad/gamepad_shared_memory_reader.cc
- M third_party/blink/renderer/modules/gamepad/gamepad_shared_memory_reader.h
- M third_party/blink/renderer/modules/gamepad/navigator_gamepad.h
- M third_party/blink/renderer/modules/xr/xr_input_source.h
- M third_party/blink/renderer/platform/graphics/canvas_resource.cc
- M third_party/blink/renderer/platform/widget/input/synchronous_compositor_proxy.cc
- M ui/ozone/common/gl_surface_egl_readback.cc
- M ui/ozone/common/gl_surface_egl_readback.h
- M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.cc
Change size: L
Delta: 64 files changed, 421 insertions(+), 336 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Coverage: +1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by Reilly Grant, +1 by Jonathan Ross, +1 by Alex Moshchuk, +1 by Daniel Cheng, +1 by James Cook, +1 by Khushal Sagar, +1 by Avi Drissman, +1 by Łukasz Anforowicz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages