Deleted some comments and cleaning up the code. CL formatted. [chromium/src : main]
Guido Urdaneta (Gerrit)
Guido Urdaneta added 16 comments![]( "Open in Gerrit")
Some comments to start polishing `SerializedDataForMessageEvent`.
bool iskDataTypeSerializedScriptValue = false;This field should be inside `SerializedDataForMessageEvent`, or maybe it shouldn't exist at all and can be a method in SDFME.
size_t MessageEvent::SizeOfExternalMemoryInBytes() {Remove this method.
void MessageEvent::RegisterAmountOfExternallyAllocatedMemory() {Remove this method. Just make sure it's invoked in all constructors of SDFME
void MessageEvent::UnregisterAmountOfExternallyAllocatedMemory() {Remove this method.
Just make sure it's invoked in the destructor of SDFME
MessageEvent::MessageEvent() {Initialize `serialized_data_` with initializer.
serialized_data_->dataAsV8Value().Set(initializer->data().GetIsolate(),This should be a single method `SetDataAsV8Value()` or maybe just `SetData()`
serialized_data_ = MakeGarbageCollected<SerializedDataForMessageEvent>();Initialize with initializer.
iskDataTypeSerializedScriptValue = true;Solve this without using this field.
serialized_data_ = MakeGarbageCollected<SerializedDataForMessageEvent>(data);Initialize with initializer. Maybe you can move the call to SSV::Unpack to here and make the destructor receive an `UnpackedSerializedScriptValue*` if that helps get rid of the `iskData...` field.
iskDataTypeSerializedScriptValue = true;Same comments here (initializer, remove field)
serialized_data_ = MakeGarbageCollected<SerializedDataForMessageEvent>(true);Initialize with initializer. We might want a different way to signal that it's null. Maybe we can use static CreateXXX methods.
serialized_data_->initMessageEvent(data);rename method to `InitWithScriptValue()`, or `SetDataAsScriptValue()` or just `SetData()`
serialized_data_->initMessageEvent(data);Same comment
serialized_data_->initMessageEvent(data);Similar comment
return serialized_data_->data(script_state, message_ports);We probably want to rename the `data()` method to something like `DeserializeUsingPorts()` or just `Deserialize()`.
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 1 comment![]( "Open in Gerrit")
return serialized_data_->data(script_state, message_ports);We probably want to rename the `data()` method to something like `DeserializeUsingPorts()` or just `Deserialize()`.
Just call the method `Deserialize()` and have it receive a `SerializedScriptValue::DeserializeOptions` as parameter.
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo added 15 comments![]( "Open in Gerrit")
This field should be inside `SerializedDataForMessageEvent`, or maybe it shouldn't exist at all and can be a method in SDFME.
Done
size_t MessageEvent::SizeOfExternalMemoryInBytes() {Lucía Alonso MozoRemove this method.
Done
void MessageEvent::RegisterAmountOfExternallyAllocatedMemory() {Remove this method. Just make sure it's invoked in all constructors of SDFME
Done
void MessageEvent::UnregisterAmountOfExternallyAllocatedMemory() {Remove this method.
Just make sure it's invoked in the destructor of SDFME
Done
MessageEvent::MessageEvent() {Lucía Alonso MozoInitialize `serialized_data_` with initializer.
Done
serialized_data_->dataAsV8Value().Set(initializer->data().GetIsolate(),This should be a single method `SetDataAsV8Value()` or maybe just `SetData()`
Done
serialized_data_ = MakeGarbageCollected<SerializedDataForMessageEvent>();Lucía Alonso MozoInitialize with initializer.
Done
Solve this without using this field.
Done
serialized_data_ = MakeGarbageCollected<SerializedDataForMessageEvent>(data);Initialize with initializer. Maybe you can move the call to SSV::Unpack to here and make the destructor receive an `UnpackedSerializedScriptValue*` if that helps get rid of the `iskData...` field.
Done
Same comments here (initializer, remove field)
Done
serialized_data_ = MakeGarbageCollected<SerializedDataForMessageEvent>(true);Initialize with initializer. We might want a different way to signal that it's null. Maybe we can use static CreateXXX methods.
Done
rename method to `InitWithScriptValue()`, or `SetDataAsScriptValue()` or just `SetData()`
As we also use this method to init the message event where data is a String and not just a Script value, I'm going to call it SetData().
serialized_data_->initMessageEvent(data);Lucía Alonso MozoSame comment
Done
serialized_data_->initMessageEvent(data);Similar comment
Why do we need the ScriptState::Current(isolate))?
return serialized_data_->data(script_state, message_ports);Guido UrdanetaWe probably want to rename the `data()` method to something like `DeserializeUsingPorts()` or just `Deserialize()`.
Just call the method `Deserialize()` and have it receive a `SerializedScriptValue::DeserializeOptions` as parameter.
- Guido Urdaneta
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 5 comments![]( "Open in Gerrit")
PostMessageOptions* options = PostMessageOptions::Create();Use `StructuredSerializedOptions` instead of `PostMessageOptions`.
void DedicatedWorker::PostEvent(Merge this into the other version of `PostEvent`. Change it to receive `StructuredSerializedOptions*` instead of `PostMessageOptions*`.
->StoreCurrentStackTrace("Worker.postMessage");rename this to `"Worker.PostEventWithData"`
// "devtools.timeline", "SchedulePostMessage", "data",Rename this to `SchedulePostEventWithData`
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo added 1 comment![]( "Open in Gerrit")
serialized_data_->initMessageEvent(data);Lucía Alonso MozoSimilar comment
Why do we need the ScriptState::Current(isolate))?
- Guido Urdaneta
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo added 5 comments![]( "Open in Gerrit")
PostMessageOptions* options = PostMessageOptions::Create();Use `StructuredSerializedOptions` instead of `PostMessageOptions`.
Done
Merge this into the other version of `PostEvent`. Change it to receive `StructuredSerializedOptions*` instead of `PostMessageOptions*`.
Done
->StoreCurrentStackTrace("Worker.postMessage");Lucía Alonso Mozorename this to `"Worker.PostEventWithData"`
Done
// TRACE_EVENT_INSTANT(Lucía Alonso Mozouncomment this.
Done
// "devtools.timeline", "SchedulePostMessage", "data",Lucía Alonso MozoRename this to `SchedulePostEventWithData`
Done
Related details
- Guido Urdaneta
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 10 comments![]( "Open in Gerrit")
void RegisterAmountOfExternallyAllocatedMemory();These can be private
size_t SizeOfExternalMemoryInBytes();This one can probably be private
// size_t amount_of_external_memory_ = 0;remove this commented line
bool IsDataDirty() const { return serialized_data_->IsDataDirty(); }It looks like this method is not used at all. If that's the case, delete it and also any other methods changed to delegate to SDFE that are not used anymore.
void DedicatedWorker::PostEventWithData(Remove this one and merge it to the other one.
// DEFINE_ATTRIBUTE_EVENT_LISTENER(rtctransform, kRtctransform)Delete this commented line
// attribute EventHandler onrtctransform;delete commented line
CrossThreadFunction<Event*(ScriptState* script_state)> error_callback,rename to `event_factory_error_callback` (It's also an event factory).
Apply to other places that receive this callback.
CrossThreadFunction<Event*(ScriptState* script_state,Remove the names for the parameters inside the callback type to avoid confusion with the actual parameter names of `ReceiveEventWithData`.
Do it in other places that receive the callback too.
ImplementedAs=DedicatedWorkerGlobalScopeRTCTransformWhat name do you need to use if you don't use ImplementedAs?
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo added 9 comments![]( "Open in Gerrit")
void RegisterAmountOfExternallyAllocatedMemory();Lucía Alonso MozoThese can be private
Done
size_t SizeOfExternalMemoryInBytes();This one can probably be private
Done
// size_t amount_of_external_memory_ = 0;Lucía Alonso Mozoremove this commented line
Done
bool IsDataDirty() const { return serialized_data_->IsDataDirty(); }It looks like this method is not used at all. If that's the case, delete it and also any other methods changed to delegate to SDFE that are not used anymore.
If I delete it, I get this comment: gen/third_party/blink/renderer/bindings/core/v8/v8_message_event.cc:102:22: error: no member named 'IsDataDirty' in 'blink::MessageEvent'
102 | if (!blink_receiver->IsDataDirty()) {
It's being used from JS.
void DedicatedWorker::PostEventWithData(Remove this one and merge it to the other one.
Done
// DEFINE_ATTRIBUTE_EVENT_LISTENER(rtctransform, kRtctransform)Lucía Alonso MozoDelete this commented line
Done
// attribute EventHandler onrtctransform;Lucía Alonso Mozodelete commented line
Done
CrossThreadFunction<Event*(ScriptState* script_state)> error_callback,rename to `event_factory_error_callback` (It's also an event factory).
Apply to other places that receive this callback.
Done
CrossThreadFunction<Event*(ScriptState* script_state,Remove the names for the parameters inside the callback type to avoid confusion with the actual parameter names of `ReceiveEventWithData`.
Do it in other places that receive the callback too.
- Guido Urdaneta
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo added 1 comment![]( "Open in Gerrit")
ImplementedAs=DedicatedWorkerGlobalScopeRTCTransformWhat name do you need to use if you don't use ImplementedAs?
Couldn't find the name so I'm going to leave it with the implemented as.
Related details
- Guido Urdaneta
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lucía Alonso Mozo
| Commit-Queue | +1 |
1 comment
ImplementedAs=DedicatedWorkerGlobalScopeRTCTransformLucía Alonso MozoWhat name do you need to use if you don't use ImplementedAs?
Couldn't find the name so I'm going to leave it with the implemented as.
- Guido Urdaneta
- Lucía Alonso Mozo
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 9 comments![]( "Open in Gerrit")
size_t SizeOfExternalMemoryInBytes();Declare methods before fields.
void SerializedDataForEvent::SetData(const String& data) {You have to add tests for this method (see that the lines are orange).
return false;Try to add a test that exercises this branch
case kDataTypeBlob:Try to add tests that exercise these branches
Member<SerializedDataForEvent> serialized_data_ =Can't use Member as local variable. Use `SerializedDataForEvent*`. Fix everywhere.
MakeGarbageCollected<SerializedDataForEvent>(nullptr);Use the `CreateNull` static method which automatically also exercises the constructor
scoped_refptr<SerializedScriptValue> serializedScriptValue =why 2 serialized_script_values? also, this is not the right naming style for variables.
DCHECK(!GetExecutionContext() || GetExecutionContext()->IsContextThread());Move these checks to the beginning.
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo added 9 comments![]( "Open in Gerrit")
size_t SizeOfExternalMemoryInBytes();Lucía Alonso MozoDeclare methods before fields.
Done
void SerializedDataForEvent::SetData(const String& data) {You have to add tests for this method (see that the lines are orange).
Done
return false;Try to add a test that exercises this branch
Done
case kDataTypeBlob:Try to add tests that exercise these branches
For next iteration.
Member<SerializedDataForEvent> serialized_data_ =Can't use Member as local variable. Use `SerializedDataForEvent*`. Fix everywhere.
Done
MakeGarbageCollected<SerializedDataForEvent>(nullptr);Use the `CreateNull` static method which automatically also exercises the constructor
Done
// Check empty constructorLucía Alonso Mozoremove comment
Done
scoped_refptr<SerializedScriptValue> serializedScriptValue =why 2 serialized_script_values? also, this is not the right naming style for variables.
Done
DCHECK(!GetExecutionContext() || GetExecutionContext()->IsContextThread());Move these checks to the beginning.
- Guido Urdaneta
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 1 comment![]( "Open in Gerrit")
HeapVector<ScriptValue> transfer = {};I think you need to add `script_value` to transfer to indicate that you are transferring (not copying) script_value.
Also, you should probably use something other than an event. It would look confusing that you're going to fire an event that has an event as data/options.
Use some other type. Try all possibilities (null, integer, string, some other complex type that is transferable).
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 2 comments![]( "Open in Gerrit")
} else if (message.message && message.message->CanDeserializeIn(this)) {at this point message.message is guaranteed to not be null, so you can remove that.
#include "third_party/blink/renderer/modules/peerconnection/rtc_rtp_script_transform.h"As discussed offline, let's remove these tests and just use the more comprehensive JS tests.
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo voted and added 3 comments![]( "Open in Gerrit")
Votes added by Lucía Alonso Mozo
| Commit-Queue | +1 |
3 comments
} else if (message.message && message.message->CanDeserializeIn(this)) {at this point message.message is guaranteed to not be null, so you can remove that.
Done
#include "third_party/blink/renderer/modules/peerconnection/rtc_rtp_script_transform.h"As discussed offline, let's remove these tests and just use the more comprehensive JS tests.
Done
I think you need to add `script_value` to transfer to indicate that you are transferring (not copying) script_value.
Also, you should probably use something other than an event. It would look confusing that you're going to fire an event that has an event as data/options.
Use some other type. Try all possibilities (null, integer, string, some other complex type that is transferable).
- Guido Urdaneta
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 11 comments![]( "Open in Gerrit")
class CORE_EXPORT SerializedDataForEventThis file (and the corresponding .cc ones) should be in core/events
return false;Add test coverage to this path
return serialized_data_->IsLockedToAgentCluster(locked_to_agent_cluster_);The `if (locked_to_agent_cluster_)` line should stay.
locked_to_agent_cluster_ = true;Do you need to forward this to SerializeData?
serialized_data_->AssociateWithWrapper(isolate, wrapper);remind me to double-check about the proper way to pass `wrapper` around
ExceptionState& exception_state) {We should cover all this with unit tests. Didn't you have them?
EventWithDataInfo data;Find a way to reliably cover this with tests.
PostMessageToWorkerGlobalScope version has this covered, so it should be possible to follow the same pattern.
PostCrossThreadTask(Same here, but I guess covering the previous one will cover this one as well.
Event* event = event_factory_callback.Run(Add coverage to this path
ScriptController()->GetScriptState(), nullptr);Add `/*param_name=*/` comment before nullptr
ScriptPromise<IDLUndefined> promise = EmptyPromise();Can this be done in a single line, without a temporary variable?
Also applies to `generateKeyFrame()`
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo voted and added 9 comments![]( "Open in Gerrit")
Votes added by Lucía Alonso Mozo
| Commit-Queue | +1 |
9 comments
This file (and the corresponding .cc ones) should be in core/events
Talked on Google Chat that it's better to do move it to core/workers.
Add test coverage to this path
Done
return serialized_data_->IsLockedToAgentCluster(locked_to_agent_cluster_);The `if (locked_to_agent_cluster_)` line should stay.
Done
if (locked_to_agent_cluster_) {do I add a test for this? they didn't have one before and it's not really code that I implemented.
Do you need to forward this to SerializeData?
Done
PostCrossThreadTask(Same here, but I guess covering the previous one will cover this one as well.
they also don't cover this part of the tests? (in the for(auto& task: tasks))
Add coverage to this path
Done
Add `/*param_name=*/` comment before nullptr
Done
ScriptPromise<IDLUndefined> promise = EmptyPromise();Can this be done in a single line, without a temporary variable?
Also applies to `generateKeyFrame()`
- Guido Urdaneta
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo added 3 comments![]( "Open in Gerrit")
We should cover all this with unit tests. Didn't you have them?
Done
Find a way to reliably cover this with tests.
PostMessageToWorkerGlobalScope version has this covered, so it should be possible to follow the same pattern.
Done
Lucía Alonso MozoSame here, but I guess covering the previous one will cover this one as well.
they also don't cover this part of the tests? (in the for(auto& task: tasks))
Done
Related details
- Guido Urdaneta
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 11 comments![]( "Open in Gerrit")
Haven't reviewed everything, but some comments here.
if (locked_to_agent_cluster_) {do I add a test for this? they didn't have one before and it's not really code that I implemented.
Yes, please.
You need at least a test for SerializedDataForEvent
struct EventWithDataInfo {Rename
void CreateWaitForEvent(DedicatedWorkerThreadForTest* worker_thread,Rename this function to `WaitForMessageOrMessageErrorEvent`.
void CreateWaitForEvent(DedicatedWorkerThreadForTest* worker_thread,Put all these utility functions in an unnamed namespace at the top.
WTF::CrossThreadOnceClosure quit_1,rename `quit_1` to `listeners_registered_closure`
rename `quit_2` to `event_fired_closure`.
Event* event = MakeGarbageCollected<MessageEvent>("data", "origin");You should create an event that uses `data` so that you can verify in the test that `data` is passed correctly.
And update a test to verify that.
TEST_F(DedicatedWorkerTest, DispatchEventWithDataOnWorkerGlobalScope) {Move the new tests to the end of the file
base::RunLoop run_loop_1;The whole code with the 2 RunLoops and the waits looks reusable across tests.
bool OverrideCanSerialize(DedicatedWorkerThreadForTest* worker_thread,Move to unnamed namespace at the top.
ScriptValue CreateScriptValue(ScriptState* script_state) {Move utility functions to unnamed namespace at the top
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Warning: gwsq did not assign any reviewers.
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo added 10 comments![]( "Open in Gerrit")
if (locked_to_agent_cluster_) {Guido Urdanetado I add a test for this? they didn't have one before and it's not really code that I implemented.
Yes, please.
You need at least a test for SerializedDataForEvent
Done
struct EventWithDataInfo {Rename
CustomEventInfo
void CreateWaitForEvent(DedicatedWorkerThreadForTest* worker_thread,Put all these utility functions in an unnamed namespace at the top.
it can't be at the top because some definitions are after. I moved it the more top I could.
void CreateWaitForEvent(DedicatedWorkerThreadForTest* worker_thread,Rename this function to `WaitForMessageOrMessageErrorEvent`.
Done
rename `quit_1` to `listeners_registered_closure`
rename `quit_2` to `event_fired_closure`.
Done
Event* event = MakeGarbageCollected<MessageEvent>("data", "origin");You should create an event that uses `data` so that you can verify in the test that `data` is passed correctly.
And update a test to verify that.
Done
TEST_F(DedicatedWorkerTest, DispatchEventWithDataOnWorkerGlobalScope) {Move the new tests to the end of the file
Done
The whole code with the 2 RunLoops and the waits looks reusable across tests.
Done
bool OverrideCanSerialize(DedicatedWorkerThreadForTest* worker_thread,Move to unnamed namespace at the top.
Done
ScriptValue CreateScriptValue(ScriptState* script_state) {Move utility functions to unnamed namespace at the top
- Guido Urdaneta
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 27 comments![]( "Open in Gerrit")
void PostEventWithData(Let's rename to PostCustomEvent
->StoreCurrentStackTrace("Worker.PostEventWithData");PostCustomEvent
"devtools.timeline", "SchedulePostEventWithData", "data",SchedulePostCustomEvent
perfetto::Flow::Global(trace_id)); // SchedulePostEventWithDataSchedulePostCustomEvent
EventWithDataInfo data;You can intialize in place
void ProcessEventWithDataFromWorkerObject(rename to ProcessCustomEventFromWorkerObject
Event* CreateEventWithDataNoMessage(ScriptState* script_state,Rename to CreateMessageEvent.
Event* event = MakeGarbageCollected<MessageEvent>("noMessage", "origin");Is there a Create() method variant you can use?
Event* CreateErrorEvent(ScriptState* script_state) {Rename to CreateMessageErrorEvent
Event* CreateEventWithData(ScriptState* script_state,Rename to CreateMessageEventWithData
void WaitForMessageOrMessageErrorEvent(Rename to `RegisterMessageEventListener`
WTF::CrossThreadOnceClosure listeners_registered_closure,rename to `listener_registered_closure`
WTF::CrossThreadOnceClosure event_fired_closure) {rename to `event_listener`
WTF::CrossThreadOnceClosure quit_closure) {rename this to `event_listener_closure` or just `event_listener`.
EXPECT_EQ(execution_context, worker_thread->GlobalScope());Normally it's better to have expectations in the tests themselves, so probably better to re-inline this.
CreateCallbackForPostEventWithData(base::RunLoop& run_loop) {Receive a `base::RepeatingClosure quit_closure` instead of the RunLoop.
Invoke the quit_closure instead of `run_loop.Quit()`.
From the tests, pass `run_loop.QuitClosure()` instead of the `run_loop`.
CreateCallbackForPostEventWithData(base::RunLoop& run_loop) {Rename to `CreateCallbackForPostCustomEvent`
CreateErrorCallbackForPostEventWithData(base::RunLoop& run_loop) {Rename to `CreateErrorCallbackForPostCustomEvent`
void DedicatedWorkerTest::CreateAndPostMessage(WaitForEvent** wait) {You can define this inside the class.
void DedicatedWorkerTest::CreateAndPostMessage(WaitForEvent** wait) {WaitForEvent should be an input, not an output.
It should be created by the tests and pass via a single pointer.
However, see other message below about `CreateAndPostMessage()`
CreateAndPostMessage(&wait);I see that `CreateAndPostMessage()` is used only by existing tests.
Probably better to revert if you're not using it in new tests.
CrossThreadFunction<void(BlinkTransferableMessage)> handle_event_callback) {Use `base::OnceCallback`, since this function is not called across threads (i.e., with `PostCrossThreadTask`).
You can use BindLambdaForTesting directly (no need to wrap in a CrossThreadFunction).
CrossThreadFunction<void(BlinkTransferableMessage)> handle_event_callback) {handle_event_callback is an input, so it should be first.
You can remove `event_type` since it's included in `event`
CrossThreadFunction<void(BlinkTransferableMessage)> handle_event_callback) {rename `handle_event_callback` to `post_event_callback` since it is this callback what actually posts the event. Nothing else in this function posts an event in the worker.
CreateAndDispatchEvent(&event, &event_type, std::move(handle_event_callback));You can pass the callback inline ( no need for a single-use variable). And no need to wrap in CrossThreadFunction if you use base::OnceCallback (or base::RepeatingCallback)
CreateAndDispatchEvent(&event, &event_type, std::move(handle_event_callback));You can create the callback directly in the call (and if you switch to `base::OnceCallback`, you shouldn't need to wrap in a `CrossThreadFunction`.
TEST_F(DedicatedWorkerTest, postEventWithData) {These should start with uppercase
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lucía Alonso Mozo (Gerrit)
Lucía Alonso Mozo added 29 comments![]( "Open in Gerrit")
serialized_data_->AssociateWithWrapper(isolate, wrapper);remind me to double-check about the proper way to pass `wrapper` around
Done
Let's rename to PostCustomEvent
Done
->StoreCurrentStackTrace("Worker.PostEventWithData");Lucía Alonso MozoPostCustomEvent
Done
"devtools.timeline", "SchedulePostEventWithData", "data",Lucía Alonso MozoSchedulePostCustomEvent
Done
perfetto::Flow::Global(trace_id)); // SchedulePostEventWithDataLucía Alonso MozoSchedulePostCustomEvent
Done
struct EventWithDataInfo {Lucía Alonso MozoRename
CustomEventInfo
Done
You can intialize in place
Done
void ProcessEventWithDataFromWorkerObject(Lucía Alonso Mozorename to ProcessCustomEventFromWorkerObject
Done
Event* CreateEventWithDataNoMessage(ScriptState* script_state,Lucía Alonso MozoRename to CreateMessageEvent.
Done
Event* event = MakeGarbageCollected<MessageEvent>("noMessage", "origin");Is there a Create() method variant you can use?
Done
Event* CreateErrorEvent(ScriptState* script_state) {Lucía Alonso MozoRename to CreateMessageErrorEvent
Done
Event* CreateEventWithData(ScriptState* script_state,Lucía Alonso MozoRename to CreateMessageEventWithData
Done
void WaitForMessageOrMessageErrorEvent(Lucía Alonso MozoRename to `RegisterMessageEventListener`
Done
WTF::CrossThreadOnceClosure listeners_registered_closure,Lucía Alonso Mozorename to `listener_registered_closure`
Done
WTF::CrossThreadOnceClosure event_fired_closure) {Lucía Alonso Mozorename to `event_listener`
Done
rename this to `event_listener_closure` or just `event_listener`.
Done
EXPECT_EQ(execution_context, worker_thread->GlobalScope());Normally it's better to have expectations in the tests themselves, so probably better to re-inline this.
Done
CreateCallbackForPostEventWithData(base::RunLoop& run_loop) {Lucía Alonso MozoRename to `CreateCallbackForPostCustomEvent`
Done
CreateCallbackForPostEventWithData(base::RunLoop& run_loop) {Receive a `base::RepeatingClosure quit_closure` instead of the RunLoop.
Invoke the quit_closure instead of `run_loop.Quit()`.
From the tests, pass `run_loop.QuitClosure()` instead of the `run_loop`.
Not sure how to pass it
CreateErrorCallbackForPostEventWithData(base::RunLoop& run_loop) {Lucía Alonso MozoRename to `CreateErrorCallbackForPostCustomEvent`
Done
void DedicatedWorkerTest::CreateAndPostMessage(WaitForEvent** wait) {WaitForEvent should be an input, not an output.
It should be created by the tests and pass via a single pointer.However, see other message below about `CreateAndPostMessage()`
I deleted the function because of the other comment.
void DedicatedWorkerTest::CreateAndPostMessage(WaitForEvent** wait) {You can define this inside the class.
Done
I see that `CreateAndPostMessage()` is used only by existing tests.
Probably better to revert if you're not using it in new tests.
Done
CrossThreadFunction<void(BlinkTransferableMessage)> handle_event_callback) {rename `handle_event_callback` to `post_event_callback` since it is this callback what actually posts the event. Nothing else in this function posts an event in the worker.
Done
CrossThreadFunction<void(BlinkTransferableMessage)> handle_event_callback) {Use `base::OnceCallback`, since this function is not called across threads (i.e., with `PostCrossThreadTask`).
You can use BindLambdaForTesting directly (no need to wrap in a CrossThreadFunction).
Done
CrossThreadFunction<void(BlinkTransferableMessage)> handle_event_callback) {handle_event_callback is an input, so it should be first.
You can remove `event_type` since it's included in `event`
Done
CreateAndDispatchEvent(&event, &event_type, std::move(handle_event_callback));You can pass the callback inline ( no need for a single-use variable). And no need to wrap in CrossThreadFunction if you use base::OnceCallback (or base::RepeatingCallback)
Done
CreateAndDispatchEvent(&event, &event_type, std::move(handle_event_callback));You can create the callback directly in the call (and if you switch to `base::OnceCallback`, you shouldn't need to wrap in a `CrossThreadFunction`.
Done
These should start with uppercase
- Guido Urdaneta
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guido Urdaneta (Gerrit)
Guido Urdaneta added 1 comment![]( "Open in Gerrit")
name: "RTCRtpEncodedTransforms",Rename to RTCRtpScriptTransform
Related details
- Lucía Alonso Mozo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |