Fix some DCHECKS when using BrowserPlugin for extension mime type handlers

Fix some DCHECKS when using BrowserPlugin for extension mime type handlers

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

[Zachary Kuznia, 2014-06-19 01:34:31]

yoz: Could you take a look at plugin_manager.{cc,h}?
mmenke: Could you take a look at buffered_resource_handler.cc?

Thanks,
-Zach

[Yoyo Zhou, 2014-06-19 01:50:39]

I have some comments - it looks like the behavior could be different, but I'm
not sure why that is intended.

https://codereview.chromium.org/340953006/diff/1/chrome/browser/extensions/pl...
File chrome/browser/extensions/plugin_manager.cc (left):

https://codereview.chromium.org/340953006/diff/1/chrome/browser/extensions/pl...
chrome/browser/extensions/plugin_manager.cc:125: plugins_or_nacl_changed = true;
Why is this no longer?

https://codereview.chromium.org/340953006/diff/1/chrome/browser/extensions/pl...
File chrome/browser/extensions/plugin_manager.cc (right):

https://codereview.chromium.org/340953006/diff/1/chrome/browser/extensions/pl...
chrome/browser/extensions/plugin_manager.cc:85:
mime_type_handler_extension_ids_.insert(handler->extension_id());
It doesn't look like we ever erase IDs from this.

[mmenke, 2014-06-19 15:08:42]

https://codereview.chromium.org/340953006/diff/1/content/browser/loader/buffe...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/340953006/diff/1/content/browser/loader/buffe...
content/browser/loader/buffered_resource_handler.cc:389:
next_handler_->OnWillRead(&buf, &size, -1);
Why this change?

[Zachary Kuznia, 2014-06-19 18:02:24]

https://codereview.chromium.org/340953006/diff/1/chrome/browser/extensions/pl...
File chrome/browser/extensions/plugin_manager.cc (left):

https://codereview.chromium.org/340953006/diff/1/chrome/browser/extensions/pl...
chrome/browser/extensions/plugin_manager.cc:125: plugins_or_nacl_changed = true;
On 2014/06/19 01:50:39, Yoyo Zhou wrote:
> Why is this no longer?

It looks like I was missing another call as well.  Fixed.

https://codereview.chromium.org/340953006/diff/1/chrome/browser/extensions/pl...
File chrome/browser/extensions/plugin_manager.cc (right):

https://codereview.chromium.org/340953006/diff/1/chrome/browser/extensions/pl...
chrome/browser/extensions/plugin_manager.cc:85:
mime_type_handler_extension_ids_.insert(handler->extension_id());
On 2014/06/19 01:50:39, Yoyo Zhou wrote:
> It doesn't look like we ever erase IDs from this.

This set is no longer needed.  Removed.

https://codereview.chromium.org/340953006/diff/1/content/browser/loader/buffe...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/340953006/diff/1/content/browser/loader/buffe...
content/browser/loader/buffered_resource_handler.cc:389:
next_handler_->OnWillRead(&buf, &size, -1);
On 2014/06/19 15:08:42, mmenke wrote:
> Why this change?

This always ends up forwarding to AsyncResourceHandler::OnWillRead(), which does
a DCHECK_EQ(-1, min_size);

[mmenke, 2014-06-19 19:36:26]

LGTM

https://codereview.chromium.org/340953006/diff/1/content/browser/loader/buffe...
File content/browser/loader/buffered_resource_handler.cc (right):

https://codereview.chromium.org/340953006/diff/1/content/browser/loader/buffe...
content/browser/loader/buffered_resource_handler.cc:389:
next_handler_->OnWillRead(&buf, &size, -1);
On 2014/06/19 18:02:24, Zachary Kuznia wrote:
> On 2014/06/19 15:08:42, mmenke wrote:
> > Why this change?
> 
> This always ends up forwarding to AsyncResourceHandler::OnWillRead(), which
does
> a DCHECK_EQ(-1, min_size);

Ahh...right.  Ugly, but we're really bad about how that works, as-is.  Since we
have a CHECK just below, this is fine.  We really should figure out a better way
to do all this, but that ball's not in your court.

[Yoyo Zhou, 2014-06-19 21:50:50]

LGTM

[Zachary Kuznia, 2014-06-20 00:34:37]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-06-20 00:38:55]

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

[commit-bot: I haz the power, 2014-06-20 08:20:25]

Change committed as 278647