Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(268)

Issue 922533003: Eliminated the logic that accumulated multiple preconnect requests (Closed)

Created:
5 years, 10 months ago by Pat Meenan
Modified:
5 years, 10 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Eliminated the logic that accumulated multiple preconnect requests Eliminated the logic that accumulated multiple preconnect requests and inferred a connection count from the number of requests. The logic doesn't play well with the preload scanner and I'm working with the spec authors to come up with a more deterministic way to indicate "many connections" to differ from the case where devs just want a single connection. BUG=450682 Committed: https://crrev.com/16b951d63b306f03ca2825f58489bf6ea9bede8d Cr-Commit-Position: refs/heads/master@{#317839}

Patch Set 1 #

Patch Set 2 : Added browser test #

Total comments: 18

Patch Set 3 : Everything except for custom observer #

Patch Set 4 : Switched to custom NetLog observer #

Total comments: 10

Patch Set 5 : Addressed review feedback #

Total comments: 4

Patch Set 6 : Use existing switches and fix runloop #

Unified diffs Side-by-side diffs Delta from patch set Stats (+87 lines, -48 lines) Patch
M chrome/browser/net/predictor_browsertest.cc View 1 2 3 4 5 5 chunks +83 lines, -0 lines 0 comments Download
M components/network_hints/renderer/renderer_preconnect.h View 2 chunks +1 line, -16 lines 0 comments Download
M components/network_hints/renderer/renderer_preconnect.cc View 2 chunks +3 lines, -32 lines 0 comments Download

Messages

