Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1839473002:
Centralize the setting of Accept headers (Closed)
|
Created:
4 years, 9 months ago by Nate Chapin Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-style_chromium.org, chromium-reviews, gavinp+loader_chromium.org, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCentralize the setting of Accept headers
Set the accept header in MimeTypeResourceHandler based on the
request's ResourceType. All network requests flow through
this point, so we can observe child process requests, whether
or not they are from blink, as well as PlzNavigate requests.
MimeTypeResourceHandler still needs to defer to blink in
certain situations, mostly because an Accept header can be
manually set on an XHR. However, ResourceType also isn't
precise enough to handle every case: it doesn't distinguish
between CSS and XSL stylesheets. It may be worth adding a
separate ResourceType for XSL in the future, but for now,
manually set the Accept header in blink's
XSLStyleSheetResource.
BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/1f3d63b1d496218b7c90a27dd1699560592a6dcc
Cr-Commit-Position: refs/heads/master@{#385827}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 1
Patch Set 6 : Move Accept header setting to MimeTypeResourceHandler #Patch Set 7 : #Patch Set 8 : Remove accept header checking from LinkLoaderTest.Preload #
Total comments: 8
Patch Set 9 : Address mmenke's comments #
Total comments: 10
Patch Set 10 : Comment cleanup + unittest #
Total comments: 2
Patch Set 11 : Rebase #
Total comments: 4
Patch Set 12 : Drop unused variables in test #
Total comments: 2
Patch Set 13 : Rebase #Patch Set 14 : Rebase again #Messages
Total messages: 75 (32 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839473002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839473002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Centralize blink's setting of Accept headers BUG= ========== to ========== Centralize blink's setting of Accept headers This has the added benefit of removing a spurious variable from Resource. BUG= ==========
Description was changed from ========== Centralize blink's setting of Accept headers This has the added benefit of removing a spurious variable from Resource. BUG= ========== to ========== Centralize blink's setting of Accept headers Always set the accept header in ResourceFetcher based on the Resource::Type. This has the added benefit of removing a spurious variable from Resource. BUG= ==========
japhet@chromium.org changed reviewers: + hiroshige@chromium.org
One more small-ish cleanup :)
yoav@yoav.ws changed reviewers: + yoav@yoav.ws
https://codereview.chromium.org/1839473002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/1839473002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:211: } What about other Resource::Types? Where are they set? e.g. when looking at Font requests, they're set to "*/*". Aside: When looking at navigational requests, they're set to "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8". Looking where that happens, I found that render_frame_impl adds that for frame requests where accept is not set by Blink. Maybe worth while mentioning that in a comment here?
On 2016/03/30 06:18:58, Yoav Weiss wrote: > https://codereview.chromium.org/1839473002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/1839473002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:211: } > What about other Resource::Types? Where are they set? > e.g. when looking at Font requests, they're set to "*/*". > > Aside: When looking at navigational requests, they're set to > "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8". > Looking where that happens, I found that render_frame_impl adds that for frame > requests where accept is not set by Blink. > Maybe worth while mentioning that in a comment here? Yeah, the main resource request's Accept header is set in render_frame_impl.cc so that PlzNavigate and non-PlzNavigate gets the navigation's Accept header set in the same place. I think there's some code somewhere (maybe in src/net?) that sets a default Accept header, but I have thus far failed to hunt it down.
Description was changed from ========== Centralize blink's setting of Accept headers Always set the accept header in ResourceFetcher based on the Resource::Type. This has the added benefit of removing a spurious variable from Resource. BUG= ========== to ========== Centralize blink's setting of Accept headers Always set the accept header in ResourceFetcher based on the Resource::Type. This has the added benefit of removing a spurious variable from Resource. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839473002/120001
Description was changed from ========== Centralize blink's setting of Accept headers Always set the accept header in ResourceFetcher based on the Resource::Type. This has the added benefit of removing a spurious variable from Resource. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Centralize the setting of Accept headers Set the accept header in MimeTypeResourceHandler based on the request's ResourceType. All network requests flow through this point, so we can observe child process requests, whether or not they are from blink, as well as PlzNavigate requests. MimeTypeResourceHandler still needs to defer to blink in certain situations, mostly because an Accept header can be manually set on an XHR. However, ResourceType also isn't precise enough to handle every case: it doesn't distinguish between CSS and XSL stylesheets. It may be worth adding a separate ResourceType for XSL in the future, but for now, manually set the Accept header in blink's XSLStyleSheetResource. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
japhet@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: How does this look for moving Accept header handling to content/browser/loader/?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839473002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839473002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Not a huge fan of having a bunch of magic strings that depend on other random files MimeTypeResourceHandler doesn't know about, but I think having all this in one place is preferable to having it all over the place, and don't have a better suggestion, so seems like a reasonable change. https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:37: #include "net/http/http_response_headers.h" Probably a pre-existing issue, but should include "net/url_request/url_request.h" https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:128: std::string accept_value; const char*? Not sure if it really matters, depends how smart the compiler is. https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:159: } nit: Suggest a blank line here, for easier reading.
On 2016/03/30 16:51:21, Nate Chapin wrote: > On 2016/03/30 06:18:58, Yoav Weiss wrote: > > > https://codereview.chromium.org/1839473002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > https://codereview.chromium.org/1839473002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:211: } > > What about other Resource::Types? Where are they set? > > e.g. when looking at Font requests, they're set to "*/*". > > > > Aside: When looking at navigational requests, they're set to > > "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8". > > Looking where that happens, I found that render_frame_impl adds that for frame > > requests where accept is not set by Blink. > > Maybe worth while mentioning that in a comment here? > > Yeah, the main resource request's Accept header is set in render_frame_impl.cc > so that PlzNavigate and non-PlzNavigate gets the navigation's Accept header set > in the same place. I think there's some code somewhere (maybe in src/net?) that > sets a default Accept header, but I have thus far failed to hunt it down. Also, just FYI: I'm pretty sure net does not set a default accept header. We've tried to keep it as ignorant as we can about what to actually do with received content, though the actual mime sniffer code does currently live there, I suppose.
https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:46: "*;q=0.8"; This seems like a weird place to break the string. Can we move the final "*/" down to the next row?
On 2016/03/31 19:21:08, mmenke wrote: > On 2016/03/30 16:51:21, Nate Chapin wrote: > > On 2016/03/30 06:18:58, Yoav Weiss wrote: > > > > > > https://codereview.chromium.org/1839473002/diff/80001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > > > > > https://codereview.chromium.org/1839473002/diff/80001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:211: } > > > What about other Resource::Types? Where are they set? > > > e.g. when looking at Font requests, they're set to "*/*". > > > > > > Aside: When looking at navigational requests, they're set to > > > > "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8". > > > Looking where that happens, I found that render_frame_impl adds that for > frame > > > requests where accept is not set by Blink. > > > Maybe worth while mentioning that in a comment here? > > > > Yeah, the main resource request's Accept header is set in render_frame_impl.cc > > so that PlzNavigate and non-PlzNavigate gets the navigation's Accept header > set > > in the same place. I think there's some code somewhere (maybe in src/net?) > that > > sets a default Accept header, but I have thus far failed to hunt it down. > > Also, just FYI: I'm pretty sure net does not set a default accept header. > We've tried to keep it as ignorant as we can about what to actually do with > received content, though the actual mime sniffer code does currently live there, > I suppose. I eventually found that default header, it was in web_url_request_util.cc. It's removed in this CL.
https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:37: #include "net/http/http_response_headers.h" On 2016/03/31 19:16:31, mmenke wrote: > Probably a pre-existing issue, but should include > "net/url_request/url_request.h" Done. https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:46: "*;q=0.8"; On 2016/03/31 19:23:41, mmenke wrote: > This seems like a weird place to break the string. Can we move the final "*/" > down to the next row? Done. I copy/pasted without thinking about it :/ https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:128: std::string accept_value; On 2016/03/31 19:16:31, mmenke wrote: > const char*? Not sure if it really matters, depends how smart the compiler is. Done. https://codereview.chromium.org/1839473002/diff/140001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:159: } On 2016/03/31 19:16:31, mmenke wrote: > nit: Suggest a blank line here, for easier reading. Done.
content/browser/loader/ LGTM. Skimmed over the rest, but don't know any of it well enough to sign off on it. https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:163: // which we need because JS can manually set an accept header on an XHR. nit: Avoid "we" in heaers https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.h (right): https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.h:36: // net::kMaxBytesToSniff * 2. Think it's worth mentioning its new job of setting Accept header based on ResourceType in another paragraph.
On 2016/04/01 15:29:09, mmenke wrote: > content/browser/loader/ LGTM. Skimmed over the rest, but don't know any of it > well enough to sign off on it. > > https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... > File content/browser/loader/mime_type_resource_handler.cc (right): > > https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... > content/browser/loader/mime_type_resource_handler.cc:163: // which we need > because JS can manually set an accept header on an XHR. > nit: Avoid "we" in heaers > > https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... > File content/browser/loader/mime_type_resource_handler.h (right): > > https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... > content/browser/loader/mime_type_resource_handler.h:36: // net::kMaxBytesToSniff > * 2. > Think it's worth mentioning its new job of setting Accept header based on > ResourceType in another paragraph. Great, thanks! yoav@, do the core/ changes (especially the LinkLoaderTest ones) look ok?
Almost :) We're losing some test coverage here that I'd love to see added back https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:160: } Could we add a test here that makes sure that the resourceType=>Accept header translation happens as it should be? We're losing some testing coverage on the Blink side, so it'd be great to catch up on this end. https://codereview.chromium.org/1839473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp (right): https://codereview.chromium.org/1839473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp:100: {"http://example.test/cat.jpg", "image", "", "", ResourceLoadPriorityVeryLow, WebURLRequest::RequestContextImage, true, true}, Bummer that we can no longer verify Accept header values here. Can you test resourceType so that at least it would be aligned with the value that impacts the Accept value on the Chrome side of things?
https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:163: // which we need because JS can manually set an accept header on an XHR. On 2016/04/01 15:29:08, mmenke wrote: > nit: Avoid "we" in heaers Err..."heaers" -> comments (Don't ask me how that happened).
https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:160: } On 2016/04/01 17:54:10, Yoav Weiss wrote: > Could we add a test here that makes sure that the resourceType=>Accept header > translation happens as it should be? > > We're losing some testing coverage on the Blink side, so it'd be great to catch > up on this end. Done. https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:163: // which we need because JS can manually set an accept header on an XHR. On 2016/04/01 17:57:00, mmenke wrote: > On 2016/04/01 15:29:08, mmenke wrote: > > nit: Avoid "we" in heaers > > Err..."heaers" -> comments (Don't ask me how that happened). I figured it out :) Done. https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.h (right): https://codereview.chromium.org/1839473002/diff/160001/content/browser/loader... content/browser/loader/mime_type_resource_handler.h:36: // net::kMaxBytesToSniff * 2. On 2016/04/01 15:29:08, mmenke wrote: > Think it's worth mentioning its new job of setting Accept header based on > ResourceType in another paragraph. Done. https://codereview.chromium.org/1839473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp (right): https://codereview.chromium.org/1839473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp:100: {"http://example.test/cat.jpg", "image", "", "", ResourceLoadPriorityVeryLow, WebURLRequest::RequestContextImage, true, true}, On 2016/04/01 17:54:10, Yoav Weiss wrote: > Bummer that we can no longer verify Accept header values here. Can you test > resourceType so that at least it would be aligned with the value that impacts > the Accept value on the Chrome side of things? The resourceType is set based on the WebURLRequest::RequestContext, so we're already covering that here.
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839473002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839473002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
core/ LGTM https://codereview.chromium.org/1839473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp (right): https://codereview.chromium.org/1839473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/LinkLoaderTest.cpp:100: {"http://example.test/cat.jpg", "image", "", "", ResourceLoadPriorityVeryLow, WebURLRequest::RequestContextImage, true, true}, On 2016/04/01 19:01:24, Nate Chapin wrote: > On 2016/04/01 17:54:10, Yoav Weiss wrote: > > Bummer that we can no longer verify Accept header values here. Can you test > > resourceType so that at least it would be aligned with the value that impacts > > the Accept value on the Chrome side of things? > > The resourceType is set based on the WebURLRequest::RequestContext, so we're > already covering that here. fair enough
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Plz wrap your change descriptions at 80 or 100 cols or something. :) It's very weird to remove the getter but leave the setter, but rs lgtm for the platform/ change if you think it's good. https://codereview.chromium.org/1839473002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceRequest.h (right): https://codereview.chromium.org/1839473002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceRequest.h:149: void setHTTPAccept(const AtomicString& httpAccept) { setHTTPHeaderField(HTTPNames::Accept, httpAccept); } it's pretty weird that we're deleting the getter but leaving the setter.
https://codereview.chromium.org/1839473002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceRequest.h (right): https://codereview.chromium.org/1839473002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceRequest.h:149: void setHTTPAccept(const AtomicString& httpAccept) { setHTTPHeaderField(HTTPNames::Accept, httpAccept); } On 2016/04/01 23:35:51, esprehn wrote: > it's pretty weird that we're deleting the getter but leaving the setter. Agreed. I hope to remove it when I resolve the TODO in XSLStyleSheetResource.cpp.
Description was changed from ========== Centralize the setting of Accept headers Set the accept header in MimeTypeResourceHandler based on the request's ResourceType. All network requests flow through this point, so we can observe child process requests, whether or not they are from blink, as well as PlzNavigate requests. MimeTypeResourceHandler still needs to defer to blink in certain situations, mostly because an Accept header can be manually set on an XHR. However, ResourceType also isn't precise enough to handle every case: it doesn't distinguish between CSS and XSL stylesheets. It may be worth adding a separate ResourceType for XSL in the future, but for now, manually set the Accept header in blink's XSLStyleSheetResource. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Centralize the setting of Accept headers Set the accept header in MimeTypeResourceHandler based on the request's ResourceType. All network requests flow through this point, so we can observe child process requests, whether or not they are from blink, as well as PlzNavigate requests. MimeTypeResourceHandler still needs to defer to blink in certain situations, mostly because an Accept header can be manually set on an XHR. However, ResourceType also isn't precise enough to handle every case: it doesn't distinguish between CSS and XSL stylesheets. It may be worth adding a separate ResourceType for XSL in the future, but for now, manually set the Accept header in blink's XSLStyleSheetResource. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839473002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839473002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tests LGTM. I'd tend to favor more integration-y tests, to make sure we actually send the headers, but RDH/ResourceLoader really don't lend themselves to that, unfortunately. https://codereview.chromium.org/1839473002/diff/200001/content/browser/loader... File content/browser/loader/mime_type_resource_handler_unittest.cc (right): https://codereview.chromium.org/1839473002/diff/200001/content/browser/loader... content/browser/loader/mime_type_resource_handler_unittest.cc:283: GURL url("http://www.google.com"); Not used. https://codereview.chromium.org/1839473002/diff/200001/content/browser/loader... content/browser/loader/mime_type_resource_handler_unittest.cc:356: GURL url("http://www.google.com"); Not used.
nasko@chromium.org changed reviewers: + nasko@chromium.org
content/ LGTM
https://codereview.chromium.org/1839473002/diff/200001/content/browser/loader... File content/browser/loader/mime_type_resource_handler_unittest.cc (right): https://codereview.chromium.org/1839473002/diff/200001/content/browser/loader... content/browser/loader/mime_type_resource_handler_unittest.cc:283: GURL url("http://www.google.com"); On 2016/04/04 19:08:58, mmenke wrote: > Not used. Done. https://codereview.chromium.org/1839473002/diff/200001/content/browser/loader... content/browser/loader/mime_type_resource_handler_unittest.cc:356: GURL url("http://www.google.com"); On 2016/04/04 19:08:58, mmenke wrote: > Not used. Done.
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws, esprehn@chromium.org, mmenke@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1839473002/#ps220001 (title: "Drop unused variables in test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839473002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839473002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_site_isolation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
lgtm. I confirmed that Accept headers observed by service worker is unchanged (test CL: https://codereview.chromium.org/1853333005/). https://codereview.chromium.org/1839473002/diff/220001/content/browser/loader... File content/browser/loader/mime_type_resource_handler.cc (right): https://codereview.chromium.org/1839473002/diff/220001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:46: "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp," (not related to the changes in this CL/not to be handled in this CL) BTW, the Accept header values used by Chromium/Blink are different from the values specified in the spec. Should/can we set this value to the spec-conformant one in the future? According to the spec, this should be "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8". Spec: (See Step 3 of Fetching) https://fetch.spec.whatwg.org/#fetching https://codereview.chromium.org/1839473002/diff/220001/content/browser/loader... content/browser/loader/mime_type_resource_handler.cc:49: const char kImageAcceptHeader[] = "image/webp,image/*,*/*;q=0.8"; (not to be handled in this CL) Similar to the comment above: according to the spec, this should be "image/png,image/svg+xml,image/*;q=0.8,*/*;q=0.5".
On 2016/04/05 09:34:31, hiroshige wrote: > lgtm. > > I confirmed that Accept headers observed by service worker is unchanged (test > CL: https://codereview.chromium.org/1853333005/). > > https://codereview.chromium.org/1839473002/diff/220001/content/browser/loader... > File content/browser/loader/mime_type_resource_handler.cc (right): > > https://codereview.chromium.org/1839473002/diff/220001/content/browser/loader... > content/browser/loader/mime_type_resource_handler.cc:46: > "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp," > (not related to the changes in this CL/not to be handled in this CL) > > BTW, the Accept header values used by Chromium/Blink are different from the > values specified in the spec. > Should/can we set this value to the spec-conformant one in the future? > > According to the spec, this should be > "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8". > > Spec: (See Step 3 of Fetching) > https://fetch.spec.whatwg.org/#fetching > > https://codereview.chromium.org/1839473002/diff/220001/content/browser/loader... > content/browser/loader/mime_type_resource_handler.cc:49: const char > kImageAcceptHeader[] = "image/webp,image/*,*/*;q=0.8"; > (not to be handled in this CL) > Similar to the comment above: according to the spec, this should be > "image/png,image/svg+xml,image/*;q=0.8,*/*;q=0.5". I think the spec should not be specifying these values, and having a "spec compliant" Accept headers will render them completely useless. There's a good reason for which we advertise webp support (and MSEdge advertises jxr support). Content negotiation relies on that, since underlying support is different between browsers.
On 2016/04/05 09:58:49, Yoav Weiss wrote: > I think the spec should not be specifying these values, and having a "spec > compliant" Accept headers will render them completely useless. > There's a good reason for which we advertise webp support (and MSEdge advertises > jxr support). Content negotiation relies on that, since underlying support is > different between browsers. Sounds reasonable. I found a discussion that says the values are recommended values (expressed as "user agent *should* set") and a user agent can have different values. https://github.com/whatwg/fetch/issues/43#issuecomment-98877690 (the github thread where the Accept values in the spec was introduced)
On 2016/04/05 13:33:41, hiroshige wrote: > On 2016/04/05 09:58:49, Yoav Weiss wrote: > > I think the spec should not be specifying these values, and having a "spec > > compliant" Accept headers will render them completely useless. > > There's a good reason for which we advertise webp support (and MSEdge > advertises > > jxr support). Content negotiation relies on that, since underlying support is > > different between browsers. > > Sounds reasonable. > > I found a discussion that says the values are recommended values (expressed as > "user agent *should* set") and a user agent can have different values. > https://github.com/whatwg/fetch/issues/43#issuecomment-98877690 > (the github thread where the Accept values in the spec was introduced) If we want to revisit our Accept: header values, we should probably notify blink-dev and do it in a separate patch anyway.
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839473002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839473002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_site_isolation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, yoav@yoav.ws, esprehn@chromium.org, mmenke@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1839473002/#ps240001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839473002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839473002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by japhet@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, yoav@yoav.ws, esprehn@chromium.org, mmenke@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1839473002/#ps260001 (title: "Rebase again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839473002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839473002/260001
Message was sent while issue was closed.
Description was changed from ========== Centralize the setting of Accept headers Set the accept header in MimeTypeResourceHandler based on the request's ResourceType. All network requests flow through this point, so we can observe child process requests, whether or not they are from blink, as well as PlzNavigate requests. MimeTypeResourceHandler still needs to defer to blink in certain situations, mostly because an Accept header can be manually set on an XHR. However, ResourceType also isn't precise enough to handle every case: it doesn't distinguish between CSS and XSL stylesheets. It may be worth adding a separate ResourceType for XSL in the future, but for now, manually set the Accept header in blink's XSLStyleSheetResource. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Centralize the setting of Accept headers Set the accept header in MimeTypeResourceHandler based on the request's ResourceType. All network requests flow through this point, so we can observe child process requests, whether or not they are from blink, as well as PlzNavigate requests. MimeTypeResourceHandler still needs to defer to blink in certain situations, mostly because an Accept header can be manually set on an XHR. However, ResourceType also isn't precise enough to handle every case: it doesn't distinguish between CSS and XSL stylesheets. It may be worth adding a separate ResourceType for XSL in the future, but for now, manually set the Accept header in blink's XSLStyleSheetResource. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Centralize the setting of Accept headers Set the accept header in MimeTypeResourceHandler based on the request's ResourceType. All network requests flow through this point, so we can observe child process requests, whether or not they are from blink, as well as PlzNavigate requests. MimeTypeResourceHandler still needs to defer to blink in certain situations, mostly because an Accept header can be manually set on an XHR. However, ResourceType also isn't precise enough to handle every case: it doesn't distinguish between CSS and XSL stylesheets. It may be worth adding a separate ResourceType for XSL in the future, but for now, manually set the Accept header in blink's XSLStyleSheetResource. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Centralize the setting of Accept headers Set the accept header in MimeTypeResourceHandler based on the request's ResourceType. All network requests flow through this point, so we can observe child process requests, whether or not they are from blink, as well as PlzNavigate requests. MimeTypeResourceHandler still needs to defer to blink in certain situations, mostly because an Accept header can be manually set on an XHR. However, ResourceType also isn't precise enough to handle every case: it doesn't distinguish between CSS and XSL stylesheets. It may be worth adding a separate ResourceType for XSL in the future, but for now, manually set the Accept header in blink's XSLStyleSheetResource. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/1f3d63b1d496218b7c90a27dd1699560592a6dcc Cr-Commit-Position: refs/heads/master@{#385827} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/1f3d63b1d496218b7c90a27dd1699560592a6dcc Cr-Commit-Position: refs/heads/master@{#385827} |
