Forward MIME types to BrowserPlugin when a viewer is specified.

Forward MIME types to BrowserPlugin when a viewer is specified.

When the mime_type_handler key is available in an extension, this creates a
BrowserPlugin and gives the ID of it to the background page.

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

[Zachary Kuznia, 2014-04-30 00:42:44]

Hi Fady,

Could you take a look at this?  In particular, the registration of BrowserPlugin
mime types in plugin_manager.{cc,h} and forwarding of data to BrowserPlugin in
content/*?

[Zachary Kuznia, 2014-04-30 00:43:51]

Hi Ben,

Could you take a look at the changes to chrome/*/extensions?

Thanks,
-Zach

[not at google - send to devlin, 2014-04-30 00:48:51]

is there somebody who knows the plugin code that you can ask to review? I've
never really seen it before.

it would be nice if there were a single abstraction for a "view" and "tab", and
maybe that's "view"?

https://codereview.chromium.org/263513004/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.h
(right):

https://codereview.chromium.org/263513004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.h:36: const
std::string& view_id,
this function needs to explain what these parameters are

https://codereview.chromium.org/263513004/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/plugin_manager.h (right):

https://codereview.chromium.org/263513004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/plugin_manager.h:8: #include <set>
<string> while you're here too

[Zachary Kuznia, 2014-04-30 01:23:35]

I'll add someone else for chrome/browser/extensions/plugin_manager.{cc,h}

I'm actually going to deprecate tabId once everything is working in plugins, so
after that there will only be viewId.

https://codereview.chromium.org/263513004/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.h
(right):

https://codereview.chromium.org/263513004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.h:36: const
std::string& view_id,
On 2014/04/30 00:48:51, kalman wrote:
> this function needs to explain what these parameters are

Done.

https://codereview.chromium.org/263513004/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/plugin_manager.h (right):

https://codereview.chromium.org/263513004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/plugin_manager.h:8: #include <set>
On 2014/04/30 00:48:51, kalman wrote:
> <string> while you're here too

Done.

[Zachary Kuznia, 2014-04-30 01:32:32]

Hello yoz,

Could you take a look at:
chrome/browser/extensions/plugin_manager.{cc,h}?

Thanks,
-Zach

[Yoyo Zhou, 2014-04-30 14:47:18]

PluginManager LGTM

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/plugin_manager.h (right):

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/plugin_manager.h:69: std::set<std::string>
mime_type_handler_;
This should be documented. It's not a great name - these aren't MimeTypeHandlers
but extension ids.

[not at google - send to devlin, 2014-04-30 15:42:43]

lgtm, sorry to keep harping on about the comments.

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:73:
info.tab_id.reset(new int(ExtensionTabUtil::GetTabId(web_contents)));
with regards to my previous comment, it's a little bit surprising to see you
only set the tab ID if there's a view ID, since if you have a |web_contents| you
*could* set the tab ID if you wanted.

perhaps it's worth just setting it, or indicating (as your response implied)
that tab_id will be deleted and |view_id| the only relevant parameter for
routing.

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.h
(right):

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.h:38: //
viewer.
the implementation implies that |web_contents| is ignored with |view_id| is
non-empty. Perhaps it's worth mentioning that (in a non-implementation-detaily
way) like

If the viewer is being opened in a BrowserPlugin, specify a non-empty |view_id|
of the plugin. This takes precedence over the tab ID.

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

https://codereview.chromium.org/263513004/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/streams_private.idl:17: // The ID of the tab that
opened the stream.
"Either/exactly-one-of |tabId| or |viewId| will be specified."?

https://codereview.chromium.org/263513004/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/streams_private.idl:20: // The ID of the view that
will render the stream.
ditto

[Zachary Kuznia, 2014-05-01 00:38:22]

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.cc
(right):

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.cc:73:
info.tab_id.reset(new int(ExtensionTabUtil::GetTabId(web_contents)));
On 2014/04/30 15:42:43, kalman wrote:
> with regards to my previous comment, it's a little bit surprising to see you
> only set the tab ID if there's a view ID, since if you have a |web_contents|
you
> *could* set the tab ID if you wanted.
> 
> perhaps it's worth just setting it, or indicating (as your response implied)
> that tab_id will be deleted and |view_id| the only relevant parameter for
> routing.

I'll go the route of always setting it.

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/streams_private/streams_private_api.h
(right):

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/streams_private/streams_private_api.h:38: //
viewer.
On 2014/04/30 15:42:43, kalman wrote:
> the implementation implies that |web_contents| is ignored with |view_id| is
> non-empty. Perhaps it's worth mentioning that (in a non-implementation-detaily
> way) like
> 
> If the viewer is being opened in a BrowserPlugin, specify a non-empty
|view_id|
> of the plugin. This takes precedence over the tab ID.

Updated the language for view_id.

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/plugin_manager.h (right):

https://codereview.chromium.org/263513004/diff/40001/chrome/browser/extension...
chrome/browser/extensions/plugin_manager.h:69: std::set<std::string>
mime_type_handler_;
On 2014/04/30 14:47:19, Yoyo Zhou wrote:
> This should be documented. It's not a great name - these aren't
MimeTypeHandlers
> but extension ids.

Added a comment and renamed the variable.

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

https://codereview.chromium.org/263513004/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/streams_private.idl:17: // The ID of the tab that
opened the stream.
On 2014/04/30 15:42:43, kalman wrote:
> "Either/exactly-one-of |tabId| or |viewId| will be specified."?

Reverted this to old language and behavior.

https://codereview.chromium.org/263513004/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/streams_private.idl:20: // The ID of the view that
will render the stream.
On 2014/04/30 15:42:43, kalman wrote:
> ditto

Clarified the conditions where this will be set.

[Fady Samuel, 2014-05-06 21:06:33]

I'm not terribly familiar with StreamResourceHandler and related code. I didn't
really touch these while writing BrowserPlugin. I don't think I'm a good
reviewer for this patch. With that said, nothing scary stands out to me, but
again I'm not too familiar with this code.

[Zachary Kuznia, 2014-05-06 22:00:24]

Hi Matt,

Could you take a look at:
content/browser/loader/buffered_resource_handler.{cc,h}
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.{cc,h}
content/browser/loader/stream_resource_handler.{cc,h}

Thanks,
-Zach

[mmenke, 2014-05-06 22:01:57]

On 2014/05/06 22:00:24, Zachary Kuznia wrote:
> Hi Matt,
> 
> Could you take a look at:
> content/browser/loader/buffered_resource_handler.{cc,h}
> chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.{cc,h}
> content/browser/loader/stream_resource_handler.{cc,h}
> 
> Thanks,
> -Zach

Sure, I'll take a look tomorrow.  Feel free to ping me if I haven't gotten back
to you by the end of the day, EST.

[mmenke, 2014-05-12 16:35:34]

So sorry I forgot about this.  Looking at this now, will get back to you later
today.

[mmenke, 2014-05-12 17:12:13]

https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:328: if
(handler->HasPayload()) {
I'm really not following any of this code.

https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/s...
File content/browser/loader/stream_resource_handler.cc (right):

https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/s...
content/browser/loader/stream_resource_handler.cc:68: &defer_ignored);
Why do we just send whatever portion of the payload we've already received to
the next handler, and then just claim that was the entire body, never sending it
the rest?

Actually...why do we need the payload at all?  The BufferedResourceHandler will
replay what we've received so far already and pass it to the
StreamResourceHandler, which can then send it to the next_handler_.

[mmenke, 2014-05-13 15:14:32]

https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:336: return
CopyReadBufferToNextHandler(request_id);
I think it's less than ideal to split the responsibility to handle the
particulars of this case between the BufferedResourceHandler and the
StreamResourceHandler like this - I don't think both classes should know about
the behaviors in this case.

Given that we want to use "UseAlternateNextHander" in the non-payload case, my
suggestion is to do this:

// Feel free to come up with a better name.
std::string payload_to_send_old_handler;

scoped_ptr<StreamResourceHandler> handler(request(), response_.get(),
&payload_to_send_old_handler);

if (handler) {
  UseAlternateNextHandler(handler.PassAs<ResourceHandler>(),
payload_to_send_old_handler);
}

Then just maintain the old behavior unless the payload is non-empty.

I'm also not sure about the non-200 response case, where we cancel the request
in UseAlternateNextHandler - do we want this behavior or not, in the new case?


An alternate approach would be to move all the logic into the
StreamResourceHandler, but I think you'd still need the check if you should call
UseAlternateNextHandler or not, so I prefer the other approach (Also, that means
only the BufferedResourceHandler is responsible for sending information to the
original next handler, in all cases, which I think is a little cleaner).

[Zachary Kuznia, 2014-05-20 22:24:39]

https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:336: return
CopyReadBufferToNextHandler(request_id);
On 2014/05/13 15:14:32, mmenke wrote:
> I think it's less than ideal to split the responsibility to handle the
> particulars of this case between the BufferedResourceHandler and the
> StreamResourceHandler like this - I don't think both classes should know about
> the behaviors in this case.
> 
> Given that we want to use "UseAlternateNextHander" in the non-payload case, my
> suggestion is to do this:
> 
> // Feel free to come up with a better name.
> std::string payload_to_send_old_handler;
> 
> scoped_ptr<StreamResourceHandler> handler(request(), response_.get(),
> &payload_to_send_old_handler);
> 
> if (handler) {
>   UseAlternateNextHandler(handler.PassAs<ResourceHandler>(),
> payload_to_send_old_handler);
> }
> 
> Then just maintain the old behavior unless the payload is non-empty.
> 
> I'm also not sure about the non-200 response case, where we cancel the request
> in UseAlternateNextHandler - do we want this behavior or not, in the new case?
> 
> 
> An alternate approach would be to move all the logic into the
> StreamResourceHandler, but I think you'd still need the check if you should
call
> UseAlternateNextHandler or not, so I prefer the other approach (Also, that
means
> only the BufferedResourceHandler is responsible for sending information to the
> original next handler, in all cases, which I think is a little cleaner).

If I understand correctly, you're saying that I should move the code in
StreamResourceHandler() into UseAlternateNextHandler(), and use that codepath
instead of cancelling the request if a payload is passed in via
payload_to_send_old_handler?

[mmenke, 2014-05-21 15:17:48]

https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:336: return
CopyReadBufferToNextHandler(request_id);
On 2014/05/20 22:24:39, Zachary Kuznia wrote:
> On 2014/05/13 15:14:32, mmenke wrote:
> > I think it's less than ideal to split the responsibility to handle the
> > particulars of this case between the BufferedResourceHandler and the
> > StreamResourceHandler like this - I don't think both classes should know
about
> > the behaviors in this case.
> > 
> > Given that we want to use "UseAlternateNextHander" in the non-payload case,
my
> > suggestion is to do this:
> > 
> > // Feel free to come up with a better name.
> > std::string payload_to_send_old_handler;
> > 
> > scoped_ptr<StreamResourceHandler> handler(request(), response_.get(),
> > &payload_to_send_old_handler);
> > 
> > if (handler) {
> >   UseAlternateNextHandler(handler.PassAs<ResourceHandler>(),
> > payload_to_send_old_handler);
> > }
> > 
> > Then just maintain the old behavior unless the payload is non-empty.
> > 
> > I'm also not sure about the non-200 response case, where we cancel the
request
> > in UseAlternateNextHandler - do we want this behavior or not, in the new
case?
> > 
> > 
> > An alternate approach would be to move all the logic into the
> > StreamResourceHandler, but I think you'd still need the check if you should
> call
> > UseAlternateNextHandler or not, so I prefer the other approach (Also, that
> means
> > only the BufferedResourceHandler is responsible for sending information to
the
> > original next handler, in all cases, which I think is a little cleaner).
> 
> If I understand correctly, you're saying that I should move the code in
> StreamResourceHandler() into UseAlternateNextHandler(), and use that codepath
> instead of cancelling the request if a payload is passed in via
> payload_to_send_old_handler?

That's my suggestion.  I don't think the new behavior should be split between
the BufferedResourceHandler and the StreamResourceHandler.  I'm certainly open
to doing that in other ways, but that seems the simplest option to me.

[Zachary Kuznia, 2014-05-29 00:14:26]

On 2014/05/21 15:17:48, mmenke wrote:
>
https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/b...
> File content/browser/loader/buffered_resource_handler.cc (right):
> 
>
https://codereview.chromium.org/263513004/diff/60001/content/browser/loader/b...
> content/browser/loader/buffered_resource_handler.cc:336: return
> CopyReadBufferToNextHandler(request_id);
> On 2014/05/20 22:24:39, Zachary Kuznia wrote:
> > On 2014/05/13 15:14:32, mmenke wrote:
> > > I think it's less than ideal to split the responsibility to handle the
> > > particulars of this case between the BufferedResourceHandler and the
> > > StreamResourceHandler like this - I don't think both classes should know
> about
> > > the behaviors in this case.
> > > 
> > > Given that we want to use "UseAlternateNextHander" in the non-payload
case,
> my
> > > suggestion is to do this:
> > > 
> > > // Feel free to come up with a better name.
> > > std::string payload_to_send_old_handler;
> > > 
> > > scoped_ptr<StreamResourceHandler> handler(request(), response_.get(),
> > > &payload_to_send_old_handler);
> > > 
> > > if (handler) {
> > >   UseAlternateNextHandler(handler.PassAs<ResourceHandler>(),
> > > payload_to_send_old_handler);
> > > }
> > > 
> > > Then just maintain the old behavior unless the payload is non-empty.
> > > 
> > > I'm also not sure about the non-200 response case, where we cancel the
> request
> > > in UseAlternateNextHandler - do we want this behavior or not, in the new
> case?
> > > 
> > > 
> > > An alternate approach would be to move all the logic into the
> > > StreamResourceHandler, but I think you'd still need the check if you
should
> > call
> > > UseAlternateNextHandler or not, so I prefer the other approach (Also, that
> > means
> > > only the BufferedResourceHandler is responsible for sending information to
> the
> > > original next handler, in all cases, which I think is a little cleaner).
> > 
> > If I understand correctly, you're saying that I should move the code in
> > StreamResourceHandler() into UseAlternateNextHandler(), and use that
codepath
> > instead of cancelling the request if a payload is passed in via
> > payload_to_send_old_handler?
> 
> That's my suggestion.  I don't think the new behavior should be split between
> the BufferedResourceHandler and the StreamResourceHandler.  I'm certainly open
> to doing that in other ways, but that seems the simplest option to me.

The logic is now moved into UseAlternateNextHandler.

[mmenke, 2014-05-29 16:10:29]

Just nits.

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:365: if
(payload_for_old_handler.empty()) {
Suggest a short comment about this check here.

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:401: CHECK(size >=
(int)payload_for_old_handler.length());
nit:  CHECK_GE

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:401: CHECK(size >=
(int)payload_for_old_handler.length());
C-style casts are forbidden.  use static_cast.

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:406: defer_ignored = false;
This isn't done on the above "next_handler_->OnResponseCompleted" - should
either do it for all of them, or none.  I'm fine, either way, not sure setting
it to false really gets us anything, but I don't think it hurts, either.

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.h:218: std::string*
payload);
Please document payload, and how behavior is different if it's left empty.

[Zachary Kuznia, 2014-05-29 20:42:39]

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/b...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:365: if
(payload_for_old_handler.empty()) {
On 2014/05/29 16:10:29, mmenke wrote:
> Suggest a short comment about this check here.

After further consideration, I decided that this error handling case should
remain as it was.

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:401: CHECK(size >=
(int)payload_for_old_handler.length());
On 2014/05/29 16:10:29, mmenke wrote:
> nit:  CHECK_GE

Done.

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/b...
content/browser/loader/buffered_resource_handler.cc:406: defer_ignored = false;
On 2014/05/29 16:10:29, mmenke wrote:
> This isn't done on the above "next_handler_->OnResponseCompleted" - should
> either do it for all of them, or none.  I'm fine, either way, not sure setting
> it to false really gets us anything, but I don't think it hurts, either.

Done.

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/263513004/diff/80001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.h:218: std::string*
payload);
On 2014/05/29 16:10:29, mmenke wrote:
> Please document payload, and how behavior is different if it's left empty.

Done.

[mmenke, 2014-05-30 15:24:32]

LGTM!

https://codereview.chromium.org/263513004/diff/100001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/263513004/diff/100001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:215: // the value should
be sent to the old resource handler instead of cancelling
nit:  Should -> will.

https://codereview.chromium.org/263513004/diff/100001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:216: // it.
nit:  Maybe add "except on HTTP errors"?

[Zachary Kuznia, 2014-05-30 17:47:21]

https://codereview.chromium.org/263513004/diff/100001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/263513004/diff/100001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:216: // it.
On 2014/05/30 15:24:32, mmenke wrote:
> nit:  Maybe add "except on HTTP errors"?

Done.

https://codereview.chromium.org/263513004/diff/100001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:216: // it.
On 2014/05/30 15:24:32, mmenke wrote:
> nit:  Maybe add "except on HTTP errors"?

Done.

[Zachary Kuznia, 2014-05-30 17:49:45]

Hello jam,

Could you do an OWNERS review of:
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.h
content/public/browser/resource_dispatcher_host_delegate.cc
content/public/browser/resource_dispatcher_host_delegate.h

Thanks,
-Zach

[jam, 2014-05-30 21:05:27]

https://codereview.chromium.org/263513004/diff/120001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/263513004/diff/120001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:114: std::string*
view_id);
can you document what payload and view_id are and how they're supposed to be
used? looking at this, i have no idea what view_id is

[Zachary Kuznia, 2014-05-30 21:21:05]

https://codereview.chromium.org/263513004/diff/120001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/263513004/diff/120001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:114: std::string*
view_id);
On 2014/05/30 21:05:27, jam wrote:
> can you document what payload and view_id are and how they're supposed to be
> used? looking at this, i have no idea what view_id is

Done.

[jam, 2014-05-30 23:04:16]

https://codereview.chromium.org/263513004/diff/140001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/263513004/diff/140001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:120: std::string*
view_id);
if my reading is correct, content calls chrome with this method to get a
"view_id", and then it calls it back right after with the view_id that chrome
gave it.

why can't chrome cache this itself in its delegate implementation, based on a
key that is based on the existing parameters?

[Zachary Kuznia, 2014-05-30 23:40:54]

https://codereview.chromium.org/263513004/diff/140001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/263513004/diff/140001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:120: std::string*
view_id);
On 2014/05/30 23:04:16, jam wrote:
> if my reading is correct, content calls chrome with this method to get a
> "view_id", and then it calls it back right after with the view_id that chrome
> gave it.
> 
> why can't chrome cache this itself in its delegate implementation, based on a
> key that is based on the existing parameters?

None of the other parameters are guaranteed to be unique.  The target_id could
possibly be used for this purpose, but it will be removed once all the clients
have migrated to the new API.

[jam, 2014-06-02 06:41:52]

https://codereview.chromium.org/263513004/diff/140001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/263513004/diff/140001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:120: std::string*
view_id);
On 2014/05/30 23:40:55, Zachary Kuznia wrote:
> On 2014/05/30 23:04:16, jam wrote:
> > if my reading is correct, content calls chrome with this method to get a
> > "view_id", and then it calls it back right after with the view_id that
chrome
> > gave it.
> > 
> > why can't chrome cache this itself in its delegate implementation, based on
a
> > key that is based on the existing parameters?
> 
> None of the other parameters are guaranteed to be unique.  The target_id could
> possibly be used for this purpose, but it will be removed once all the clients
> have migrated to the new API.

I see. Ok, since I'd really like to avoid this extra concept in the public API
as it's chrome-specific, let's make this method and the one below have a first
parameter as the net::URLRequest* to match most of the methods in this
interface. the chrome delegate can then keep its own mapping from
child_id+request_id to target_id+view_id. this method can then drop the
resource_context/url, while the next method can drop all the parameters other
than the StreamHandle since they're accessible from the net::URLRequest

[Zachary Kuznia, 2014-06-05 22:28:22]

https://codereview.chromium.org/263513004/diff/140001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/263513004/diff/140001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:120: std::string*
view_id);
On 2014/06/02 06:41:53, jam wrote:
> On 2014/05/30 23:40:55, Zachary Kuznia wrote:
> > On 2014/05/30 23:04:16, jam wrote:
> > > if my reading is correct, content calls chrome with this method to get a
> > > "view_id", and then it calls it back right after with the view_id that
> chrome
> > > gave it.
> > > 
> > > why can't chrome cache this itself in its delegate implementation, based
on
> a
> > > key that is based on the existing parameters?
> > 
> > None of the other parameters are guaranteed to be unique.  The target_id
could
> > possibly be used for this purpose, but it will be removed once all the
clients
> > have migrated to the new API.
> 
> I see. Ok, since I'd really like to avoid this extra concept in the public API
> as it's chrome-specific, let's make this method and the one below have a first
> parameter as the net::URLRequest* to match most of the methods in this
> interface. the chrome delegate can then keep its own mapping from
> child_id+request_id to target_id+view_id. this method can then drop the
> resource_context/url, while the next method can drop all the parameters other
> than the StreamHandle since they're accessible from the net::URLRequest

Done.

[jam, 2014-06-06 15:00:41]

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/url_request_util.cc (right):

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/url_request_util.cc:137: return true;
ditto

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:449: *allowed =
true;
is this a local testing change?

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:584:
stream_target_info_[request] = target_info;
nit: check in the destructor of this class that this map is empty

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:605:
request->GetExpectedContentSize(), render_process_id,
nit: once you don't take in the render_process_id and render_view_id and extract
them from ResourceRequestInfo, use request_info->GetRenderFrameID. in the target
method,  you can just write
RenderFrameHost* rfh = RenderFrameHost::FromID(render_process_id,
render_frame_id);
WebContents* wc = WebContents::FromRenderFrameHost(rfh);

(i.e. no null checks needed since WC::FromRFH does them)

https://codereview.chromium.org/263513004/diff/160001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/263513004/diff/160001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:61: class
StreamResourceHandler;
nit: not needed

https://codereview.chromium.org/263513004/diff/160001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/263513004/diff/160001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:112:
content::ResourceContext* resource_context,
nit: don't pass this as it's available from ResourceRequestInfo which embedders
can easily get from the request through ResourceRequestInfo::ForRequest

https://codereview.chromium.org/263513004/diff/160001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:123: int
render_view_id,
ditto for the above three parameters

[Zachary Kuznia, 2014-06-07 01:45:37]

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_render_message_filter.cc:449: *allowed =
true;
On 2014/06/06 15:00:40, jam wrote:
> is this a local testing change?

Yes.  Forgot to specify the right branch when uploading.  Sorry about that.

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/renderer...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:584:
stream_target_info_[request] = target_info;
On 2014/06/06 15:00:40, jam wrote:
> nit: check in the destructor of this class that this map is empty

Done.

https://codereview.chromium.org/263513004/diff/160001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:605:
request->GetExpectedContentSize(), render_process_id,
On 2014/06/06 15:00:40, jam wrote:
> nit: once you don't take in the render_process_id and render_view_id and
extract
> them from ResourceRequestInfo, use request_info->GetRenderFrameID. in the
target
> method,  you can just write
> RenderFrameHost* rfh = RenderFrameHost::FromID(render_process_id,
> render_frame_id);
> WebContents* wc = WebContents::FromRenderFrameHost(rfh);
> 
> (i.e. no null checks needed since WC::FromRFH does them)

Done.

https://codereview.chromium.org/263513004/diff/160001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/263513004/diff/160001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:61: class
StreamResourceHandler;
On 2014/06/06 15:00:40, jam wrote:
> nit: not needed

Done.

https://codereview.chromium.org/263513004/diff/160001/content/public/browser/...
File content/public/browser/resource_dispatcher_host_delegate.h (right):

https://codereview.chromium.org/263513004/diff/160001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:112:
content::ResourceContext* resource_context,
On 2014/06/06 15:00:40, jam wrote:
> nit: don't pass this as it's available from ResourceRequestInfo which
embedders
> can easily get from the request through ResourceRequestInfo::ForRequest

Done.

https://codereview.chromium.org/263513004/diff/160001/content/public/browser/...
content/public/browser/resource_dispatcher_host_delegate.h:123: int
render_view_id,
On 2014/06/06 15:00:40, jam wrote:
> ditto for the above three parameters

Done.

[jam, 2014-06-09 22:56:57]

lgtm

[Zachary Kuznia, 2014-06-12 17:59:50]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-06-12 18:02:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zork@chromium.org/263513004/220001

[commit-bot: I haz the power, 2014-06-12 22:57:58]

Change committed as 276841