Total messages: 32 (2 generated)
Pat Meenan
5 years, 10 months ago (2015-02-12 15:35:34 UTC) #2
mmenke
One option would be to accumulate preconnect requests in the browser process, and just preconnect ...
5 years, 10 months ago (2015-02-12 15:42:38 UTC) #3
mmenke
On 2015/02/12 15:42:38, mmenke wrote: > One option would be to accumulate preconnect requests in ...
5 years, 10 months ago (2015-02-12 15:47:27 UTC) #4
mmenke
On 2015/02/12 15:47:27, mmenke wrote: > On 2015/02/12 15:42:38, mmenke wrote: > > One option ...
5 years, 10 months ago (2015-02-12 15:57:05 UTC) #5
Pat Meenan
On 2015/02/12 15:57:05, mmenke wrote: > On 2015/02/12 15:47:27, mmenke wrote: > > On 2015/02/12 ...
5 years, 10 months ago (2015-02-12 16:02:21 UTC) #6
mmenke
On 2015/02/12 16:02:21, Pat Meenan wrote: > On 2015/02/12 15:57:05, mmenke wrote: > > On ...
5 years, 10 months ago (2015-02-12 16:23:07 UTC) #7
Pat Meenan
On 2015/02/12 16:23:07, mmenke wrote: > On 2015/02/12 16:02:21, Pat Meenan wrote: > > On ...
5 years, 10 months ago (2015-02-12 17:09:09 UTC) #8
mmenke
On 2015/02/12 17:09:09, Pat Meenan wrote: > On 2015/02/12 16:23:07, mmenke wrote: > > On ...
5 years, 10 months ago (2015-02-12 17:14:12 UTC) #9
Pat Meenan
On 2015/02/12 17:14:12, mmenke wrote: > On 2015/02/12 17:09:09, Pat Meenan wrote: > > On ...
5 years, 10 months ago (2015-02-12 21:46:24 UTC) #10
mmenke
On 2015/02/12 21:46:24, Pat Meenan wrote: > On 2015/02/12 17:14:12, mmenke wrote: > > On ...
5 years, 10 months ago (2015-02-12 22:08:55 UTC) #11
mmenke
On 2015/02/12 22:08:55, mmenke wrote: > On 2015/02/12 21:46:24, Pat Meenan wrote: > > On ...
5 years, 10 months ago (2015-02-12 22:09:53 UTC) #12
chromium-reviews
Oh, if browser_tests can use the same command-line options as Chrome then --enable-blink-features=LinkPreconnect will do ...
5 years, 10 months ago (2015-02-12 22:11:20 UTC) #13
mmenke
On 2015/02/12 22:11:20, chromium-reviews wrote: > Oh, if browser_tests can use the same command-line options ...
5 years, 10 months ago (2015-02-12 22:13:06 UTC) #14
Pat Meenan
On 2015/02/12 22:13:06, mmenke wrote: > On 2015/02/12 22:11:20, chromium-reviews wrote: > > Oh, if ...
5 years, 10 months ago (2015-02-13 01:12:47 UTC) #15
mmenke
https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predictor_browsertest.cc File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predictor_browsertest.cc#newcode220 chrome/browser/net/predictor_browsertest.cc:220: net::CapturingNetLogObserver net_log; Calling a NetLogObserver net_log is a little ...
5 years, 10 months ago (2015-02-13 16:42:17 UTC) #16
Pat Meenan
Addressed everything except for the custom observer. Working on that now (though if you have ...
5 years, 10 months ago (2015-02-13 20:45:26 UTC) #17
mmenke
https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predictor_browsertest.cc File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/20001/chrome/browser/net/predictor_browsertest.cc#newcode236 chrome/browser/net/predictor_browsertest.cc:236: ui_test_utils::NavigateToURL(browser(), GURL(data_uri)); On 2015/02/13 20:45:26, Pat Meenan wrote: > ...
5 years, 10 months ago (2015-02-13 20:55:05 UTC) #18
chromium-reviews
Doh, I thought I had to pump the UI thread and was copying what the ...
5 years, 10 months ago (2015-02-13 21:35:12 UTC) #19
Pat Meenan
OK, the custom observer is in place now and I verified it fails as expected ...
5 years, 10 months ago (2015-02-13 22:36:51 UTC) #20
mmenke
Quick comments. https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predictor_browsertest.cc File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predictor_browsertest.cc#newcode107 chrome/browser/net/predictor_browsertest.cc:107: explicit ConnectNetLogObserver(const std::string host_port_pair) nit: const std::String& ...
5 years, 10 months ago (2015-02-13 22:49:09 UTC) #21
Pat Meenan
https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predictor_browsertest.cc File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/60001/chrome/browser/net/predictor_browsertest.cc#newcode107 chrome/browser/net/predictor_browsertest.cc:107: explicit ConnectNetLogObserver(const std::string host_port_pair) On 2015/02/13 22:49:09, mmenke wrote: ...
5 years, 10 months ago (2015-02-14 00:42:56 UTC) #22
mmenke
https://codereview.chromium.org/922533003/diff/80001/chrome/browser/net/predictor_browsertest.cc File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/80001/chrome/browser/net/predictor_browsertest.cc#newcode32 chrome/browser/net/predictor_browsertest.cc:32: "--enable-experimental-web-platform-features"; Should get these strings from the header (public/common/content_switches.h), ...
5 years, 10 months ago (2015-02-17 16:53:34 UTC) #23
Pat Meenan
https://codereview.chromium.org/922533003/diff/80001/chrome/browser/net/predictor_browsertest.cc File chrome/browser/net/predictor_browsertest.cc (right): https://codereview.chromium.org/922533003/diff/80001/chrome/browser/net/predictor_browsertest.cc#newcode32 chrome/browser/net/predictor_browsertest.cc:32: "--enable-experimental-web-platform-features"; On 2015/02/17 16:53:33, mmenke wrote: > Should get ...
5 years, 10 months ago (2015-02-17 17:38:08 UTC) #24
mmenke
LGTM! Thanks for adding the test!
5 years, 10 months ago (2015-02-17 18:08:14 UTC) #25
Pat Meenan
ttuttle@ if you get a chance could you do an OWNERS review?
5 years, 10 months ago (2015-02-17 18:47:39 UTC) #26
Deprecated (see juliatuttle)
On 2015/02/17 18:47:39, Pat Meenan wrote: > ttuttle@ if you get a chance could you ...
5 years, 10 months ago (2015-02-24 16:43:37 UTC) #27
Deprecated (see juliatuttle)
On 2015/02/24 16:43:37, ttuttle wrote: > On 2015/02/17 18:47:39, Pat Meenan wrote: > > ttuttle@ ...
5 years, 10 months ago (2015-02-24 16:46:55 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/922533003/100001
5 years, 10 months ago (2015-02-24 17:31:28 UTC) #30
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 10 months ago (2015-02-24 18:44:04 UTC) #31
commit-bot: I haz the power
5 years, 10 months ago (2015-02-24 18:45:04 UTC) #32
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/16b951d63b306f03ca2825f58489bf6ea9bede8d
Cr-Commit-Position: refs/heads/master@{#317839}

Powered by Google App Engine
This is Rietveld 408576698