Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2652973002:
[scheduler] Plumb websocket information to scheduler (Closed)
Description[scheduler] Plumb websocket information to scheduler
Notify scheduler about websocket connections and stop budget-based
throttling when one is present.
BUG=639852
Review-Url: https://codereview.chromium.org/2652973002
Cr-Commit-Position: refs/heads/master@{#449243}
Committed: https://chromium.googlesource.com/chromium/src/+/0618218ad2514070703af6237b0ae3579276b433
Patch Set 1 #Patch Set 2 : Removed include #
Total comments: 25
Patch Set 3 : Addressed comments from alexclarke@ #Patch Set 4 : Rebased #Patch Set 5 : Introduce WebFrameScheduler::ActiveConnectionHandle #
Total comments: 16
Patch Set 6 : Addressed comments & added simtest #
Total comments: 4
Patch Set 7 : git #Messages
Total messages: 40 (21 generated)
Description was changed from ========== [scheduler] Plumb websocket information to scheduler ========== to ========== [scheduler] Plumb websocket information to scheduler Notify scheduler about websocket connections and stop budget-based throttling when one is present. BUG=639852 ==========
altimin@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
altimin@chromium.org changed reviewers: + haraken@chromium.org, yhirano@chromium.org
PTAL
https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:147: DCHECK(!m_blobLoader); How well do we understand the lifetime of this object? It's GarbageCollectedFinalized so this destructor does get called. Is it possible for DocumentWebSocketChannel::didConnect to get called but not one of DocumentWebSocketChannel::didFail / DocumentWebSocketChannel::didClose? https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:465: } nit: although I prefer the google3 style braces I think chromium style is to not have there in this case. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:513: } Ditto. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:65: virtual void didCloseWebSocket() {} I wonder if these should really all be pure virtual (possibly not in this patch), Sami / Haraken WDYT? https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:182: --websocket_connections_; DCHECK_GE(websocket_connections, 0); https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h:84: int websocket_connections_; Lets be explicit: websocket_connection_count_ https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:262: has_websocket_connection = nit ||= ? https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:365: if (has_websocket_connection_) { I'd favor this for tersness: if (page_visible_ || has_websocket_connection_) { should_throttle_frames_ = false; for (WebFrameSchedulerImpl* frame_scheduler : frame_schedulers_) { frame_scheduler->setPageThrottled(!page_visible_); } UpdateBackgroundBudgetPoolThrottlingState(); } else { // TODO(altimin): Consider moving this logic into PumpThrottledTasks. renderer_scheduler_->ControlTaskRunner()->PostDelayedTask( FROM_HERE, delayed_background_throttling_enabler_.callback(), kBackgroundThrottlingGracePeriod); } https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:109: int has_websocket_connection_; bool? https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:110: int websocket_connections_; unused? https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc:688: TEST_F(WebViewSchedulerImplTest, WebSocketExemptsFromBudgetThrottling) { Maybe OpenWebSocketExemptsFromBudgetThrottling
PTAL https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:147: DCHECK(!m_blobLoader); On 2017/01/24 18:15:33, alex clarke wrote: > How well do we understand the lifetime of this object? It's > GarbageCollectedFinalized so this destructor does get called. Is it possible for > DocumentWebSocketChannel::didConnect to get called but not one of > DocumentWebSocketChannel::didFail / DocumentWebSocketChannel::didClose? Done. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:465: } On 2017/01/24 18:15:33, alex clarke wrote: > nit: although I prefer the google3 style braces I think chromium style is to not > have there in this case. Acknowledged. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/websockets/DocumentWebSocketChannel.cpp:513: } On 2017/01/24 18:15:33, alex clarke wrote: > Ditto. Acknowledged. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:182: --websocket_connections_; On 2017/01/24 18:15:33, alex clarke wrote: > DCHECK_GE(websocket_connections, 0); Done. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h:84: int websocket_connections_; On 2017/01/24 18:15:33, alex clarke wrote: > Lets be explicit: websocket_connection_count_ Done. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:262: has_websocket_connection = On 2017/01/24 18:15:34, alex clarke wrote: > nit ||= ? C++ does not have operator ||= :) Used |= instead. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:365: if (has_websocket_connection_) { On 2017/01/24 18:15:34, alex clarke wrote: > I'd favor this for tersness: > > if (page_visible_ || has_websocket_connection_) { > should_throttle_frames_ = false; > for (WebFrameSchedulerImpl* frame_scheduler : frame_schedulers_) { > frame_scheduler->setPageThrottled(!page_visible_); > } > UpdateBackgroundBudgetPoolThrottlingState(); > } else { > // TODO(altimin): Consider moving this logic into PumpThrottledTasks. > renderer_scheduler_->ControlTaskRunner()->PostDelayedTask( > FROM_HERE, delayed_background_throttling_enabler_.callback(), > kBackgroundThrottlingGracePeriod); > } Personally, I don't like this option. For example, you forgot about should_throttle_frames_ handling. I believe that current option is more readable. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:109: int has_websocket_connection_; On 2017/01/24 18:15:34, alex clarke wrote: > bool? Done. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:110: int websocket_connections_; On 2017/01/24 18:15:34, alex clarke wrote: > unused? Done. https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc:688: TEST_F(WebViewSchedulerImplTest, WebSocketExemptsFromBudgetThrottling) { On 2017/01/24 18:15:34, alex clarke wrote: > Maybe OpenWebSocketExemptsFromBudgetThrottling Done.
The CQ bit was checked by altimin@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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:65: virtual void didCloseWebSocket() {} On 2017/01/24 18:15:33, alex clarke wrote: > I wonder if these should really all be pure virtual (possibly not in this > patch), Sami / Haraken WDYT? Yeah, pure virtual sounds nicer.
Managing m_notifiedDocumentAboutConnection manually looks tiresome and error-prone. Do you have any utilities that can be shared with other throttling targets?
PTAL https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:65: virtual void didCloseWebSocket() {} On 2017/01/24 21:49:08, haraken wrote: > On 2017/01/24 18:15:33, alex clarke wrote: > > I wonder if these should really all be pure virtual (possibly not in this > > patch), Sami / Haraken WDYT? > > Yeah, pure virtual sounds nicer. I prefer keeping EmptyFrameScheduler free of unneeded overrides.
The CQ bit was checked by altimin@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: This issue passed the CQ dry run.
Overall logic seems fine to me. Could we add a new test suite (probably simtest-based) that exercises the WebSocket Web APIs and checks that things aren't throttled? I'm hoping we can add more cases there in the future (IndexedDB, postMessage, WebRTC, notifications, etc.) https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:65: virtual void didCloseWebSocket() {} On 2017/01/26 11:26:15, altimin wrote: > On 2017/01/24 21:49:08, haraken wrote: > > On 2017/01/24 18:15:33, alex clarke wrote: > > > I wonder if these should really all be pure virtual (possibly not in this > > > patch), Sami / Haraken WDYT? > > > > Yeah, pure virtual sounds nicer. > > I prefer keeping EmptyFrameScheduler free of unneeded overrides. I think everything here should be pure virtual. The reason the existing methods aren't is because the header and the implementation used to live in different repositories, but we can't use that excuse any longer :) https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:82: // Notifies scheduler that this frame has established an active connection nit: mention that this means real time connections which imply the frame shouldn't be throttled. https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h:100: int active_connections_count_; nit: active_connection_count_ to avoid the double plural. https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:365: // If websocket is active, update state immediately to stop throttling. Ditto about WebSockets. https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:90: // number of websocket connections. s/websocket/active/ since we're not limiting these to WebSockets.
https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:65: virtual void didCloseWebSocket() {} On 2017/01/26 15:53:34, Sami wrote: > On 2017/01/26 11:26:15, altimin wrote: > > On 2017/01/24 21:49:08, haraken wrote: > > > On 2017/01/24 18:15:33, alex clarke wrote: > > > > I wonder if these should really all be pure virtual (possibly not in this > > > > patch), Sami / Haraken WDYT? > > > > > > Yeah, pure virtual sounds nicer. > > > > I prefer keeping EmptyFrameScheduler free of unneeded overrides. > > I think everything here should be pure virtual. The reason the existing methods > aren't is because the header and the implementation used to live in different > repositories, but we can't use that excuse any longer :) Fell free to add TODO(alexclarke): Make this API pure virtual https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:21: class ActiveConnectionHandle { So I like the RAII style handle (we should think about using that pattern elsewhere), although I'm a little sad we have to do a heap allocation (returning std::unique_ptr). Could you add a reset method which just clears the weak ptr in the impl? https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:24: virtual ~ActiveConnectionHandle() {} Please make this pure virtual. https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:40: if (frame_scheduler) Nit: if (frame_scheduler_) frame_scheduler_->didCloseActiveConnection();
https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:21: class ActiveConnectionHandle { On 2017/01/26 16:02:36, alex clarke wrote: > So I like the RAII style handle (we should think about using that pattern > elsewhere), although I'm a little sad we have to do a heap allocation (returning > std::unique_ptr). > > Could you add a reset method which just clears the weak ptr in the impl? Actually that would only work if this wasn't virtual. Can probably ignore.
https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:21: class ActiveConnectionHandle { On 2017/01/26 16:13:56, alex clarke wrote: > On 2017/01/26 16:02:36, alex clarke wrote: > > So I like the RAII style handle (we should think about using that pattern > > elsewhere), although I'm a little sad we have to do a heap allocation > (returning > > std::unique_ptr). > > > > Could you add a reset method which just clears the weak ptr in the impl? > > Actually that would only work if this wasn't virtual. Can probably ignore. Actually we can do that with move-only types: class WebFrameSchedulerImpl; class ActiveConnectionHandle { ActiveConnectionHandle(WebFrameSchedulerImpl* frame_scheduler); ~ActiveConnectionHandle(); void reset(); private: base::WeakPtr<WebFrameSchedulerImpl> frame_scheduler_; DISALLOW_COPY_AND_ASSIGN(ACH); }; I don't like that it uses base::WeakPtr from WebFrameScheduler. I'm not sure if we're allowed to do it. WDYT about this?
https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:21: class ActiveConnectionHandle { On 2017/01/26 16:23:13, altimin wrote: > On 2017/01/26 16:13:56, alex clarke wrote: > > On 2017/01/26 16:02:36, alex clarke wrote: > > > So I like the RAII style handle (we should think about using that pattern > > > elsewhere), although I'm a little sad we have to do a heap allocation > > (returning > > > std::unique_ptr). > > > > > > Could you add a reset method which just clears the weak ptr in the impl? > > > > Actually that would only work if this wasn't virtual. Can probably ignore. > > Actually we can do that with move-only types: > > class WebFrameSchedulerImpl; > > class ActiveConnectionHandle { > ActiveConnectionHandle(WebFrameSchedulerImpl* frame_scheduler); > ~ActiveConnectionHandle(); > void reset(); > private: > base::WeakPtr<WebFrameSchedulerImpl> frame_scheduler_; > > DISALLOW_COPY_AND_ASSIGN(ACH); > }; > > I don't like that it uses base::WeakPtr from WebFrameScheduler. I'm not sure if > we're allowed to do it. > > WDYT about this? Can use WTF::WeakPtr (which is an alias for base::WeakPtr). I think I may changed the task queue voters to do that.
modules/websockets lgtm
PTAL yhirano@: I've made minor changes to DocumentWebSocketChannel (moved connection handle initialization to DWSC::connect from DWSC::didConnect and added handle resets to DWSC::fail and DWSC::disconnect). https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/WebFrameScheduler.h (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:24: virtual ~ActiveConnectionHandle() {} On 2017/01/26 16:02:36, alex clarke wrote: > Please make this pure virtual. We can't have pure virtual destructors. https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/WebFrameScheduler.h:82: // Notifies scheduler that this frame has established an active connection On 2017/01/26 15:53:34, Sami wrote: > nit: mention that this means real time connections which imply the frame > shouldn't be throttled. Done. https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc:40: if (frame_scheduler) On 2017/01/26 16:02:36, alex clarke wrote: > Nit: if (frame_scheduler_) frame_scheduler_->didCloseActiveConnection(); Done. https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h:100: int active_connections_count_; On 2017/01/26 15:53:34, Sami wrote: > nit: active_connection_count_ to avoid the double plural. Done. https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:365: // If websocket is active, update state immediately to stop throttling. On 2017/01/26 15:53:34, Sami wrote: > Ditto about WebSockets. Done. https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2652973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:90: // number of websocket connections. On 2017/01/26 15:53:34, Sami wrote: > s/websocket/active/ since we're not limiting these to WebSockets. Done.
lgtm % one question about the test https://codereview.chromium.org/2652973002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp (right): https://codereview.chromium.org/2652973002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp:18: SimRequest mainResource("https://example.com/", "text/html"); In the other tests we needed to webView().settings()->setJavaScriptEnabled(true). No longer needed? https://codereview.chromium.org/2652973002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp:26: EXPECT_TRUE(webView().scheduler()->hasActiveConnectionForTest()); Please also check this is false before loading anything.
The CQ bit was checked by altimin@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 checked by altimin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2652973002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp (right): https://codereview.chromium.org/2652973002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp:18: SimRequest mainResource("https://example.com/", "text/html"); On 2017/02/08 17:26:58, Sami wrote: > In the other tests we needed to > webView().settings()->setJavaScriptEnabled(true). No longer needed? I'm not 100% sure, but it seems so. (This test failed initially and it's ok after I made some changes to modules/websocket. So we can be reasonably sure that javascript is working). https://codereview.chromium.org/2652973002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/ActiveConnectionThrottlingTest.cpp:26: EXPECT_TRUE(webView().scheduler()->hasActiveConnectionForTest()); On 2017/02/08 17:26:58, Sami wrote: > Please also check this is false before loading anything. Done.
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: This issue passed the CQ dry run.
still lgtm
LGTM
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2652973002/#ps120001 (title: "git")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486631417810230,
"parent_rev": "8b4749e153da27c06c59b5a104fcffd5164842b3", "commit_rev":
"0618218ad2514070703af6237b0ae3579276b433"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Plumb websocket information to scheduler Notify scheduler about websocket connections and stop budget-based throttling when one is present. BUG=639852 ========== to ========== [scheduler] Plumb websocket information to scheduler Notify scheduler about websocket connections and stop budget-based throttling when one is present. BUG=639852 Review-Url: https://codereview.chromium.org/2652973002 Cr-Commit-Position: refs/heads/master@{#449243} Committed: https://chromium.googlesource.com/chromium/src/+/0618218ad2514070703af6237b0a... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0618218ad2514070703af6237b0a... |
