Update NavigationHandleImpl#GetResponseHeaders method.

Update NavigationHandleImpl#GetResponseHeaders method.

Now GetResponseHeaders won't check the navigation state, instead it will just return a nullptr if there are no response headers available.
In some cases the response headers are not available, but in the previous code there's no safe way to know that.
This removes the CHECK for the navigation state that was causing this problem.

BUG=526291
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7bf92606f08d7fb9731b369da80108acdfb3ab7f
Cr-Commit-Position: refs/heads/master@{#374747}

[Donn Denman, 2016-02-08 19:56:26]

Description was changed from

==========
Add NavigationHandleImpl#HasResponseHeaders method.

For Contextual Search we'd like to get the response headers for the current
request in the main frame.
It looks like we can usually simply get them.  However, when offline, trying to
get them triggers
a CHECK exception.  Since there may be other cases where the headers are not
available, it seems
safest to add a HasResponseHeaders method.

BUG=526291
==========

to

==========
Add NavigationHandleImpl#HasResponseHeaders method.

For Contextual Search we'd like to get the response headers for the current
request in the main frame.
It looks like we can usually simply get them.  However, when offline, trying to
get them triggers
a CHECK exception.  Since there may be other cases where the headers are not
available, it seems
safest to add a HasResponseHeaders method.

BUG=526291
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Donn Denman, 2016-02-09 01:06:52]

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

[Donn Denman, 2016-02-09 01:06:53]

Hi Camile, PTAL.

You probably know better than I do about how best to expose and/or extend this
API, so I'm open to your suggestions.  Let me know what you think is best!

This is part of updating Contextual Search (aka Touch to Search) to a model that
uses a single request instead of making two requests.  We need to get the
resolved search term to display in the UX from the response header from our GWS
search request.  Details in go/cs-single-request.

Feel free to ping me with questions!

And in case you want more context for this change, I wrote an design doc a while
ago that is now obsolete thanks to your help in adding this response-header API.

https://docs.google.com/document/d/1tck1atujE4VA16b4NXeP1hOCYEBnCvkYeynBetQbp...

[clamy, 2016-02-09 10:42:40]

Thanks! Looking at the code, I think it just makes more sense to drop the DCHECK
in GetResponseHeaders. This DCHECK is useful in other cases where we don't
return a pointer value, but in that case we can just return nullptr while we
don't have the headers.

https://codereview.chromium.org/1681703002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/1681703002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.cc:136: return state_ >=
WILL_REDIRECT_REQUEST && response_headers_.get();
nit: the state_ >= WILL_REDIRECT_REQUEST is not really useful here since
response_headers_ should always be null before we reach that state.

https://codereview.chromium.org/1681703002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1681703002/diff/1/content/browser/frame_host/...
content/browser/frame_host/navigation_handle_impl.h:116: bool
HasResponseHeaders() const;
nit: wrong indentation

[donnd, 2016-02-09 15:06:27]

donnd@google.com changed reviewers:
+ donnd@google.com

[donnd, 2016-02-09 15:06:27]

Thanks for the quick reply; I'll update this soon.

[Donn Denman, 2016-02-10 01:02:15]

Description was changed from

==========
Add NavigationHandleImpl#HasResponseHeaders method.

For Contextual Search we'd like to get the response headers for the current
request in the main frame.
It looks like we can usually simply get them.  However, when offline, trying to
get them triggers
a CHECK exception.  Since there may be other cases where the headers are not
available, it seems
safest to add a HasResponseHeaders method.

BUG=526291
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update NavigationHandleImpl#GetResponseHeaders method.
    
Now GetResponseHeaders won't check the navigation state, instead it will just
return a nullptr if there are no response headers available.
In some cases the response headers are not available, but in the previous code
there's no safe way to know that.
This removes the CHECK for the navigation state that was causing this problem.
    
BUG=526291
==========

[Donn Denman, 2016-02-10 01:03:09]

Description was changed from

==========
Update NavigationHandleImpl#GetResponseHeaders method.
    
Now GetResponseHeaders won't check the navigation state, instead it will just
return a nullptr if there are no response headers available.
In some cases the response headers are not available, but in the previous code
there's no safe way to know that.
This removes the CHECK for the navigation state that was causing this problem.
    
BUG=526291
==========

to

==========
Update NavigationHandleImpl#GetResponseHeaders method.
    
Now GetResponseHeaders won't check the navigation state, instead it will just
return a nullptr if there are no response headers available.
In some cases the response headers are not available, but in the previous code
there's no safe way to know that.
This removes the CHECK for the navigation state that was causing this problem.
    
BUG=526291
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Donn Denman, 2016-02-10 01:05:05]

Camile, PTAL.

https://chromiumcodereview.appspot.com/1681703002/diff/1/content/browser/fram...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://chromiumcodereview.appspot.com/1681703002/diff/1/content/browser/fram...
content/browser/frame_host/navigation_handle_impl.cc:136: return state_ >=
WILL_REDIRECT_REQUEST && response_headers_.get();
On 2016/02/09 10:42:40, clamy wrote:
> nit: the state_ >= WILL_REDIRECT_REQUEST is not really useful here since
> response_headers_ should always be null before we reach that state.

Done.

https://chromiumcodereview.appspot.com/1681703002/diff/1/content/browser/fram...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://chromiumcodereview.appspot.com/1681703002/diff/1/content/browser/fram...
content/browser/frame_host/navigation_handle_impl.h:116: bool
HasResponseHeaders() const;
On 2016/02/09 10:42:40, clamy wrote:
> nit: wrong indentation

Acknowledged.

[clamy, 2016-02-10 12:15:11]

Thanks! Lgtm with one nit.

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

https://codereview.chromium.org/1681703002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:111: // navigation is ready
to commit. It should not be modified, as modifications
nit: s/It/The headers returned

[Donn Denman, 2016-02-10 19:56:09]

Thanks Camile!

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

https://codereview.chromium.org/1681703002/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:111: // navigation is ready
to commit. It should not be modified, as modifications
On 2016/02/10 12:15:11, clamy wrote:
> nit: s/It/The headers returned

Done.

[Donn Denman, 2016-02-10 19:56:14]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2016-02-10 19:56:14]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/1681703002/#ps40001
(title: "Updated a comment.")

[commit-bot: I haz the power, 2016-02-10 19:57:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681703002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681703002/40001

[commit-bot: I haz the power, 2016-02-10 22:13:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-16 22:31:40]

Description was changed from

==========
Update NavigationHandleImpl#GetResponseHeaders method.
    
Now GetResponseHeaders won't check the navigation state, instead it will just
return a nullptr if there are no response headers available.
In some cases the response headers are not available, but in the previous code
there's no safe way to know that.
This removes the CHECK for the navigation state that was causing this problem.
    
BUG=526291
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Update NavigationHandleImpl#GetResponseHeaders method.

Now GetResponseHeaders won't check the navigation state, instead it will just
return a nullptr if there are no response headers available.
In some cases the response headers are not available, but in the previous code
there's no safe way to know that.
This removes the CHECK for the navigation state that was causing this problem.

BUG=526291
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/7bf92606f08d7fb9731b369da80108acdfb3ab7f
Cr-Commit-Position: refs/heads/master@{#374747}
==========

[commit-bot: I haz the power, 2016-02-16 22:31:41]

Patchset 3 (id:??) landed as
https://crrev.com/7bf92606f08d7fb9731b369da80108acdfb3ab7f
Cr-Commit-Position: refs/heads/master@{#374747}