Browser Plugin: New Implementation (Renderer Side)

Browser Plugin: New Implementation (Renderer Side)


BUG=140306
Tested: Added new browser tests!

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150863

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

[Fady Samuel, 2012-08-01 16:59:11]

Hi Nate,

This is some code for a new renderer side implementation of the browser plugin
[the C++ under the hood of the browser tag]. I'm least confident about my JS
Binding code. I was hoping you could please take a look at the following here
and give me some feedback:

1. browser_plugin_bindings.cc/h
2. BrowserPlugin::AddEventListener/RemoveEventListener
3. browser_plugin_browsertest (some of them involve bindings)

Thanks,

Fady

[Nate Chapin, 2012-08-01 17:36:17]

I think the bindings piece look ok to me (comment aside), with two caveats:

1. I normally work in WebCore, so I can't tell if there are layering violations.
2. NP bindings tend to be easy to get wrong and hard to review (especially with
things like memory leaks), so my level of confidence in their correctness is
pretty low.

https://chromiumcodereview.appspot.com/10830072/diff/13033/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/13033/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:232: return true;
I believe you need to call Dispose() on the v8::Persistent here or it will leak.

[Fady Samuel, 2012-08-01 17:44:22]

On 2012/08/01 17:36:17, Nate Chapin wrote:
> I think the bindings piece look ok to me (comment aside), with two caveats:
> 
> 1. I normally work in WebCore, so I can't tell if there are layering
violations.
> 2. NP bindings tend to be easy to get wrong and hard to review (especially
with
> things like memory leaks), so my level of confidence in their correctness is
> pretty low.
> 
>
https://chromiumcodereview.appspot.com/10830072/diff/13033/content/renderer/b...
> File content/renderer/browser_plugin/browser_plugin.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10830072/diff/13033/content/renderer/b...
> content/renderer/browser_plugin/browser_plugin.cc:232: return true;
> I believe you need to call Dispose() on the v8::Persistent here or it will
leak.

Maybe this is a dumb question but I currently have a
BrowserPluginBindingsEnumerate callback that's not implemented. When would this
callback get called, and what is the expected output?

Another more general question: Is there a set of guidelines or common errors I
should watch out for? Thanks.

[Nate Chapin, 2012-08-01 18:00:45]

On 2012/08/01 17:44:22, Fady Samuel wrote:
> On 2012/08/01 17:36:17, Nate Chapin wrote:
> > I think the bindings piece look ok to me (comment aside), with two caveats:
> > 
> > 1. I normally work in WebCore, so I can't tell if there are layering
> violations.
> > 2. NP bindings tend to be easy to get wrong and hard to review (especially
> with
> > things like memory leaks), so my level of confidence in their correctness is
> > pretty low.
> > 
> >
>
https://chromiumcodereview.appspot.com/10830072/diff/13033/content/renderer/b...
> > File content/renderer/browser_plugin/browser_plugin.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10830072/diff/13033/content/renderer/b...
> > content/renderer/browser_plugin/browser_plugin.cc:232: return true;
> > I believe you need to call Dispose() on the v8::Persistent here or it will
> leak.
> 
> Maybe this is a dumb question but I currently have a
> BrowserPluginBindingsEnumerate callback that's not implemented. When would
this
> callback get called, and what is the expected output?

I'm assuming that should map to NPN_Enumerate:
https://developer.mozilla.org/en/NPN_Enumerate

> 
> Another more general question: Is there a set of guidelines or common errors I
> should watch out for? Thanks.

No, but based on bugs I've fixed, the 2 I can think of are:

NPObjects are refcounted and start at 1. General refcounting rules about
matching retain and release calls, avoiding cycles, blah blah blah.
Unfortunately, we don't have smart pointers for NPObjects that I know of, so
it's all manual.

If you care about mapping NPObjects to other objects and not, e.g., making
multiple NPObjects that map to a single v8::Object, you may find cases where it
doesn't work. We've got most of the kinks out, but the code is poorly tested and
poorly understood, so it could happen.

[Fady Samuel, 2012-08-01 20:32:07]

Hi Charlie,

I'm beginning to upstream the new browser plugin implementation. This is the
renderer side implementation with lots of content_browsertests.

I'm still working through the JS bindings stuff with a fine tooth comb, but I
was hoping you'd have an overall look.

There's pretty good code coverage through the browsertests, but if you see any
gaping holes here please let me know so I can close them.

This cl is designed to be reviewable independently of the browser side stuff.

https://chromiumcodereview.appspot.com/10830072/diff/13033/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/13033/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:232: return true;
On 2012/08/01 17:36:17, Nate Chapin wrote:
> I believe you need to call Dispose() on the v8::Persistent here or it will
leak.

Done.

[Charlie Reis, 2012-08-01 23:20:51]

Cool.  I guess my main high level question is whether there are long-term
disadvantages to using NPAPI infrastructure for this.  I'm not familiar with
plugins, though, so maybe it's a non-issue.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
content/common/browser_plugin_messages.h:28: // These messages are from the
embedder to the browser process
nit: End with period.  Same below.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
content/common/browser_plugin_messages.h:30:
IPC_STRUCT_BEGIN(BrowserPluginHostMsg_PostMessage_Params)
Why does this need to be duplicated from ViewMsg_PostMessage_Params?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
content/common/browser_plugin_messages.h:72: // create a browser plugin.  The
browser will a guest renderer process
nit: The browser will create

The comment makes this sound like a request to create, but the message name
sounds like a request to navigate, which could create a guest or work within an
existing one.  Is this intended to serve both roles?  Please update the comment
if so.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
content/common/browser_plugin_messages.h:101: // the BrowserPluginMsg_UpdateURL
message.
Why does the embedder need to know?  Just to expose it as an event via an API?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/content_test...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/content_test...
content/content_tests.gypi:590:
'renderer/browser_plugin/tests/mock_browser_plugin.h',
Most tests don't go in their own tests/ subdirectory, which makes it easy to
spot test classes amongst implementation classes.  I don't think we really need
the subdirectory here.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:41: const char kNavigation[] =
"navigation";
Please make it clear what this is used for.  And why is only one of these two
constants in a namespace?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:73: // should work fine in
practice.
Could you have MockBrowserPlugin call Cleanup instead?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:94: return std::string();
I don't see why we wouldn't return src_ after a crash.  We might just want to
reload the previous page, and it would be useful to know what it was.  (For
comparison, we keep the crashed URL in the Omnibox when a top-level page
crashes.)

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:102: return;
This prevents using SetSrcAttribute for doing a reload.  Is that intentional?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:115: void
BrowserPlugin::ParseSrcAttribute(
There's no way to tell if this succeeded or failed.  Should it return a bool
indicating whether the src attribute was found?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:194: src_.clear();
Again, I don't think we should clear this.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:215:
callFunctionEvenIfScriptDisabled(*it,
Is this common practice?  I'm surprised we would call an event listener if
script were disabled.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:297:
webkit::PaintSadPlugin(canvas, plugin_rect_, *sad_guest_);
While this is technically implemented as a plugin, I wonder if the sad plugin
image is the right message to send to the user (since it's Chrome crashing, not
a plugin that can be disabled).  Maybe we want something more like Aw Snap, or
even a new one with a sad page icon?

Just something to think about and possibly have a bug or TODO for.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:24: class CONTENT_EXPORT
BrowserPlugin : public WebKit::WebPlugin {
I'm not familiar with plugin classes, but I notice the use of NPObject here. 
Does that mean this new approach is an NPAPI plugin?  Will that be a problem if
NPAPI is later deprecated?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:45: void UpdateURL(const GURL&
url);
I think I'm partly confused by this because it matches the name of
RenderViewImpl::UpdateURL, which is called from WebKit ("below") when we commit
a navigation.  This version of UpdateURL appears to be called from the browser
process ("above"), likely as part of the DidNavigate set of events.

Would it be clearer to call this DidNavigate?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:54: v8::Local<v8::Function>
function);
I'm surprised to see a V8 dependency here.  Maybe it's necessary, but is there a
way to generalize this to work with other script engines (JSC, Dart, etc)?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:55: // Add a custom event
listener from this BrowserPlugin instance.
s/Add/Remove/

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:58: // WebKit::WebPlugin
implementation.
Please use blank lines between unrelated declarations (e.g., at least above this
line, but I would put space between unrelated groups above as well).

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:99: BrowserPlugin(
Please clarify what these parameters refer to.  Embedder?  Guest?  What kind of
id? etc.

Ditto for member names below.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin_backing_store.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin_backing_store.h:23: class
BrowserPluginBackingStore {
Please add a class level comment (for those of us less familiar with rendering
details).

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin_manager.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin_manager.h:27: // by the render
thread.
Can we enforce this, using something like ThreadChecker::CalledOnValidThread in
the methods?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin_manager.h:37: virtual
BrowserPlugin* CreateBrowserPlugin(
It's not clear from here that the created BrowserPlugin will take care of
calling AddBrowserPlugin and RemoveBrowserPlugin.  I was wondering if RenderView
was expected to manage that.  Perhaps you can clarify in comments?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin_manager_impl.h:30: //
RenderProcessObserver override. Call on render thread.
As above, we should enforce this.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/tests/browser_plugin_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/tests/browser_plugin_browsertest.cc:132:
BrowserPluginHostMsg_NavigateFromEmbedder::ID);
Just trying to make sure I follow this correctly-- we're checking the message
being sent to the browser process, which would normally initiate the actual
navigation, right?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/tests/browser_plugin_browsertest.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/tests/browser_plugin_browsertest.h:33: );
No newline here.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/tests/mock_browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/tests/mock_browser_plugin.h:24: // Allow pocking
at a few private members.
poking

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/tests/mock_browser_plugin_manager.cc
(right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/tests/mock_browser_plugin_manager.cc:39: // This
is a copy-and-paste from MockRenderThread::Send.
I don't suppose there's a way to share this code?  Copy-paste tends to lead to
bugs when one copy gets updated and the other doesn't.

[Charlie Reis, 2012-08-01 23:22:20]

Also, please try to be in the habit of filling in the bug number for CL
descriptions.  It helps a lot when we go back to look for CLs later (via the
bug), or if anything needs to be reverted.

[Fady Samuel, 2012-08-02 18:32:46]

Hi Charlie,

I've addressed most of your points. There are a couple of points that I haven't
quite addressed yet (reusing some Mock code, disabling a function call when
script is disabled, and reusing ViewMsg_PostMessage_Params) but I'm looking into
those now. Those aside (I am attempting to address those now), are there any
other concerns?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
content/common/browser_plugin_messages.h:28: // These messages are from the
embedder to the browser process
On 2012/08/01 23:20:51, creis wrote:
> nit: End with period.  Same below.

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
content/common/browser_plugin_messages.h:30:
IPC_STRUCT_BEGIN(BrowserPluginHostMsg_PostMessage_Params)
On 2012/08/01 23:20:51, creis wrote:
> Why does this need to be duplicated from ViewMsg_PostMessage_Params?

Short answer: Badness happens when I try to access anything in view_messages.h
from here: it doesn't compile. Maybe I didn't try hard enough to make it work,
though.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
content/common/browser_plugin_messages.h:72: // create a browser plugin.  The
browser will a guest renderer process
On 2012/08/01 23:20:51, creis wrote:
> nit: The browser will create
> 
> The comment makes this sound like a request to create, but the message name
> sounds like a request to navigate, which could create a guest or work within
an
> existing one.  Is this intended to serve both roles?  Please update the
comment
> if so.

Agreed, this is very confusing and this is not a good comment. I renamed this
BrowserPluginHostMsg_NavigateOrCreateGuest and updated the comment
appropriately. Thanks.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
content/common/browser_plugin_messages.h:101: // the BrowserPluginMsg_UpdateURL
message.
On 2012/08/01 23:20:51, creis wrote:
> Why does the embedder need to know?  Just to expose it as an event via an API?

Mihai wrote a small browser demo with the browser tag, but the URL bar did not
update on guest-initiated navigation. I added this to fill that obvious gap in
functionality.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/content_test...
File content/content_tests.gypi (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/content_test...
content/content_tests.gypi:590:
'renderer/browser_plugin/tests/mock_browser_plugin.h',
On 2012/08/01 23:20:51, creis wrote:
> Most tests don't go in their own tests/ subdirectory, which makes it easy to
> spot test classes amongst implementation classes.  I don't think we really
need
> the subdirectory here.

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:73: // should work fine in
practice.
No longer doing a virtual method call after iterating over a related CL with
jam@. :-)

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:94: return std::string();
On 2012/08/01 23:20:51, creis wrote:
> I don't see why we wouldn't return src_ after a crash.  We might just want to
> reload the previous page, and it would be useful to know what it was.  (For
> comparison, we keep the crashed URL in the Omnibox when a top-level page
> crashes.)

Removed.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:102: return;
On 2012/08/01 23:20:51, creis wrote:
> This prevents using SetSrcAttribute for doing a reload.  Is that intentional?

Actually, it's the exact opposite. If guest_crashed_ then we allow reloading :-)

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:115: void
BrowserPlugin::ParseSrcAttribute(
On 2012/08/01 23:20:51, creis wrote:
> There's no way to tell if this succeeded or failed.  Should it return a bool
> indicating whether the src attribute was found?

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:194: src_.clear();
On 2012/08/01 23:20:51, creis wrote:
> Again, I don't think we should clear this.

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:215:
callFunctionEvenIfScriptDisabled(*it,
On 2012/08/01 23:20:51, creis wrote:
> Is this common practice?  I'm surprised we would call an event listener if
> script were disabled.

That's what I've seen done elsewhere, but there might be a better way to do
this. japhet@ would probably know. I will ask him.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:297:
webkit::PaintSadPlugin(canvas, plugin_rect_, *sad_guest_);
On 2012/08/01 23:20:51, creis wrote:
> While this is technically implemented as a plugin, I wonder if the sad plugin
> image is the right message to send to the user (since it's Chrome crashing,
not
> a plugin that can be disabled).  Maybe we want something more like Aw Snap, or
> even a new one with a sad page icon?
> 
> Just something to think about and possibly have a bug or TODO for.

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:24: class CONTENT_EXPORT
BrowserPlugin : public WebKit::WebPlugin {
On 2012/08/01 23:20:51, creis wrote:
> I'm not familiar with plugin classes, but I notice the use of NPObject here. 
> Does that mean this new approach is an NPAPI plugin?  Will that be a problem
if
> NPAPI is later deprecated?

Pepper does the same thing: see webkit/plugins/ppapi/message_channel.cc

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:45: void UpdateURL(const GURL&
url);
On 2012/08/01 23:20:51, creis wrote:
> I think I'm partly confused by this because it matches the name of
> RenderViewImpl::UpdateURL, which is called from WebKit ("below") when we
commit
> a navigation.  This version of UpdateURL appears to be called from the browser
> process ("above"), likely as part of the DidNavigate set of events.
> 
> Would it be clearer to call this DidNavigate?

Yes, that would be far less confusing. Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:54: v8::Local<v8::Function>
function);
On 2012/08/01 23:20:51, creis wrote:
> I'm surprised to see a V8 dependency here.  Maybe it's necessary, but is there
a
> way to generalize this to work with other script engines (JSC, Dart, etc)?

I honestly don't know :P Pepper does much of the same thing:
webkit/plugins/ppapi/message_channel.cc

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:55: // Add a custom event
listener from this BrowserPlugin instance.
On 2012/08/01 23:20:51, creis wrote:
> s/Add/Remove/

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:58: // WebKit::WebPlugin
implementation.
On 2012/08/01 23:20:51, creis wrote:
> Please use blank lines between unrelated declarations (e.g., at least above
this
> line, but I would put space between unrelated groups above as well).

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:99: BrowserPlugin(
On 2012/08/01 23:20:51, creis wrote:
> Please clarify what these parameters refer to.  Embedder?  Guest?  What kind
of
> id? etc.
> 
> Ditto for member names below.

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin_backing_store.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin_backing_store.h:23: class
BrowserPluginBackingStore {
On 2012/08/01 23:20:51, creis wrote:
> Please add a class level comment (for those of us less familiar with rendering
> details).

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin_backing_store.h:23: class
BrowserPluginBackingStore {
On 2012/08/01 23:20:51, creis wrote:
> Please add a class level comment (for those of us less familiar with rendering
> details).

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin_manager.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin_manager.h:27: // by the render
thread.
On 2012/08/01 23:20:51, creis wrote:
> Can we enforce this, using something like ThreadChecker::CalledOnValidThread
in
> the methods?

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin_manager.h:37: virtual
BrowserPlugin* CreateBrowserPlugin(
On 2012/08/01 23:20:51, creis wrote:
> It's not clear from here that the created BrowserPlugin will take care of
> calling AddBrowserPlugin and RemoveBrowserPlugin.  I was wondering if
RenderView
> was expected to manage that.  Perhaps you can clarify in comments?

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/tests/browser_plugin_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/tests/browser_plugin_browsertest.cc:132:
BrowserPluginHostMsg_NavigateFromEmbedder::ID);
On 2012/08/01 23:20:51, creis wrote:
> Just trying to make sure I follow this correctly-- we're checking the message
> being sent to the browser process, which would normally initiate the actual
> navigation, right?

Yup. We're checking to make sure that writing to and reading from the src
attribute does the right thing (send an IPC to the browser process on write, and
update src_, and reading reads the src_ string).

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/tests/browser_plugin_browsertest.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/tests/browser_plugin_browsertest.h:33: );
On 2012/08/01 23:20:51, creis wrote:
> No newline here.

Oops. Fixed, thanks!

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/tests/mock_browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/tests/mock_browser_plugin.h:24: // Allow pocking
at a few private members.
On 2012/08/01 23:20:51, creis wrote:
> poking

Done.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/tests/mock_browser_plugin_manager.cc
(right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/tests/mock_browser_plugin_manager.cc:39: // This
is a copy-and-paste from MockRenderThread::Send.
On 2012/08/01 23:20:51, creis wrote:
> I don't suppose there's a way to share this code?  Copy-paste tends to lead to
> bugs when one copy gets updated and the other doesn't.

I'm not going to address this immediately, but I'll speak to jam@ about
this...maybe a IPC::MockSenderImpl?

[Fady Samuel, 2012-08-02 21:59:45]

+mihaip

[Charlie Reis, 2012-08-03 18:06:02]

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
content/common/browser_plugin_messages.h:30:
IPC_STRUCT_BEGIN(BrowserPluginHostMsg_PostMessage_Params)
On 2012/08/02 18:32:47, Fady Samuel wrote:
> On 2012/08/01 23:20:51, creis wrote:
> > Why does this need to be duplicated from ViewMsg_PostMessage_Params?
> 
> Short answer: Badness happens when I try to access anything in view_messages.h
> from here: it doesn't compile. Maybe I didn't try hard enough to make it work,
> though.

Ok.  I'm guessing we'll need to tweak the parameters on this message anyway.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/common/brows...
content/common/browser_plugin_messages.h:72: // create a browser plugin.  The
browser will a guest renderer process
On 2012/08/02 18:32:47, Fady Samuel wrote:
> On 2012/08/01 23:20:51, creis wrote:
> > nit: The browser will create
> > 
> > The comment makes this sound like a request to create, but the message name
> > sounds like a request to navigate, which could create a guest or work within
> an
> > existing one.  Is this intended to serve both roles?  Please update the
> comment
> > if so.
> 
> Agreed, this is very confusing and this is not a good comment. I renamed this
> BrowserPluginHostMsg_NavigateOrCreateGuest and updated the comment
> appropriately. Thanks.

The comment is much better, thanks.  I'd recommend
BrowserPluginHostMsg_NavigateGuest as the message name, since it currently
sounds like it will either navigate or create (perhaps based on a parameter). 
The comment makes it clear that the guest will be created if necessary.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.cc:102: return;
On 2012/08/02 18:32:47, Fady Samuel wrote:
> On 2012/08/01 23:20:51, creis wrote:
> > This prevents using SetSrcAttribute for doing a reload.  Is that
intentional?
> 
> Actually, it's the exact opposite. If guest_crashed_ then we allow reloading
:-)

Yes, but why would you want to prevent reload if the guest hasn't crashed?

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:24: class CONTENT_EXPORT
BrowserPlugin : public WebKit::WebPlugin {
On 2012/08/02 18:32:47, Fady Samuel wrote:
> On 2012/08/01 23:20:51, creis wrote:
> > I'm not familiar with plugin classes, but I notice the use of NPObject here.

> > Does that mean this new approach is an NPAPI plugin?  Will that be a problem
> if
> > NPAPI is later deprecated?
> 
> Pepper does the same thing: see webkit/plugins/ppapi/message_channel.cc

Agreed.  Talked with Darin and this is just how plugins work, to try to abstract
a bit of V8 away.

https://chromiumcodereview.appspot.com/10830072/diff/102/content/renderer/bro...
content/renderer/browser_plugin/browser_plugin.h:54: v8::Local<v8::Function>
function);
On 2012/08/02 18:32:47, Fady Samuel wrote:
> On 2012/08/01 23:20:51, creis wrote:
> > I'm surprised to see a V8 dependency here.  Maybe it's necessary, but is
there
> a
> > way to generalize this to work with other script engines (JSC, Dart, etc)?
> 
> I honestly don't know :P Pepper does much of the same thing:
> webkit/plugins/ppapi/message_channel.cc

Ok, don't worry about it.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/common/bro...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/common/bro...
content/common/browser_plugin_messages.h:36: // frame in the destination
process.
This comment seems stale, since you appear to be implementing postMessage in a
different way.  For example, you're passing in the instance_id_, rather than the
routing_id of the embedder's RenderView.

How much of postMessage are you trying to support in this CL?  I don't see how
you're able to send a reply from the guest to the embedder at the moment, or
even where the guest receives the message.

Since there doesn't appear to be tests for this part either, it feels like it
might be incomplete.  Perhaps the postMessage stuff should be moved to a future
CL?

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:45: 
Remove one of these blank lines.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:182: src_ = url.spec();
We're losing some information by storing a GURL in a string, but maybe that's ok
here, since that's all the embedder app is going to see anyway.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:193: // TODO(fsamuel): Make
this respect content settings.
Guess we don't need this TODO after the discussion (since platform apps don't
react to content settings from the browser).

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:215: routing_id_, params));
Once the browser receives and routes this, where does the guest renderer receive
it?  How does it reply?

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:37: 
This blank line should be above the comment.  :)

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:107: // to it on creation.
Much better.  Whose routing_id is passed in, though?  Sounds like |render_view|
is the embedder.  Is |routing_id| just render_view->routing_id_, or is it the
guest's routing_id?

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:139: if
(IdentifierIsPostMessage(name) && (arg_count == 2)) {
Is this a typical pattern for bindings classes, or do we usually break it into
helper functions?  This Invoke function could grow quite large if we add more
handled identifiers.  (Not a big problem right now, I suppose.)

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:166:
bindings->instance()->PostMessage(message, target_origin);
Just to confirm my understanding, this allows JS code in the embedder to post a
message to the guest, by routing it through the BrowserPlugin class and then via
the browser process, right?

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_manager.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_manager.cc:34:
DCHECK(CalledOnValidThread());
This sounds like it would return true on any valid thread.  Don't we want
something specific to the render thread?  Or does this do the right thing?

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_manager_impl.h:30: //
RenderProcessObserver override. Call on render thread.
I think you missed this comment.  Can we enforce this?

[Fady Samuel, 2012-08-03 19:34:43]

Charlie,

I've addressed all your comments.

Mihai: I added you as a reviewer, in case you're interested in taking a look at
this code. If not, I'll add jam@ as an OWNER reviewer once Charlie is happy with
the state of the code. Thanks.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/common/bro...
File content/common/browser_plugin_messages.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/common/bro...
content/common/browser_plugin_messages.h:36: // frame in the destination
process.
On 2012/08/03 18:06:02, creis wrote:
> This comment seems stale, since you appear to be implementing postMessage in a
> different way.  For example, you're passing in the instance_id_, rather than
the
> routing_id of the embedder's RenderView.
> 
> How much of postMessage are you trying to support in this CL?  I don't see how
> you're able to send a reply from the guest to the embedder at the moment, or
> even where the guest receives the message.
> 
> Since there doesn't appear to be tests for this part either, it feels like it
> might be incomplete.  Perhaps the postMessage stuff should be moved to a
future
> CL?

Yup, postMessage really requires both renderer and browser side code to land.
For reference the browser side code is here (not quite ready for formal review):
http://codereview.chromium.org/10560022/.

I will move postMessage out of here.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:45: 
On 2012/08/03 18:06:02, creis wrote:
> Remove one of these blank lines.

Done.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:182: src_ = url.spec();
On 2012/08/03 18:06:02, creis wrote:
> We're losing some information by storing a GURL in a string, but maybe that's
ok
> here, since that's all the embedder app is going to see anyway.

Indeed. The embedder doesn't know about GURLs, only URL strings.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:193: // TODO(fsamuel): Make
this respect content settings.
On 2012/08/03 18:06:02, creis wrote:
> Guess we don't need this TODO after the discussion (since platform apps don't
> react to content settings from the browser).

Done.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.cc:215: routing_id_, params));
On 2012/08/03 18:06:02, creis wrote:
> Once the browser receives and routes this, where does the guest renderer
receive
> it?  How does it reply?

I'll move postMessage to a separate change so that we can study it together more
carefully. :-)

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:37: 
On 2012/08/03 18:06:02, creis wrote:
> This blank line should be above the comment.  :)

This is fixed already :-)

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin.h:107: // to it on creation.
On 2012/08/03 18:06:02, creis wrote:
> Much better.  Whose routing_id is passed in, though?  Sounds like
|render_view|
> is the embedder.  Is |routing_id| just render_view->routing_id_, or is it the
> guest's routing_id?

This routing_id is completely unnecessary. It originally existed for testing,
but I rewrote testing and made this extra field irrelevant. Removed.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_bindings.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:139: if
(IdentifierIsPostMessage(name) && (arg_count == 2)) {
On 2012/08/03 18:06:02, creis wrote:
> Is this a typical pattern for bindings classes, or do we usually break it into
> helper functions?  This Invoke function could grow quite large if we add more
> handled identifiers.  (Not a big problem right now, I suppose.)

I agree, this will grow long fairly quickly as the API gets richer. I promise to
clean it up...in a separate patch, when we start adding an error API :-)

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_bindings.cc:166:
bindings->instance()->PostMessage(message, target_origin);
On 2012/08/03 18:06:02, creis wrote:
> Just to confirm my understanding, this allows JS code in the embedder to post
a
> message to the guest, by routing it through the BrowserPlugin class and then
via
> the browser process, right?

That is correct.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_manager.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_manager.cc:34:
DCHECK(CalledOnValidThread());
On 2012/08/03 18:06:02, creis wrote:
> This sounds like it would return true on any valid thread.  Don't we want
> something specific to the render thread?  Or does this do the right thing?

