Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2324833004:
Define Stable API for WebRTC/Quartc (Closed)
DescriptionDefine a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other.
The API contains two parts. One is the interfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the delegates and abstract classes which will be implemented on WebRTC side. The stable API will not introduce any QUIC specific dependencies to the API user (except the /base/net_export.h, which is a set of macro without other dependencies.).
The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHYjM/edit#heading=h.fz18u0qu9v4o
Committed: https://crrev.com/33e7a9c59b4933acedf9d1b23d0033affaa4cd5c
Cr-Commit-Position: refs/heads/master@{#428778}
Patch Set 1 : Create Quartc API #
Total comments: 38
Patch Set 2 : Code style fix. #
Total comments: 3
Patch Set 3 : Minor Fix. #Patch Set 4 : Patch Set 4 : Create QuartcFactory. Made modification on the API. Change the constructor of QuartcS… #
Total comments: 34
Patch Set 5 : Renaming and style fix. #Patch Set 6 : Add virtual destructors required by WebRTC compiler. #
Total comments: 108
Patch Set 7 : Fix the issues when testing with WebRTC. #
Total comments: 41
Patch Set 8 : Simplify the AtExitManager #
Total comments: 5
Patch Set 9 : Modification of AtExitManager. #
Total comments: 4
Patch Set 10 : Solve the conflict when merging. #Patch Set 11 : Fix the memory leak. #
Total comments: 4
Patch Set 12 : Fix the IOS simulator and merge. #
Messages
Total messages: 110 (82 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Create quartc api. ========== to ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. This CL doesn't define all the API for Quartc. The helper classes and QuartcReliableStream are defined. The API contains two parts. One is the interfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the observers which will be implemented on WebRTC side. The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... ==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. This CL doesn't define all the API for Quartc. The helper classes and QuartcReliableStream are defined. The API contains two parts. One is the interfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the observers which will be implemented on WebRTC side. The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... ========== to ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. The API contains two parts. One is the in +terfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the observers which will be implemented on WebRTC side. The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... ==========
Description was changed from ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. The API contains two parts. One is the in +terfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the observers which will be implemented on WebRTC side. The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... ========== to ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. The API contains two parts. One is the interfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the observers which will be implemented on WebRTC side. The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... ==========
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
skvlad@chromium.org changed reviewers: + skvlad@chromium.org
Great work overall! I'm not done reading QuartcSessionTest yet, sharing what I have so far. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_connection_helper.h (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_connection_helper.h:10: #include "net/quic/core/quic_alarm_factory.h" Are these two includes necessary? https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_connection_helper.h:17: // This is the helper class for Quartc to create a QuicConnection. The name 'Quartc' may not tell much to someone who's new to the codebase. I think we should either include a comment into each file that clarifies that 'Quartc' is 'QUIC in WebRTC', or have a short README file in the /quartc/ directory. Not sure what fits better with the Chromium style - probably a comment. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.cc (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.cc:36: return kMaxPacketSize; Is this always correct? The underlying transport such as TURN may reduce the max packet size. Should be ok for now though - we can add a function to Transport later. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.h (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:28: // Transport::OnCanWrite() is called. Did you mean Transport::Observer::OnCanWrite()? https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_reliable_stream.h (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. (c) 2016 :) https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream.h:25: // QuartcReliableInterface overrides. typo: QuartcReliableStreamInterface https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_reliable_stream_test.cc (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_test.cc:46: return QuicConsumedData(len, false); shouldn't this return QuicConsumedData(len, fin)? https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_test.cc:153: QuartcAlarmFactory* alarm_factory = new QuartcAlarmFactory( Shouldn't the factory and the connection be owned by a unique_ptr inside the test class? Otherwise they will leak at the end of the test. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session.cc (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session.cc:206: quic_result.copy(reinterpret_cast<char*>(result), result_len); I'd probably DCHECK(quic_result.length() == result_len) here just to be sure. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session.cc:229: session_observer_ = session_observer; DCHECK(session_observer_)? https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_test.cc (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_test.cc:316: // Make the channel synchronous so that two peer will not keep calling each "asynchronous"?
Patchset #2 (id:180001) has been deleted
zhihuang@chromium.org changed reviewers: + rch@chromium.org
Hi Vlad, Thanks for reviewing it! Hi Ryan, This is the initial implementation of the API. Peter is not in office for next a few days so maybe we can do the first round review without waiting for him. There is a one-line change in quic/core/crypto/ directory. I'll make a separate CL for it if the change reasonable. Please feel free to add other reviewers if necessary. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_connection_helper.h (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_connection_helper.h:10: #include "net/quic/core/quic_alarm_factory.h" On 2016/09/22 01:54:25, skvlad-chromium wrote: > Are these two includes necessary? No, these should be removed. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.cc (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.cc:36: return kMaxPacketSize; On 2016/09/22 01:54:25, skvlad-chromium wrote: > Is this always correct? The underlying transport such as TURN may reduce the max > packet size. Should be ok for now though - we can add a function to Transport > later. This is a good point. I am not ware of the package size thing of Turn. This is used to negotiate the MTU. If the Turn server becomes the bottleneck, then we may need a function like "GetMaxPacketSize" in QuartcSessionInterface::Transport. I'll leave a TODO here for further discussion. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.h (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:28: // Transport::OnCanWrite() is called. On 2016/09/22 01:54:25, skvlad-chromium wrote: > Did you mean Transport::Observer::OnCanWrite()? Ah, yes. Thanks for pointing out. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_reliable_stream.h (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2016/09/22 01:54:25, skvlad-chromium wrote: > (c) 2016 :) Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_reliable_stream_test.cc (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_test.cc:46: return QuicConsumedData(len, false); On 2016/09/22 01:54:25, skvlad-chromium wrote: > shouldn't this return QuicConsumedData(len, fin)? You're right. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_test.cc:153: QuartcAlarmFactory* alarm_factory = new QuartcAlarmFactory( On 2016/09/22 01:54:25, skvlad-chromium wrote: > Shouldn't the factory and the connection be owned by a unique_ptr inside the > test class? Otherwise they will leak at the end of the test. Yes. The connection will not own the factory. Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session.cc (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session.cc:206: quic_result.copy(reinterpret_cast<char*>(result), result_len); On 2016/09/22 01:54:26, skvlad-chromium wrote: > I'd probably DCHECK(quic_result.length() == result_len) here just to be sure. Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session.cc:229: session_observer_ = session_observer; On 2016/09/22 01:54:25, skvlad-chromium wrote: > DCHECK(session_observer_)? Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_test.cc (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_test.cc:316: // Make the channel synchronous so that two peer will not keep calling each On 2016/09/22 01:54:26, skvlad-chromium wrote: > "asynchronous"? Yes. Silly mistake. :)
honghaiz@chromium.org changed reviewers: + honghaiz@chromium.org
Add a few comments. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_reliable_stream_interface.h (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:18: virtual void Write(const char* data, Can you add comments for each of the method except for those obvious ones. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:31: // TODO(zhihuang) Create a map from the integer error_code to WebRTC native Use singular verbs for method comments ("Creates" instead of "Create") according to the style guide. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:42: } comment on the namespace. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_interface.h (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:8: #include "net/quic/quartc/quartc_reliable_stream_interface.h" Probably do not need to include this. Is it enough to just declare the class to be used? https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:19: // part of the RFC defining each exporter Please explain all arguments. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:20: // usage (IN) This can be merged to the previous line? https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:29: // For forward-compatibility Can you explain a little more? forward compatibility with what? https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:39: // writable. writable => "is writable" https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:40: virtual bool CanWrite() = 0; Is Writable a better name? And below, OnWritable() is probably better than OnCanWrite().
zhihuang@chromium.org changed reviewers: - rch@chromium.org, skvlad@chromium.org
Thanks for taking a look. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_reliable_stream_interface.h (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:18: virtual void Write(const char* data, On 2016/09/22 18:57:00, honghaiz wrote: > Can you add comments for each of the method except for those obvious ones. Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:31: // TODO(zhihuang) Create a map from the integer error_code to WebRTC native On 2016/09/22 18:57:00, honghaiz wrote: > Use singular verbs for method comments ("Creates" instead of "Create") according > to the style guide. Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:42: } On 2016/09/22 18:57:01, honghaiz wrote: > comment on the namespace. Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_interface.h (right): https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:8: #include "net/quic/quartc/quartc_reliable_stream_interface.h" On 2016/09/22 18:57:03, honghaiz wrote: > Probably do not need to include this. Is it enough to just declare the class to > be used? We need this for QuartcReliableStreamInterface in virtual QuartcReliableStreamInterface* CreateOutgoingStream( const OutgoingStreamParameters& params) = 0; etc. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:19: // part of the RFC defining each exporter On 2016/09/22 18:57:04, honghaiz wrote: > Please explain all arguments. Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:20: // usage (IN) On 2016/09/22 18:57:03, honghaiz wrote: > This can be merged to the previous line? Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:29: // For forward-compatibility On 2016/09/22 18:57:05, honghaiz wrote: > Can you explain a little more? forward compatibility with what? Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:39: // writable. On 2016/09/22 18:57:01, honghaiz wrote: > writable => "is writable" Done. https://codereview.chromium.org/2324833004/diff/160001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:40: virtual bool CanWrite() = 0; On 2016/09/22 18:57:06, honghaiz wrote: > Is Writable a better name? > And below, OnWritable() is probably better than OnCanWrite(). We choose this is to make it consistent with the name in QUIC.
skvlad@chromium.org changed reviewers: + skvlad@chromium.org
lgtm https://codereview.chromium.org/2324833004/diff/200001/net/quic/quartc/quartc... File net/quic/quartc/quartc_reliable_stream_test.cc (right): https://codereview.chromium.org/2324833004/diff/200001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_test.cc:149: QuicConnectionHelperInterface* quic_helper = new QuartcConnectionHelper; Looks like the helper would leak, wouldn't it? https://codereview.chromium.org/2324833004/diff/200001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_test.cc (right): https://codereview.chromium.org/2324833004/diff/200001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_test.cc:39: void SetTimedOutAndQuitLoop(const base::WeakPtr<bool> timed_out, I believe this is good enough as it is if the timeouts aren't slowing down successful tests. It seems, however, that most of the existing QUIC tests don't post messages to threads, and instead have an external "pump" of some sort that moves messages from one transport to another. Here is one example: https://cs.chromium.org/chromium/src/net/quic/test_tools/crypto_test_utils.cc... We could do something similar if we wanted to: instead of the TransportChannels calling each other's callbacks, we can have them put messages into each other's queue. They would also have a method to process items from the queue, and the test could call it repeatedly until both queues are empty: class FakeTransportChannel { std::queue<std::string> incoming_; void send(std::string data) { other->incoming_.enqueue(data); } bool ProcessIncomingMessage() { if (incoming_.empty()) return false; observer_.OnIncomingPacket(incoming_.front()); incoming_.pop(); return true; } } and in the test: StartHandshake(); while (server_channel_.ProcessIncomingMessage() || client_channel_.ProcessIncomingMessage()); ASSERT_EQ( handshake successful ) This is certainly not the only or the preferred way to do it, and I think it works good as it is. Just wanted to propose this in case you ever want a completely deterministic, single threaded test with no waits. You'd probably need a manually controlled alarm factory to go with this approach. https://codereview.chromium.org/2324833004/diff/200001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_test.cc:494: TEST_F(QuartcSessionTest, CloseQuartcStream) { Do we need a test for closing the stream from the remote side, too?
Patchset #4 (id:240001) has been deleted
zhihuang@chromium.org changed reviewers: + rch@chromium.org
Hi, I have some update for this CL. Please ignore previous one if you haven't started yet. Thanks.
https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.cc (right): https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:25: config_.reset(new QuicConfig); Do you actually need to keep the config around? It appears that QuicSession creates a copy for its own use - so you can discard the config after it's created.
First, some comments on the interface definitions, which are mostly just formatting nits and a few naming/comment suggestions. Next, I'l start looking at the actual code. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory_interface.h (right): https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:11: class QuartcFactoryInterface { nit: comments and newlines, please. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:16: // Used for creating a unique server ID. Only used by the client side. The phrasing of this comment is confused to me, but of course I don't actually know WebRTC. If this string is used to create a server ID I would have expected it to be used by the server not the client. Can you say more? https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:24: const QuartcConfig& quartc_config) = 0; Can you comment on the ownership of quartc_config.transport? Presumably it's still owned by the caller. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... File net/quic/quartc/quartc_reliable_stream_interface.h (right): https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:8: namespace net { nit: newline after https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:9: class QuartcReliableStreamInterface { nit: Just to make this shorter, I'd drop "Reliable" from the name since streams are always reliable. (I should remove Reliable from the underlying QUIC class too) https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:9: class QuartcReliableStreamInterface { Please add a class comment. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:11: virtual uint32_t stream_id() = 0; nit: it's common in net to have a newline after each of these methods. Also, this presumably returns the ID of the underlying QUIC stream. Can you comment to this effect? https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:12: // The size of how much data has been buffered by the QuicConnection so far. What does it mean for data to be buffered by the connection? Is this data being sent or received? https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:23: virtual void Close() = 0; I assume this does not delete the stream? https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:26: class Observer { nit: From a recent net discussion I think Delegate would be a better name for this class. Observer is more commonly an optional possibly many to one object for which AddObserver and RemoveObserver would be implemented. Whereas delegate more strongly implies a 1:1 relationship. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:42: virtual void SetObserver(Observer*) = 0; Can you comment on lifetime/ownership here? I assume the delegate/observer is expected to outlive the stream and is still owned by the caller and that this method is not expected to be called multiple times? https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_interface.h (right): https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:10: namespace net { nit: newline after https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:11: class QuartcSessionInterface { nit: class comment, please https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:13: virtual void StartCryptoHandshake() = 0; What does this method do when the underlying session is a server session? https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:34: // For forward-compatibility. We can add more parameters to the function nit: Avoid using first person in comments. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:46: virtual bool CanWrite() = 0; nit: newlines between methods, please https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:50: // Implemented by QUIC code. Can you add a comment to explain what this class does. I'm sure I'll be able to figure it out when I see the implementation, but it would be nice if the interface made it clear. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:51: class Observer { I think the same Observer->Delegate change would be a good idea here. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:59: class Observer { I think the same Observer->Delegate change would be a good idea here.
Patchset #5 (id:280001) has been deleted
Patchset #5 (id:300001) has been deleted
Patchset #5 (id:320001) has been deleted
Please take another look. Thanks. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory_interface.h (right): https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:11: class QuartcFactoryInterface { On 2016/09/23 19:56:43, Ryan Hamilton wrote: > nit: comments and newlines, please. Done. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:16: // Used for creating a unique server ID. Only used by the client side. On 2016/09/23 19:56:43, Ryan Hamilton wrote: > The phrasing of this comment is confused to me, but of course I don't actually > know WebRTC. If this string is used to create a server ID I would have expected > it to be used by the server not the client. Can you say more? Done. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:24: const QuartcConfig& quartc_config) = 0; On 2016/09/23 19:56:43, Ryan Hamilton wrote: > Can you comment on the ownership of quartc_config.transport? Presumably it's > still owned by the caller. Done. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... File net/quic/quartc/quartc_reliable_stream_interface.h (right): https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:8: namespace net { On 2016/09/23 19:56:44, Ryan Hamilton wrote: > nit: newline after Done. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:9: class QuartcReliableStreamInterface { On 2016/09/23 19:56:44, Ryan Hamilton wrote: > nit: Just to make this shorter, I'd drop "Reliable" from the name since streams > are always reliable. (I should remove Reliable from the underlying QUIC class > too) Sounds good. I like shorter name :) https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:12: // The size of how much data has been buffered by the QuicConnection so far. On 2016/09/23 19:56:44, Ryan Hamilton wrote: > What does it mean for data to be buffered by the connection? Is this data being > sent or received? This means when the underlying WebRTC network is not writable, then QuicPacketWriter::IsWriteBlocked() will return true and the QuicConnection is expected to buffered the unsent data. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:23: virtual void Close() = 0; On 2016/09/23 19:56:44, Ryan Hamilton wrote: > I assume this does not delete the stream? I think it does. This will unregister the stream and call QuicSession::CloseStream(id) which will put the current stream to the closed_stream_ set. This set will be cleared when calling PostProcessAfterData() https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_reliable_stream_interface.h:42: virtual void SetObserver(Observer*) = 0; On 2016/09/23 19:56:44, Ryan Hamilton wrote: > Can you comment on lifetime/ownership here? I assume the delegate/observer is > expected to outlive the stream and is still owned by the caller and that this > method is not expected to be called multiple times? You are right. The delegate will not be owned and this method is expected to be called once. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_interface.h (right): https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:13: virtual void StartCryptoHandshake() = 0; On 2016/09/23 19:56:45, Ryan Hamilton wrote: > What does this method do when the underlying session is a server session? Then it just call QuicSession::Initialize(). I think from the view of WebRTC, it will be better for the session not to have many Server and Client specific things. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:34: // For forward-compatibility. We can add more parameters to the function On 2016/09/23 19:56:45, Ryan Hamilton wrote: > nit: Avoid using first person in comments. Done. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:46: virtual bool CanWrite() = 0; On 2016/09/23 19:56:44, Ryan Hamilton wrote: > nit: newlines between methods, please Done. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:50: // Implemented by QUIC code. On 2016/09/23 19:56:44, Ryan Hamilton wrote: > Can you add a comment to explain what this class does. I'm sure I'll be able to > figure it out when I see the implementation, but it would be nice if the > interface made it clear. Sure. I should have done this. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:51: class Observer { On 2016/09/23 19:56:45, Ryan Hamilton wrote: > I think the same Observer->Delegate change would be a good idea here. Done. https://codereview.chromium.org/2324833004/diff/260001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:59: class Observer { On 2016/09/23 19:56:45, Ryan Hamilton wrote: > I think the same Observer->Delegate change would be a good idea here. Done.
Hi Ryan, Please take take another look. This CL just adds some virtual destructors which are required when compiling quartc in WebRTC repo.
pthatcher@chromium.org changed reviewers: + pthatcher@chromium.org
I haven't look at the unit tests yet. The rest looks quite good, other than some improvement needed on the comments. I also had a few comments on names and some object structuring. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_alarm_factory.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.cc:85: }; So if the only thing different between the QuartcAlarm and the QuicChromeAlarm is the name, why don't we just use the QuicChromeAlarm? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_alarm_factory.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:19: class NET_EXPORT_PRIVATE QuartcAlarmFactory : public QuicAlarmFactory { Can you add a comment on the class? Something that explain why it's needed would be nice. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:33: // QuartcAlarmFactoryObserver* factory_observer_ = nullptr; Should this be deleted? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_connection_helper.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_connection_helper.h:15: // This is the helper class used to create QuicConnections. This comment could be a little more descriptive. Perhaps something like "It's a central point for creating or getting objects QuicConnection needs, such as clocks and buffer allocators". https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:25: config_.reset(new QuicConfig); Why do we bother storing this as a member variable? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:35: // The QuicConnection will take ownership. That might be more clear if you used std::unique_ptr and std::move. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:37: helper_.reset(new QuartcConnectionHelper); Why do we bother storing it in a member variable? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:40: IPAddress ip(0, 0, 0, 0); This could use a comment with some explanation. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:42: 0, IPEndPoint(ip, 0), helper_.get(), alarm_factory_.get(), writer, A local variable the 0 here a name would help readability. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.h:30: std::unique_ptr<QuicConfig> config_; Should this be quic_config_ to make it clear that it's not a QuartcConfig? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.h:30: std::unique_ptr<QuicConfig> config_; This could use a comment also. For example, where does the config come from, and what effect does it have? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.h:31: std::unique_ptr<QuartcConnectionHelper> helper_; Why do we have the QuartcConnetionHelper owned by the QuartcFactory? Why not just make the QuartcFactory be the QuartcConnectionHelper (own the clock and buffer allocator)? It already owns the QuartcAlarmFactory. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory_interface.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:12: // Used to create instances for QuartcSession. Either we should call it QuartcSessionFactory or the comment should say "Creates Quartc objects, such as QuartcSession". In other words, either we expect to someday create more than just a QuartcSession, in which case the comment should reflect that. Or we don't, in which case the name should reflect that. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:17: // This struct contains the required parameters to create a QuartcSession. You can probably just remove this comment. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:18: struct QuartcConfig { Shouldn't this be named QuartcSessionConfig? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:19: // Used to determine the perspective of the session. This could be more clear by saying something like "In a QuartcSession, there are two endpoints. One endpoint must be the server and one endpoint must be the client." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:23: std::string remote_fingerprint_value; Why not something like "unique_remote_server_id" Then the comment could just be something like "This is only needed when is_server = false. It must be unique for each endpoint the local endpoint may communicate with. For example, a WebRTC client could use the remote endpoint's crypto fingerprint." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:26: // take ownership of it. A better comment would be something like "The way the QuicConnection will send and receive packets, like a virtual UDP socket. For WebRTC, this will typically be an IceTransport." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:27: QuartcSessionInterface::Transport* transport = nullptr; Can we call this a QuartcSessionInterface::PacketTransport? That would make it more clear it's about the low-level thing moving packets. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.cc:40: return kMaxPacketSize; We may want to drop it somewhat so that TURN encapsulation doesn't cause a UDP packet to get too big. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:14: // implemented on WebRTC side. This might be more clear as "Implements a QuicPacketWriter using a QuartcSessionInterface::PacketTransport, which allows a QuicConnection to use (for example), a WebRTC IceTransport." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:20: // Called from QuicConnection when the session has packets to write. This would be more clear simply as "The QuicConnection calls WritePacket and the QuicPacketWriter writes them to the QuartcSessionInterface::PacketTransport." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:27: // If this is set to false, then QuicConnection buffers unsent packets. Please put the implementation here so it's clear it's always false. Then the comment can say "Returns false so the QuicConnection will buffer unset packets." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:36: QuicByteCount GetMaxPacketSize(const IPEndPoint& peer_address) const override; Same here: can you put the implementation here to make it more readable? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:40: void SetWritable() override; And here too https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:43: // QuartcPacketWriter will not own the transport. What does own the transport? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session.cc:240: // Decrypts an incoming QUIC packet to a data stream. I don't think this comment is needed. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_interface.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:14: // reading/writing of data using QUIC. The comment does need "This interface defines a ...". Plus, we don't need to be peer-to-peer specific. We could just say "Given a PacketTransport, provides a way to send and receive separate streams of reliable, in-order, encrypted data. For example, this can build on top of a WebRTC IceTransport for sending and receiving data over QUIC." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:48: // (hopefully UDP) packets. This would be more clear as "Send and receive packets, like a virtual UDP socket. For example, this could be implemented by WebRTC's IceTransport." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:54: // writable. "transport writable" => "transport is writable" And it could be more clear with a first sentence which says "True if packets written are expected to be sent. False if packets will be dropped." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:58: virtual int Write(const char* buffer, size_t buf_len) = 0; We should comment on what the return value is. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:61: // WebRTC side through the Transport. Would be more clear as "Callbacks called by the PacketTransport to notify the user of the PacketTransport of certain events". https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:66: // Called when the WebRTC network layer becomes writable. This would be more clear as "Called when CanWrite() changes from false to true." We don't need to reference WebRTC. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:70: // responsible for further processing such as decryption. I think this would be more clear as just "Called when a packet has been received and should be handled by the QuicConnection." We don't need to reference WebRTC. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:80: // from QUIC side. "Callbacks called by the QuartcSession to notify the user of the QuartcSession of certain events". https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:85: // Called when the crypto handshake complete. The WebRTC side will change "handshake complete" => "handshake is complete" https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:86: // the QUIC states based on this. No need to reference WebRTC here. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:89: // Called when the received the data from a incoming stream. More clear would be "Called when a new stream is received from the remote endpoint". https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:94: virtual void OnConnectionClosed(int error_code, bool from_remote) = 0; Needs something like "Called when the connection is closed. This means all of the streams will be closed and no new streams can be created." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:100: // called once. Why only once? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_stream.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream.cc:57: } Why not just set it to something different? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_stream.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream.h:15: public QuartcStreamInterface { This could use a comment like "Implements a QuartcStreamInterface using a ReliableQuicStream." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_stream_interface.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:11: // receive data. It would be more clear as "Sends and receives data with a particular QUIC stream ID, reliably and in-order. To send/receive data out of order, use separate streams. To send/receive unreliably, Close a stream after reliability is no longer needed." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:16: // Return the ID of the underlying QUIC stream. Just "The QUIC stream ID" would be enough. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:20: virtual uint64_t buffered_amount() = 0; Just "The amount of data buffered by the QuicConnection" would be enough. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:22: // For forward-compatibility This comment can probably be removed. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:24: bool fin = false; This could use a comment. When should one set fin = true? What effect does it have? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:27: // Write data to the stream. Perhaps "Sends data reliably and in-order, buffering if necessary until Close() is called." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:32: virtual void Close() = 0; This could use a comment like "Once Close is called, no more data can be sent, all buffered data will be dropped and no data will be retransmitted." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:32: virtual void Close() = 0; What happens to reading data when Close is called? Does it no longer receive any? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:34: // Implemented by the WebRTC code. This would be more clear as "Implemented by the user of the QuartcStreamInterface to receive incoming data and be notified of state changes." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:39: // Called when the data are ready(processed by QUIC). Would be more clear as "Called when data has been received" https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:45: // error code. Would be more clear as "Called when the stream is closed, either locally or by the remote endpoint". Also, we should document what the error codes are, or use an enum for them. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:48: // Called when the buffered data has been sent. When it changes or only when it decreases? If the former, the comment should say "Called when buffered_amount() changes". If the latter, the it should be called OnBufferedAmountDecreased and say "Called when buffered_amount() decreases". https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:53: // called once. Why should it only be called once? If the API user wants to change it, why not?
pthatcher has lots of good comments. here are a few additional from me. https://codereview.chromium.org/2324833004/diff/360001/net/quic/core/crypto/q... File net/quic/core/crypto/quic_crypto_server_config.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/core/crypto/q... net/quic/core/crypto/quic_crypto_server_config.cc:633: *error_details = "Missing or invalid crypto proof."; nit: This should probably land in a distinct CL and probably in the internal repo first. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_alarm_factory.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:32: const QuicClock* clock_; nit: Can you comment on ownership? (I assume both of these are unowned) https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:25: config_.reset(new QuicConfig); On 2016/10/05 22:12:05, pthatcher2 wrote: > Why do we bother storing this as a member variable? Agreed. This seems confusing because it means that from call to call, config_ changes. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.h:16: class QuartcFactory : public QuartcFactoryInterface { class comment, please. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session.h:18: // This class is used for creating the QuicCryptoServerStream. Is this used for "creating" the crypto stream or is it used by the crypto stream?
Patchset #7 (id:380001) has been deleted
Please take another look. Sorry for making such as a large CL. I just wanna make sure all the pieces can work together. https://codereview.chromium.org/2324833004/diff/360001/net/quic/core/crypto/q... File net/quic/core/crypto/quic_crypto_server_config.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/core/crypto/q... net/quic/core/crypto/quic_crypto_server_config.cc:633: *error_details = "Missing or invalid crypto proof."; On 2016/10/10 19:48:30, Ryan Hamilton wrote: > nit: This should probably land in a distinct CL and probably in the internal > repo first. Done. https://codereview.chromium.org/2302643003/ https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_alarm_factory.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:19: class NET_EXPORT_PRIVATE QuartcAlarmFactory : public QuicAlarmFactory { On 2016/10/05 22:12:04, pthatcher2 wrote: > Can you add a comment on the class? Something that explain why it's needed > would be nice. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:32: const QuicClock* clock_; On 2016/10/10 19:48:30, Ryan Hamilton wrote: > nit: Can you comment on ownership? (I assume both of these are unowned) Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:33: // QuartcAlarmFactoryObserver* factory_observer_ = nullptr; On 2016/10/05 22:12:04, pthatcher2 wrote: > Should this be deleted? Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_connection_helper.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_connection_helper.h:15: // This is the helper class used to create QuicConnections. On 2016/10/05 22:12:05, pthatcher2 wrote: > This comment could be a little more descriptive. Perhaps something like "It's a > central point for creating or getting objects QuicConnection needs, such as > clocks and buffer allocators". Merged the QuartcConnectionHelper with QuartcFactory. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:25: config_.reset(new QuicConfig); On 2016/10/10 19:48:30, Ryan Hamilton wrote: > On 2016/10/05 22:12:05, pthatcher2 wrote: > > Why do we bother storing this as a member variable? > > Agreed. This seems confusing because it means that from call to call, config_ > changes. I'll remove these member variables. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:35: // The QuicConnection will take ownership. On 2016/10/05 22:12:05, pthatcher2 wrote: > That might be more clear if you used std::unique_ptr and std::move. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:40: IPAddress ip(0, 0, 0, 0); On 2016/10/05 22:12:05, pthatcher2 wrote: > This could use a comment with some explanation. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:42: 0, IPEndPoint(ip, 0), helper_.get(), alarm_factory_.get(), writer, On 2016/10/05 22:12:05, pthatcher2 wrote: > A local variable the 0 here a name would help readability. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.h:30: std::unique_ptr<QuicConfig> config_; On 2016/10/05 22:12:05, pthatcher2 wrote: > Should this be quic_config_ to make it clear that it's not a QuartcConfig? Agreed. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory_interface.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:12: // Used to create instances for QuartcSession. On 2016/10/05 22:12:06, pthatcher2 wrote: > Either we should call it QuartcSessionFactory or the comment should say "Creates > Quartc objects, such as QuartcSession". In other words, either we expect to > someday create more than just a QuartcSession, in which case the comment should > reflect that. Or we don't, in which case the name should reflect that. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:17: // This struct contains the required parameters to create a QuartcSession. On 2016/10/05 22:12:05, pthatcher2 wrote: > You can probably just remove this comment. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:18: struct QuartcConfig { On 2016/10/05 22:12:05, pthatcher2 wrote: > Shouldn't this be named QuartcSessionConfig? Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:19: // Used to determine the perspective of the session. On 2016/10/05 22:12:06, pthatcher2 wrote: > This could be more clear by saying something like "In a QuartcSession, there are > two endpoints. One endpoint must be the server and one endpoint must be the > client." I think this should be something like "The QuartcSession must act as a Server on one endpoint and act as a Client on the other one." https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:23: std::string remote_fingerprint_value; On 2016/10/05 22:12:06, pthatcher2 wrote: > Why not something like "unique_remote_server_id" Then the comment could just be > something like "This is only needed when is_server = false. It must be unique > for each endpoint the local endpoint may communicate with. For example, a > WebRTC client could use the remote endpoint's crypto fingerprint." Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:27: QuartcSessionInterface::Transport* transport = nullptr; On 2016/10/05 22:12:06, pthatcher2 wrote: > Can we call this a QuartcSessionInterface::PacketTransport? That would make it > more clear it's about the low-level thing moving packets. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.cc:40: return kMaxPacketSize; On 2016/10/05 22:12:06, pthatcher2 wrote: > We may want to drop it somewhat so that TURN encapsulation doesn't cause a UDP > packet to get too big. Make it half? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:14: // implemented on WebRTC side. On 2016/10/05 22:12:06, pthatcher2 wrote: > This might be more clear as "Implements a QuicPacketWriter using a > QuartcSessionInterface::PacketTransport, which allows a QuicConnection to use > (for example), a WebRTC IceTransport." Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:27: // If this is set to false, then QuicConnection buffers unsent packets. On 2016/10/05 22:12:06, pthatcher2 wrote: > Please put the implementation here so it's clear it's always false. Then the > comment can say "Returns false so the QuicConnection will buffer unset packets." This is not allowed by the Chromium style checker. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:36: QuicByteCount GetMaxPacketSize(const IPEndPoint& peer_address) const override; On 2016/10/05 22:12:06, pthatcher2 wrote: > Same here: can you put the implementation here to make it more readable? Same here. This is not allowed because of Chromium style. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:43: // QuartcPacketWriter will not own the transport. On 2016/10/05 22:12:06, pthatcher2 wrote: > What does own the transport? The cricket::QuicTransportChannel will implement the PacketTransport and pass itself in. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session.cc:240: // Decrypts an incoming QUIC packet to a data stream. On 2016/10/05 22:12:06, pthatcher2 wrote: > I don't think this comment is needed. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session.h:18: // This class is used for creating the QuicCryptoServerStream. On 2016/10/10 19:48:30, Ryan Hamilton wrote: > Is this used for "creating" the crypto stream or is it used by the crypto > stream? Should be "used by" https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_interface.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:14: // reading/writing of data using QUIC. On 2016/10/05 22:12:07, pthatcher2 wrote: > The comment does need "This interface defines a ...". Plus, we don't need to be > peer-to-peer specific. We could just say "Given a PacketTransport, provides a > way to send and receive separate streams of reliable, in-order, encrypted data. > For example, this can build on top of a WebRTC IceTransport for sending and > receiving data over QUIC." Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:48: // (hopefully UDP) packets. On 2016/10/05 22:12:07, pthatcher2 wrote: > This would be more clear as "Send and receive packets, like a virtual UDP > socket. For example, this could be implemented by WebRTC's IceTransport." Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:54: // writable. On 2016/10/05 22:12:07, pthatcher2 wrote: > "transport writable" => "transport is writable" > > And it could be more clear with a first sentence which says "True if packets > written are expected to be sent. False if packets will be dropped." Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:58: virtual int Write(const char* buffer, size_t buf_len) = 0; On 2016/10/05 22:12:06, pthatcher2 wrote: > We should comment on what the return value is. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:61: // WebRTC side through the Transport. On 2016/10/05 22:12:07, pthatcher2 wrote: > Would be more clear as "Callbacks called by the PacketTransport to notify the > user of the PacketTransport of certain events". I found that moving the callbacks of PacketTransport::Delegate to QuartcSessionInterface will simplify the code. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:66: // Called when the WebRTC network layer becomes writable. On 2016/10/05 22:12:07, pthatcher2 wrote: > This would be more clear as "Called when CanWrite() changes from false to true." > We don't need to reference WebRTC. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:70: // responsible for further processing such as decryption. On 2016/10/05 22:12:07, pthatcher2 wrote: > I think this would be more clear as just "Called when a packet has been received > and should be handled by the QuicConnection." We don't need to reference > WebRTC. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:80: // from QUIC side. On 2016/10/05 22:12:07, pthatcher2 wrote: > "Callbacks called by the QuartcSession to notify the user of the QuartcSession > of certain events". Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:86: // the QUIC states based on this. On 2016/10/05 22:12:07, pthatcher2 wrote: > No need to reference WebRTC here. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:100: // called once. On 2016/10/05 22:12:07, pthatcher2 wrote: > Why only once? I feel that the delegate implies a kind of 1:1 relationship. Beside, it seems that other delegates in QUIC are only be set once. For example. https://cs.chromium.org/chromium/src/net/quic/chromium/quic_chromium_client_s... But I'm not sure if there are other consideration (delegate context switch?) about this. In our case, I think it will also be fine to replace the delegate. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_stream.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream.cc:57: } On 2016/10/05 22:12:07, pthatcher2 wrote: > Why not just set it to something different? Same as before. I feel that the delegate implies a kind of 1:1 relationship. Beside, it seems that other delegates in QUIC are only be set once. For example. https://cs.chromium.org/chromium/src/net/quic/chromium/quic_chromium_client_s... But I'm not sure if there are other consideration (delegate context switch?) about this. In our case, I think it will also be fine to replace the delegate. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_stream.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream.h:15: public QuartcStreamInterface { On 2016/10/05 22:12:07, pthatcher2 wrote: > This could use a comment like "Implements a QuartcStreamInterface using a > ReliableQuicStream." Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_stream_interface.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:11: // receive data. On 2016/10/05 22:12:08, pthatcher2 wrote: > It would be more clear as "Sends and receives data with a particular QUIC stream > ID, reliably and in-order. To send/receive data out of order, use separate > streams. To send/receive unreliably, Close a stream after reliability is no > longer needed." Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:16: // Return the ID of the underlying QUIC stream. On 2016/10/05 22:12:08, pthatcher2 wrote: > Just "The QUIC stream ID" would be enough. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:20: virtual uint64_t buffered_amount() = 0; On 2016/10/05 22:12:07, pthatcher2 wrote: > Just "The amount of data buffered by the QuicConnection" would be enough. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:22: // For forward-compatibility On 2016/10/05 22:12:08, pthatcher2 wrote: > This comment can probably be removed. Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:24: bool fin = false; On 2016/10/05 22:12:07, pthatcher2 wrote: > This could use a comment. When should one set fin = true? What effect does it > have? Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:27: // Write data to the stream. On 2016/10/05 22:12:07, pthatcher2 wrote: > Perhaps "Sends data reliably and in-order, buffering if necessary until Close() > is called." Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:32: virtual void Close() = 0; On 2016/10/05 22:12:07, pthatcher2 wrote: > This could use a comment like "Once Close is called, no more data can be sent, > all buffered data will be dropped and no data will be retransmitted." Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:39: // Called when the data are ready(processed by QUIC). On 2016/10/05 22:12:08, pthatcher2 wrote: > Would be more clear as "Called when data has been received" Done. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:45: // error code. On 2016/10/05 22:12:08, pthatcher2 wrote: > Would be more clear as "Called when the stream is closed, either locally or by > the remote endpoint". > > Also, we should document what the error codes are, or use an enum for them. Done. There are quite a few QuicErrorCode. Should we get some of them which we may interested in? https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:48: // Called when the buffered data has been sent. On 2016/10/05 22:12:08, pthatcher2 wrote: > When it changes or only when it decreases? If the former, the comment should > say "Called when buffered_amount() changes". If the latter, the it should be > called OnBufferedAmountDecreased and say "Called when buffered_amount() > decreases". In our case, the buffered amount will change only when buffered date is sent. It should be decrease only. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory_interface.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:8: #include "net/base/net_export.h" Same as before. This is a stand-alone header with compiling related macro. It will not introduce other indirect dependency
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
Mostly comments and names. The only structure comments are mostly around the AtExitManager and simplifying it. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.cc (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.cc:40: return kMaxPacketSize; On 2016/10/13 06:22:40, zhihuang1 wrote: > On 2016/10/05 22:12:06, pthatcher2 wrote: > > We may want to drop it somewhat so that TURN encapsulation doesn't cause a UDP > > packet to get too big. > > Make it half? No, that's way too drastic. I looked into our current video and SCTP code, and both use 1200: https://cs.chromium.org/chromium/src/third_party/webrtc/media/engine/webrtcvi... https://cs.chromium.org/chromium/src/third_party/webrtc/media/sctp/sctpdataen... So we probably want to use that. But I think passing it in via the config makes sense. https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:20: // Called from QuicConnection when the session has packets to write. On 2016/10/05 22:12:06, pthatcher2 wrote: > This would be more clear simply as "The QuicConnection calls WritePacket and the > QuicPacketWriter writes them to the QuartcSessionInterface::PacketTransport." Did you look at this comment? https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_alarm_factory.cc (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.cc:104: new QuartcAlarm(clock_, task_runner_, std::move(delegate))); I know it's minor, but the The factory takes (task_runner, clock), but the alarm takes (clock, task_runner, ...). Can you make it the same between them? (task_runner, clock, ...) seems more logical to me. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_alarm_factory.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:19: // Creates Chromium based QuartcAlarms used throughout QUIC. The alarm posts "Chromium based" => "Chromium-based" https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:20: // messages to Chromium message queue for tasks such as retransmission. "to Chromium message queue" => "to the Chromium message queue" https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:37: const QuicClock* clock_; If the task_runner_ has "= nullptr", shouldn't the clock_ as well? Or neither if they are both set in the constructor? https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_at_exit.cc (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_at_exit.cc:14: // protected. "as the" => "because the" https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_at_exit.cc:28: if (at_exit_manager_) { If the QuartcAtExitManager itsn't usable until after SetAtExitManager is called, and can't be changed after this is called, then basically this is an Init method. So why not call it that? But then that brings up the question: why not just put it in the constructor? https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_at_exit.cc:38: } An alternative is to rename AtExitManagerForTest to TestableAtExitManager and then make this: at_exit_manager_.reset(new TestableAtExitManager(test_mode)); https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_at_exit.cc:42: return std::unique_ptr<QuartcAtExitManagerInterface>(new QuartcAtExitManager); What's the point of having a function that just calls the constructor? Why not just call the constructor? https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.cc (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:30: CancelImpl(); Isn't SetImpl guaranteed to only be called after CancelImpl already is? https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:39: canceler_.reset(quartc_task_runner_->Schedule(this, delay_ms).release()); It seems like a std::move should be used. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:71: std::unique_ptr<net::QuartcTaskRunnerInterface::Canceler> canceler_; Sorry for the churn, but I thought of a better name for Canceler: ScheduledTask. The method Cancel() can be the same. It just seems like ScheduledTask better represents a return value of a method called Schedule(Task). https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.h:56: std::unique_ptr<QuartcTaskRunnerInterface> quartc_task_runner_; Could just be task_runner_, I think. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory_interface.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:42: QuartcTaskRunnerInterface* quartc_task_runner; "task_runner" is probably sufficient. And should it have "= nullptr" like the others? https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.cc (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.cc:35: // TODO(zhihuang) To figure out what is the better value to return. The "To figure out" => Figure out https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.cc:37: // the MTU. We should probably make this an option in the QuartcFactoryConfig with this as the default so that WebRTC (or other users) can set it without updating the QUIC code. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_session.h:36: const std::string& remote_fingerprint_value, Didn't you rename this to unique_remote_server_id? https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_interface.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:85: // code. "TODO(X)" => "TODO(X):" https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_task_runner_interface.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_task_runner_interface.h:31: class Canceler { Like I mentioned, I think ScheduledTask would be a good name for this. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_task_runner_interface.h:35: // Called when canceling a scheduled task. "Cancels a scheduled task, meaning the task will not be run". https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_task_runner_interface.h:39: // Called when scheduling a task. Return a Canceler of the specific task. "Schedules a task, which will be run after the given delay. A ScheduledTask may be used to cancel the task".
Patchset #8 (id:420001) has been deleted
Hi, Please take another look. I made some changes on the AtExitManager which simplified the code. I created a static AtExitManager in the QuartcFactory to make sure it is only initialized once. Please pay extra attention on that part. Thanks! https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.h (right): https://codereview.chromium.org/2324833004/diff/360001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.h:20: // Called from QuicConnection when the session has packets to write. On 2016/10/14 18:58:58, pthatcher1 wrote: > On 2016/10/05 22:12:06, pthatcher2 wrote: > > This would be more clear simply as "The QuicConnection calls WritePacket and > the > > QuicPacketWriter writes them to the QuartcSessionInterface::PacketTransport." > > Did you look at this comment? Ah yes, I did and I also made the changes. Sorry for not replying for this. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_alarm_factory.cc (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.cc:104: new QuartcAlarm(clock_, task_runner_, std::move(delegate))); On 2016/10/14 18:58:59, pthatcher1 wrote: > I know it's minor, but the The factory takes (task_runner, clock), but the alarm > takes (clock, task_runner, ...). Can you make it the same between them? > (task_runner, clock, ...) seems more logical to me. Agreed. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_alarm_factory.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:19: // Creates Chromium based QuartcAlarms used throughout QUIC. The alarm posts On 2016/10/14 18:58:59, pthatcher1 wrote: > "Chromium based" => "Chromium-based" Done. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:20: // messages to Chromium message queue for tasks such as retransmission. On 2016/10/14 18:58:59, pthatcher1 wrote: > "to Chromium message queue" => "to the Chromium message queue" Done. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_alarm_factory.h:37: const QuicClock* clock_; On 2016/10/14 18:58:59, pthatcher1 wrote: > If the task_runner_ has "= nullptr", shouldn't the clock_ as well? Or neither > if they are both set in the constructor? I'll remove this since it will be set in the constructor. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_at_exit.cc (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_at_exit.cc:14: // protected. On 2016/10/14 18:58:59, pthatcher1 wrote: > "as the" => "because the" Done. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_at_exit.cc:28: if (at_exit_manager_) { On 2016/10/14 18:58:59, pthatcher1 wrote: > If the QuartcAtExitManager itsn't usable until after SetAtExitManager is called, > and can't be changed after this is called, then basically this is an Init > method. So why not call it that? > > But then that brings up the question: why not just put it in the constructor? One reason I'm doing this when working in WebRTC, sometimes it is more flexible to determine the mode of the AtExitManager later. But I think you are right, this can be simplified. I have an idea to simplify this by keeping a static at_exit_manager in the quartc_factory since quartc_factory is the entry of all the quartc stuff. The at_exit_manager in the quartc factory will only be created once. So we can also get rid of the test_mode at_exit_manager. My only concern is the way to use static stuff in Chromium correctly. Please pay extra attention on that. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_at_exit.cc:38: } On 2016/10/14 18:58:59, pthatcher1 wrote: > An alternative is to rename AtExitManagerForTest to TestableAtExitManager and > then make this: > > at_exit_manager_.reset(new TestableAtExitManager(test_mode)); > I think you mean we can have something like this: "TestableAtExitManager(bool test_mode) : AtExitManager(test_mode) {}" https://cs.chromium.org/chromium/src/base/at_exit.cc?sq=package:chromium&rcl=... It seems that AtExitManager() is not equivalent to AtExitManager(false). So we may need something like: "TestableAtExitManager(bool test_mode) { if (test_mode) { AtExitManager(true); } else AtExitManager(); // This is the normal mode. }" https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_at_exit.cc:42: return std::unique_ptr<QuartcAtExitManagerInterface>(new QuartcAtExitManager); On 2016/10/14 18:58:59, pthatcher1 wrote: > What's the point of having a function that just calls the constructor? Why not > just call the constructor? I thought this QuartcAtExitManager is something like the QuartcFactory. WebRTC can only depend on the interface and we need an additional function to create instances of the QuartcAtExitManager. I have another way to simplify this. Please check the update. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.cc (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:30: CancelImpl(); On 2016/10/14 18:59:00, pthatcher1 wrote: > Isn't SetImpl guaranteed to only be called after CancelImpl already is? https://cs.chromium.org/chromium/src/net/quic/core/quic_alarm.h?q=quicalar&sq... When testing with WebRTC I found that calling CancelImpl is necessary. I'll look into to make sure I did't miss something. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:39: canceler_.reset(quartc_task_runner_->Schedule(this, delay_ms).release()); On 2016/10/14 18:59:00, pthatcher1 wrote: > It seems like a std::move should be used. The compiler complains when I use move. I must miss something there. I'll keep it like this for further review and make sure to fix it before submitting it. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:71: std::unique_ptr<net::QuartcTaskRunnerInterface::Canceler> canceler_; On 2016/10/14 18:59:00, pthatcher1 wrote: > Sorry for the churn, but I thought of a better name for Canceler: > ScheduledTask. The method Cancel() can be the same. It just seems like > ScheduledTask better represents a return value of a method called > Schedule(Task). Done. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.h:56: std::unique_ptr<QuartcTaskRunnerInterface> quartc_task_runner_; On 2016/10/14 18:59:00, pthatcher1 wrote: > Could just be task_runner_, I think. Done. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory_interface.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:42: QuartcTaskRunnerInterface* quartc_task_runner; On 2016/10/14 18:59:00, pthatcher1 wrote: > "task_runner" is probably sufficient. > > And should it have "= nullptr" like the others? I removed the "=nullptr" from other classes because it will be set in the constructor. But I think it make sense to have "=nullptr" here because it won't be set in the constructor of the Config. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_packet_writer.cc (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.cc:35: // TODO(zhihuang) To figure out what is the better value to return. The On 2016/10/14 18:59:00, pthatcher1 wrote: > "To figure out" => Figure out Done. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_packet_writer.cc:37: // the MTU. On 2016/10/14 18:59:00, pthatcher1 wrote: > We should probably make this an option in the QuartcFactoryConfig with this as > the default so that WebRTC (or other users) can set it without updating the QUIC > code. Agreed. This is a good point. Because the PacketTransport related stuff is in QuartcSessionInterface, I think it would make sense for max_packet_size to be part of QuartcSessionConfig. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_session.h:36: const std::string& remote_fingerprint_value, On 2016/10/14 18:59:00, pthatcher1 wrote: > Didn't you rename this to unique_remote_server_id? Ah. sorry for missing this. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_interface.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:85: // code. On 2016/10/14 18:59:00, pthatcher1 wrote: > "TODO(X)" => "TODO(X):" Done. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... File net/quic/quartc/quartc_task_runner_interface.h (right): https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_task_runner_interface.h:31: class Canceler { On 2016/10/14 18:59:00, pthatcher1 wrote: > Like I mentioned, I think ScheduledTask would be a good name for this. Done. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_task_runner_interface.h:35: // Called when canceling a scheduled task. On 2016/10/14 18:59:00, pthatcher1 wrote: > "Cancels a scheduled task, meaning the task will not be run". Done. https://codereview.chromium.org/2324833004/diff/400001/net/quic/quartc/quartc... net/quic/quartc/quartc_task_runner_interface.h:39: // Called when scheduling a task. Return a Canceler of the specific task. On 2016/10/14 18:59:00, pthatcher1 wrote: > "Schedules a task, which will be run after the given delay. A ScheduledTask may > be used to cancel the task". Done.
Other than the AtExitManager stuff, this looks good. https://codereview.chromium.org/2324833004/diff/440001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.cc (right): https://codereview.chromium.org/2324833004/diff/440001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:84: if (!at_exit_manager) { I think a simpler solution would be to just have a boolean in the factory_config which is .create_at_exit_manager. Then use a unique_ptr in the class and do this: if (factory_config.create_at_exit_manager) { at_exit_manager_.reset(new base::AtExitManager()); } That way, Chrome and unit tests can control it, and normal apps get behavior that just works. As written, this code may break and some other code created an AtExitManager and then QuartcFactory did as well. Also, we should specify in the class constructor comment that only one QuartcFactory should be created. https://codereview.chromium.org/2324833004/diff/440001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:88: at_exit_manager = new base::AtExitManager; What happened to the test one?
https://codereview.chromium.org/2324833004/diff/440001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.cc (right): https://codereview.chromium.org/2324833004/diff/440001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:84: if (!at_exit_manager) { On 2016/10/16 01:15:27, pthatcher1 wrote: > I think a simpler solution would be to just have a boolean in the factory_config > which is .create_at_exit_manager. Then use a unique_ptr in the class and do > this: > > if (factory_config.create_at_exit_manager) { > at_exit_manager_.reset(new base::AtExitManager()); > } > > > That way, Chrome and unit tests can control it, and normal apps get behavior > that just works. As written, this code may break and some other code created an > AtExitManager and then QuartcFactory did as well. > > Also, we should specify in the class constructor comment that only one > QuartcFactory should be created. Yes, I agree. We don't have much control on the AtExitManager in this way. This is the first solution I came up with. It works fine for most of the tests except the end-to-end tests. There are questions to answer. First, we need to determine when to create the QuartcFactory. I have two options. 1. Each QuicTransportChannel has its own factory. In this way, everything Quartc-related will be hidden from upper layer. 2. Create a QuartcFactory in the WebRtcSession then pass it down through TransportController to each QuicTransport and then finally to QuicTransportChannels. Second, we need to have a way to pass in a test mode factory config in the end-to-end test. The only created in a peerconnectioninterface proxy and we cannot do much about it . One solution I came up with is making some changes on the RtcConfig in the peerconnectioninterface. Instead of having a boolean enable-quic, we can have some enum like: enum class QuicMode { QUIC, TEST_QUIC, NON_QUIC } when initializing WebRtcSession, we can determine if the AtExitManager should be created based on the config. Sounds reasonable? https://codereview.chromium.org/2324833004/diff/440001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:88: at_exit_manager = new base::AtExitManager; On 2016/10/16 01:15:27, pthatcher1 wrote: > What happened to the test one? We need the test mode AtExitManager only in the unit tests. In the tests, we need two peers, each of them has at least one QuartcFactory. There will be a problem when creating more than one AtExitManager. If we can make sure that only one is created (either by controlling with the config or make it a global), we don't need it. Another downside of using test mode AtExitManager is that we have to control the order of the destructors because the AtExitManagers should be deleted in some order. So it is good to get rid of the test mode thing.
Patchset #9 (id:460001) has been deleted
Hi, Please take another look. There is one AtExitManager for each QuartcFactory now. I changed the QuartcFactoryConfig so that we can control if the QuartcFactroy should create an AtExitManager.
lgtm With a few nits https://codereview.chromium.org/2324833004/diff/440001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory.cc (right): https://codereview.chromium.org/2324833004/diff/440001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory.cc:84: if (!at_exit_manager) { On 2016/10/16 19:38:12, zhihuang1 wrote: > On 2016/10/16 01:15:27, pthatcher1 wrote: > > I think a simpler solution would be to just have a boolean in the > factory_config > > which is .create_at_exit_manager. Then use a unique_ptr in the class and do > > this: > > > > if (factory_config.create_at_exit_manager) { > > at_exit_manager_.reset(new base::AtExitManager()); > > } > > > > > > That way, Chrome and unit tests can control it, and normal apps get behavior > > that just works. As written, this code may break and some other code created > an > > AtExitManager and then QuartcFactory did as well. > > > > Also, we should specify in the class constructor comment that only one > > QuartcFactory should be created. > > Yes, I agree. We don't have much control on the AtExitManager in this way. > > This is the first solution I came up with. It works fine for most of the tests > except the end-to-end tests. There are questions to answer. > > First, we need to determine when to create the QuartcFactory. I have two > options. > 1. Each QuicTransportChannel has its own factory. In this way, everything > Quartc-related will be hidden from upper layer. > 2. Create a QuartcFactory in the WebRtcSession then pass it down through > TransportController to each QuicTransport and then finally to > QuicTransportChannels. > > Second, we need to have a way to pass in a test mode factory config in the > end-to-end test. The only created in a peerconnectioninterface proxy and we > cannot do much about it . One solution I came up with is making some changes on > the RtcConfig in the peerconnectioninterface. Instead of having a boolean > enable-quic, we can have some enum like: > enum class QuicMode { > QUIC, TEST_QUIC, NON_QUIC > } > > when initializing WebRtcSession, we can determine if the AtExitManager should be > created based on the config. > > Sounds reasonable? It depends on how we use QuicTransportChannels. I'm guessing that those will also have some kind of factory (such as WebRtcSession) and that thing can own the QuartcFactory. So, I agree with #2, basically. But we can figure out the details in later CLs. For now, this CL is sufficient. https://codereview.chromium.org/2324833004/diff/480001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory_interface.h (right): https://codereview.chromium.org/2324833004/diff/480001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:15: // QuartcFactory is expected to be created. This comment should be on the QuartcFactory, not the QuartcFactoryInterface, since it's an implementation detail. https://codereview.chromium.org/2324833004/diff/480001/net/quic/quartc/quartc... File net/quic/quartc/quartc_stream_test.cc (right): https://codereview.chromium.org/2324833004/diff/480001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_test.cc:74: void ActivateReliableStream(ReliableQuicStream* stream) { Should ActivateReliableStream also take a std::unique_ptr<ReliableQuicStream>?
Hi Ryan, This is LTGMed by Peter, please take a look. Thanks. https://codereview.chromium.org/2324833004/diff/480001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory_interface.h (right): https://codereview.chromium.org/2324833004/diff/480001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:15: // QuartcFactory is expected to be created. On 2016/10/24 17:42:49, pthatcher2 wrote: > This comment should be on the QuartcFactory, not the QuartcFactoryInterface, > since it's an implementation detail. Done. https://codereview.chromium.org/2324833004/diff/480001/net/quic/quartc/quartc... File net/quic/quartc/quartc_stream_test.cc (right): https://codereview.chromium.org/2324833004/diff/480001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_test.cc:74: void ActivateReliableStream(ReliableQuicStream* stream) { On 2016/10/24 17:42:49, pthatcher2 wrote: > Should ActivateReliableStream also take a std::unique_ptr<ReliableQuicStream>? Done.
On 2016/10/24 19:08:10, zhihuang1 wrote: > Hi Ryan, > > This is LTGMed by Peter, please take a look. Very exciting! LGTM
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by zhihuang@chromium.org
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by zhihuang@chromium.org
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
The CQ bit was unchecked by zhihuang@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #13 (id:560001) has been deleted
Patchset #12 (id:540001) has been deleted
Patchset #12 (id:580001) has been deleted
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
The CQ bit was unchecked by zhihuang@chromium.org
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by zhihuang@chromium.org
Patchset #13 (id:620001) has been deleted
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:500001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by zhihuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skvlad@chromium.org, pthatcher@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/2324833004/#ps640001 (title: "Fix the trybots issue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2324833004/diff/640001/net/quic/quartc/quartc... File net/quic/quartc/quartc_factory_interface.h (right): https://codereview.chromium.org/2324833004/diff/640001/net/quic/quartc/quartc... net/quic/quartc/quartc_factory_interface.h:8: #include "net/base/net_export.h" Please add includes for system types: #include <memory> for unique_ptr #include <stddef.h> for size_t #include <stdint.h> for uint64_t https://codereview.chromium.org/2324833004/diff/640001/net/quic/quartc/quartc... File net/quic/quartc/quartc_session_interface.h (right): https://codereview.chromium.org/2324833004/diff/640001/net/quic/quartc/quartc... net/quic/quartc/quartc_session_interface.h:7: Please add includes for system types: #include <stddef.h> for size_t #include <stdint.h> for uint64_t https://codereview.chromium.org/2324833004/diff/640001/net/quic/quartc/quartc... File net/quic/quartc/quartc_stream_interface.h (right): https://codereview.chromium.org/2324833004/diff/640001/net/quic/quartc/quartc... net/quic/quartc/quartc_stream_interface.h:6: #define NET_QUIC_QUARTC_QUARTC_STREAM_INTERFACE_H_ Please add includes for system types: #include <stddef.h> for size_t #include <stdint.h> for uint64_t https://codereview.chromium.org/2324833004/diff/640001/net/quic/quartc/quartc... File net/quic/quartc/quartc_task_runner_interface.h (right): https://codereview.chromium.org/2324833004/diff/640001/net/quic/quartc/quartc... net/quic/quartc/quartc_task_runner_interface.h:6: #define NET_QUIC_QUARTC_QUARTC_TASKRUNNER_INTERFACE_H_ Please add includes for system types: #include <memory> for unique_ptr #include <stdint.h> for uint64_t
Patchset #11 (id:600001) has been deleted
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. The API contains two parts. One is the interfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the observers which will be implemented on WebRTC side. The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... ========== to ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. The API contains two parts. One is the interfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the delegates and abstract classes which will be implemented on WebRTC side. The stable API will not introduce any QUIC specific dependencies to the API user (except the /base/net_export.h, which is a set of macro without other dependencies.). The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zhihuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skvlad@chromium.org, pthatcher@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/2324833004/#ps660001 (title: "Fix the TaskRunner issue on IOS-simulator.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by zhihuang@chromium.org
Patchset #12 (id:660001) has been deleted
Patchset #12 (id:680001) has been deleted
The CQ bit was checked by zhihuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by zhihuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skvlad@chromium.org, pthatcher@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/2324833004/#ps700001 (title: "Fix the IOS simulator and merge.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. The API contains two parts. One is the interfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the delegates and abstract classes which will be implemented on WebRTC side. The stable API will not introduce any QUIC specific dependencies to the API user (except the /base/net_export.h, which is a set of macro without other dependencies.). The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... ========== to ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. The API contains two parts. One is the interfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the delegates and abstract classes which will be implemented on WebRTC side. The stable API will not introduce any QUIC specific dependencies to the API user (except the /base/net_export.h, which is a set of macro without other dependencies.). The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:700001)
Message was sent while issue was closed.
Description was changed from ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. The API contains two parts. One is the interfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the delegates and abstract classes which will be implemented on WebRTC side. The stable API will not introduce any QUIC specific dependencies to the API user (except the /base/net_export.h, which is a set of macro without other dependencies.). The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... ========== to ========== Define a stable API between the RTC (WebRTC/Quartc) code and the QUIC code, so that both projects can operate independently without breaking the other. The API contains two parts. One is the interfaces which will be called on WebRTC side and be implemented in QUIC side. The other is the delegates and abstract classes which will be implemented on WebRTC side. The stable API will not introduce any QUIC specific dependencies to the API user (except the /base/net_export.h, which is a set of macro without other dependencies.). The design doc for this issue: https://docs.google.com/document/d/1Nmjn9-YgemT-CrttOVmC3POhjBWpuMq0A7PsixfHY... Committed: https://crrev.com/33e7a9c59b4933acedf9d1b23d0033affaa4cd5c Cr-Commit-Position: refs/heads/master@{#428778} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/33e7a9c59b4933acedf9d1b23d0033affaa4cd5c Cr-Commit-Position: refs/heads/master@{#428778} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
