Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2213683003:
Update Effective Connection Type only on main frame requests (Closed)
|
Created:
4 years, 4 months ago by tbansal1 Modified:
4 years, 4 months ago CC:
chromium-reviews, loading-reviews_chromium.org, jam, darin-cc_chromium.org, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate Effective Connection Type only on main frame requests
Currently, the effective connection type (ECT) is computed
every time there is a request that belongs to main frame.
This CL changes it to change it on every main frame request.
BUG=635575
Committed: https://crrev.com/dd6cda7fa2075ce73445716bf6698479c572979a
Cr-Commit-Position: refs/heads/master@{#410464}
Patch Set 1 : PS #
Total comments: 9
Patch Set 2 : Addressed ryan's comments #Patch Set 3 : Use enum #
Messages
Total messages: 36 (22 generated)
The CQ bit was checked by tbansal@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Add main frame check BUG= ========== to ========== Update Effective Connection Type only on main frame requests Currently, the effective connection type (ECT) is computed every time there is a request that belongs to main frame. This CL changes it to change it on every main frame request. BUG=635575 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. Thanks.
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/2213683003/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:94: if (info->IsMainFrame() && Is the info->IsMainFrame() part of this necessary? Seems Like (info->GetResourceType() == RESOURCE_TYPE_MAIN_FRAME) implies (info->IsMainFrame()) https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:95: (info->GetResourceType() == RESOURCE_TYPE_MAIN_FRAME)) { #include "content/public/common/resource_type.h" This should have already been included. https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:600: ResourceType resource_type = nit: Would it be better to pass in ResourceType instead of is_main_frame_request?
https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:601: is_main_frame_request ? RESOURCE_TYPE_MAIN_FRAME : RESOURCE_TYPE_OBJECT; #include "content/public/common/resource_type.h"
ryansturm: ptal. Thanks. https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:94: if (info->IsMainFrame() && On 2016/08/08 18:13:37, RyanSturm wrote: > Is the info->IsMainFrame() part of this necessary? Seems Like > (info->GetResourceType() == RESOURCE_TYPE_MAIN_FRAME) implies > (info->IsMainFrame()) Done. https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader.cc:95: (info->GetResourceType() == RESOURCE_TYPE_MAIN_FRAME)) { On 2016/08/08 18:13:37, RyanSturm wrote: > #include "content/public/common/resource_type.h" > > This should have already been included. Done. https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:600: ResourceType resource_type = On 2016/08/08 18:13:37, RyanSturm wrote: > nit: Would it be better to pass in ResourceType instead of > is_main_frame_request? Not sure. May be we can change in future if there is a need to precisely control the resource type. https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:601: is_main_frame_request ? RESOURCE_TYPE_MAIN_FRAME : RESOURCE_TYPE_OBJECT; On 2016/08/08 18:16:45, RyanSturm wrote: > #include "content/public/common/resource_type.h" Done.
lgtm > https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... > File content/browser/loader/resource_loader_unittest.cc (right): > > https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... > content/browser/loader/resource_loader_unittest.cc:600: ResourceType > resource_type = > On 2016/08/08 18:13:37, RyanSturm wrote: > > nit: Would it be better to pass in ResourceType instead of > > is_main_frame_request? > > Not sure. May be we can change in future if there is a need to precisely control > the resource type. > Seems strange that it passes in a bool to change it to a enum value, but I'm fine with it either way.
tbansal@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: ptal. thanks. https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... File content/browser/loader/resource_loader_unittest.cc (right): https://codereview.chromium.org/2213683003/diff/40001/content/browser/loader/... content/browser/loader/resource_loader_unittest.cc:600: ResourceType resource_type = On 2016/08/08 18:35:25, tbansal1 wrote: > On 2016/08/08 18:13:37, RyanSturm wrote: > > nit: Would it be better to pass in ResourceType instead of > > is_main_frame_request? > > Not sure. May be we can change in future if there is a need to precisely control > the resource type. Done.
The CQ bit was checked by tbansal@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...
LGTM
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 tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org Link to the patchset: https://codereview.chromium.org/2213683003/#ps80001 (title: "Use enum")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tbansal@chromium.org changed reviewers: + nasko@chromium.org
nasko: ptal at DEPS. I have added a file to DEPS. ResourceLoader was already dependent on this file, but this CL just makes it explicit in the DEPS. Thanks.
Rubberstamp LGTM
The CQ bit was checked by tbansal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Update Effective Connection Type only on main frame requests Currently, the effective connection type (ECT) is computed every time there is a request that belongs to main frame. This CL changes it to change it on every main frame request. BUG=635575 ========== to ========== Update Effective Connection Type only on main frame requests Currently, the effective connection type (ECT) is computed every time there is a request that belongs to main frame. This CL changes it to change it on every main frame request. BUG=635575 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Update Effective Connection Type only on main frame requests Currently, the effective connection type (ECT) is computed every time there is a request that belongs to main frame. This CL changes it to change it on every main frame request. BUG=635575 ========== to ========== Update Effective Connection Type only on main frame requests Currently, the effective connection type (ECT) is computed every time there is a request that belongs to main frame. This CL changes it to change it on every main frame request. BUG=635575 Committed: https://crrev.com/dd6cda7fa2075ce73445716bf6698479c572979a Cr-Commit-Position: refs/heads/master@{#410464} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/dd6cda7fa2075ce73445716bf6698479c572979a Cr-Commit-Position: refs/heads/master@{#410464} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