Not quite: See base/threading/thread_checker_impl.cc

This will fail if we access this method on a different thread from where
BrowserPluginManager was created. This is sufficient to ensure we don't
accidentally do anything weird/stupid, I think.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_manager_impl.h:30: //
RenderProcessObserver override. Call on render thread.
On 2012/08/03 18:06:02, creis wrote:
> I think you missed this comment.  Can we enforce this?

Done.

[Charlie Reis, 2012-08-03 20:01:00]

Thanks.  LGTM with nits.  You'll need John's approval for the new MIME type in
content/public.

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
File content/renderer/browser_plugin/browser_plugin_manager.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/14036/content/renderer/b...
content/renderer/browser_plugin/browser_plugin_manager.cc:34:
DCHECK(CalledOnValidThread());
On 2012/08/03 19:34:43, Fady Samuel wrote:
> On 2012/08/03 18:06:02, creis wrote:
> > This sounds like it would return true on any valid thread.  Don't we want
> > something specific to the render thread?  Or does this do the right thing?
> 
> Not quite: See base/threading/thread_checker_impl.cc
> 
> This will fail if we access this method on a different thread from where
> BrowserPluginManager was created. This is sufficient to ensure we don't
> accidentally do anything weird/stupid, I think.

Great, thanks for confirming.

