Pass http response headers to the streamsPrivate API callback

Pass http response headers to the streamsPrivate API callback

This passes the response headers through to the API callback allowing the
application to access the headers without having to make an additional
http request.

BUG=303491
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255593

[Zachary Kuznia, 2014-03-04 00:48:55]

lgtm

https://codereview.chromium.org/185663002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/streams_private.idl (right):

https://codereview.chromium.org/185663002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/streams_private.idl:25: // The HTTP response
headers of the itercepted request.
nit: intercepted

[raymes, 2014-03-05 02:57:47]

+jochen for OWNERS of:
  chrome/browser/extensions/api/streams_private/streams_private_api.cc
  chrome/common/extensions/api/streams_private.idl
  content/browser/loader/resource_dispatcher_host_impl.cc
  content/browser/streams/stream.cc
  content/browser/streams/stream.h
  content/browser/streams/stream_handle_impl.cc
  content/browser/streams/stream_handle_impl.h
  content/public/browser/stream_handle.h

https://codereview.chromium.org/185663002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/streams_private.idl (right):

https://codereview.chromium.org/185663002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/streams_private.idl:25: // The HTTP response
headers of the itercepted request.
On 2014/03/04 00:48:55, Zachary Kuznia wrote:
> nit: intercepted

Done.

[raymes, 2014-03-05 02:58:13]

+jochen for OWNERS of:
  chrome/browser/extensions/api/streams_private/streams_private_api.cc
  chrome/common/extensions/api/streams_private.idl
  content/browser/loader/resource_dispatcher_host_impl.cc
  content/browser/streams/stream.cc
  content/browser/streams/stream.h
  content/browser/streams/stream_handle_impl.cc
  content/browser/streams/stream_handle_impl.h
  content/public/browser/stream_handle.h

[raymes, 2014-03-05 03:02:25]

jochen is OOO so +sky for OWNERS

[sky, 2014-03-05 15:30:15]

Sorry, I'm not familiar with this code and I'm also not an owner in
content/public.
+jam
-sky

[jam, 2014-03-05 22:27:33]

lgtm

[raymes, 2014-03-05 23:00:57]

+kalman for chrome/common/extensions/api/streams_private.idl

[not at google - send to devlin, 2014-03-05 23:02:38]

https://codereview.chromium.org/185663002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/api/streams_private.idl (right):

https://codereview.chromium.org/185663002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/streams_private.idl:26: DOMString responseHeaders;
should this be a list of strings?

[raymes, 2014-03-06 00:31:22]

+rsleevi for his thoughts on the API.

It's debatable I guess. We could provide the raw header string, as
I've currently implemented. We could split on newline and provide a
list of strings. Or we could omit the response line and provide a
dictionary mapping header property to value.

Thoughts? Keep in mind it's a private API at the moment and we can
always change it.

On Thu, Mar 6, 2014 at 10:02 AM,  <kalman@chromium.org> wrote:
>
>
>
>
https://codereview.chromium.org/185663002/diff/40001/chrome/common/extensions...
> File chrome/common/extensions/api/streams_private.idl (right):
>
>
https://codereview.chromium.org/185663002/diff/40001/chrome/common/extensions...
> chrome/common/extensions/api/streams_private.idl:26: DOMString
> responseHeaders;
> should this be a list of strings?
>
> https://codereview.chromium.org/185663002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[not at google - send to devlin, 2014-03-06 00:35:06]

Ok. A dictionary of string -> string sounds even better.

But lgtm in its current form since the CL has already been lg'd by others. It
would be good to explore a nicer version of the API even though it's private.

[raymes, 2014-03-06 00:37:30]

The CQ bit was checked by raymes@chromium.org

[raymes, 2014-03-06 00:37:52]

Ok thanks, - I'll make that change in a followup CL!

On Thu, Mar 6, 2014 at 11:35 AM,  <kalman@chromium.org> wrote:
> Ok. A dictionary of string -> string sounds even better.
>
> But lgtm in its current form since the CL has already been lg'd by others.
> It
> would be good to explore a nicer version of the API even though it's
> private.
>
> https://codereview.chromium.org/185663002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-03-06 00:38:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/185663002/40001

[commit-bot: I haz the power, 2014-03-06 01:34:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 01:34:41]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[raymes, 2014-03-07 06:48:53]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-03-07 06:49:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raymes@chromium.org/185663002/40001

[commit-bot: I haz the power, 2014-03-07 12:05:03]

Change committed as 255593

[Ryan Sleevi, 2014-03-07 22:27:06]

On 2014/03/07 12:05:03, I haz the power (commit-bot) wrote:
> Change committed as 255593

Sorry for the delay in response.

Would you mind filing a bug specifically to look at what you want to expose via
the streamsPrivate? I absolutely want to make sure we're exposing the right
thing from the net/ stack.

A map of string -> string isn't going to be strictly sufficient, since multiple
instances of a header may exist (and they may be non-coalescing - aka distinct
values). 

I'm not convinced that giving the 'raw' headers to the API is the right thing.
There are so many edge cases with parsing headers, we should be doing right by
the consumers - public or private. As you can note, the "GetNormalizedHeaders"
is an API that should be removed. Not a good sign here for the API - private or
not.

[raymes, 2014-03-09 23:56:45]

I filed crbug.com/350755

I think we should pretty much just expose the raw header string. The
streamsPrivate API intercepts an http response and exposes the body of
the response by passing a blob URL to the extension which can be
streamed using and XHR. The problem is that the headers of the
response are lost. An application could easily re-request the original
headers but it costs an extra http request. So it seems to me that
maybe we should be exposing the missing bit of the response by passing
the raw headers to the application?

On Fri, Mar 7, 2014 at 2:27 PM,  <rsleevi@chromium.org> wrote:
> On 2014/03/07 12:05:03, I haz the power (commit-bot) wrote:
>>
>> Change committed as 255593
>
>
> Sorry for the delay in response.
>
> Would you mind filing a bug specifically to look at what you want to expose
> via
> the streamsPrivate? I absolutely want to make sure we're exposing the right
> thing from the net/ stack.
>
> A map of string -> string isn't going to be strictly sufficient, since
> multiple
> instances of a header may exist (and they may be non-coalescing - aka
> distinct
> values).
>
> I'm not convinced that giving the 'raw' headers to the API is the right
> thing.
> There are so many edge cases with parsing headers, we should be doing right
> by
> the consumers - public or private. As you can note, the
> "GetNormalizedHeaders"
> is an API that should be removed. Not a good sign here for the API - private
> or
> not.
>
> https://codereview.chromium.org/185663002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.