Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925963002:
Create NetworkQuietDetector. (Closed)
DescriptionCreate NetworkQuietDetector.
This patch implements network quiet detection associated to Document, it
observes active network connection, and signals network quiet signal to GRC
when there are no more than 2 connections in 1 second.
We want to eventually make FMPDetector use NetworkQuietDetector, but we split
the patch to make sure the network quiet signal won't be affected on the merge,
and vice versa. Follow-up patches will decouple the network quiet detection from
FMPDetector, and make FMPDetector as a client of NetworkQuietDetector.
BUG=730098
Review-Url: https://codereview.chromium.org/2925963002
Cr-Commit-Position: refs/heads/master@{#479540}
Committed: https://chromium.googlesource.com/chromium/src/+/36cd3153a788c9cb0ccfce928d5aef91504acc95
Patch Set 1 #Patch Set 2 : Fix nullptr crash #Patch Set 3 : Send network idle signal to GRC #
Total comments: 4
Patch Set 4 : NetworkQuietDetector as Supplement<Document> #Patch Set 5 : Updated comments and rebased #
Total comments: 2
Patch Set 6 : Add kNetworkIdle state. #
Total comments: 12
Patch Set 7 : Addressed comments and rebased #
Total comments: 9
Patch Set 8 : Addressed comments #Patch Set 9 : Dont create NetworkQuietDetector when FrameResourceCoordinator is not enabled #
Total comments: 1
Messages
Total messages: 67 (42 generated)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Description was changed from ========== iCreate NetworkQuietDetector. This patch implements network quiet detection associated to FrameFetchContext, it observes active network connection, and signals network idleness signal to GRC when there are no more than 2 connection in 1 second. BUG=730098 ========== to ========== Create NetworkQuietDetector. This patch implements network quiet detection associated to FrameFetchContext, it observes active network connection, and signals network idleness signal to GRC when there are no more than 2 connection in 1 second. BUG=730098 ==========
Description was changed from ========== Create NetworkQuietDetector. This patch implements network quiet detection associated to FrameFetchContext, it observes active network connection, and signals network idleness signal to GRC when there are no more than 2 connection in 1 second. BUG=730098 ========== to ========== Create NetworkQuietDetector. This patch implements network quiet detection associated to FrameFetchContext, it observes active network connection, and signals network idleness signal to GRC when there are no more than 2 connections in 1 second. BUG=730098 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lpy@chromium.org changed reviewers: + zhenw@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lpy@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.
The CQ bit was checked by lpy@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...
zhenw@google.com changed reviewers: + zhenw@google.com
It is probably better to split this CL into 2 CLs. The 1st one creates the NetworkQuietDetector and update FMPDetector to use it. The 2nd CL talks to GRC to send signals. The reviewers for those 2 CLs will be different, too. https://codereview.chromium.org/2925963002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2925963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:176: network_quiet_detector_ = new NetworkQuietDetector(document); I think there is a question about whether NetworkQuietDetector should be scoped to FrameFetchContext or Document. I lean toward Document scope. First of all, this line here actually means NetworkQuietDetector is effectively scoped to Document. And scoping to Document means it can also be used by FMP, TTI, etc. https://codereview.chromium.org/2925963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:512: network_quiet_detector_->CheckNetworkStable(); I think the plan is to let FMPDetector also use this network quiet detector?
On 2017/06/07 17:32:56, zhenw wrote: > It is probably better to split this CL into 2 CLs. The 1st one creates the > NetworkQuietDetector and update FMPDetector to use it. The 2nd CL talks to GRC > to send signals. The reviewers for those 2 CLs will be different, too. This patch aims to create the simplest version of network quiet detector + sends a signal, and we use a different timeout for our loading done signal. Until we finish our experiment, I think it's better we don't touch FMP for now. I have a prototype that does the merge https://codereview.chromium.org/2927633002/
The CQ bit was checked by lpy@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 ========== Create NetworkQuietDetector. This patch implements network quiet detection associated to FrameFetchContext, it observes active network connection, and signals network idleness signal to GRC when there are no more than 2 connections in 1 second. BUG=730098 ========== to ========== Create NetworkQuietDetector. This patch implements network quiet detection associated to Document, it observes active network connection, and signals network idleness signal to GRC when there are no more than 2 connections in 1 second. We want to eventually make FMPDetector use NetworkQuietDetector, but we split the patch to make sure the network quiet signal won't be affected on the merge, and vice versa. Follow-up patches will decouple the network quiet detection from FMPDetector, and make FMPDetector as a client of NetworkQuietDetector. BUG=730098 ==========
I updated the patch to reflect our discussion offline. https://codereview.chromium.org/2925963002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2925963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:176: network_quiet_detector_ = new NetworkQuietDetector(document); On 2017/06/07 17:32:56, zhenw wrote: > I think there is a question about whether NetworkQuietDetector should be scoped > to FrameFetchContext or Document. > > I lean toward Document scope. First of all, this line here actually means > NetworkQuietDetector is effectively scoped to Document. And scoping to Document > means it can also be used by FMP, TTI, etc. Done. https://codereview.chromium.org/2925963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:512: network_quiet_detector_->CheckNetworkStable(); On 2017/06/07 17:32:56, zhenw wrote: > I think the plan is to let FMPDetector also use this network quiet detector? Discussed offline, updated the description to reflect the plan.
lpy@chromium.org changed reviewers: + japhet@chromium.org, oysteine@chromium.org
+japhet@ for Blink review on loader/ +oysteine@ for GRC Nate, Oystein, ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by lpy@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by lpy@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...
https://codereview.chromium.org/2925963002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): https://codereview.chromium.org/2925963002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:34: SetNetworkQuietTimers(ActiveConnections()); Does it mean ActiveConnections() will decrease monotonically here? I am not sure if it is such case. CheckNetworkStable() is called in FrameFetchContext::DidLoadResource(), which just means it finishes loading a resource. But some other place can start loading new resources, right? For example: At time t1, there are 2 connections left. At t2, 1 connections is finished, we have 1 left. But if between t1 and t2, 3 more connections are created to load 3 more resources, the number of connections left here should be 1+3 = 4. So number of connections increase from 2 to 4. I am not entirely sure if my understanding is correct here. Nate may help. :) https://codereview.chromium.org/2925963002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:54: if (network2_quiet_reached_ || active_connections > 2) If |active_connections| can increase here, I think we should stop the timer if it is active. And we wait until it drops down to 2 connections again to start a new timer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/07 22:23:50, zhenw wrote: > https://codereview.chromium.org/2925963002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): > > https://codereview.chromium.org/2925963002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:34: > SetNetworkQuietTimers(ActiveConnections()); > Does it mean ActiveConnections() will decrease monotonically here? I am not sure > if it is such case. CheckNetworkStable() is called in > FrameFetchContext::DidLoadResource(), which just means it finishes loading a > resource. But some other place can start loading new resources, right? > > For example: > At time t1, there are 2 connections left. At t2, 1 connections is finished, we > have 1 left. But if between t1 and t2, 3 more connections are created to load 3 > more resources, the number of connections left here should be 1+3 = 4. So number > of connections increase from 2 to 4. > > I am not entirely sure if my understanding is correct here. Nate may help. :) I will lean this to Nate to answer, but if I understand correctly, it is possible. ResourceFetcher is the place we store loaders for resources request, and each time we receive a resource loading done from FrameFetchContext, we ask the fetcher how many connections it has, so at the point we ask ResourceFetcher, the number of connections we get is always correct. > https://codereview.chromium.org/2925963002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:54: if > (network2_quiet_reached_ || active_connections > 2) > If |active_connections| can increase here, I think we should stop the timer if > it is active. And we wait until it drops down to 2 connections again to start a > new timer. We can but that won't be necessary. ``` if (active_connections == 2 || !network2_quiet_timer_.IsActive()) { network2_quiet_timer_.StartOneShot(kNetwork2QuietWindowSeconds, BLINK_FROM_HERE); } ``` Because we reset the 2-quiet timer when the connection drops down to 2 again. When `active_connections` drops down to 2, it means the previous state has more than 2 active connections, so even if we have an active 2-quiet timer, it will be reset.
> https://codereview.chromium.org/2925963002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:54: if > > (network2_quiet_reached_ || active_connections > 2) > > If |active_connections| can increase here, I think we should stop the timer if > > it is active. And we wait until it drops down to 2 connections again to start > a > > new timer. > > We can but that won't be necessary. > > ``` > if (active_connections == 2 || !network2_quiet_timer_.IsActive()) { > network2_quiet_timer_.StartOneShot(kNetwork2QuietWindowSeconds, > BLINK_FROM_HERE); > } > ``` > > Because we reset the 2-quiet timer when the connection drops down to 2 again. > When `active_connections` drops down to 2, it means the previous state has more > than 2 active connections, so even if we have an active 2-quiet timer, it will > be reset. If we do not reset timer when there are more than 2 connections, consider this case: 1. When we first get 2 active connections, we start timer. 2. Then the tab starts to load more resources, so now there are more than 2 active connections. 3. After kNetwork2QuietWindowSeconds, timer fires, network2_quiet_reached_ is set. At this time point, there are still more than 2 active connections. 4. After another some time, active connections drop to 2 again, another timer is started. 5. After kNetwork2QuietWindowSeconds, timer fires again, network2_quiet_reached_ is set again. The actual network quiet is after 5. But the above means network quiet is after 3.
On 2017/06/08 06:14:15, Zhen Wang wrote: > > > https://codereview.chromium.org/2925963002/diff/80001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:54: if > > > (network2_quiet_reached_ || active_connections > 2) > > > If |active_connections| can increase here, I think we should stop the timer > if > > > it is active. And we wait until it drops down to 2 connections again to > start > > a > > > new timer. > > > > We can but that won't be necessary. > > > > ``` > > if (active_connections == 2 || !network2_quiet_timer_.IsActive()) { > > network2_quiet_timer_.StartOneShot(kNetwork2QuietWindowSeconds, > > BLINK_FROM_HERE); > > } > > ``` > > > > Because we reset the 2-quiet timer when the connection drops down to 2 again. > > When `active_connections` drops down to 2, it means the previous state has > more > > than 2 active connections, so even if we have an active 2-quiet timer, it will > > be reset. > > If we do not reset timer when there are more than 2 connections, consider this > case: > 1. When we first get 2 active connections, we start timer. > 2. Then the tab starts to load more resources, so now there are more than 2 > active connections. > 3. After kNetwork2QuietWindowSeconds, timer fires, network2_quiet_reached_ is > set. At this time point, there are still more than 2 active connections. This is not possible, when the timer fired, we check the number of active connections again, so if we have more than 2 active connections, it does nothing. See the first `if` statement in Network2QuietTimerFired. > 4. After another some time, active connections drop to 2 again, another timer is > started. If we already reach network 2-quiet, we won't set timer any more.
> This is not possible, when the timer fired, we check the number of active > connections again, so if we have more than 2 active connections, it does > nothing. See the first `if` statement in Network2QuietTimerFired. Ah, I see where that comes together. Thanks for the explanation!
lgtm with nit https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrame.h (right): https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrame.h:240: FrameResourceCoordinator* GetFrameResourceCoordinator() { // can be null nit: the comment should be in its own line, right? https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:26: // This function is called when the number of active connections is decreased. nit: This comment could be misleading that active connections decrease monotonically.
Nate, gentle ping for review :)
https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:38: network2_quiet_timer_( What's the significance of '2' in this variable name? https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:43: Document* NetworkQuietDetector::GetDocument() { I don't feel strongly, but it's not clear to me that this helper makes the code more readable. https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:52: void NetworkQuietDetector::SetNetworkQuietTimers(int active_connections) { This is only called once. Maybe inline it in CheckNetworkStable?
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Thanks for the feedback, I updated the patch and addressed comments. PTAL. https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalFrame.h (right): https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalFrame.h:240: FrameResourceCoordinator* GetFrameResourceCoordinator() { // can be null On 2017/06/08 16:25:56, Zhen Wang wrote: > nit: the comment should be in its own line, right? Done. https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:26: // This function is called when the number of active connections is decreased. On 2017/06/08 16:25:56, Zhen Wang wrote: > nit: This comment could be misleading that active connections decrease > monotonically. Done. https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:38: network2_quiet_timer_( On 2017/06/12 17:16:39, Nate Chapin wrote: > What's the significance of '2' in this variable name? It means it has no more than 2 active connection. We put a comment in the header file. https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:43: Document* NetworkQuietDetector::GetDocument() { On 2017/06/12 17:16:39, Nate Chapin wrote: > I don't feel strongly, but it's not clear to me that this helper makes the code > more readable. Done. I don't think we need it for now, so removed it and replaced with GetSupplementable(). https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:52: void NetworkQuietDetector::SetNetworkQuietTimers(int active_connections) { On 2017/06/12 17:16:39, Nate Chapin wrote: > This is only called once. Maybe inline it in CheckNetworkStable? We need it in tests.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm LGTM w/nit https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:38: network2_quiet_timer_( On 2017/06/12 18:06:29, lpy wrote: > On 2017/06/12 17:16:39, Nate Chapin wrote: > > What's the significance of '2' in this variable name? > > It means it has no more than 2 active connection. We put a comment in the header > file. I'd be inclined to leave the number out of the variable name for two reasons: 1. If you change the number of connections, you can just change a constant rather than having to rename the variable. 2. I kept reading it as "network too quiet timer", which was confusing. English is annoying sometimes :(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/comment https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.h (right): https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.h:31: virtual ~NetworkQuietDetector() {} ~NetworkQuietDetector() override; instead of virtual and put a = default body in the .cpp.
lpy@chromium.org changed reviewers: + dcheng@chromium.org - zhenw@google.com
+dcheng@ for security review on mojom files.
https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): https://codereview.chromium.org/2925963002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:38: network2_quiet_timer_( On 2017/06/12 18:12:17, Nate Chapin wrote: > On 2017/06/12 18:06:29, lpy wrote: > > On 2017/06/12 17:16:39, Nate Chapin wrote: > > > What's the significance of '2' in this variable name? > > > > It means it has no more than 2 active connection. We put a comment in the > header > > file. > > I'd be inclined to leave the number out of the variable name for two reasons: > 1. If you change the number of connections, you can just change a constant > rather than having to rename the variable. > 2. I kept reading it as "network too quiet timer", which was confusing. English > is annoying sometimes :( (I'd echo this comment, except for basically everywhere "2" is used. It's difficult to tell if it refers to two connections, two seconds, or "to". Might be easiest to just drop '2' from the variable names given the ambiguity; it's easy enough to make it clear that this class watches for fewer than 2 connections in comments) https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:50: if (network2_quiet_reached_ || active_connections > 2) I would suggest putting "2" as a constant somewhere and then using it consistently throughout the test of this class. https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:55: if (active_connections == 2 || !network2_quiet_timer_.IsActive()) { The comment says < 2, but the condition says == 2. https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:68: if (frame_resource_coordinator) { It's not really clear to me how this supplementable object is installed, but maybe it shouldn't be installed if FrameResourceCoordinator isn't present.
Forgot to mention... ipc lgtm. So just some incidental comments.
The CQ bit was checked by lpy@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 ========== Create NetworkQuietDetector. This patch implements network quiet detection associated to Document, it observes active network connection, and signals network idleness signal to GRC when there are no more than 2 connections in 1 second. We want to eventually make FMPDetector use NetworkQuietDetector, but we split the patch to make sure the network quiet signal won't be affected on the merge, and vice versa. Follow-up patches will decouple the network quiet detection from FMPDetector, and make FMPDetector as a client of NetworkQuietDetector. BUG=730098 ========== to ========== Create NetworkQuietDetector. This patch implements network quiet detection associated to Document, it observes active network connection, and signals network quiet signal to GRC when there are no more than 2 connections in 1 second. We want to eventually make FMPDetector use NetworkQuietDetector, but we split the patch to make sure the network quiet signal won't be affected on the merge, and vice versa. Follow-up patches will decouple the network quiet detection from FMPDetector, and make FMPDetector as a client of NetworkQuietDetector. BUG=730098 ==========
Thanks for all the feedback/comments. I addressed the issue of using 2 by removing 2, and adding comments to reflect the precise meanings. https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:50: if (network2_quiet_reached_ || active_connections > 2) On 2017/06/14 00:12:52, dcheng wrote: > I would suggest putting "2" as a constant somewhere and then using it > consistently throughout the test of this class. Done. https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:55: if (active_connections == 2 || !network2_quiet_timer_.IsActive()) { On 2017/06/14 00:12:52, dcheng wrote: > The comment says < 2, but the condition says == 2. Done. https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:68: if (frame_resource_coordinator) { On 2017/06/14 00:12:52, dcheng wrote: > It's not really clear to me how this supplementable object is installed, but > maybe it shouldn't be installed if FrameResourceCoordinator isn't present. Isn't the supplementable object installed when it's created? https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.h (right): https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.h:31: virtual ~NetworkQuietDetector() {} On 2017/06/12 21:20:00, oystein wrote: > ~NetworkQuietDetector() override; instead of virtual and put a = default body in > the .cpp. It's not an override.
https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:68: if (frame_resource_coordinator) { On 2017/06/14 18:19:35, lpy wrote: > On 2017/06/14 00:12:52, dcheng wrote: > > It's not really clear to me how this supplementable object is installed, but > > maybe it shouldn't be installed if FrameResourceCoordinator isn't present. > > Isn't the supplementable object installed when it's created? Right, I missed the call in DidLoadResource. But my point still remains: maybe we should only be installing/creating the supplement if frame resource coordinator is enabled.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/14 18:32:40, dcheng wrote: > https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): > > https://codereview.chromium.org/2925963002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:68: if > (frame_resource_coordinator) { > On 2017/06/14 18:19:35, lpy wrote: > > On 2017/06/14 00:12:52, dcheng wrote: > > > It's not really clear to me how this supplementable object is installed, but > > > maybe it shouldn't be installed if FrameResourceCoordinator isn't present. > > > > Isn't the supplementable object installed when it's created? > > Right, I missed the call in DidLoadResource. > > But my point still remains: maybe we should only be installing/creating the > supplement if frame resource coordinator is enabled. Done.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org, oysteine@chromium.org, japhet@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2925963002/#ps160001 (title: "Dont create NetworkQuietDetector when FrameResourceCoordinator is not enabled")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2925963002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp (right): https://codereview.chromium.org/2925963002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/NetworkQuietDetector.cpp:74: if (frame_resource_coordinator) { Note: I believe that by installing the supplement only when enabled, this check is no longer needed. One possibility is to: DCHECK() that frame resource coordinator is enabled in the accessor and return a reference. But I'm not really sure what the final plan is (is it to have this always enabled)?
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497475677570760,
"parent_rev": "2b1421dbef056581421fa4e8dc7d65743a9ffc2f", "commit_rev":
"36cd3153a788c9cb0ccfce928d5aef91504acc95"}
Message was sent while issue was closed.
Description was changed from ========== Create NetworkQuietDetector. This patch implements network quiet detection associated to Document, it observes active network connection, and signals network quiet signal to GRC when there are no more than 2 connections in 1 second. We want to eventually make FMPDetector use NetworkQuietDetector, but we split the patch to make sure the network quiet signal won't be affected on the merge, and vice versa. Follow-up patches will decouple the network quiet detection from FMPDetector, and make FMPDetector as a client of NetworkQuietDetector. BUG=730098 ========== to ========== Create NetworkQuietDetector. This patch implements network quiet detection associated to Document, it observes active network connection, and signals network quiet signal to GRC when there are no more than 2 connections in 1 second. We want to eventually make FMPDetector use NetworkQuietDetector, but we split the patch to make sure the network quiet signal won't be affected on the merge, and vice versa. Follow-up patches will decouple the network quiet detection from FMPDetector, and make FMPDetector as a client of NetworkQuietDetector. BUG=730098 Review-Url: https://codereview.chromium.org/2925963002 Cr-Commit-Position: refs/heads/master@{#479540} Committed: https://chromium.googlesource.com/chromium/src/+/36cd3153a788c9cb0ccfce928d5a... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/36cd3153a788c9cb0ccfce928d5a... |