https://chromiumcodereview.appspot.com/10830072/diff/9008/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/9008/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:276: if (!sad_guest_)  //
Lazily initialize bitmap.
nit: Needs braces, since the code below doesn't fit on one line.

https://chromiumcodereview.appspot.com/10830072/diff/9008/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:316: return;
nit: Needs braces, since the conditional doesn't fit on one line.

https://chromiumcodereview.appspot.com/10830072/diff/9008/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/9008/content/renderer/re...
content/renderer/render_view_impl.cc:2313: CreateBrowserPlugin(this, frame,
params);
nit: Looks like the indent is wrong on these two lines.

[Fady Samuel, 2012-08-03 22:04:12]

-mihaip as he asked me to move on to OWNER review.
+jam for OWNER review

https://chromiumcodereview.appspot.com/10830072/diff/9008/content/renderer/br...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/9008/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:276: if (!sad_guest_)  //
Lazily initialize bitmap.
On 2012/08/03 20:01:00, creis wrote:
> nit: Needs braces, since the code below doesn't fit on one line.

Done.

https://chromiumcodereview.appspot.com/10830072/diff/9008/content/renderer/br...
content/renderer/browser_plugin/browser_plugin.cc:316: return;
On 2012/08/03 20:01:00, creis wrote:
> nit: Needs braces, since the conditional doesn't fit on one line.

