Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
972963002:
Per-renderer WebSocket throttling (Closed)
DescriptionPer-renderer WebSocket throttling
Design Doc:
https://docs.google.com/document/d/1aw2oN5PKfk-1gLnBrlv1OwLA8K3-ykM2ckwX2lubTg4/edit?usp=sharing
BUG=459377
Committed: https://crrev.com/58d344316cf2b2cc110619c0fea4979ef143b3a9
Cr-Commit-Position: refs/heads/master@{#320074}
Patch Set 1 #Patch Set 2 : Style fixes, bug fixes. #
Total comments: 32
Patch Set 3 : #Patch Set 4 : Fix content_unittests #
Total comments: 20
Patch Set 5 : Fix off-by-one (Patch Set 4 accepts up to 256 pending connections, not 255). #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : Add unit tests. #Patch Set 9 : Fix content_unittests. #Patch Set 10 : Rebase. #
Total comments: 18
Patch Set 11 : WIP #Patch Set 12 : Split into smaller tests. #
Total comments: 4
Patch Set 13 : Add a comment / blank lines. #
Messages
Total messages: 21 (6 generated)
ricea@chromium.org changed reviewers: + ricea@chromium.org
https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.cc (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:84: if (num_pending_connections_ > 255) { // FIXME: 255 Nitpick: use "TODO(hiroshige)" rather than "FIXME" in Chromium code. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:153: DCHECK(num_pending_connections_ >= 0); Nitpick: Use DCHECK_GE(). https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:154: ++num_current_succeeded_connections_; Are we sure that num_current_succeeded_connections_ can't overflow? Maybe we should include a DCHECK? https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:249: int64 WebSocketDispatcherHost::CalculateDelayInMs() const { This would be a good place to have a link to the design doc to explain where this formula came from. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:139: int64 CalculateDelayInMs() const; This method could return a base::TimeDelta object, then it wouldn't need to specify "InMs" in the name. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:142: void FinishThrottlingPeriod(); This is not a very clear name for the method. Maybe "UpdateSuccessFailureCountsTimerCallback()"? Okay, that's not a great name either. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:157: // Timer and counters for Per-renderer WebSocket throttling. Nitpick: The P of per-renderer should be lowercase. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:158: base::RepeatingTimer<WebSocketDispatcherHost> timer_; A slightly more specific name might be better, like counting_period_timer_. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_host.cc (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.cc:394: channel_->SendFlowControl(pending_flow_control_quota_); I suggest setting pending_flow_control_quota_ to 0, just to be safe. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.cc:417: pending_flow_control_quota_ = quota; This should be += because flow control quota is always added to any remaining quota. It doesn't make any difference at the moment because the renderer only sends one FlowControl message before data is received, but the implementation could change in future. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.cc:435: dispatcher_->DoDropChannel(routing_id_, was_clean, code, reason); We should not use the values from the renderer in DoDropChannel. It should be was_clean = false, code = kWebSocketErrorAbnormalClosure and reason = "", the same as at websocket_channel.cc:484 ( https://code.google.com/p/chromium/codesearch#chromium/src/net/websockets/web... ) https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_host.h (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.h:42: int delay_in_ms); I would make this argument type a base::TimeDelta so that it is not necessary to specify the units, and to make it even more obvious what it is. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.h:55: bool handshakeSucceeded() const { return handshake_succeeded_; } This should be named handshake_succeeded() by Chromium style. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.h:97: // A negative value indicates there is no pending SendFlowControl(). I would just use 0 to mean no pending flow control. SendFlowControl(0) does nothing so there would be no point in calling it.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.cc (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:84: if (num_pending_connections_ > 255) { // FIXME: 255 On 2015/03/03 15:10:02, Adam Rice wrote: > Nitpick: use "TODO(hiroshige)" rather than "FIXME" in Chromium code. Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:153: DCHECK(num_pending_connections_ >= 0); On 2015/03/03 15:10:02, Adam Rice wrote: > Nitpick: Use DCHECK_GE(). Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:154: ++num_current_succeeded_connections_; On 2015/03/03 15:10:02, Adam Rice wrote: > Are we sure that num_current_succeeded_connections_ can't overflow? Maybe we > should include a DCHECK? About >9M successful (or failed) handshakes per second (i.e. 100ns per handshake) would lead to overflow. Changed to int64 and added DCHECK. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:249: int64 WebSocketDispatcherHost::CalculateDelayInMs() const { On 2015/03/03 15:10:02, Adam Rice wrote: > This would be a good place to have a link to the design doc to explain where > this formula came from. Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:139: int64 CalculateDelayInMs() const; On 2015/03/03 15:10:02, Adam Rice wrote: > This method could return a base::TimeDelta object, then it wouldn't need to > specify "InMs" in the name. Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:142: void FinishThrottlingPeriod(); On 2015/03/03 15:10:02, Adam Rice wrote: > This is not a very clear name for the method. Maybe > "UpdateSuccessFailureCountsTimerCallback()"? Okay, that's not a great name > either. How about "ThrottlingPeriodTimerCallback()" (and changing timer_ to throttling_period_timer_)? https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:157: // Timer and counters for Per-renderer WebSocket throttling. On 2015/03/03 15:10:02, Adam Rice wrote: > Nitpick: The P of per-renderer should be lowercase. Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_host.cc (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.cc:394: channel_->SendFlowControl(pending_flow_control_quota_); On 2015/03/03 15:10:03, Adam Rice wrote: > I suggest setting pending_flow_control_quota_ to 0, just to be safe. Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.cc:417: pending_flow_control_quota_ = quota; On 2015/03/03 15:10:02, Adam Rice wrote: > This should be += because flow control quota is always added to any remaining > quota. It doesn't make any difference at the moment because the renderer only > sends one FlowControl message before data is received, but the implementation > could change in future. Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.cc:435: dispatcher_->DoDropChannel(routing_id_, was_clean, code, reason); On 2015/03/03 15:10:03, Adam Rice wrote: > We should not use the values from the renderer in DoDropChannel. It should be > was_clean = false, code = kWebSocketErrorAbnormalClosure and reason = "", the > same as at websocket_channel.cc:484 ( > https://code.google.com/p/chromium/codesearch#chromium/src/net/websockets/web... > ) Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_host.h (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.h:42: int delay_in_ms); On 2015/03/03 15:10:03, Adam Rice wrote: > I would make this argument type a base::TimeDelta so that it is not necessary to > specify the units, and to make it even more obvious what it is. Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.h:55: bool handshakeSucceeded() const { return handshake_succeeded_; } On 2015/03/03 15:10:03, Adam Rice wrote: > This should be named handshake_succeeded() by Chromium style. Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_host.h:97: // A negative value indicates there is no pending SendFlowControl(). On 2015/03/03 15:10:03, Adam Rice wrote: > I would just use 0 to mean no pending flow control. SendFlowControl(0) does > nothing so there would be no point in calling it. Done.
https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.cc (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:154: ++num_current_succeeded_connections_; On 2015/03/04 06:02:37, hiroshige wrote: > On 2015/03/03 15:10:02, Adam Rice wrote: > > Are we sure that num_current_succeeded_connections_ can't overflow? Maybe we > > should include a DCHECK? > About >9M successful (or failed) handshakes per second (i.e. 100ns per > handshake) would lead to overflow. > Changed to int64 and added DCHECK. 9M successes per second would never be possible without radical architectural changes, but 9M failures per second might be within reach of the very fastest machines now that we've added a fast-path for failure. So I think switching to int64 was a good choice. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:142: void FinishThrottlingPeriod(); On 2015/03/04 06:02:37, hiroshige wrote: > On 2015/03/03 15:10:02, Adam Rice wrote: > > This is not a very clear name for the method. Maybe > > "UpdateSuccessFailureCountsTimerCallback()"? Okay, that's not a great name > > either. > How about "ThrottlingPeriodTimerCallback()" (and changing timer_ to > throttling_period_timer_)? Sounds good to me. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.cc (right): https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:27: // Max of pending connections per WebSocketDispatcherHost Nitpick: "Max number of" https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:160: DCHECK_LT(num_current_succeeded_connections_, kint64max / 2); I don't think the "/ 2" is doing anything useful here. It will just make anyone reading the code confused. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:253: DCHECK(static_cast<size_t>(num_pending_connections_) <= hosts_.size()); DCHECK_LE. Also, you should use checked_cast<> from base/safe_conversions.h instead of static_cast here. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:256: // Calculate delay according to I think "as described in" sounds nicer than "according to". https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:267: (1 << std::min(p + f / (s + 1), static_cast<int64_t>(16))) / 65536); I think it is probably clearer to write 16LL rather than static_cast<int64_t>(16) here. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:164: int64 num_current_succeeded_connections_; It seems this type should be written int64_t (this style change was made 2 years ago and I didn't notice until today!). We need to #include <stdint.h> and probably remove "base/basictypes.h". https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... File content/browser/renderer_host/websocket_host.cc (right): https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_host.cc:359: if (delay_ > base::TimeDelta::FromMilliseconds(0)) { You can just use base::TimeDelta() for a zero delay.
https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.cc (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:154: ++num_current_succeeded_connections_; On 2015/03/04 08:20:49, Adam Rice wrote: > On 2015/03/04 06:02:37, hiroshige wrote: > > On 2015/03/03 15:10:02, Adam Rice wrote: > > > Are we sure that num_current_succeeded_connections_ can't overflow? Maybe we > > > should include a DCHECK? > > About >9M successful (or failed) handshakes per second (i.e. 100ns per > > handshake) would lead to overflow. > > Changed to int64 and added DCHECK. > > 9M successes per second would never be possible without radical architectural > changes, but 9M failures per second might be within reach of the very fastest > machines now that we've added a fast-path for failure. So I think switching to > int64 was a good choice. Another approach would be to stop incrementing num_current_failed_connections_ if it reaches 1<<30, and stop incrementing num_current_succeeded_connections_ if it reaches 1<<26. Achieving millions of failures per second might be achievable now that we've made failing very cheap, but millions of successes per second will be impossible for the foreseeable future on a single thread.
https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:142: void FinishThrottlingPeriod(); On 2015/03/04 08:20:49, Adam Rice wrote: > On 2015/03/04 06:02:37, hiroshige wrote: > > On 2015/03/03 15:10:02, Adam Rice wrote: > > > This is not a very clear name for the method. Maybe > > > "UpdateSuccessFailureCountsTimerCallback()"? Okay, that's not a great name > > > either. > > How about "ThrottlingPeriodTimerCallback()" (and changing timer_ to > > throttling_period_timer_)? > > Sounds good to me. Done. https://codereview.chromium.org/972963002/diff/20001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:158: base::RepeatingTimer<WebSocketDispatcherHost> timer_; On 2015/03/03 15:10:02, Adam Rice wrote: > A slightly more specific name might be better, like counting_period_timer_. Done. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.cc (right): https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:27: // Max of pending connections per WebSocketDispatcherHost On 2015/03/04 08:20:50, Adam Rice wrote: > Nitpick: "Max number of" Done. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:160: DCHECK_LT(num_current_succeeded_connections_, kint64max / 2); On 2015/03/04 08:20:50, Adam Rice wrote: > I don't think the "/ 2" is doing anything useful here. It will just make anyone > reading the code confused. I added "/ 2" here because this variable is used like: "num_previous_succeeded_connections_ + num_current_succeeded_connections_". Or should we remove this DCHECK? (Now I made this int64_t, this cannot overflow in normal operations) https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:253: DCHECK(static_cast<size_t>(num_pending_connections_) <= hosts_.size()); On 2015/03/04 08:20:49, Adam Rice wrote: > DCHECK_LE. > > Also, you should use checked_cast<> from base/safe_conversions.h instead of > static_cast here. Done. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:256: // Calculate delay according to On 2015/03/04 08:20:49, Adam Rice wrote: > I think "as described in" sounds nicer than "according to". Done. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:267: (1 << std::min(p + f / (s + 1), static_cast<int64_t>(16))) / 65536); Sadly 16LL doesn't pass compilation; std::min requires arguments have exactly the same type, and int64_t is long in 64-bit Linux, 16LL is long long. (16L would also fail in 32-bit environment?) https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.h:164: int64 num_current_succeeded_connections_; On 2015/03/04 08:20:50, Adam Rice wrote: > It seems this type should be written int64_t (this style change was made 2 years > ago and I didn't notice until today!). > > We need to #include <stdint.h> and probably remove "base/basictypes.h". Done. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... File content/browser/renderer_host/websocket_host.cc (right): https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_host.cc:359: if (delay_ > base::TimeDelta::FromMilliseconds(0)) { On 2015/03/04 08:20:50, Adam Rice wrote: > You can just use base::TimeDelta() for a zero delay. Done.
Needs some tests adding to websocket_dispatcher_host_unittest.cc. We need to test at least the following cases: 1. Send 3 AddChannelRequest messages. Check that delay is 0. 2. Send 7 AddChannelRequest messages. Check that delay is > 0. 3. Send 16 AddChannelRequest messages. Check that delay is >=1000 and <= 5000. 4. Send 16 AddChannelRequest / DropChannel pairs. Check that delay is >= 1000 and <= 5000. It would be good to check that success counting works correctly as well if that is possible. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.cc (right): https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:160: DCHECK_LT(num_current_succeeded_connections_, kint64max / 2); On 2015/03/04 11:13:15, hiroshige wrote: > On 2015/03/04 08:20:50, Adam Rice wrote: > > I don't think the "/ 2" is doing anything useful here. It will just make > anyone > > reading the code confused. > I added "/ 2" here because this variable is used like: > "num_previous_succeeded_connections_ + num_current_succeeded_connections_". > > Or should we remove this DCHECK? > (Now I made this int64_t, this cannot overflow in normal operations) Yes, just remove it. Even if there is a bug in the code this DCHECK will never fire in practice. It's more trouble than it is worth. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:267: (1 << std::min(p + f / (s + 1), static_cast<int64_t>(16))) / 65536); On 2015/03/04 11:13:14, hiroshige wrote: > Sadly 16LL doesn't pass compilation; > std::min requires arguments have exactly the same type, and > int64_t is long in 64-bit Linux, 16LL is long long. > (16L would also fail in 32-bit environment?) Sorry. I just remembered how to do this properly. There is an INT64_C() macro in <stdint.h> which will add the platform-appropriate suffix. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... File content/browser/renderer_host/websocket_host.cc (right): https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_host.cc:434: // Per-renderer WebSocket throttling. s/Per/per/ Also in one other comment above. https://codereview.chromium.org/972963002/diff/120001/content/browser/rendere... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/120001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host.h:143: void ThrottlingPeriodTimerCallback(); Please add a comment such as // Rotates the counts of successful and failed connections for current // and previous time periods. Called every two minutes while the counts // are non-zero.
https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... File content/browser/renderer_host/websocket_dispatcher_host.cc (right): https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:160: DCHECK_LT(num_current_succeeded_connections_, kint64max / 2); On 2015/03/05 07:48:27, Adam Rice wrote: > On 2015/03/04 11:13:15, hiroshige wrote: > > On 2015/03/04 08:20:50, Adam Rice wrote: > > > I don't think the "/ 2" is doing anything useful here. It will just make > > anyone > > > reading the code confused. > > I added "/ 2" here because this variable is used like: > > "num_previous_succeeded_connections_ + num_current_succeeded_connections_". > > > > Or should we remove this DCHECK? > > (Now I made this int64_t, this cannot overflow in normal operations) > > Yes, just remove it. Even if there is a bug in the code this DCHECK will never > fire in practice. It's more trouble than it is worth. Done. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_dispatcher_host.cc:267: (1 << std::min(p + f / (s + 1), static_cast<int64_t>(16))) / 65536); On 2015/03/05 07:48:27, Adam Rice wrote: > On 2015/03/04 11:13:14, hiroshige wrote: > > Sadly 16LL doesn't pass compilation; > > std::min requires arguments have exactly the same type, and > > int64_t is long in 64-bit Linux, 16LL is long long. > > (16L would also fail in 32-bit environment?) > > Sorry. I just remembered how to do this properly. There is an INT64_C() macro in > <stdint.h> which will add the platform-appropriate suffix. Done. https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... File content/browser/renderer_host/websocket_host.cc (right): https://codereview.chromium.org/972963002/diff/80001/content/browser/renderer... content/browser/renderer_host/websocket_host.cc:434: // Per-renderer WebSocket throttling. On 2015/03/05 07:48:27, Adam Rice wrote: > s/Per/per/ > > Also in one other comment above. Done. https://codereview.chromium.org/972963002/diff/120001/content/browser/rendere... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/120001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host.h:143: void ThrottlingPeriodTimerCallback(); On 2015/03/05 07:48:27, Adam Rice wrote: > Please add a comment such as > > // Rotates the counts of successful and failed connections for current > // and previous time periods. Called every two minutes while the counts > // are non-zero. Done.
https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host.h:138: // Calculate delay for per-renderer WebSocket throttling. Should be "Calculates the delay" as mentioned at http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... ("These comments should be descriptive ("Opens the file") rather than imperative ("Open the file"); the comment describes the function, it does not tell the function what to do.") https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... File content/browser/renderer_host/websocket_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:52: default: Blank line above default: please. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:235: TEST_F(WebSocketDispatcherHostTest, PerRendererThrottling) { Please split this into three separate unit tests for the 0, non-0 and [1000, 5000] case. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:238: requested_protocols.push_back("hello"); This makes no difference to the outcome of the tests so please remove it. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:239: url::Origin origin("http://example.com/test"); An origin never contains a path. It should be just "http://example.com". https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:242: for (int i = 0; i < 256; ++i) { Since you are going to need this in several tests, it might be useful to factor it out into a separate method on the text fixture, for example: void AddMultipleChannels(GURL url, int number_of_channels_to_add); https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:266: for (int i = 16; i < 255; ++i) { You don't need to test every number. Tests should be written as if the CPU and compiler don't have bugs. Of course, in reality the CPU and compiler do have bugs, but they are rarely caught by unit tests, and they're not our problem here. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:280: // 256 AddChannelRequest are sent but are cancelled by DropChannel during s/during/while/ https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:294: if (i < 4) { I like to avoid having if statements in tests because it means the reader has to follow the logic to work out what is being tested. I think it is better to have many short tests that are explicit about what they are testing than a few cleverer tests.
Patchset #12 (id:240001) has been deleted
Patchset #12 (id:260001) has been deleted
https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host.h:138: // Calculate delay for per-renderer WebSocket throttling. On 2015/03/06 11:01:06, Adam Rice wrote: > Should be "Calculates the delay" as mentioned at > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm... > ("These comments should be descriptive ("Opens the file") rather than imperative > ("Open the file"); the comment describes the function, it does not tell the > function what to do.") Done. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... File content/browser/renderer_host/websocket_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:52: default: On 2015/03/06 11:01:06, Adam Rice wrote: > Blank line above default: please. Done. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:235: TEST_F(WebSocketDispatcherHostTest, PerRendererThrottling) { On 2015/03/06 11:01:06, Adam Rice wrote: > Please split this into three separate unit tests for the 0, non-0 and [1000, > 5000] case. Done. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:238: requested_protocols.push_back("hello"); On 2015/03/06 11:01:06, Adam Rice wrote: > This makes no difference to the outcome of the tests so please remove it. Done. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:239: url::Origin origin("http://example.com/test"); On 2015/03/06 11:01:06, Adam Rice wrote: > An origin never contains a path. It should be just "http://example.com". Done. Also fixed other tests in this file. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:242: for (int i = 0; i < 256; ++i) { On 2015/03/06 11:01:06, Adam Rice wrote: > Since you are going to need this in several tests, it might be useful to factor > it out into a separate method on the text fixture, for example: > > void AddMultipleChannels(GURL url, int number_of_channels_to_add); Done. Omitted |url| parameter because there is no need to set different URLs for the added tests. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:266: for (int i = 16; i < 255; ++i) { On 2015/03/06 11:01:06, Adam Rice wrote: > You don't need to test every number. Tests should be written as if the CPU and > compiler don't have bugs. > > Of course, in reality the CPU and compiler do have bugs, but they are rarely > caught by unit tests, and they're not our problem here. Done. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:280: // 256 AddChannelRequest are sent but are cancelled by DropChannel during On 2015/03/06 11:01:06, Adam Rice wrote: > s/during/while/ Done. https://codereview.chromium.org/972963002/diff/200001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:294: if (i < 4) { Split into separate tests.
lgtm after adding a comment to MockWebSocketDispatcherHost::Send. https://codereview.chromium.org/972963002/diff/280001/content/browser/rendere... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/280001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host.h:169: // The current number of pending connections. (Optional) I think this section would look better with a blank line before each comment. https://codereview.chromium.org/972963002/diff/280001/content/browser/rendere... File content/browser/renderer_host/websocket_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/972963002/diff/280001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:75: bool Send(IPC::Message* message) override { Add a comment like // This is needed because BrowserMessageFilter::Send() tries post the task to the IO thread, which doesn't exist in the context of these tests.
Done. Thanks for reviewing and providing useful links! https://codereview.chromium.org/972963002/diff/280001/content/browser/rendere... File content/browser/renderer_host/websocket_dispatcher_host.h (right): https://codereview.chromium.org/972963002/diff/280001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host.h:169: // The current number of pending connections. On 2015/03/11 07:20:50, Adam Rice wrote: > (Optional) I think this section would look better with a blank line before each > comment. Done. https://codereview.chromium.org/972963002/diff/280001/content/browser/rendere... File content/browser/renderer_host/websocket_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/972963002/diff/280001/content/browser/rendere... content/browser/renderer_host/websocket_dispatcher_host_unittest.cc:75: bool Send(IPC::Message* message) override { On 2015/03/11 07:20:50, Adam Rice wrote: > Add a comment like > > // This is needed because BrowserMessageFilter::Send() tries post the task to > the IO thread, which doesn't exist in the context of these tests. Done.
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ricea@chromium.org Link to the patchset: https://codereview.chromium.org/972963002/#ps300001 (title: "Add a comment / blank lines.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/972963002/300001
Message was sent while issue was closed.
Committed patchset #13 (id:300001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/58d344316cf2b2cc110619c0fea4979ef143b3a9 Cr-Commit-Position: refs/heads/master@{#320074}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:300001) has been created in https://codereview.chromium.org/1009303002/ by henrika@chromium.org. The reason for reverting is: Suspected to have caused WebSocket breakage. Failure seen on apprtc.appspot.com and in browser_tests. Run the manual MANUAL_WorksWithAppRTC test for repro. See https://code.google.com/p/chromium/issues/detail?id=467471&thanks=467471&ts=1... for details. . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
