Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2842763003:
media: Send chrome-cache: frfr on first request. (Closed)
Descriptionmedia: Send chrome-cache: frfr on first request.
This allows flywheel to know when it's safe to switch resources.
BUG=504194
Review-Url: https://codereview.chromium.org/2842763003
Cr-Commit-Position: refs/heads/master@{#471139}
Committed: https://chromium.googlesource.com/chromium/src/+/03772bfdc7bc49e6d4486ea98a41738e89ded9b5
Patch Set 1 #
Total comments: 4
Patch Set 2 : remove header when not sending to proxy #
Total comments: 10
Patch Set 3 : flywheel -> the data reduction proxy #Patch Set 4 : merge #
Messages
Total messages: 37 (21 generated)
The CQ bit was checked by hubbe@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...
hubbe@chromium.org changed reviewers: + bengr@chromium.org, tombergan@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... File media/blink/resource_multibuffer_data_provider.cc (right): https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... media/blink/resource_multibuffer_data_provider.cc:93: request.SetHTTPHeaderField(WebString::FromUTF8("chrome-proxy"), You are unconditionally adding a header that we conditionally send only to the data reduction proxy. Doing this will cause VerifyHttpRequestHeaders in data_reduction_proxy_network_delegate.cc (https://codesearch.chromium.org/chromium/src/components/data_reduction_proxy/...) to fail a DCHECK that this header isn't present when the request is not proxied. Since blink code doesn't know what proxy connection will be used, your best bet is to add it here as you did and remove it in DRP-specific network code. The DRP network code calls VerifyHttpRequestHeaders in three places in that delegate file. Your best bet is to remove 'C-P: frfr' just before those calls if the d-r-p is not being used.
tombergan@google.com changed reviewers: + tombergan@google.com
https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... File media/blink/resource_multibuffer_data_provider.cc (right): https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... media/blink/resource_multibuffer_data_provider.cc:93: request.SetHTTPHeaderField(WebString::FromUTF8("chrome-proxy"), Why does the following code not run into the same error? https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren...
https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... File media/blink/resource_multibuffer_data_provider.cc (right): https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... media/blink/resource_multibuffer_data_provider.cc:93: request.SetHTTPHeaderField(WebString::FromUTF8("chrome-proxy"), On 2017/04/27 23:02:52, tombergan wrote: > Why does the following code not run into the same error? > https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren... Uses different headers..
On 2017/04/27 23:16:26, hubbe wrote: > https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... > File media/blink/resource_multibuffer_data_provider.cc (right): > > https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... > media/blink/resource_multibuffer_data_provider.cc:93: > request.SetHTTPHeaderField(WebString::FromUTF8("chrome-proxy"), > On 2017/04/27 23:02:52, tombergan wrote: > > Why does the following code not run into the same error? > > > https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren... > > Uses different headers.. It uses chrome-proxy-accept-transform, so I expected that this check would fail: https://codesearch.chromium.org/chromium/src/components/data_reduction_proxy/... (I don't know this code very well though.)
https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... File media/blink/resource_multibuffer_data_provider.cc (right): https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... media/blink/resource_multibuffer_data_provider.cc:93: request.SetHTTPHeaderField(WebString::FromUTF8("chrome-proxy"), On 2017/04/27 23:16:26, hubbe wrote: > On 2017/04/27 23:02:52, tombergan wrote: > > Why does the following code not run into the same error? > > > https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren... > > Uses different headers.. Or maybe because it dumps two headers into one field?
The CQ bit was checked by hubbe@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.
On 2017/04/27 23:21:32, hubbe wrote: > https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... > File media/blink/resource_multibuffer_data_provider.cc (right): > > https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... > media/blink/resource_multibuffer_data_provider.cc:93: > request.SetHTTPHeaderField(WebString::FromUTF8("chrome-proxy"), > On 2017/04/27 23:16:26, hubbe wrote: > > On 2017/04/27 23:02:52, tombergan wrote: > > > Why does the following code not run into the same error? > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren... > > > > Uses different headers.. > > Or maybe because it dumps two headers into one field? All tests are passing: PTAL
On 2017/04/28 20:12:48, hubbe wrote: > On 2017/04/27 23:21:32, hubbe wrote: > > > https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... > > File media/blink/resource_multibuffer_data_provider.cc (right): > > > > > https://codereview.chromium.org/2842763003/diff/1/media/blink/resource_multib... > > media/blink/resource_multibuffer_data_provider.cc:93: > > request.SetHTTPHeaderField(WebString::FromUTF8("chrome-proxy"), > > On 2017/04/27 23:16:26, hubbe wrote: > > > On 2017/04/27 23:02:52, tombergan wrote: > > > > Why does the following code not run into the same error? > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren... > > > > > > Uses different headers.. > > > > Or maybe because it dumps two headers into one field? > > All tests are passing: PTAL ping?
lgtm, but I can't approve. Thanks!
Looking good... https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:265: headers->RemoveHeader(chrome_proxy_header()); Please add a case to data_reduction_proxy_network_delegate_unittest.cc that verifies that the holdback produces no chrome-proxy header even if frfr was set previously. https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:293: headers->RemoveHeader(chrome_proxy_header()); Please add a case to data_reduction_proxy_network_delegate_unittest.cc that verifies that Chrome produces no chrome-proxy when a direct connection or other proxty is in use, even if frfr was set previously. https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:344: VerifyHttpRequestHeaders(true, *headers); Does video not affect this case? https://codereview.chromium.org/2842763003/diff/20001/media/blink/resource_mu... File media/blink/resource_multibuffer_data_provider.cc (right): https://codereview.chromium.org/2842763003/diff/20001/media/blink/resource_mu... media/blink/resource_multibuffer_data_provider.cc:90: // This lets flywheel know that we don't have anything previously cached In code we call this the data reduction proxy, not flywheel.
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:265: headers->RemoveHeader(chrome_proxy_header()); On 2017/05/03 21:37:42, bengr wrote: > Please add a case to data_reduction_proxy_network_delegate_unittest.cc that > verifies that the holdback produces no chrome-proxy header even if frfr was set > previously. Is there a test that does something similar I can use as a template? https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:344: VerifyHttpRequestHeaders(true, *headers); On 2017/05/03 21:37:42, bengr wrote: > Does video not affect this case? This calls VerifyHttpRequestHeaders() with "true". I assume that if we remove the header here, it will never make it anywhere ever... https://codereview.chromium.org/2842763003/diff/20001/media/blink/resource_mu... File media/blink/resource_multibuffer_data_provider.cc (right): https://codereview.chromium.org/2842763003/diff/20001/media/blink/resource_mu... media/blink/resource_multibuffer_data_provider.cc:90: // This lets flywheel know that we don't have anything previously cached On 2017/05/03 21:37:42, bengr wrote: > In code we call this the data reduction proxy, not flywheel. 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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm. Let me know about the test. https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:265: headers->RemoveHeader(chrome_proxy_header()); On 2017/05/05 20:51:45, hubbe wrote: > On 2017/05/03 21:37:42, bengr wrote: > > Please add a case to data_reduction_proxy_network_delegate_unittest.cc that > > verifies that the holdback produces no chrome-proxy header even if frfr was > set > > previously. > > Is there a test that does something similar I can use as a template? Shockingly there isn't. We rely on the DCHECKs in VerifyHttpRequestHeaders. To write a test, which I won't require, you could follow the pattern of BuildSocket (see: https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro...) but you'd need to write everything from scratch. If you don't want to do this, please add a TODO(bengr). Thanks. The test would go something like this: BuildSocketForDirectConnection(...); std::unique_ptr<net::URLRequest> request = context_.CreateRequest(GURL(kTestURL), net::IDLE, &delegate); request->SetExtraRequestHeaderByName("chrome-proxy", "frfr", true); request->Start(); base::RunLoop().RunUntilIdle(); net::HttpRequestHeaders sent_request_header; request->GetFulLRequestHeaders(&sent_request_headers) EXPECT_FALSE(sent_request_headers.GetHeader(chrome_proxy_header(), &val); https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:344: VerifyHttpRequestHeaders(true, *headers); On 2017/05/05 20:51:45, hubbe wrote: > On 2017/05/03 21:37:42, bengr wrote: > > Does video not affect this case? > > This calls VerifyHttpRequestHeaders() with "true". > I assume that if we remove the header here, it will never make it anywhere > ever... Acknowledged.
You can refer to this bug on the TODO: https://bugs.chromium.org/p/chromium/issues/detail?id=719713 On 2017/05/08 21:27:38, bengr wrote: > lgtm. Let me know about the test. > > https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... > File > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc > (right): > > https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:265: > headers->RemoveHeader(chrome_proxy_header()); > On 2017/05/05 20:51:45, hubbe wrote: > > On 2017/05/03 21:37:42, bengr wrote: > > > Please add a case to data_reduction_proxy_network_delegate_unittest.cc that > > > verifies that the holdback produces no chrome-proxy header even if frfr was > > set > > > previously. > > > > Is there a test that does something similar I can use as a template? > > Shockingly there isn't. We rely on the DCHECKs in VerifyHttpRequestHeaders. To > write a test, which I won't require, you could follow the pattern of BuildSocket > (see: > https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro...) > but you'd need to write everything from scratch. If you don't want to do this, > please add a TODO(bengr). Thanks. > > The test would go something like this: > > BuildSocketForDirectConnection(...); > std::unique_ptr<net::URLRequest> request = > context_.CreateRequest(GURL(kTestURL), net::IDLE, &delegate); > > request->SetExtraRequestHeaderByName("chrome-proxy", "frfr", true); > > request->Start(); > base::RunLoop().RunUntilIdle(); > > net::HttpRequestHeaders sent_request_header; > request->GetFulLRequestHeaders(&sent_request_headers) > EXPECT_FALSE(sent_request_headers.GetHeader(chrome_proxy_header(), &val); > > https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... > components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:344: > VerifyHttpRequestHeaders(true, *headers); > On 2017/05/05 20:51:45, hubbe wrote: > > On 2017/05/03 21:37:42, bengr wrote: > > > Does video not affect this case? > > > > This calls VerifyHttpRequestHeaders() with "true". > > I assume that if we remove the header here, it will never make it anywhere > > ever... > > Acknowledged.
https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2842763003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:265: headers->RemoveHeader(chrome_proxy_header()); On 2017/05/08 21:27:38, bengr wrote: > On 2017/05/05 20:51:45, hubbe wrote: > > On 2017/05/03 21:37:42, bengr wrote: > > > Please add a case to data_reduction_proxy_network_delegate_unittest.cc that > > > verifies that the holdback produces no chrome-proxy header even if frfr was > > set > > > previously. > > > > Is there a test that does something similar I can use as a template? > > Shockingly there isn't. We rely on the DCHECKs in VerifyHttpRequestHeaders. To > write a test, which I won't require, you could follow the pattern of BuildSocket > (see: > https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro...) > but you'd need to write everything from scratch. If you don't want to do this, > please add a TODO(bengr). Thanks. > > The test would go something like this: > > BuildSocketForDirectConnection(...); > std::unique_ptr<net::URLRequest> request = > context_.CreateRequest(GURL(kTestURL), net::IDLE, &delegate); > > request->SetExtraRequestHeaderByName("chrome-proxy", "frfr", true); > > request->Start(); > base::RunLoop().RunUntilIdle(); > > net::HttpRequestHeaders sent_request_header; > request->GetFulLRequestHeaders(&sent_request_headers) > EXPECT_FALSE(sent_request_headers.GetHeader(chrome_proxy_header(), &val); Added TODO
The CQ bit was checked by hubbe@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 hubbe@chromium.org
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tombergan@google.com, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2842763003/#ps60001 (title: "merge")
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": 60001, "attempt_start_ts": 1494545856909380,
"parent_rev": "7cf2f122818ad39afafea382ecf2a6cee9d350fd", "commit_rev":
"03772bfdc7bc49e6d4486ea98a41738e89ded9b5"}
Message was sent while issue was closed.
Description was changed from ========== media: Send chrome-cache: frfr on first request. This allows flywheel to know when it's safe to switch resources. BUG=504194 ========== to ========== media: Send chrome-cache: frfr on first request. This allows flywheel to know when it's safe to switch resources. BUG=504194 Review-Url: https://codereview.chromium.org/2842763003 Cr-Commit-Position: refs/heads/master@{#471139} Committed: https://chromium.googlesource.com/chromium/src/+/03772bfdc7bc49e6d4486ea98a41... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/03772bfdc7bc49e6d4486ea98a41... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