Done.

https://chromiumcodereview.appspot.com/10830072/diff/9008/content/renderer/re...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10830072/diff/9008/content/renderer/re...
content/renderer/render_view_impl.cc:2313: CreateBrowserPlugin(this, frame,
params);
On 2012/08/03 20:01:00, creis wrote:
> nit: Looks like the indent is wrong on these two lines.

Done.

[jam, 2012-08-06 20:26:43]

lgtm for the public bits. creis: in the future, minor changes like this are fine
for you to approve. i care more about new interfaces or significant changes to
existing content api interfaces :)

fsamuel: why is there a browser_plugin namespace? content seems enough

[Fady Samuel, 2012-08-06 23:59:17]

> fsamuel: why is there a browser_plugin namespace? content seems enough

Name collision with existing BrowserPlugin classes.

[Fady Samuel, 2012-08-07 19:32:04]

On 2012/08/06 23:59:17, Fady Samuel wrote:
> > fsamuel: why is there a browser_plugin namespace? content seems enough
> 
> Name collision with existing BrowserPlugin classes.

Moved out of browser_plugin namespace. Moved old code to content::old in a
different cl that just landed. Landing this now.

[commit-bot: I haz the power, 2012-08-07 19:32:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/7016

[commit-bot: I haz the power, 2012-08-07 19:32:26]

Presubmit check for 10830072-7016 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
  content/renderer/browser_plugin/mock_browser_plugin.cc \
  content/renderer/browser_plugin/mock_browser_plugin.h \
  content/renderer/browser_plugin/mock_browser_plugin_manager.h

Presubmit checks took 2.2s to calculate.

[commit-bot: I haz the power, 2012-08-07 19:49:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/16044

[commit-bot: I haz the power, 2012-08-07 19:57:25]

Try job failure for 10830072-16044 (retry) on mac_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-07 20:15:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/12040

[commit-bot: I haz the power, 2012-08-07 20:24:16]

Try job failure for 10830072-12040 (retry) on mac_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-07 20:38:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/12044

[commit-bot: I haz the power, 2012-08-07 20:45:46]

Try job failure for 10830072-12044 (retry) on mac_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-07 23:13:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/17113

[commit-bot: I haz the power, 2012-08-07 23:20:01]

Try job failure for 10830072-17113 (retry) on mac_rel for step "runhooks".
It's a second try, previously, step "runhooks" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-08 05:21:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/17113

[commit-bot: I haz the power, 2012-08-08 05:21:07]

Failed to apply patch for content/renderer/render_view_impl.cc:
While running patch -p1 --forward --force;
patching file content/renderer/render_view_impl.cc
Hunk #1 FAILED at 55.
1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/render_view_impl.cc.rej

[commit-bot: I haz the power, 2012-08-08 15:00:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/167

[commit-bot: I haz the power, 2012-08-08 15:39:08]

Try job failure for 10830072-167 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-08 15:59:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/10042

[commit-bot: I haz the power, 2012-08-08 16:39:21]

Try job failure for 10830072-10042 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-08-08 16:53:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/8027

[commit-bot: I haz the power, 2012-08-08 17:56:37]

Try job failure for 10830072-8027 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-08 18:04:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/9044

[commit-bot: I haz the power, 2012-08-08 19:12:52]

Try job failure for 10830072-9044 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-08 19:46:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/17126

[commit-bot: I haz the power, 2012-08-08 21:01:21]

Try job failure for 10830072-17126 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-08 21:05:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/17126

[commit-bot: I haz the power, 2012-08-08 22:27:21]

Try job failure for 10830072-17126 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-08 22:57:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/17126

[commit-bot: I haz the power, 2012-08-08 23:37:56]

Try job failure for 10830072-17126 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-08-08 23:47:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/17126

[commit-bot: I haz the power, 2012-08-09 18:13:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/1067

[commit-bot: I haz the power, 2012-08-09 19:24:40]

Change committed as 150863

[commit-bot: I haz the power, 2012-08-09 21:38:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10830072/5074