[Blink] Add CloneableMessage support to WebSerializedScriptValue [chromium/src : main]
Justin Lulejian (Gerrit)
Justin Lulejian added 2 comments![]( "Open in Gerrit")
Hi Dave! I realized that BigBuffer will send many things except Blob so switching to CloneableMessage will give us that.
// Converts a `blink::CloneableMessage` (Mojo type in public/common) to a
// `mojom::blink::CloneableMessagePtr` (Blink variant Mojo type).
//
// Note: This conversion does not include fields like `sender_origin`,
// `stack_trace`, or `file_system_access_tokens` as they are not currently
// needed for this use case. It only populates `encoded_message`,
// `sender_agent_cluster_id`, `locked_to_sender_agent_cluster`, and `blobs`.
// This partial conversion is sufficient for the current purposes of
// WebSerializedScriptValue and specifically the extension messaging use-case.
mojom::blink::CloneableMessagePtr ToMojoCloneableMessage(
CloneableMessage input) {
auto output = mojom::blink::CloneableMessage::New();
output->encoded_message = mojo_base::BigBuffer(input.encoded_message);
output->sender_agent_cluster_id = input.sender_agent_cluster_id;
output->locked_to_sender_agent_cluster = input.locked_to_sender_agent_cluster;
for (auto& blob : input.blobs) {
output->blobs.push_back(
BlobDataHandle::Create(String::FromUTF8(blob->uuid),
String::FromUTF8(blob->content_type), blob->size,
mojo::PendingRemote<mojom::blink::Blob>(
std::move(blob->blob).PassPipe(), 0)));
}
return output;
}
// Converts a `mojom::blink::CloneableMessagePtr` (Blink variant Mojo type) to a
// `blink::CloneableMessage` (Mojo type in public/common).
//
// Note: This conversion only populates the basic fields needed for structured
// clone: `encoded_message`, `sender_agent_cluster_id`,
// `locked_to_sender_agent_cluster`, and `blobs`. Other fields in
// `blink::CloneableMessage` are left with their default values. This partial
// conversion is sufficient for the current purposes of
// WebSerializedScriptValue and specifically the extension messaging use-case.
CloneableMessage ToCloneableMessage(mojom::blink::CloneableMessagePtr input) {
CloneableMessage output;
auto encoded_span = base::span(input->encoded_message);
output.owned_encoded_message =
std::vector<uint8_t>(encoded_span.begin(), encoded_span.end());
output.encoded_message = output.owned_encoded_message;
output.sender_agent_cluster_id = input->sender_agent_cluster_id;
output.locked_to_sender_agent_cluster = input->locked_to_sender_agent_cluster;
for (auto& blob_handle : input->blobs) {
output.blobs.push_back(mojom::SerializedBlob::New(
blob_handle->Uuid().Utf8(), blob_handle->GetType().Utf8(),
blob_handle->size(),
mojo::PendingRemote<mojom::Blob>(
blob_handle->CloneBlobRemote().PassPipe(), 0)));
}
return output;
}I created ToMojoCloneableMessage/ToCloneableMessage at this level to keep the //extensions-specific logic as as high-level as possible, but since WebSerializedScriptValue seems to be more of a wrapper around SerializedScriptValue, I'd be happy to move it down to the SerializedScriptValue class itself if that is more appropriate. Then we don't need the new unit test file either.
Related details
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
bool locked_to_sender_agent_cluster_ = false;nit: Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”. Consider renaming 'locked_to_sender_agent_cluster_' to 'is_locked_to_sender_agent_cluster_'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Dave Tapuska
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. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
base::UnguessableToken sender_agent_cluster_id_;Where are these values set and used?
How does this relate to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/serialization/serialized_script_value.h;l=311;bpv=1;bpt=1
?
Related details
- Justin Lulejian
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. |
Justin Lulejian (Gerrit)
Justin Lulejian added 2 comments![]( "Open in Gerrit")
nit: Blink Style Guide: Naming - Precede boolean values with words like “is” and “did”. Consider renaming 'locked_to_sender_agent_cluster_' to 'is_locked_to_sender_agent_cluster_'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Where are these values set and used?
How does this relate to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/serialization/serialized_script_value.h;l=311;bpv=1;bpt=1
?
Realized that while sender_agent_cluster_id has to be populated to satisfy the mojom serializer, it's not something we use/need and so we don't need to store this. Changed it to just set a value when converting from/to CloneableMessage.
Related details
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 2 comments![]( "Open in Gerrit")
mojom::blink::CloneableMessagePtr ToMojoCloneableMessage(I'm confused. why can't we just the mojo message traits?
Why do we need specific implementations here?
// The `CloneableMessage` mojo definition requires `sender_agent_cluster_id`I feel this comment should be in SerializedScriptValue::GetCloneableMessage
Related details
- Justin Lulejian
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. |
Justin Lulejian (Gerrit)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
mojom::blink::CloneableMessagePtr ToMojoCloneableMessage(I'm confused. why can't we just the mojo message traits?
Why do we need specific implementations here?
Could you clarify which mojo message traits you think we could use? This [one](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/messaging/cloneable_message_mojom_traits.h)?
Related details
- Dave Tapuska
- Justin Lulejian
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. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
mojom::blink::CloneableMessagePtr ToMojoCloneableMessage(Justin LulejianI'm confused. why can't we just the mojo message traits?
Why do we need specific implementations here?
Could you clarify which mojo message traits you think we could use? This [one](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/messaging/cloneable_message_mojom_traits.h)?
- Justin Lulejian
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. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
mojom::blink::CloneableMessagePtr ToMojoCloneableMessage(Justin LulejianI'm confused. why can't we just the mojo message traits?
Why do we need specific implementations here?
Dave TapuskaCould you clarify which mojo message traits you think we could use? This [one](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/messaging/cloneable_message_mojom_traits.h)?
Yes
Or why do we need to convert
third_party/blink/renderer/bindings/core/v8/serialization/serialized_script_value.h
to/from mojom can't it use
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/messaging/cloneable_message.h?q=CloneableMessage%20third_party%2Fblink%20-path:out&ss=chromium%2Fchromium%2Fsrc
Is it layering?
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 2 comments![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Commit-Queue | +1 |
2 comments
mojom::blink::CloneableMessagePtr ToMojoCloneableMessage(Justin LulejianI'm confused. why can't we just the mojo message traits?
Why do we need specific implementations here?
Dave TapuskaCould you clarify which mojo message traits you think we could use? This [one](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/messaging/cloneable_message_mojom_traits.h)?
Dave TapuskaYes
Or why do we need to convert
third_party/blink/renderer/bindings/core/v8/serialization/serialized_script_value.h
to/from mojom can't it use
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/messaging/cloneable_message.h?q=CloneableMessage%20third_party%2Fblink%20-path:out&ss=chromium%2Fchromium%2FsrcIs it layering?
Thanks! Was able to use traits, but it required adding some missing includes into third_party/blink/public/common/messaging/cloneable_message_mojom_traits.h and third_party/blink/renderer/platform/blob/serialized_blob_mojom_traits.h to fix compile errors. I moved the impl down to SerializedScriptValue vs WebSerializedScriptValue (and the tests) to keep the current design intact. PTAL.
// The `CloneableMessage` mojo definition requires `sender_agent_cluster_id`I feel this comment should be in SerializedScriptValue::GetCloneableMessage
I moved the essence of this comment to WebSerializedScriptValue since that is where we pass the ID in now.
When thinking about it I don't think we should restrict callers to have to accept a randomly generated ID from this impl if they wanted to specify it, but for the current purposes it is a randomly generated token (but provided by the //extensions layer to keep that detail out of the blink layer). LMK if it would be better to generated it inside of blink though.
Related details
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
mojom::blink::CloneableMessagePtr ToMojoCloneableMessage(Justin LulejianI'm confused. why can't we just the mojo message traits?
Why do we need specific implementations here?
Dave TapuskaCould you clarify which mojo message traits you think we could use? This [one](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/messaging/cloneable_message_mojom_traits.h)?
Dave TapuskaYes
Justin LulejianOr why do we need to convert
third_party/blink/renderer/bindings/core/v8/serialization/serialized_script_value.h
to/from mojom can't it use
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/common/messaging/cloneable_message.h?q=CloneableMessage%20third_party%2Fblink%20-path:out&ss=chromium%2Fchromium%2FsrcIs it layering?
Thanks! Was able to use traits, but it required adding some missing includes into third_party/blink/public/common/messaging/cloneable_message_mojom_traits.h and third_party/blink/renderer/platform/blob/serialized_blob_mojom_traits.h to fix compile errors. I moved the impl down to SerializedScriptValue vs WebSerializedScriptValue (and the tests) to keep the current design intact. PTAL.
I moved the impl back up to WebSerializedScriptValue from SerializedScriptValue since it balloons the compile-size to add the necessary includes for the mojom traits.
Related details
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |