Make the logic for stream interception by MimeHandlerViews consistent with starting the plugin

Make the logic for stream interception by MimeHandlerViews consistent with starting the plugin

Currently there is logic in the renderer which checks whether a plugin can
handle a mime type and if it can the plugin is started. This is also how
MimeHandlerView plugins are started in the renderer. MimeHandlerViews also
rely on the original http request being intercepted as a stream which the
plugin can read when it chooses. This happens in the browser. Currently
the logic for determining whether the stream should be intercepted is different
to the logic for determining whether to start the plugin which can lead to
problems in some cases. This CL makes sure the logic is the same: on both
sides we check whether the plugin can handle the mime type.

MimeHandlerView plugins load a chrome extension inside a BrowserPlugin.
Once it has been determined that the plugin can handle the mime type inside
BufferedResourceHandler, the extension ID of the plugin is passed down through into
ChromeResourceDispatcherHostDelegate::ShouldInterceptResourceAsStream.
It is then checked that the extension is enabled and if so the stream is intercepted.
There is some complexity because we also need to support the legacy case
where the stream is intercepted for the streamsPrivate api in which case there is no
MimeHandlerView plugin.

This CL was originally written by deepak.m1@samsung.com (Deepak Mittal)
and landed here: https://codereview.chromium.org/953793003/. It was reverted
because of a bug it triggered with not downloading PDFs when the PDF plugin
was disabled. This bug has been addressed and a test added for that case.

BUG=443466
Committed: https://crrev.com/9dcebac2889710b6e92b751d0434d06bf240dba4
Cr-Commit-Position: refs/heads/master@{#339816}

[raymes, 2015-06-05 00:26:28]

raymes@chromium.org changed reviewers:
+ mmenke@chromium.org

[raymes, 2015-06-05 00:26:29]

mmenke: PTAL :) Note that this is the same as the patch which Deepak landed and
you reviewed in https://codereview.chromium.org/953793003/ with the following
changes:
1) Fixed the bug that was causing PDFs not to be downloaded and added a test for
that.
2) Added an explanation for the plugin/extension stuff you were confused about.
3) Extended the CL description to try to add more context.

Hopefully you wont have to re-review the whole thing :)

[Deepak, 2015-06-05 03:03:59]

deepak.m1@samsung.com changed reviewers:
+ deepak.m1@samsung.com, thakis@chromium.org

[Deepak, 2015-06-05 03:04:00]

As far as I remember earlier @thakis was asking test case that should hit 
ChromeResourceDispatcherHostDelegate::ShouldInterceptResourceAsStream() and
check 
if (mime_handler_extension_url == extension->url()) and else conditions 
and do DCHECK.
@thakis please correct me if I missed something.

[raymes, 2015-06-05 03:08:50]

On 2015/06/05 03:04:00, Deepak wrote:
> As far as I remember earlier @thakis was asking test case that should hit 
> ChromeResourceDispatcherHostDelegate::ShouldInterceptResourceAsStream() and
> check 
> if (mime_handler_extension_url == extension->url()) and else conditions 
> and do DCHECK.
> @thakis please correct me if I missed something.

Thanks Deepak. The test case I added does cover that code path. It's very hard
to add a test specifically for the code in there because that class isn't easily
testable. Unfortunately I don't really have the time to invest in doing that but
I think the value in landing this patch is still high and worthwhile and the
test I added covers an important use of this code.

[mmenke, 2015-06-05 18:45:50]

I hate reviewing this code.  It's not at all clear what it's doing, due to a
combination of unclear method names, and lack of documentation.

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:326: return false;
Can this happen?  When?

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:334: // go away.
Don't use "we" in comments

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler.h (right):

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.h:62: // be cancelled and true
otherwise.
Is the request actually cancelled, or handed off to something else...Or what?

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.h:66: const std::string&
payload_for_old_handler);
Can we document the return value of UseAlternateNextHandler?

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

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:244: // testing.
Can we document what this method actually does, rather than just when it must be
called?

[raymes, 2015-06-09 00:10:46]

mmenke: I'm really sorry that it's painful code to review :( Please note that I
didn't write the code here originally and so I'm not too excited about it
either, I'm just trying to make it work well. I've tried to address your
comments below Thanks for your help!

Raymes

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:326: return false;
On 2015/06/05 18:45:49, mmenke wrote:
> Can this happen?  When?

This actually should never happen. If the plugin is there, then the stream
should always be intercepted. I've added NOTREACHED and an explanation.

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:334: // go away.
On 2015/06/05 18:45:49, mmenke wrote:
> Don't use "we" in comments

Done.

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler.h (right):

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.h:62: // be cancelled and true
otherwise.
The request is actually cancelled AFAICT

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.h:66: const std::string&
payload_for_old_handler);
On 2015/06/05 18:45:50, mmenke wrote:
> Can we document the return value of UseAlternateNextHandler?

Done.

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

https://codereview.chromium.org/1164073006/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:244: // testing.
On 2015/06/05 18:45:50, mmenke wrote:
> Can we document what this method actually does, rather than just when it must
be
> called?

Done.

[raymes, 2015-06-10 23:58:05]

ping

[mmenke, 2015-06-11 16:19:19]

On 2015/06/09 00:10:46, raymes wrote:
> mmenke: I'm really sorry that it's painful code to review :( Please note that
I
> didn't write the code here originally and so I'm not too excited about it
> either, I'm just trying to make it work well. I've tried to address your
> comments below Thanks for your help!

Not blaming you, just giving a reason why I always prioritize this review last
(And make comments only indirectly related to modified code).  Hoping if we
clean things up a little each CL, the code will eventually be easier to review.

Sorry, this somehow fell off my radar between Tuesday and Wednesday.  I'll get
to it today.

[mmenke, 2015-06-11 19:38:04]

Haven't looked at the unittest

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:178: url_ = GURL();
Is there a reason we need to clear the URL?  Should only start one download,
right?

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:182: void
OnDownloadCreated(content::DownloadManager* manager,
Maybe add something like:

// content::DownloadManager::Observer implementation

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:183: content::DownloadItem* item)
override {
Should include download_item.h

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:186: CHECK(!url_.is_empty());
Need to include base/logging.h for CHECK, and url/gurl.h for GURL

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:196:
browser()->tab_strip_model()->GetActiveWebContents();
Need to include the browser and tab string model headers

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:197: content::BrowserContext*
browser_context = web_contents->GetBrowserContext();
Need the web_contents header.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:198: Profile* profile =
Profile::FromBrowserContext(browser_context);
Need to include the profile header.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:199: scoped_refptr<PluginPrefs>
prefs(PluginPrefs::GetForProfile(profile));
Include ref_counted.h?

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:213: ASSERT_EQ(url,
download_observer.GetAndClearLastUrl());
I'm not sufficiently familiar with navigation/download code to verify this is
non-racy.  Could we add a RunLoop to make GetAndClearLastUrl wait as well as get
the URL?  Otherwise, someone else should probably review this.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:217:
std::vector<content::DownloadItem*> downloads;
include <vector>

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

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:612:
GURL mime_handler_extension_url(plugin_path.AsUTF8Unsafe());
I'm not following this...We have a path, we get it as a string...Then we stick
it in a GURL?  Not adding a scheme or anything?

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:629:
target_info.extension_id = extension_id;
I can't verify the correctness of this code.

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

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:324: plugin.path, request(),
response_.get(), &payload));
This exactly matches the fallthrough logic.  Maybe this should be:

if (has_plugin && plugin.type != WebPluginInfo::PLUGIN_TYPE_BROWSER_PLUGIN) {
  // Some comment here
  return true;
}

// If it is a MimeHandlerView plugin or using the legacy streamsPrivate
extensions
// API, attempt to intercept the stream.
// TODO...
...
if (handler) {
  ...
  return true;
}

DCHECK(!has_plugin);
// Could have special case logic here if has_plugin is true, if you want.
return false;


It does require a slightly bigger change when the streamsPrivate API case is
removed, but think it's clearer that both cases follow the same logic.

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:330: // intercepted as a
stream so this point should never be reached.
This is content/, not chrome/.  I assume MimeHandlerView is a chrome/ concept?

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:339: // streamsPrivate
extensions API. This API is deprecated and should go away.
The streamsPrivate API is a chrome feature.  We're in content.  Can we document
this in some more content-friendly manner.

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:373: if (handled_by_plugin)
If |defer| is set to true, the method also must return true, which seems a
little unexpected.  Suggest "if (handled_by_plugin || defer)", here and below,
and documenting what is returned in the defer case in the header.

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler.h (right):

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.h:62: // be cancelled and true
otherwise.
A method called "IsBlah" really should not mean "do something if Blah is true". 
Maybe SelectPluginHandler()?

Also, I suggest swapping the return value and |request_handled|.  The other
related methods (SelectNextHandler, UseAlternateNextHandler) return false to
indicate a fatal failure.  Seems like we should be consistent.

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

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:253: const
base::FilePath& plugin_path,
Should also document plugin path.

[mmenke, 2015-06-11 20:01:26]

Also, FYI:  I have a CL out to rename BufferedResourceHandler to
MimeTypeResourceHandler.

[mmenke, 2015-06-15 16:40:02]

On 2015/06/11 20:01:26, mmenke wrote:
> Also, FYI:  I have a CL out to rename BufferedResourceHandler to
> MimeTypeResourceHandler.

Note that I've landed that renaming CL, so you'll need to merge/rebase.

[raymes, 2015-07-01 05:13:16]

raymes@chromium.org changed reviewers:
+ sammc@chromium.org
- thakis@chromium.org

[raymes, 2015-07-01 05:13:19]

+sammc to take a look at chrome_resource_dispatcher_host_delegate.cc

Thanks mmenke! This has also been low on my todo list too (for the same reasons)
but I think it's still worth landing. Addressed your concerns (hopefully).

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:178: url_ = GURL();
On 2015/06/11 19:38:04, mmenke (Off June 26 to July 5) wrote:
> Is there a reason we need to clear the URL?  Should only start one download,
> right?

Done.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:182: void
OnDownloadCreated(content::DownloadManager* manager,
On 2015/06/11 19:38:04, mmenke (Off June 26 to July 5) wrote:
> Maybe add something like:
> 
> // content::DownloadManager::Observer implementation

Done.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:183: content::DownloadItem* item)
override {
On 2015/06/11 19:38:04, mmenke (Off June 26 to July 5) wrote:
> Should include download_item.h

Done.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:186: CHECK(!url_.is_empty());
On 2015/06/11 19:38:03, mmenke (Off June 26 to July 5) wrote:
> Need to include base/logging.h for CHECK, and url/gurl.h for GURL

Done.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:196:
browser()->tab_strip_model()->GetActiveWebContents();
On 2015/06/11 19:38:03, mmenke (Off June 26 to July 5) wrote:
> Need to include the browser and tab string model headers

Done.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:197: content::BrowserContext*
browser_context = web_contents->GetBrowserContext();
On 2015/06/11 19:38:03, mmenke (Off June 26 to July 5) wrote:
> Need the web_contents header.

Done.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:198: Profile* profile =
Profile::FromBrowserContext(browser_context);
On 2015/06/11 19:38:04, mmenke (Off June 26 to July 5) wrote:
> Need to include the profile header.

Done.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:199: scoped_refptr<PluginPrefs>
prefs(PluginPrefs::GetForProfile(profile));
On 2015/06/11 19:38:03, mmenke (Off June 26 to July 5) wrote:
> Include ref_counted.h?

Done.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:213: ASSERT_EQ(url,
download_observer.GetAndClearLastUrl());
On 2015/06/11 19:38:03, mmenke (Off June 26 to July 5) wrote:
> I'm not sufficiently familiar with navigation/download code to verify this is
> non-racy.  Could we add a RunLoop to make GetAndClearLastUrl wait as well as
get
> the URL?  Otherwise, someone else should probably review this.

The comment in NavigateToURL says:
// Navigates the selected tab of |browser| to |url|, blocking until the
navigation finishes

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:217:
std::vector<content::DownloadItem*> downloads;
On 2015/06/11 19:38:03, mmenke (Off June 26 to July 5) wrote:
> include <vector>

Done.

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

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:612:
GURL mime_handler_extension_url(plugin_path.AsUTF8Unsafe());
You're right, it doesn't make much sense, especially in the context here. I've
reworked the code. PTAL.

https://codereview.chromium.org/1164073006/diff/60001/chrome/browser/renderer...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:629:
target_info.extension_id = extension_id;
That's ok, I'll get sammc@ to review this. He's familiar with it.

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

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:324: plugin.path, request(),
response_.get(), &payload));
That's much simpler, thanks!

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:330: // intercepted as a
stream so this point should never be reached.
On 2015/06/11 19:38:04, mmenke (Off June 26 to July 5) wrote:
> This is content/, not chrome/.  I assume MimeHandlerView is a chrome/ concept?

Done.

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:339: // streamsPrivate
extensions API. This API is deprecated and should go away.
On 2015/06/11 19:38:04, mmenke (Off June 26 to July 5) wrote:
> The streamsPrivate API is a chrome feature.  We're in content.  Can we
document
> this in some more content-friendly manner.

Done.

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.cc:373: if (handled_by_plugin)
On 2015/06/11 19:38:04, mmenke (Off June 26 to July 5) wrote:
> If |defer| is set to true, the method also must return true, which seems a
> little unexpected.  Suggest "if (handled_by_plugin || defer)", here and below,
> and documenting what is returned in the defer case in the header.

Done.

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
File content/browser/loader/buffered_resource_handler.h (right):

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/buffered_resource_handler.h:62: // be cancelled and true
otherwise.
On 2015/06/11 19:38:04, mmenke (Off June 26 to July 5) wrote:
> A method called "IsBlah" really should not mean "do something if Blah is
true". 
> Maybe SelectPluginHandler()?
> 
> Also, I suggest swapping the return value and |request_handled|.  The other
> related methods (SelectNextHandler, UseAlternateNextHandler) return false to
> indicate a fatal failure.  Seems like we should be consistent.

Done.

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

https://codereview.chromium.org/1164073006/diff/60001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.h:253: const
base::FilePath& plugin_path,
On 2015/06/11 19:38:04, mmenke (Off June 26 to July 5) wrote:
> Should also document plugin path.

Done.

[Sam McNally, 2015-07-03 06:21:31]

https://codereview.chromium.org/1164073006/diff/140001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1164073006/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:625: if
(!plugin_path.empty()) {
How about looking up the extension using |plugin_path| when it's non-empty? It
would require specifying a particular meaning for browser plugin paths, but that
doesn't seem too bad.

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

https://codereview.chromium.org/1164073006/diff/140001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:102: const
base::FilePath& plugin_path,
Please document |plugin_path|.

[raymes, 2015-07-06 02:30:04]

https://codereview.chromium.org/1164073006/diff/140001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1164073006/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:625: if
(!plugin_path.empty()) {
On 2015/07/03 06:21:31, Sam McNally wrote:
> How about looking up the extension using |plugin_path| when it's non-empty? It
> would require specifying a particular meaning for browser plugin paths, but
that
> doesn't seem too bad.

I think I would rather keep that as an implementation detail in
MimeTypesHandler.

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

https://codereview.chromium.org/1164073006/diff/140001/content/public/browser...
content/public/browser/resource_dispatcher_host_delegate.h:102: const
base::FilePath& plugin_path,
On 2015/07/03 06:21:31, Sam McNally wrote:
> Please document |plugin_path|.

Done.

[Sam McNally, 2015-07-07 06:44:28]

chrome_resource_dispatcher_host_delegate LGTM

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

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:636: }
else {
else if

[mmenke, 2015-07-09 20:35:40]

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/pdf/pdf...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:188: GURL GetLastUrl() {
nit:  const GURL& last_url() const

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:189: return url_;
nit: url_ -> last_url_?

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:222: ASSERT_EQ(url,
download_observer.GetLastUrl());
On 2015/07/01 05:13:18, raymes wrote:
> On 2015/06/11 19:38:03, mmenke (Off June 26 to July 5) wrote:
> > I'm not sufficiently familiar with navigation/download code to verify this
is
> > non-racy.  Could we add a RunLoop to make GetAndClearLastUrl wait as well as
> get
> > the URL?  Otherwise, someone else should probably review this.
> 
> The comment in NavigateToURL says:
> // Navigates the selected tab of |browser| to |url|, blocking until the
> navigation finishes

Right...But the download is not considered part of the navigation, so it's not
clear whether or not the navigation stops after the download is created.

It's possible there could be something like:

if (navigation_is_really_download) {
  PostUITask(StopNavigation);
  PostIOTask(ConvertRequestToDownload);
}

(Of course, with the renderer process involved, things get much uglier).  I
don't think one can argue this is non-racy without digging into the download
code, and how the navigation is actually stopped.

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

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:622: //
If a plugin path is provided then we are trying to intercept the stream
nit:  SHouldn't use we in comments.

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:296: bool* request_handled)
{
request_handled -> handled_by_plugin (Clearer, and matches callers).

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:298: *request_handled =
false;
I don't think initialization is needed.  Alternatively, could just not have the
callers initialize it.

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:358: bool request_handled =
SelectPluginHandler(defer, &handled_by_plugin);
request_handled is a misnomer (And easy to confuse with "handled_by_plugin"). 
Maybe "no_fatal_error"?  Or could just inline the call, and not bother with a
name.

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:380: bool request_handled =
SelectPluginHandler(defer, &handled_by_plugin);
See above comment about "request_handled"

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

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.h:69: // Determines whether a
plugin will handle the current request. Returns false
Maybe:  "Determines whether a plugin will handle the current request." ->
"Determines whether a plugin will handle the current request, and if so, sets up
the handler to direct the response to that plugin."

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.h:74: // is set to true.
Maybe "If a plugin can handle the request, ...." -> "If the request is directed
to a plugin, ...."

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.h:75: bool
SelectPluginHandler(bool* defer, bool* request_handled);
request_handled -> handled_by_plugin

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

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:725: std::string*
payload) {
Suggest payload->clear() here.  Makes for a clearer API contract.  Or could
update docs to say it being changed indicates we should send it.

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

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:249: // and associated
with the request. If |payload| is set to a non-empty value,
This makes it sound like the RDH sends the payload, which is incorrect.

Maybe "If |payload| is set to a non-empty value, the caller must send it to the
old...."

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:250: // the  value will
be sent to the old resource handler instead of canceling
"the  value" -> "the value"

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:250: // the  value will
be sent to the old resource handler instead of canceling
"canceling" -> "canceling it."

[raymes, 2015-07-17 03:21:01]

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/pdf/pdf...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:188: GURL GetLastUrl() {
On 2015/07/09 20:35:39, mmenke wrote:
> nit:  const GURL& last_url() const

Done.

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:189: return url_;
On 2015/07/09 20:35:39, mmenke wrote:
> nit: url_ -> last_url_?

Done.

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/pdf/pdf...
chrome/browser/pdf/pdf_extension_test.cc:222: ASSERT_EQ(url,
download_observer.GetLastUrl());
I agree with you, it's not completely clear whether it should work. I added a
RunLoop as suggested earlier.

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

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:622: //
If a plugin path is provided then we are trying to intercept the stream
On 2015/07/09 20:35:39, mmenke wrote:
> nit:  SHouldn't use we in comments.

Done.

https://codereview.chromium.org/1164073006/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:636: }
else {
On 2015/07/07 06:44:28, Sam McNally wrote:
> else if

Done. I reworked the code slightly

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:296: bool* request_handled)
{
On 2015/07/09 20:35:39, mmenke wrote:
> request_handled -> handled_by_plugin (Clearer, and matches callers).

Done.

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:298: *request_handled =
false;
On 2015/07/09 20:35:39, mmenke wrote:
> I don't think initialization is needed.  Alternatively, could just not have
the
> callers initialize it.

Done.

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:358: bool request_handled =
SelectPluginHandler(defer, &handled_by_plugin);
On 2015/07/09 20:35:39, mmenke wrote:
> request_handled is a misnomer (And easy to confuse with "handled_by_plugin"). 
> Maybe "no_fatal_error"?  Or could just inline the call, and not bother with a
> name.

Done.

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:380: bool request_handled =
SelectPluginHandler(defer, &handled_by_plugin);
On 2015/07/09 20:35:39, mmenke wrote:
> See above comment about "request_handled"

Done.

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

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.h:69: // Determines whether a
plugin will handle the current request. Returns false
On 2015/07/09 20:35:39, mmenke wrote:
> Maybe:  "Determines whether a plugin will handle the current request." ->
> "Determines whether a plugin will handle the current request, and if so, sets
up
> the handler to direct the response to that plugin."

Done.

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.h:74: // is set to true.
On 2015/07/09 20:35:39, mmenke wrote:
> Maybe "If a plugin can handle the request, ...." -> "If the request is
directed
> to a plugin, ...."

Done.

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.h:75: bool
SelectPluginHandler(bool* defer, bool* request_handled);
On 2015/07/09 20:35:39, mmenke wrote:
> request_handled -> handled_by_plugin

Done.

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

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:725: std::string*
payload) {
On 2015/07/09 20:35:39, mmenke wrote:
> Suggest payload->clear() here.  Makes for a clearer API contract.  Or could
> update docs to say it being changed indicates we should send it.

Done.

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

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:249: // and associated
with the request. If |payload| is set to a non-empty value,
On 2015/07/09 20:35:39, mmenke wrote:
> This makes it sound like the RDH sends the payload, which is incorrect.
> 
> Maybe "If |payload| is set to a non-empty value, the caller must send it to
the
> old...."

Done.

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:250: // the  value will
be sent to the old resource handler instead of canceling
On 2015/07/09 20:35:39, mmenke wrote:
> "canceling" -> "canceling it."

Done.

https://codereview.chromium.org/1164073006/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:250: // the  value will
be sent to the old resource handler instead of canceling
On 2015/07/09 20:35:39, mmenke wrote:
> "the  value" -> "the value"

Done.

[mmenke, 2015-07-17 19:47:51]

Think this is my last set of comments.

https://codereview.chromium.org/1164073006/diff/260001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1164073006/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:609: //
streamsPrivate API.
Optional:  The behavior is the same, but I think the pattern:

if (!plugin_path.empty()) {
  if (...)
} else {
  if (...)
}

better fits your comment, and emphasizes that plugin path's state is being used
to choose between the two codepaths.

https://codereview.chromium.org/1164073006/diff/260001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1164073006/diff/260001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:298: *handled_by_plugin =
false;
BUG:  This should go above the #if.

https://codereview.chromium.org/1164073006/diff/260001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:302:
ResourceRequestInfoImpl* info = GetRequestInfo();
optional nit:  Suggest reordering these to match the order they're passed in to
GetPluginInfo.

[raymes, 2015-07-20 04:14:18]

Thanks!

https://codereview.chromium.org/1164073006/diff/260001/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/1164073006/diff/260001/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:609: //
streamsPrivate API.
On 2015/07/17 19:47:51, mmenke wrote:
> Optional:  The behavior is the same, but I think the pattern:
> 
> if (!plugin_path.empty()) {
>   if (...)
> } else {
>   if (...)
> }
> 
> better fits your comment, and emphasizes that plugin path's state is being
used
> to choose between the two codepaths.

Done.

https://codereview.chromium.org/1164073006/diff/260001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1164073006/diff/260001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:298: *handled_by_plugin =
false;
On 2015/07/17 19:47:51, mmenke wrote:
> BUG:  This should go above the #if.

Done.

https://codereview.chromium.org/1164073006/diff/260001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:302:
ResourceRequestInfoImpl* info = GetRequestInfo();
On 2015/07/17 19:47:51, mmenke wrote:
> optional nit:  Suggest reordering these to match the order they're passed in
to
> GetPluginInfo.

Done.

[mmenke, 2015-07-20 15:20:48]

LGTM!

https://codereview.chromium.org/1164073006/diff/300001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1164073006/diff/300001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:297: *handled_by_plugin =
false;
nit:  +2 indent

[mmenke, 2015-07-20 15:21:06]

And thanks for your patience on this one.  :)

[raymes, 2015-07-21 00:15:44]

raymes@chromium.org changed reviewers:
+ jam@chromium.org

[raymes, 2015-07-21 00:15:45]

Thanks for your patience and help too :)

+jam for OWNERS

  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

https://codereview.chromium.org/1164073006/diff/300001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler.cc (right):

https://codereview.chromium.org/1164073006/diff/300001/content/browser/loader...
content/browser/loader/mime_type_resource_handler.cc:297: *handled_by_plugin =
false;
On 2015/07/20 15:20:48, mmenke wrote:
> nit:  +2 indent

Oops, done.

[raymes, 2015-07-21 00:16:10]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-21 00:17:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164073006/340001

[commit-bot: I haz the power, 2015-07-21 01:13:01]

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

[commit-bot: I haz the power, 2015-07-21 01:13:02]

Dry run: This issue passed the CQ dry run.

[jam, 2015-07-21 16:23:16]

lgtm

[raymes, 2015-07-22 00:46:55]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-07-22 00:46:56]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1164073006/#ps340001
(title: " ")

[commit-bot: I haz the power, 2015-07-22 00:47:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164073006/340001

[commit-bot: I haz the power, 2015-07-22 00:50:54]

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

[commit-bot: I haz the power, 2015-07-22 00:50:55]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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...)

[raymes, 2015-07-22 01:01:09]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-07-22 01:01:09]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
jam@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1164073006/#ps360001
(title: " ")

[commit-bot: I haz the power, 2015-07-22 01:02:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1164073006/360001

[commit-bot: I haz the power, 2015-07-22 02:03:55]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-07-22 02:04:43]

Patchset 19 (id:??) landed as
https://crrev.com/9dcebac2889710b6e92b751d0434d06bf240dba4
Cr-Commit-Position: refs/heads/master@{#339816}