Add a function to add extra headers from NavigationThrottle

Add a function to add extra headers from NavigationThrottle

This CL introduces a function that allows NavigationThrottles to add
extra-headers to the navigation request during WillStartRequest and
WillRedirectRequest. The added headers will overwrite any headers with the same
name that already exists in the request.

BUG=537634

[clamy, 2015-11-05 16:25:21]

Description was changed from

==========
Add a function to add extra headers from NavigationThrottle

This CL introduces a function that allows NavigationThrottles to add
extra-headers to the navigation request during WillStartRequest and
WillRedirectRequest. The added headers will overwrite any headers with the same
name that already exists in the request.

BUG=537634
==========

to

==========
Add a function to add extra headers from NavigationThrottle

This CL introduces a function that allows NavigationThrottles to add
extra-headers to the navigation request during WillStartRequest and
WillRedirectRequest. The added headers will overwrite any headers with the same
name that already exists in the request.

BUG=537634
==========

[clamy, 2015-11-05 16:25:21]

clamy@chromium.org changed reviewers:
+ nasko@chromium.org

[clamy, 2015-11-05 16:26:51]

@nasko: PTAL. This is patch 2 in the serie of preparatory patches for the
DataReductionProxyNavigationThrottle. The AddExtraHeader method will added in
the public API in the final patch. This is the implementation.

[nasko, 2015-11-06 23:51:51]

I'll leave the loader/ review to davidben@, since I'd like him to take a poke at
this CL in general. I want to make sure that all APIs for messing with headers
across our codebase are consistent, but don't have good enough overview of them.

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:115: typedef
std::vector<std::pair<std::string, std::string> > ExtraHeadersList;
The new style requires "using " syntax. Please convert both typedefs to using.

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:302: for
(std::pair<std::string, std::string> header : extra_headers_list)
Why not use auto? for (const auto& header : extra_headers_list) looks much more
readable.

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:164: const
std::vector<std::pair<std:: string, std::string>>&
Why not NavigationHandleImpl::ExtraHeadersList? You have already defined it.

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:168: const
std::vector<std::pair<std:: string, std::string>>&
Same here.

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request_info.h (right):

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request_info.h:36: // The headers may be
changed during calls to
nit: Add "by throttles" so it is clear who may change it. I know the method is
listed after, but I think adding it will be a bit more clear.

[clamy, 2015-11-09 15:46:43]

clamy@chromium.org changed reviewers:
+ davidben@chromium.org

[clamy, 2015-11-09 15:47:09]

@nasko: thanks!
@davidben: PTAL

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:115: typedef
std::vector<std::pair<std::string, std::string> > ExtraHeadersList;
On 2015/11/06 23:51:51, nasko (slow to review) wrote:
> The new style requires "using " syntax. Please convert both typedefs to using.

Done.

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:302: for
(std::pair<std::string, std::string> header : extra_headers_list)
On 2015/11/06 23:51:51, nasko (slow to review) wrote:
> Why not use auto? for (const auto& header : extra_headers_list) looks much
more
> readable.

Done. That's what I did in other places, but apparently missed it in this one.

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:164: const
std::vector<std::pair<std:: string, std::string>>&
On 2015/11/06 23:51:51, nasko (slow to review) wrote:
> Why not NavigationHandleImpl::ExtraHeadersList? You have already defined it.

Done.

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:168: const
std::vector<std::pair<std:: string, std::string>>&
On 2015/11/06 23:51:51, nasko (slow to review) wrote:
> Same here.

Done.

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request_info.h (right):

https://codereview.chromium.org/1416953007/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request_info.h:36: // The headers may be
changed during calls to
On 2015/11/06 23:51:51, nasko (slow to review) wrote:
> nit: Add "by throttles" so it is clear who may change it. I know the method is
> listed after, but I think adding it will be a bit more clear.

Done.

[clamy, 2015-11-25 14:21:54]

@davidben: friendly ping :).

[nasko, 2015-11-25 14:54:37]

Just a small nit I forgot to send on this one.

https://codereview.chromium.org/1416953007/diff/60001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1416953007/diff/60001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:115: using ExtraHeadersList
= std::vector<std::pair<std::string, std::string> >;
nit: I think it is safe these days to have ">>" at the end without need for a
space.

[davidben, 2015-12-03 23:44:57]

Sorry, I'm really behind on large-ish reviews right now. I started looking
through this, but I wonder if this is quite what we want anyway. Is this only to
be used by the DRP code, or is there another consumer? That's some hack for
SafeBrowsing on Android, which might just be moot with crbug/474608.

And since it hooks in on subresources anyway, the WebContents callback might be
a better fit until we come up with a better story for that kind of check. (I
have some crazy "resumable error" idea.)

Also this API is inherently wonky because once you add a header, it applies to
all redirect legs. So you decide to add a header based on something you see at
a.com, but then a.com redirects to b.com and you keep the header. You wouldn't
have sent that header if, instead, you went to b.com straight.

(The ResourceThrottle one has the same problem. I kind of wish we could make it
so that ResourceThrottles can't set request headers...)

https://codereview.chromium.org/1416953007/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1416953007/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:225:
DCHECK(!complete_callback_.is_null());
Perhaps also DCHECK on state_, in case you add a new throttle check later?

https://codereview.chromium.org/1416953007/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1416953007/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:137: using ExtraHeadersList
= std::vector<std::pair<std::string, std::string> >;
"> >" can be >> now

https://codereview.chromium.org/1416953007/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:144: void
AddExtraHeader(const std::string& name, const std::string& value);
This is only to be used by the DRP SafeBrowsing hack, right? Given
https://crbug.com/474608, it might be that that code is short-lived anyway.
Might be worth checking with the status of this and adding a TODO to remove this
API of their plan is to remove the DRP code altogether. ResourceThrottles
changing headers makes me nervous, though that ship seems to have already
sailed.

https://codereview.chromium.org/1416953007/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl_unittest.cc (right):

https://codereview.chromium.org/1416953007/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_unittest.cc:58:
NavigationThrottle::ThrottleCheckResult WillStartRequest() override {
You'll want to take the static_cast away when AddExtraHeader gets exposed.
(Looks like https://codereview.chromium.org/1424003006/ forgets.)

https://codereview.chromium.org/1416953007/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl_unittest.cc:66:
->AddExtraHeader(key_, value_);
Ditto.

https://codereview.chromium.org/1416953007/diff/80001/content/browser/loader/...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/1416953007/diff/80001/content/browser/loader/...
content/browser/loader/navigation_resource_throttle.h:36: const
std::vector<std::pair<std::string, std::string> >& extra_headers);
> > can be >> now

https://codereview.chromium.org/1416953007/diff/80001/content/browser/loader/...
File content/browser/loader/navigation_url_loader.h (right):

https://codereview.chromium.org/1416953007/diff/80001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:50: // request.
Document what extra_headers does.

NB: This API is kind of weird because the headers actually stick from one
redirect to the next.

https://codereview.chromium.org/1416953007/diff/80001/content/browser/loader/...
content/browser/loader/navigation_url_loader.h:52: const
std::vector<std::pair<std::string, std::string> >&
 > > can be >>