Add RECV_ECN socket option in P2PSocket interface [chromium/src : main]
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
@dch...@chromium.org Seems like the test case failure got fixed. Could you please review the latest version of CL once again?
Related details
- Daniel Cheng
- Per Kjellander
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve added 1 comment![]( "Open in Gerrit")
"//gin:v8_snapshot_assets",I don't think we should add a dep onto //gin from //testing. Which tests need this file?
Related details
- Daniel Cheng
- Per Kjellander
- Sun Shin US
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
"//gin:v8_snapshot_assets",I don't think we should add a dep onto //gin from //testing. Which tests need this file?
@andrew thanks for the review the code. The reason why I added the v8_snapshot_assets in here is that I have added the V8 initialization for the new ipc_socket_factory_test as @dcheng suggested through <https://chromium-review.googlesource.com/c/chromium/src/+/5545427/comment/343c1500_3e06a45b/> comment. And I have added the v8_snapshot dependency on the third_party/blink/renderer/platform/BUILD.gn for this. However, in case of Android blink_platform_unittests couldn't read the v8_snapshot assets unless I have added the dependency to the native_test build configuration. So I have added the dependency in here. If there is a better way to use the v8_snapshot, please advise me, then I will update the CL.
Related details
- Andrew Grieve
- Daniel Cheng
- Per Kjellander
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve added 1 comment![]( "Open in Gerrit")
"//gin:v8_snapshot_assets",Sun Shin USI don't think we should add a dep onto //gin from //testing. Which tests need this file?
@andrew thanks for the review the code. The reason why I added the v8_snapshot_assets in here is that I have added the V8 initialization for the new ipc_socket_factory_test as @dcheng suggested through <https://chromium-review.googlesource.com/c/chromium/src/+/5545427/comment/343c1500_3e06a45b/> comment. And I have added the v8_snapshot dependency on the third_party/blink/renderer/platform/BUILD.gn for this. However, in case of Android blink_platform_unittests couldn't read the v8_snapshot assets unless I have added the dependency to the native_test build configuration. So I have added the dependency in here. If there is a better way to use the v8_snapshot, please advise me, then I will update the CL.
Might be enough to add it to `blink_platform_unittests`. I'd try that first.
Related details
- Daniel Cheng
- Per Kjellander
- Sun Shin US
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
"//gin:v8_snapshot_assets",Sun Shin USI don't think we should add a dep onto //gin from //testing. Which tests need this file?
Andrew Grieve@andrew thanks for the review the code. The reason why I added the v8_snapshot_assets in here is that I have added the V8 initialization for the new ipc_socket_factory_test as @dcheng suggested through <https://chromium-review.googlesource.com/c/chromium/src/+/5545427/comment/343c1500_3e06a45b/> comment. And I have added the v8_snapshot dependency on the third_party/blink/renderer/platform/BUILD.gn for this. However, in case of Android blink_platform_unittests couldn't read the v8_snapshot assets unless I have added the dependency to the native_test build configuration. So I have added the dependency in here. If there is a better way to use the v8_snapshot, please advise me, then I will update the CL.
Might be enough to add it to `blink_platform_unittests`. I'd try that first.
- Andrew Grieve
- Daniel Cheng
- Per Kjellander
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
"//gin:v8_snapshot_assets",Sun Shin USI don't think we should add a dep onto //gin from //testing. Which tests need this file?
Andrew Grieve@andrew thanks for the review the code. The reason why I added the v8_snapshot_assets in here is that I have added the V8 initialization for the new ipc_socket_factory_test as @dcheng suggested through <https://chromium-review.googlesource.com/c/chromium/src/+/5545427/comment/343c1500_3e06a45b/> comment. And I have added the v8_snapshot dependency on the third_party/blink/renderer/platform/BUILD.gn for this. However, in case of Android blink_platform_unittests couldn't read the v8_snapshot assets unless I have added the dependency to the native_test build configuration. So I have added the dependency in here. If there is a better way to use the v8_snapshot, please advise me, then I will update the CL.
Sun Shin USMight be enough to add it to `blink_platform_unittests`. I'd try that first.
Thanks. let me try this.
It seems adding the dependency is not allowed:
Step analyze failed. Error logs are shown below:
ERROR Dependency cycle:
//testing/android/native_test:native_test_java__header ->
//third_party/blink/renderer/platform:blink_platform_unittests ->
//third_party/blink/renderer/platform:blink_platform_unittests__apk ->
//third_party/blink/renderer/platform:blink_platform_unittests__apk__create ->
//testing/android/native_test:native_test_java ->
//testing/android/native_test:native_test_java__dex ->
//testing/android/native_test:native_test_java__compile_java ->
//testing/android/native_test:native_test_java__header
Related details
- Andrew Grieve
- Daniel Cheng
- Per Kjellander
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
"//gin:v8_snapshot_assets",Sun Shin USI don't think we should add a dep onto //gin from //testing. Which tests need this file?
Andrew Grieve@andrew thanks for the review the code. The reason why I added the v8_snapshot_assets in here is that I have added the V8 initialization for the new ipc_socket_factory_test as @dcheng suggested through <https://chromium-review.googlesource.com/c/chromium/src/+/5545427/comment/343c1500_3e06a45b/> comment. And I have added the v8_snapshot dependency on the third_party/blink/renderer/platform/BUILD.gn for this. However, in case of Android blink_platform_unittests couldn't read the v8_snapshot assets unless I have added the dependency to the native_test build configuration. So I have added the dependency in here. If there is a better way to use the v8_snapshot, please advise me, then I will update the CL.
Sun Shin USMight be enough to add it to `blink_platform_unittests`. I'd try that first.
Sun Shin USThanks. let me try this.
It seems adding the dependency is not allowed:
Step analyze failed. Error logs are shown below:
ERROR Dependency cycle:
//testing/android/native_test:native_test_java__header ->
//third_party/blink/renderer/platform:blink_platform_unittests ->
//third_party/blink/renderer/platform:blink_platform_unittests__apk ->
//third_party/blink/renderer/platform:blink_platform_unittests__apk__create ->
//testing/android/native_test:native_test_java ->
//testing/android/native_test:native_test_java__dex ->
//testing/android/native_test:native_test_java__compile_java ->
//testing/android/native_test:native_test_java__header
let me try some other configurations as well.
Sun Shin US (Gerrit)
@agr...@chromium.org I tried several options which are not relying on the "gin" module but couldn't find the resolution. It this is not a hard requirement, could we have the dependency at this time and resolve it in a separate CL?
Related details
- Andrew Grieve
- Daniel Cheng
- Per Kjellander
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Grieve (Gerrit)
Andrew Grieve added 1 comment![]( "Open in Gerrit")
The dependency cycle you pasted looks suspicious to me. I'd guess you either didn't remove the `//gin:v8_snapshot_assets` dep, or, more likely, the depfiles are stale and you need to either do a clean build, or clear out your depfiles via `find out/Debug -name "*.d" -delete`
I'm heading out for the day but can look into this on Monday if it's not figured out. I don't think we can move forward with adding `//gin` to every test suite.
Related details
- Daniel Cheng
- Per Kjellander
- Sun Shin US
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
OK... I'm going to have to patch in this CL and try it later. I'm not sure why we need to plumb a callback through InitializeBlink() now. As far as I know, all the Blink stuff is already initialized in platform unit tests, so it seems to me something weird is going on...
Related details
- Per Kjellander
- Sun Shin US
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 2 comments![]( "Open in Gerrit")
OK... I'm going to have to patch in this CL and try it later. I'm not sure why we need to plumb a callback through InitializeBlink() now. As far as I know, all the Blink stuff is already initialized in platform unit tests, so it seems to me something weird is going on...
@dch...@chromium.org thanks for the review.
As in the following comments in the code, there are unit tests for the heap testing which requires the HeapTestingPlatform instead of default V8 Platform. So I have added this interface to sustain the extended platform. Will add this information in the comment section of the callback.
// Initializing ThreadState for testing with a testing specific platform.
// ScopedUnittestsEnvironmentSetup keeps the platform alive until the end of
// the test. The testing platform is initialized using gin::V8Platform which
// is the default platform used by ThreadState.
// Note that the platform is not initialized by AttachMainThreadForTesting
// to avoid including test-only headers in production build targets.
- Andrew Grieve
- Daniel Cheng
- Per Kjellander
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
Sun Shin USOK... I'm going to have to patch in this CL and try it later. I'm not sure why we need to plumb a callback through InitializeBlink() now. As far as I know, all the Blink stuff is already initialized in platform unit tests, so it seems to me something weird is going on...
@dch...@chromium.org thanks for the review.
As in the following comments in the code, there are unit tests for the heap testing which requires the HeapTestingPlatform instead of default V8 Platform. So I have added this interface to sustain the extended platform. Will add this information in the comment section of the callback.
// Initializing ThreadState for testing with a testing specific platform.
// ScopedUnittestsEnvironmentSetup keeps the platform alive until the end of
// the test. The testing platform is initialized using gin::V8Platform which
// is the default platform used by ThreadState.
// Note that the platform is not initialized by AttachMainThreadForTesting
// to avoid including test-only headers in production build targets.
Added the detailed information regarding the callback in the latest CL.
Related details
- Andrew Grieve
- Daniel Cheng
- Per Kjellander
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 2 comments![]( "Open in Gerrit")
Sun Shin USOK... I'm going to have to patch in this CL and try it later. I'm not sure why we need to plumb a callback through InitializeBlink() now. As far as I know, all the Blink stuff is already initialized in platform unit tests, so it seems to me something weird is going on...
Sun Shin US@dch...@chromium.org thanks for the review.
As in the following comments in the code, there are unit tests for the heap testing which requires the HeapTestingPlatform instead of default V8 Platform. So I have added this interface to sustain the extended platform. Will add this information in the comment section of the callback.
// Initializing ThreadState for testing with a testing specific platform.
// ScopedUnittestsEnvironmentSetup keeps the platform alive until the end of
// the test. The testing platform is initialized using gin::V8Platform which
// is the default platform used by ThreadState.
// Note that the platform is not initialized by AttachMainThreadForTesting
// to avoid including test-only headers in production build targets.
Added the detailed information regarding the callback in the latest CL.
@dch...@chromium.org I am wondering if you would have a chance to try the CL? Would you be ok to have the platform unit test changes into this CL or make a separate CL? I am ok with both.
@agr...@chromium.org I have figured out that there was a missing reference for the v8_snapshot in third_party/blink/renderer/platform/BUILD.gn and added it.
Please review the latest PS#72 in this CL and let me know your opinion.
Related details
- Andrew Grieve
- Daniel Cheng
- Per Kjellander
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Hi. I am sorry this took so long to get back to. Please look at the difference between PS1 and PS2. I tested locally and I think this should work for you, once I land the parent CL.
Related details
- Sun Shin US
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
@h...@chromium.org, @ho...@chromium.org Could both of you review the latest PS from this CL?
Related details
- Andrew Grieve
- Daniel Cheng
- Per Kjellander
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Can you please have a look at https://chromium-review.googlesource.com/c/chromium/src/+/5650066? Now that we have a FakeMojoBindingContext, this should simplify your test considerably.
Related details
- Andrew Grieve
- Per Kjellander
- Sun Shin US
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
@dch...@chromium.org Thank you so much for working on this. I think this new code looks more compact and also covers what I originally trying to resolve through the CL <5545427>.
Related details
- Daniel Cheng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
Can you please have a look at https://chromium-review.googlesource.com/c/chromium/src/+/5650066? Now that we have a FakeMojoBindingContext, this should simplify your test considerably.
@dch...@chromium.org As I commented on CL<5650066>, really appreciate your effort and time to providing the fakeMojoContext class and simplify the implementation.
Related details
- Andrew Grieve
- Daniel Cheng
- Per Kjellander
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
Sun Shin USHi. I am sorry this took so long to get back to. Please look at the difference between PS1 and PS2. I tested locally and I think this should work for you, once I land the parent CL.
No worries. And thanks for your all the advices. I originally slightly misunderstood your intention and now it looks more clearer.
Related details
- Daniel Cheng
- Harald Alvestrand
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Sun Shin USHi. I am sorry this took so long to get back to. Please look at the difference between PS1 and PS2. I tested locally and I think this should work for you, once I land the parent CL.
No worries. And thanks for your all the advices. I originally slightly misunderstood your intention and now it looks more clearer.
Let's land your original CL still. It shouldn't be too hard to adapt it (you can either patch in this CL using `git cl patch 5650066` and then reset the CL to your original CL using `git cl issue 5545427`)
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
Sun Shin USHi. I am sorry this took so long to get back to. Please look at the difference between PS1 and PS2. I tested locally and I think this should work for you, once I land the parent CL.
Daniel ChengNo worries. And thanks for your all the advices. I originally slightly misunderstood your intention and now it looks more clearer.
Let's land your original CL still. It shouldn't be too hard to adapt it (you can either patch in this CL using `git cl patch 5650066` and then reset the CL to your original CL using `git cl issue 5545427`)
@dch...@chromium.org Sure, I will update the original CL based on your feedback.
Related details
- Daniel Cheng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US restored this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
Sun Shin USHi. I am sorry this took so long to get back to. Please look at the difference between PS1 and PS2. I tested locally and I think this should work for you, once I land the parent CL.
Daniel ChengNo worries. And thanks for your all the advices. I originally slightly misunderstood your intention and now it looks more clearer.
Sun Shin USLet's land your original CL still. It shouldn't be too hard to adapt it (you can either patch in this CL using `git cl patch 5650066` and then reset the CL to your original CL using `git cl issue 5545427`)
@dch...@chromium.org Sure, I will update the original CL based on your feedback.
- Daniel Cheng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
1 comment
Related details
- Sun Shin US
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
case rtc::Socket::OPT_RECV_ECN:@jack...@google.com Could you please review this change?
CC: @hans...@chromium.org, @dcla...@google.com, @cris...@google.com, @pu...@google.com
Related details
- Crisrael Lucero
- Daniel Classon
- Pu Shi
- Ryan Hansberry
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Crisrael Lucero (Gerrit)
Crisrael Lucero added 1 comment![]( "Open in Gerrit")
c/s/s/webrtc/ipc_packet_socket_factory.cc LGTM
Related details
- Daniel Classon
- Pu Shi
- Ryan Hansberry
- Sun Shin US
Code-Coverage
Code-Review
Crisrael Lucero (Gerrit)
Crisrael Lucero voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Daniel Classon
- Pu Shi
- Ryan Hansberry
- Sun Shin US
Code-Coverage
Code-Review
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
case rtc::Socket::OPT_RECV_ECN:@jack...@google.com Could you please review this change?
CC: @hans...@chromium.org, @dcla...@google.com, @cris...@google.com, @pu...@google.com
- Crisrael Lucero
- Daniel Classon
- Pu Shi
- Ryan Hansberry
Code-Coverage
Code-Review
Joone Hur (Gerrit)
Joone Hur voted Commit-Queue+2![]( "Open in Gerrit")
- Daniel Classon
- Pu Shi
- Ryan Hansberry
- Sun Shin US
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add RECV_ECN socket option in P2PSocket interface
This patch adds interface changes to 'services/network' and
'third_party/blink' to support ECN handling in WebRTC.
ECN means Explicit Congestion Notification.
See RFC9331 for details.
Purpose it to allow webrtc to add new experimental socket options.
Ex, https://webrtc-review.googlesource.com/c/src/+/332380
- M chrome/services/sharing/webrtc/ipc_packet_socket_factory.cc
- M services/network/p2p/socket_tcp.cc
- M services/network/p2p/socket_udp.cc
- M services/network/p2p/socket_udp_unittest.cc
- M services/network/public/cpp/p2p_socket_type.h
- M third_party/blink/renderer/platform/BUILD.gn
- M third_party/blink/renderer/platform/p2p/DEPS
- M third_party/blink/renderer/platform/p2p/ipc_socket_factory.cc
- A third_party/blink/renderer/platform/p2p/ipc_socket_factory_test.cc
Code-Review: +1 by Daniel Cheng, +1 by Crisrael Lucero
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |