Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1648763002:
Create QuicSession (Closed)
|
Created:
4 years, 10 months ago by mikescarlett Modified:
4 years, 10 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCreate QuicSession
This CL depends on the unofficial libquic (https://github.com/devsisters/libquic), with subtle modifications.
BUG=
Committed: https://crrev.com/cd0e4751b2fdfd646f12412543ed318d4f03bee5
Cr-Commit-Position: refs/heads/master@{#11530}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Add conditional QUIC build using GYP #Patch Set 3 : Cleanup, more unit tests, and move files to different directory #Patch Set 4 : Improve QuicAlarm unit test #Patch Set 5 : Fix broken unit tests #
Total comments: 18
Patch Set 6 : Improve unit tests #
Total comments: 2
Patch Set 7 : Modify ReliableQuicStream unit test to use string directly #Patch Set 8 : Modify QuicSession unit test string comparison asserts #
Total comments: 40
Patch Set 9 : Cleanup in response to Taylor's comments #Patch Set 10 : Change header include order #Patch Set 11 : Sync webrtc/build/common.gypi to fix patch issue #
Messages
Total messages: 64 (36 generated)
Description was changed from ========== Create QuicSession This CL depends on the unofficial libquic (https://github.com/devsisters/libquic), with subtle modifications. The build files are left out of this CL until I can add a conditional dependency for compiling with libquic. BUG= ========== to ========== Create QuicSession This CL depends on the unofficial libquic (https://github.com/devsisters/libquic), with subtle modifications. The build files are left out of this CL until I can add a conditional dependency for compiling with libquic. BUG= ==========
mikescarlett@webrtc.org changed reviewers: + pthatcher@webrtc.org
I haven't read the unit test yet, but it's likely very good for a first pass. Can you also look into adding changes to the build files along the lines of what HAVE_SCTP does? Add a HAVE_QUIC. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... File webrtc/p2p/base/quicconnectionhelper.cc (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... webrtc/p2p/base/quicconnectionhelper.cc:2: * Copyright 2011 The WebRTC Project Authors. All rights reserved. 2011?? https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... webrtc/p2p/base/quicconnectionhelper.cc:19: QuicWebrtcAlarm(const net::QuicClock* clock, Can you write a unit test for this class? https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... webrtc/p2p/base/quicconnectionhelper.cc:30: DCHECK(in_message_queue_); Because this is called on a different thread, it may be executed before the "in_message_queue_ = true" after the message is posted in SetImpl(). So I believe this may fail if the message queue wins the race. But why do we even need in_message_queue_? Is it so that we don't call Thread::Clear when nothing is in the queue? Because I think it's safe to call Thread::Clear even if nothing is in there. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... File webrtc/p2p/base/quicconnectionhelper.h (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... webrtc/p2p/base/quicconnectionhelper.h:1: /* Can we put all of these files in a directory called webrtc/p2p/quic ? I think someday we'd like to have it be webrtc/net/quic, and also have webrtc/net/ice and webrtc/net/dtls. But for now, webrtc/p2p/quic is probably better than webrtc/p2p/base. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicsession.cc File webrtc/p2p/base/quicsession.cc (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicsession.c... webrtc/p2p/base/quicsession.cc:55: VLOG(1) << "QuicSession handshake complete"; VLOG(1)? We usually use something like LOG(INFO). https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicsession.c... webrtc/p2p/base/quicsession.cc:66: SignalIncomingStream(stream); Should this also be in an "if (stream)"? https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicsession.h File webrtc/p2p/base/quicsession.h (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicsession.h... webrtc/p2p/base/quicsession.h:82: ReliableQuicStream* CreateDataStream(net::QuicStreamId id); Why is this protected but not virtual? https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... File webrtc/p2p/base/reliablequicstream.cc (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... webrtc/p2p/base/reliablequicstream.cc:33: } Are you going to write a simple unit test for this as well? https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... webrtc/p2p/base/reliablequicstream.cc:48: return net::ERR_IO_PENDING; If prefer it in the reverse: if (HasBufferedData()) { return net::ERR_IO_PENDING; } return data.size(); https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... webrtc/p2p/base/reliablequicstream.cc:48: return net::ERR_IO_PENDING; Can you convert this to an rtc::StreamResult? https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... File webrtc/p2p/base/reliablequicstream.h (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... webrtc/p2p/base/reliablequicstream.h:37: int Write(base::StringPiece data); Can we make this take something that isn't Chrome-specific, such as a char* + size_t? https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... webrtc/p2p/base/reliablequicstream.h:42: sigslot::signal2<net::QuicStreamId, net::QuicErrorCode> SignalStreamClosed; Just SignalClosed would be sufficient.
Description was changed from ========== Create QuicSession This CL depends on the unofficial libquic (https://github.com/devsisters/libquic), with subtle modifications. The build files are left out of this CL until I can add a conditional dependency for compiling with libquic. BUG= ========== to ========== Create QuicSession This CL depends on the unofficial libquic (https://github.com/devsisters/libquic), with subtle modifications. BUG= ==========
I have addressed your comments, added more unit tests, and modified the build files. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... File webrtc/p2p/base/quicconnectionhelper.cc (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... webrtc/p2p/base/quicconnectionhelper.cc:2: * Copyright 2011 The WebRTC Project Authors. All rights reserved. On 2016/01/30 03:01:52, pthatcher1 wrote: > 2011?? Changed to 2016. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... webrtc/p2p/base/quicconnectionhelper.cc:19: QuicWebrtcAlarm(const net::QuicClock* clock, On 2016/01/30 03:01:52, pthatcher1 wrote: > Can you write a unit test for this class? Sure no problem. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... webrtc/p2p/base/quicconnectionhelper.cc:30: DCHECK(in_message_queue_); On 2016/01/30 03:01:52, pthatcher1 wrote: > Because this is called on a different thread, it may be executed before the > "in_message_queue_ = true" after the message is posted in SetImpl(). So I > believe this may fail if the message queue wins the race. > > But why do we even need in_message_queue_? Is it so that we don't call > Thread::Clear when nothing is in the queue? Because I think it's safe to call > Thread::Clear even if nothing is in there. You are correct. I looked at the source code and Thread::Clear simply ignores cases when nothing is in the queue, so we don't need in_message_queue_. This is also verified via unit test. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... File webrtc/p2p/base/quicconnectionhelper.h (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicconnectio... webrtc/p2p/base/quicconnectionhelper.h:1: /* On 2016/01/30 03:01:52, pthatcher1 wrote: > Can we put all of these files in a directory called webrtc/p2p/quic ? > > I think someday we'd like to have it be webrtc/net/quic, and also have > webrtc/net/ice and webrtc/net/dtls. But for now, webrtc/p2p/quic is probably > better than webrtc/p2p/base. Done. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicsession.cc File webrtc/p2p/base/quicsession.cc (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicsession.c... webrtc/p2p/base/quicsession.cc:55: VLOG(1) << "QuicSession handshake complete"; On 2016/01/30 03:01:52, pthatcher1 wrote: > VLOG(1)? > > We usually use something like LOG(INFO). Fixed. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicsession.c... webrtc/p2p/base/quicsession.cc:66: SignalIncomingStream(stream); On 2016/01/30 03:01:52, pthatcher1 wrote: > Should this also be in an "if (stream)"? Yes it should. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicsession.h File webrtc/p2p/base/quicsession.h (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/quicsession.h... webrtc/p2p/base/quicsession.h:82: ReliableQuicStream* CreateDataStream(net::QuicStreamId id); On 2016/01/30 03:01:52, pthatcher1 wrote: > Why is this protected but not virtual? I agree this should be virtual for more flexibility. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... File webrtc/p2p/base/reliablequicstream.cc (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... webrtc/p2p/base/reliablequicstream.cc:33: } On 2016/01/30 03:01:52, pthatcher1 wrote: > Are you going to write a simple unit test for this as well? Yes I will. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... webrtc/p2p/base/reliablequicstream.cc:48: return net::ERR_IO_PENDING; On 2016/01/30 03:01:52, pthatcher1 wrote: > Can you convert this to an rtc::StreamResult? Converted to rtc::StreamResult. Also changed the method signature. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... webrtc/p2p/base/reliablequicstream.cc:48: return net::ERR_IO_PENDING; On 2016/01/30 03:01:52, pthatcher1 wrote: > If prefer it in the reverse: > > if (HasBufferedData()) { > return net::ERR_IO_PENDING; > } > > return data.size(); Reversed if statement. https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... File webrtc/p2p/base/reliablequicstream.h (right): https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... webrtc/p2p/base/reliablequicstream.h:37: int Write(base::StringPiece data); On 2016/01/30 03:01:52, pthatcher1 wrote: > Can we make this take something that isn't Chrome-specific, such as a char* + > size_t? I made this const char* and size_t https://codereview.webrtc.org/1648763002/diff/1/webrtc/p2p/base/reliablequics... webrtc/p2p/base/reliablequicstream.h:42: sigslot::signal2<net::QuicStreamId, net::QuicErrorCode> SignalStreamClosed; On 2016/01/30 03:01:52, pthatcher1 wrote: > Just SignalClosed would be sufficient. Ok I renamed this SignalClosed.
Patchset #5 (id:80001) has been deleted
This is looking very good. Only minor things left. https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicconn... File webrtc/p2p/quic/quicconnectionhelper_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper_unittest.cc:52: class MockDelegate : public QuicAlarm::Delegate { Can you call this MockAlarmDelegate? https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper_unittest.cc:106: alarm_->OnMessage(nullptr); How is this different than ProcessNextMessage? https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper_unittest.cc:130: ASSERT_EQ(700, alarm_->GetDelay()); These can all be EXPECT_EQ instead of ASSERT_EQ https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicsess... File webrtc/p2p/quic/quicsession_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:376: EXPECT_TRUE(to_session->IsEncryptionEstablished()); These should be ASSERT_TRUE https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:379: string to_out; Should this be from_key and to_key? https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:386: EXPECT_TRUE(to_success); I think these should be ASSERT_TRUEs, and then you don't need the early return below. https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/reliable... File webrtc/p2p/quic/reliablequicstream_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:170: false /* owns_writer */, perspective, net::QuicSupportedVersions()); I'd prefer to put above: bool owns_writer = false; https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:196: std::vector<std::string> read_buffer_; An rtc::Buffer might be a better fit here.
This is ready for further review. https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicconn... File webrtc/p2p/quic/quicconnectionhelper_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper_unittest.cc:52: class MockDelegate : public QuicAlarm::Delegate { On 2016/02/03 23:27:23, pthatcher1 wrote: > Can you call this MockAlarmDelegate? Done. https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper_unittest.cc:106: alarm_->OnMessage(nullptr); On 2016/02/03 23:27:23, pthatcher1 wrote: > How is this different than ProcessNextMessage? ProcessNextMessage causes rtc::Thread to process the current callback so that it invokes OnMessage(message) asynchronously. OnMessage is invoked as a side effect of SetImpl(). On second thought I am removing FireAlarmOnMessage, given that OnMessage is intended to be called by rtc::Thread and that FireAlarm covers this scenario. https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper_unittest.cc:130: ASSERT_EQ(700, alarm_->GetDelay()); On 2016/02/03 23:27:23, pthatcher1 wrote: > These can all be EXPECT_EQ instead of ASSERT_EQ Acknowledged. https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicsess... File webrtc/p2p/quic/quicsession_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:376: EXPECT_TRUE(to_session->IsEncryptionEstablished()); On 2016/02/03 23:27:24, pthatcher1 wrote: > These should be ASSERT_TRUE Agreed. https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:379: string to_out; On 2016/02/03 23:27:23, pthatcher1 wrote: > Should this be from_key and to_key? Done. That is more descriptive. https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:386: EXPECT_TRUE(to_success); On 2016/02/03 23:27:24, pthatcher1 wrote: > I think these should be ASSERT_TRUEs, and then you don't need the early return > below. Agreed and done. https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/reliable... File webrtc/p2p/quic/reliablequicstream_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:170: false /* owns_writer */, perspective, net::QuicSupportedVersions()); On 2016/02/03 23:27:24, pthatcher1 wrote: > I'd prefer to put above: > > bool owns_writer = false; Done. https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:196: std::vector<std::string> read_buffer_; On 2016/02/03 23:27:24, pthatcher1 wrote: > An rtc::Buffer might be a better fit here. Agreed that's a better fit (did not know about rtc::Buffer before).
Patchset #6 (id:120001) has been deleted
looks good, just one little tweak in the unit test. I should have asked Taylor sooner, but Taylor: can you review this CL? https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/reliable... File webrtc/p2p/quic/reliablequicstream_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:196: std::vector<std::string> read_buffer_; On 2016/02/05 21:10:29, mikescarlett wrote: > On 2016/02/03 23:27:24, pthatcher1 wrote: > > An rtc::Buffer might be a better fit here. > > Agreed that's a better fit (did not know about rtc::Buffer before). Actually, now that I see it, I think a std::string would be better. Then you can just do EXPECT_EQ("Foo bar", x_buffer_) everywhere, which would be nicer. When it's time to push on the string, just use read_buffer_.append(data, length).
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
Ooops... let's try that again. Taylor, can you review this CL as well?
Patchset #6 (id:140001) has been deleted
Patchset #7 (id:180001) has been deleted
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
Patchset #7 (id:240001) has been deleted
https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/reliable... File webrtc/p2p/quic/reliablequicstream_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/100001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:196: std::vector<std::string> read_buffer_; On 2016/02/05 21:32:34, pthatcher1 wrote: > On 2016/02/05 21:10:29, mikescarlett wrote: > > On 2016/02/03 23:27:24, pthatcher1 wrote: > > > An rtc::Buffer might be a better fit here. > > > > Agreed that's a better fit (did not know about rtc::Buffer before). > > Actually, now that I see it, I think a std::string would be better. > > Then you can just do EXPECT_EQ("Foo bar", x_buffer_) everywhere, which would be > nicer. > > When it's time to push on the string, just use read_buffer_.append(data, > length). I replaced rtc::Buffer with std::string and no longer define a custom assert method.
Looks great; you were very thorough. I just found some assorted nits, and a couple minor bugs in the unit tests. https://codereview.webrtc.org/1648763002/diff/160001/webrtc/p2p/quic/quicconn... File webrtc/p2p/quic/quicconnectionhelper.cc (right): https://codereview.webrtc.org/1648763002/diff/160001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper.cc:15: QuicAlarm* QuicConnectionHelper::CreateAlarm( Should this method be at the bottom of the file along with other QuicConnectionHelper methods? https://codereview.webrtc.org/1648763002/diff/160001/webrtc/p2p/quic/quicconn... File webrtc/p2p/quic/quicconnectionhelper.h (right): https://codereview.webrtc.org/1648763002/diff/160001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper.h:28: rtc::Thread* thread, Maybe name the Thread parameter something more descriptive (like worker_thread), or explain it in a comment above. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicconn... File webrtc/p2p/quic/quicconnectionhelper_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper_unittest.cc:15: #include "net/quic/quic_time.h" nit: Sort includes https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper_unittest.cc:105: SetTime(-1); This test will start failing if at some point in the future, SetTime(-1) causes the delegate to be called synchronously. But without rtc::Thread being able to use a mock clock, there's no good way to fix this. So I'd just add a TODO. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... File webrtc/p2p/quic/quicsession.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:17: #include "webrtc/base/logging.h" nit: Sort includes https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:48: return GetCryptoStream()->ExportKeyingMaterial(label, context, result_len, I would just use crypto_stream_ instead of GetCryptoStream(); it's more readable. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:55: LOG(INFO) << "QuicSession handshake complete"; I'm used to seeing LS_INFO; I think that's what we prefer, though maybe Peter knows. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:56: DCHECK(IsEncryptionEstablished()); RTC_DCHECK https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:82: net::QuicCryptoStream* crypto_stream = GetCryptoStream(); Same as above, I'd suggest just using crypto_stream_. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:92: nit: Whitespace seems unnecessary https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... File webrtc/p2p/quic/quicsession.h (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.h:25: #include "webrtc/base/sigslot.h" nit: Sort includes https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... File webrtc/p2p/quic/quicsession_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:31: #include "webrtc/p2p/base/faketransportcontroller.h" nit: include order https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:102: // Whether or not obtaining proof source succeeds nit: Use periods at the end of comments (it took me a while to get used to this). https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:138: virtual ~FakeQuicPacketWriter() {} Is there a reason why this destructor doesn't use `override` like the ones above? Also, do these classes even need empty destructors? https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:204: channel_(channel.release()) { Use std::move(channel). https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:242: std::string last_received_data_ = ""; Does '= ""' have a purpose here? https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:244: ReliableQuicStream* last_incoming_stream_; This variable needs to be default initialized. ReliableQuicStream* last_incoming_stream_ = nullptr; https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:394: from_session->CreateOutgoingDynamicStream(kDefaultPriority); Where is this object freed? It does say "owned by caller" so I'd use a scoped_ptr. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... File webrtc/p2p/quic/reliablequicstream.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream.cc:38: nit: whitespace https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... File webrtc/p2p/quic/reliablequicstream_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:25: #include "webrtc/base/stream.h" nit: include order https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:168: bool owns_writer = false; If the only purpose of this variable is to give a name to the boolean value, I think our style guide should allow: new QuicConnection(..., /* owns_writer */ true, perspective, ...)
https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... File webrtc/p2p/quic/quicsession.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:55: LOG(INFO) << "QuicSession handshake complete"; On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > I'm used to seeing LS_INFO; I think that's what we prefer, though maybe Peter > knows. They are exactly the same: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... Search of LOG(LS_INFO) gives 127 hits, and LOG(INFO) gives 260. We should probably just pick one someday and switch everything to it.
I fixed most of these. I was unsure what the order of includes usually is so I based it off webrtc/p2p/* files. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicconn... File webrtc/p2p/quic/quicconnectionhelper_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper_unittest.cc:15: #include "net/quic/quic_time.h" On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > nit: Sort includes Done. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicconn... webrtc/p2p/quic/quicconnectionhelper_unittest.cc:105: SetTime(-1); On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > This test will start failing if at some point in the future, SetTime(-1) causes > the delegate to be called synchronously. > > But without rtc::Thread being able to use a mock clock, there's no good way to > fix this. So I'd just add a TODO. Added TODO for this and above. I didn't see any better way to post a delayed task while guaranteeing it will be the next one called. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... File webrtc/p2p/quic/quicsession.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:17: #include "webrtc/base/logging.h" On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > nit: Sort includes Done. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:48: return GetCryptoStream()->ExportKeyingMaterial(label, context, result_len, On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > I would just use crypto_stream_ instead of GetCryptoStream(); it's more > readable. I replaced this. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:55: LOG(INFO) << "QuicSession handshake complete"; On 2016/02/06 00:40:29, pthatcher1 wrote: > On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > > I'm used to seeing LS_INFO; I think that's what we prefer, though maybe Peter > > knows. > > They are exactly the same: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > Search of LOG(LS_INFO) gives 127 hits, and LOG(INFO) gives 260. We should > probably just pick one someday and switch everything to it. I'll keep LOG(INFO) since it appears both are used. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:56: DCHECK(IsEncryptionEstablished()); On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > RTC_DCHECK Done. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:82: net::QuicCryptoStream* crypto_stream = GetCryptoStream(); On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > Same as above, I'd suggest just using crypto_stream_. This is also replaced. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.cc:92: On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > nit: Whitespace seems unnecessary I removed it. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... File webrtc/p2p/quic/quicsession.h (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession.h:25: #include "webrtc/base/sigslot.h" On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > nit: Sort includes Done. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... File webrtc/p2p/quic/quicsession_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:31: #include "webrtc/p2p/base/faketransportcontroller.h" On 2016/02/06 00:22:45, Taylor Brandstetter wrote: > nit: include order Done. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:102: // Whether or not obtaining proof source succeeds On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > nit: Use periods at the end of comments (it took me a while to get used to > this). Done. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:138: virtual ~FakeQuicPacketWriter() {} On 2016/02/06 00:22:45, Taylor Brandstetter wrote: > Is there a reason why this destructor doesn't use `override` like the ones > above? Also, do these classes even need empty destructors? I removed the empty destructors since they don't do anything and it doesn't affect the unit tests. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:204: channel_(channel.release()) { On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > Use std::move(channel). Done. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:242: std::string last_received_data_ = ""; On 2016/02/06 00:22:45, Taylor Brandstetter wrote: > Does '= ""' have a purpose here? Removed '= ""' since it doesn't do anything. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:244: ReliableQuicStream* last_incoming_stream_; On 2016/02/06 00:22:44, Taylor Brandstetter wrote: > This variable needs to be default initialized. > > ReliableQuicStream* last_incoming_stream_ = nullptr; Done. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/quicsess... webrtc/p2p/quic/quicsession_unittest.cc:394: from_session->CreateOutgoingDynamicStream(kDefaultPriority); On 2016/02/06 00:22:45, Taylor Brandstetter wrote: > Where is this object freed? It does say "owned by caller" so I'd use a > scoped_ptr. I am concluding that Chromium's documentation might be incorrect. I tried scoped_ptr, but somehow the net::QuicSession destructor deletes all of the outgoing streams including this one and crashes the unit tests. This will have to be a raw pointer for now and I added a TODO for QuicSession::CreateOutgoingDynamicStream to verify who should own these. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... File webrtc/p2p/quic/reliablequicstream.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream.cc:38: On 2016/02/06 00:22:45, Taylor Brandstetter wrote: > nit: whitespace Done. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... File webrtc/p2p/quic/reliablequicstream_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:25: #include "webrtc/base/stream.h" On 2016/02/06 00:22:45, Taylor Brandstetter wrote: > nit: include order Done. https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:168: bool owns_writer = false; On 2016/02/06 00:22:45, Taylor Brandstetter wrote: > If the only purpose of this variable is to give a name to the boolean value, I > think our style guide should allow: > > new QuicConnection(..., /* owns_writer */ true, perspective, ...) I had it like that previously until Peter said the opposite. I'm unclear which one is preferred so this is unchanged.
Patchset #9 (id:300001) has been deleted
Patchset #9 (id:320001) has been deleted
lgtm https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... File webrtc/p2p/quic/reliablequicstream_unittest.cc (right): https://codereview.webrtc.org/1648763002/diff/280001/webrtc/p2p/quic/reliable... webrtc/p2p/quic/reliablequicstream_unittest.cc:168: bool owns_writer = false; On 2016/02/06 02:14:09, mikescarlett wrote: > On 2016/02/06 00:22:45, Taylor Brandstetter wrote: > > If the only purpose of this variable is to give a name to the boolean value, I > > think our style guide should allow: > > > > new QuicConnection(..., /* owns_writer */ true, perspective, ...) > > I had it like that previously until Peter said the opposite. I'm unclear which > one is preferred so this is unchanged. In Java code we tend to do /* like this */. In C++ code, we tend to do like_this = true. I'm not sure why, but I like the C++ way a lot more.
lgtm
The CQ bit was checked by mikescarlett@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1648763002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1648763002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/5010) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5006) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7332) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/12505) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12284) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/676)
The CQ bit was checked by mikescarlett@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1648763002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1648763002/360001
The CQ bit was checked by mikescarlett@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1648763002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1648763002/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/8703) ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/5012) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5008) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7253) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/12507) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11161) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/12337) linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/8483) linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/9709) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12286) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/8958) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/678) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/6911) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12588)
The CQ bit was checked by mikescarlett@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1648763002/#ps380001 (title: "Sync webrtc/build/common.gypi with upstream")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1648763002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1648763002/380001
The CQ bit was unchecked by mikescarlett@webrtc.org
The CQ bit was checked by mikescarlett@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1648763002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1648763002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5009) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7336) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7254) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11162) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12287) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/679) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12589)
Patchset #11 (id:380001) has been deleted
The CQ bit was checked by mikescarlett@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1648763002/#ps400001 (title: "Sync webrtc/build/common.gypi with upstream")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1648763002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1648763002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/7337) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/7255) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11163) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12288) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12590)
Patchset #11 (id:400001) has been deleted
The CQ bit was checked by mikescarlett@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1648763002/#ps420001 (title: "Sync webrtc/build/common.gypi")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1648763002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1648763002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5011) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/12512) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/6914) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/12591)
Patchset #11 (id:420001) has been deleted
The CQ bit was checked by mikescarlett@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1648763002/#ps440001 (title: "Sync webrtc/build/common.gypi to fix patch issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1648763002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1648763002/440001
Message was sent while issue was closed.
Description was changed from ========== Create QuicSession This CL depends on the unofficial libquic (https://github.com/devsisters/libquic), with subtle modifications. BUG= ========== to ========== Create QuicSession This CL depends on the unofficial libquic (https://github.com/devsisters/libquic), with subtle modifications. BUG= ==========
Message was sent while issue was closed.
Committed patchset #11 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Create QuicSession This CL depends on the unofficial libquic (https://github.com/devsisters/libquic), with subtle modifications. BUG= ========== to ========== Create QuicSession This CL depends on the unofficial libquic (https://github.com/devsisters/libquic), with subtle modifications. BUG= Committed: https://crrev.com/cd0e4751b2fdfd646f12412543ed318d4f03bee5 Cr-Commit-Position: refs/heads/master@{#11530} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/cd0e4751b2fdfd646f12412543ed318d4f03bee5 Cr-Commit-Position: refs/heads/master@{#11530} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
