Conversion of content_shell target to use WebLocalFrame.

Conversion of content_shell target to use WebLocalFrame.

Fairly straightforward, as the locations where I had to add conversions
should only ever expect to be called in the context of a local frame.

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

[dcheng, 2014-04-02 22:08:15]

This patch is just the bits needed to build content_shell without errors when
WebLocalFrame subclasses WebFrame.

I've added a few type coercions of WebFrame to WebLocalFrame (the
toWebLocalFrame() calls). I believe these locations should only ever be called
in the renderer that's hosting that frame. Should I go ahead and file a bug to
track these? Or since there shouldn't be a lot of calls overall, would it be
easier to let teams search and audit these calls themselves when they're
updating their code? That way it won't get out of date like a bug.

[Charlie Reis, 2014-04-02 23:34:14]

Hmm, I guess I need a little more context to be able to review this.  A few
questions below and then I'll be able to review the rest.

On 2014/04/02 22:08:15, dcheng wrote:
> This patch is just the bits needed to build content_shell without errors when
> WebLocalFrame subclasses WebFrame.
> 
> I've added a few type coercions of WebFrame to WebLocalFrame (the
> toWebLocalFrame() calls). I believe these locations should only ever be called
> in the renderer that's hosting that frame. Should I go ahead and file a bug to
> track these? Or since there shouldn't be a lot of calls overall, would it be
> easier to let teams search and audit these calls themselves when they're
> updating their code? That way it won't get out of date like a bug.

I'd like to get very visible feedback if these calls are put in the wrong place.
 Would it make sense to CHECK that toWebLocalFrame is non-null in those call
sites?  Or maybe have an ASSERT in WebRemoteFrame::toWebLocalFrame()?  That way
we can get crash reports if someone writes code making an unsafe assumption,
rather than just hoping they stumble across a null pointer dereference.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_frame.h:13: #include
"third_party/WebKit/public/web/WebLocalFrame.h"
Why is this needed for now?  And what will change to allow us to remove it
later?

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_frame.h:39: static RenderFrame*
FromWebFrame(blink::WebFrame* web_frame);
I'm not sure why some uses of WebFrame are being replaced but not others.  For
example, this method wouldn't make sense to call on a WebRemoteFrame.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_view_observer.h:71: virtual void
FrameDetached(blink::WebFrame* frame) {}
Again, why FrameCreated but not FrameDetached, etc?

[dcheng, 2014-04-02 23:47:07]

Hmm. Right now, checking if toWebLocalFrame() is null happens to be the way to
determine if the frame is local... but there are no actual dependencies on this
behavior yet. We could probably just add an ASSERT() in blink and then add a
isWebLocalFrame() if someone does need to check.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_frame.h:13: #include
"third_party/WebKit/public/web/WebLocalFrame.h"
On 2014/04/02 23:34:15, Charlie Reis wrote:
> Why is this needed for now?  And what will change to allow us to remove it
> later?

WebLocalFrame is currently a typedef, and you cannot forward declare typedefs.
However, I cannot make it a class either--the alternative was to temporarily
double up all the methods in public/web that are changing to take a
WebLocalFrame.

Once this conversion is complete, I will convert WebLocalFrame to be a subclass
so it can be forward declared in most places again.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_frame.h:39: static RenderFrame*
FromWebFrame(blink::WebFrame* web_frame);
On 2014/04/02 23:34:15, Charlie Reis wrote:
> I'm not sure why some uses of WebFrame are being replaced but not others.  For
> example, this method wouldn't make sense to call on a WebRemoteFrame.

Because this didn't fail to compile. It's true that this doesn't make sense to
call on remote frames; would you be OK if I change this in a separate patch? I'd
like to keep the patch size somewhat manageable.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_view_observer.h:71: virtual void
FrameDetached(blink::WebFrame* frame) {}
On 2014/04/02 23:34:15, Charlie Reis wrote:
> Again, why FrameCreated but not FrameDetached, etc?

This is somewhat subtle and just a starting point. It may be incorrect and
change over time. Here's my current thoughts:

1) Actually creating a child frame is only going to happen in the parent's
renderer. So the parent is always local. But the child may be local or remote.
It's not clear to me yet that mirroring the frame tree should go through the
same callback. Maybe it should, and then we'd need to change it at that point.
It made sense at the Blink layer; maybe it doesn't make sense in content.

2) Our story for frame detach isn't yet clear. I think we may still want to do
the right thing and order frame detaches correctly. In that case, we could use
frameDetached()/frameWillClose() to implement that forwarding. It makes sense at
the Blink layer... but maybe it doesn't make sense to expose that outside of
content.

I expect both of these to become clearer with time. If we do have to change the
signatures later, it will be a lot more straightforward than these patches.

[Charlie Reis, 2014-04-03 00:37:11]

Jochen: One question for you in WebFrameTestProxy.h below.

Daniel: Looks pretty good to me.  Just a few last questions below.

On 2014/04/02 23:47:07, dcheng wrote:
> Hmm. Right now, checking if toWebLocalFrame() is null happens to be the way to
> determine if the frame is local... but there are no actual dependencies on
this
> behavior yet. We could probably just add an ASSERT() in blink and then add a
> isWebLocalFrame() if someone does need to check.

Maybe that's overzealous on my part.  Since there aren't that many places we
need to use it here, maybe we can just put CHECKs at the call sites?  

(I'm concerned about places like the Mojo class, which seems unnecessarily
centered around a webview.  I wouldn't be surprised if it broke if we ever had
WebUI in a OOPIF, and I'd like to catch that.)

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_frame.h:13: #include
"third_party/WebKit/public/web/WebLocalFrame.h"
On 2014/04/02 23:47:08, dcheng wrote:
> On 2014/04/02 23:34:15, Charlie Reis wrote:
> > Why is this needed for now?  And what will change to allow us to remove it
> > later?
> 
> WebLocalFrame is currently a typedef, and you cannot forward declare typedefs.
> However, I cannot make it a class either--the alternative was to temporarily
> double up all the methods in public/web that are changing to take a
> WebLocalFrame.
> 
> Once this conversion is complete, I will convert WebLocalFrame to be a
subclass
> so it can be forward declared in most places again.

I see, thanks.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_frame.h:39: static RenderFrame*
FromWebFrame(blink::WebFrame* web_frame);
On 2014/04/02 23:47:08, dcheng wrote:
> On 2014/04/02 23:34:15, Charlie Reis wrote:
> > I'm not sure why some uses of WebFrame are being replaced but not others. 
For
> > example, this method wouldn't make sense to call on a WebRemoteFrame.
> 
> Because this didn't fail to compile.

Just to fill me in, what didn't compile?  I'd like to make sense of which things
need to get migrated.  Is it the set of places that call methods that won't
exist on WebRemoteFrame?

> It's true that this doesn't make sense to
> call on remote frames; would you be OK if I change this in a separate patch?
I'd
> like to keep the patch size somewhat manageable.

Yes, that's fine-- we definitely don't need to update them all at once.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_view_observer.h:71: virtual void
FrameDetached(blink::WebFrame* frame) {}
On 2014/04/02 23:47:08, dcheng wrote:
> On 2014/04/02 23:34:15, Charlie Reis wrote:
> > Again, why FrameCreated but not FrameDetached, etc?
> 
> This is somewhat subtle and just a starting point. It may be incorrect and
> change over time. Here's my current thoughts:
> 
> 1) Actually creating a child frame is only going to happen in the parent's
> renderer. So the parent is always local. But the child may be local or remote.
> It's not clear to me yet that mirroring the frame tree should go through the
> same callback. Maybe it should, and then we'd need to change it at that point.
> It made sense at the Blink layer; maybe it doesn't make sense in content.
> 
> 2) Our story for frame detach isn't yet clear. I think we may still want to do
> the right thing and order frame detaches correctly. In that case, we could use
> frameDetached()/frameWillClose() to implement that forwarding. It makes sense
at
> the Blink layer... but maybe it doesn't make sense to expose that outside of
> content.
> 
> I expect both of these to become clearer with time. If we do have to change
the
> signatures later, it will be a lot more straightforward than these patches.

Yeah, guess I stumbled onto a pretty subtle one.  Hopefully this
created/detached distinction can be documented in the WebFrameClient API?

https://codereview.chromium.org/222973003/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.h:94: // This is called right after creation
with the WebFrame for this RenderFrame.
nit: WebLocalFrame

https://codereview.chromium.org/222973003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.h:426: // Stores the WebFrame we are
associated with.
nit: WebLocalFrame.

https://codereview.chromium.org/222973003/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/WebFrameTestProxy.h (left):

https://codereview.chromium.org/222973003/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/WebFrameTestProxy.h:36: blink::WebPlugin*
createPlugin(blink::WebFrame* frame, const blink::WebPluginParams& params)
Jochen: Why are these files using Blink style inside content?  I'm guessing that
Daniel is using clang-format, which is wrapping all these parameter lines like
we would do in Chromium style, but I also noticed there's a 4 space indent here.
 What's the right thing to do?

[dcheng, 2014-04-03 20:39:57]

Since no one depends on using toWebLocalFrame() to check if a WebFrame is local
yet, I think we should just add an ASSERT in the implementation of
WebRemoteFrame::toWebLocalFrame(). I'd rather default to failing in this case
rather than hoping callers remember to check it.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_frame.h:39: static RenderFrame*
FromWebFrame(blink::WebFrame* web_frame);
On 2014/04/03 00:37:11, Charlie Reis wrote:
> On 2014/04/02 23:47:08, dcheng wrote:
> > On 2014/04/02 23:34:15, Charlie Reis wrote:
> > > I'm not sure why some uses of WebFrame are being replaced but not others. 
> For
> > > example, this method wouldn't make sense to call on a WebRemoteFrame.
> > 
> > Because this didn't fail to compile.
> 
> Just to fill me in, what didn't compile?  I'd like to make sense of which
things
> need to get migrated.  Is it the set of places that call methods that won't
> exist on WebRemoteFrame?
> 
> > It's true that this doesn't make sense to
> > call on remote frames; would you be OK if I change this in a separate patch?
> I'd
> > like to keep the patch size somewhat manageable.
> 
> Yes, that's fine-- we definitely don't need to update them all at once.

For figuring out what needs to change, I changed WebLocalFrame from a typedef to
subclassing WebFrame. This patch addresses all the compile failures from trying
to down cast a WebFrame to a WebLocalFrame without going through
toWebLocalFrame().

I expect that I'll have to a second round of cleanups in Chrome as I move things
from WebFrame to WebLocalFrame, but it should be a lot smaller than these
initial patches.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_view_observer.h:71: virtual void
FrameDetached(blink::WebFrame* frame) {}
On 2014/04/03 00:37:11, Charlie Reis wrote:
> On 2014/04/02 23:47:08, dcheng wrote:
> > On 2014/04/02 23:34:15, Charlie Reis wrote:
> > > Again, why FrameCreated but not FrameDetached, etc?
> > 
> > This is somewhat subtle and just a starting point. It may be incorrect and
> > change over time. Here's my current thoughts:
> > 
> > 1) Actually creating a child frame is only going to happen in the parent's
> > renderer. So the parent is always local. But the child may be local or
remote.
> > It's not clear to me yet that mirroring the frame tree should go through the
> > same callback. Maybe it should, and then we'd need to change it at that
point.
> > It made sense at the Blink layer; maybe it doesn't make sense in content.
> > 
> > 2) Our story for frame detach isn't yet clear. I think we may still want to
do
> > the right thing and order frame detaches correctly. In that case, we could
use
> > frameDetached()/frameWillClose() to implement that forwarding. It makes
sense
> at
> > the Blink layer... but maybe it doesn't make sense to expose that outside of
> > content.
> > 
> > I expect both of these to become clearer with time. If we do have to change
> the
> > signatures later, it will be a lot more straightforward than these patches.
> 
> Yeah, guess I stumbled onto a pretty subtle one.  Hopefully this
> created/detached distinction can be documented in the WebFrameClient API?

Once we figure it out, we'll document the differences. I'm hesitant to write
comments until we have frame mirroring completely implemented.

https://codereview.chromium.org/222973003/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.h:94: // This is called right after creation
with the WebFrame for this RenderFrame.
On 2014/04/03 00:37:11, Charlie Reis wrote:
> nit: WebLocalFrame

Done.

https://codereview.chromium.org/222973003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.h:426: // Stores the WebFrame we are
associated with.
On 2014/04/03 00:37:11, Charlie Reis wrote:
> nit: WebLocalFrame.

Done.

https://codereview.chromium.org/222973003/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/WebFrameTestProxy.h (left):

https://codereview.chromium.org/222973003/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/WebFrameTestProxy.h:36: blink::WebPlugin*
createPlugin(blink::WebFrame* frame, const blink::WebPluginParams& params)
On 2014/04/03 00:37:11, Charlie Reis wrote:
> Jochen: Why are these files using Blink style inside content?  I'm guessing
that
> Daniel is using clang-format, which is wrapping all these parameter lines like
> we would do in Chromium style, but I also noticed there's a 4 space indent
here.
>  What's the right thing to do?

Yeah, I used clang-format to wrap overly long lines. This file is in Blink style
because we copied them directly over from Blink and it's not been updated since
then. Not sure what to do =/

[Charlie Reis, 2014-04-03 20:56:31]

I think I forgot to actually CC Jochen last time.

Jochen: One style question for you in WebFrameTestProxy.h.

Apart from that, LGTM.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_frame.h:39: static RenderFrame*
FromWebFrame(blink::WebFrame* web_frame);
On 2014/04/03 20:39:57, dcheng wrote:
> On 2014/04/03 00:37:11, Charlie Reis wrote:
> > On 2014/04/02 23:47:08, dcheng wrote:
> > > On 2014/04/02 23:34:15, Charlie Reis wrote:
> > > > I'm not sure why some uses of WebFrame are being replaced but not
others. 
> > For
> > > > example, this method wouldn't make sense to call on a WebRemoteFrame.
> > > 
> > > Because this didn't fail to compile.
> > 
> > Just to fill me in, what didn't compile?  I'd like to make sense of which
> things
> > need to get migrated.  Is it the set of places that call methods that won't
> > exist on WebRemoteFrame?
> > 
> > > It's true that this doesn't make sense to
> > > call on remote frames; would you be OK if I change this in a separate
patch?
> > I'd
> > > like to keep the patch size somewhat manageable.
> > 
> > Yes, that's fine-- we definitely don't need to update them all at once.
> 
> For figuring out what needs to change, I changed WebLocalFrame from a typedef
to
> subclassing WebFrame. This patch addresses all the compile failures from
trying
> to down cast a WebFrame to a WebLocalFrame without going through
> toWebLocalFrame().

I think the part that I was missing was that you have already chosen a set of
things to change from WebFrame to WebLocalFrame in WebFrameClient, and this CL
just echoes that change on the Chrome side.

In other words, you separately decided that things like frameDetached shouldn't
use WebLocalFrame, and this part is just mechanical.  (My original question was
more about how you made that decision, but I see that there's reasons for the
cases you left as WebFrame.)


> 
> I expect that I'll have to a second round of cleanups in Chrome as I move
things
> from WebFrame to WebLocalFrame, but it should be a lot smaller than these
> initial patches.

Sure.

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_view_observer.h:71: virtual void
FrameDetached(blink::WebFrame* frame) {}
On 2014/04/03 20:39:57, dcheng wrote:
> On 2014/04/03 00:37:11, Charlie Reis wrote:
> > On 2014/04/02 23:47:08, dcheng wrote:
> > > On 2014/04/02 23:34:15, Charlie Reis wrote:
> > > > Again, why FrameCreated but not FrameDetached, etc?
> > > 
> > > This is somewhat subtle and just a starting point. It may be incorrect and
> > > change over time. Here's my current thoughts:
> > > 
> > > 1) Actually creating a child frame is only going to happen in the parent's
> > > renderer. So the parent is always local. But the child may be local or
> remote.
> > > It's not clear to me yet that mirroring the frame tree should go through
the
> > > same callback. Maybe it should, and then we'd need to change it at that
> point.
> > > It made sense at the Blink layer; maybe it doesn't make sense in content.
> > > 
> > > 2) Our story for frame detach isn't yet clear. I think we may still want
to
> do
> > > the right thing and order frame detaches correctly. In that case, we could
> use
> > > frameDetached()/frameWillClose() to implement that forwarding. It makes
> sense
> > at
> > > the Blink layer... but maybe it doesn't make sense to expose that outside
of
> > > content.
> > > 
> > > I expect both of these to become clearer with time. If we do have to
change
> > the
> > > signatures later, it will be a lot more straightforward than these
patches.
> > 
> > Yeah, guess I stumbled onto a pretty subtle one.  Hopefully this
> > created/detached distinction can be documented in the WebFrameClient API?
> 
> Once we figure it out, we'll document the differences. I'm hesitant to write
> comments until we have frame mirroring completely implemented.

Ok in this case, but I disagree about skipping comments until we finish building
things.  We're never finished building, so we have to document as we go.

https://codereview.chromium.org/222973003/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/WebFrameTestProxy.h (left):

https://codereview.chromium.org/222973003/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/WebFrameTestProxy.h:36: blink::WebPlugin*
createPlugin(blink::WebFrame* frame, const blink::WebPluginParams& params)
On 2014/04/03 20:39:57, dcheng wrote:
> On 2014/04/03 00:37:11, Charlie Reis wrote:
> > Jochen: Why are these files using Blink style inside content?  I'm guessing
> that
> > Daniel is using clang-format, which is wrapping all these parameter lines
like
> > we would do in Chromium style, but I also noticed there's a 4 space indent
> here.
> >  What's the right thing to do?
> 
> Yeah, I used clang-format to wrap overly long lines. This file is in Blink
style
> because we copied them directly over from Blink and it's not been updated
since
> then. Not sure what to do =/

I defer to Jochen there.  I expect they should be left in the old style until
(unless?) we move the whole file into the new style.  Having a mix of styles
doesn't make sense to me.

[dcheng, 2014-04-03 21:03:24]

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
File content/public/renderer/render_view_observer.h (right):

https://codereview.chromium.org/222973003/diff/20001/content/public/renderer/...
content/public/renderer/render_view_observer.h:71: virtual void
FrameDetached(blink::WebFrame* frame) {}
On 2014/04/03 20:56:31, Charlie Reis wrote:
> On 2014/04/03 20:39:57, dcheng wrote:
> > On 2014/04/03 00:37:11, Charlie Reis wrote:
> > > On 2014/04/02 23:47:08, dcheng wrote:
> > > > On 2014/04/02 23:34:15, Charlie Reis wrote:
> > > > > Again, why FrameCreated but not FrameDetached, etc?
> > > > 
> > > > This is somewhat subtle and just a starting point. It may be incorrect
and
> > > > change over time. Here's my current thoughts:
> > > > 
> > > > 1) Actually creating a child frame is only going to happen in the
parent's
> > > > renderer. So the parent is always local. But the child may be local or
> > remote.
> > > > It's not clear to me yet that mirroring the frame tree should go through
> the
> > > > same callback. Maybe it should, and then we'd need to change it at that
> > point.
> > > > It made sense at the Blink layer; maybe it doesn't make sense in
content.
> > > > 
> > > > 2) Our story for frame detach isn't yet clear. I think we may still want
> to
> > do
> > > > the right thing and order frame detaches correctly. In that case, we
could
> > use
> > > > frameDetached()/frameWillClose() to implement that forwarding. It makes
> > sense
> > > at
> > > > the Blink layer... but maybe it doesn't make sense to expose that
outside
> of
> > > > content.
> > > > 
> > > > I expect both of these to become clearer with time. If we do have to
> change
> > > the
> > > > signatures later, it will be a lot more straightforward than these
> patches.
> > > 
> > > Yeah, guess I stumbled onto a pretty subtle one.  Hopefully this
> > > created/detached distinction can be documented in the WebFrameClient API?
> > 
> > Once we figure it out, we'll document the differences. I'm hesitant to write
> > comments until we have frame mirroring completely implemented.
> 
> Ok in this case, but I disagree about skipping comments until we finish
building
> things.  We're never finished building, so we have to document as we go.

Right, but currently it's completely unknown. Any comment I write will be at
best half accurate and at worst, completely wrong. It will require a high level
of coordination between Chrome and Blink, and I can't provide guidance to
implementations of WebFrameClient yet because there are still too many pieces
moving on both sides. Once we have something more solid, then it's easy enough
to add accurate comments.

[jochen (gone - plz use gerrit), 2014-04-04 09:01:38]

https://codereview.chromium.org/222973003/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/WebFrameTestProxy.h (left):

https://codereview.chromium.org/222973003/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/WebFrameTestProxy.h:36: blink::WebPlugin*
createPlugin(blink::WebFrame* frame, const blink::WebPluginParams& params)
On 2014/04/03 20:56:31, Charlie Reis wrote:
> On 2014/04/03 20:39:57, dcheng wrote:
> > On 2014/04/03 00:37:11, Charlie Reis wrote:
> > > Jochen: Why are these files using Blink style inside content?  I'm
guessing
> > that
> > > Daniel is using clang-format, which is wrapping all these parameter lines
> like
> > > we would do in Chromium style, but I also noticed there's a 4 space indent
> > here.
> > >  What's the right thing to do?
> > 
> > Yeah, I used clang-format to wrap overly long lines. This file is in Blink
> style
> > because we copied them directly over from Blink and it's not been updated
> since
> > then. Not sure what to do =/
> 
> I defer to Jochen there.  I expect they should be left in the old style until
> (unless?) we move the whole file into the new style.  Having a mix of styles
> doesn't make sense to me.

agreed.

Please consider moving files in test_runner you touch to the content namespace,
rename variables and methods to match chromium style and clang format. This
would be greatly appreciated!

[dcheng, 2014-04-04 10:07:18]

On 2014/04/04 09:01:38, jochen wrote:
>
https://codereview.chromium.org/222973003/diff/20001/content/shell/renderer/t...
> File content/shell/renderer/test_runner/WebFrameTestProxy.h (left):
> 
>
https://codereview.chromium.org/222973003/diff/20001/content/shell/renderer/t...
> content/shell/renderer/test_runner/WebFrameTestProxy.h:36: blink::WebPlugin*
> createPlugin(blink::WebFrame* frame, const blink::WebPluginParams& params)
> On 2014/04/03 20:56:31, Charlie Reis wrote:
> > On 2014/04/03 20:39:57, dcheng wrote:
> > > On 2014/04/03 00:37:11, Charlie Reis wrote:
> > > > Jochen: Why are these files using Blink style inside content?  I'm
> guessing
> > > that
> > > > Daniel is using clang-format, which is wrapping all these parameter
lines
> > like
> > > > we would do in Chromium style, but I also noticed there's a 4 space
indent
> > > here.
> > > >  What's the right thing to do?
> > > 
> > > Yeah, I used clang-format to wrap overly long lines. This file is in Blink
> > style
> > > because we copied them directly over from Blink and it's not been updated
> > since
> > > then. Not sure what to do =/
> > 
> > I defer to Jochen there.  I expect they should be left in the old style
until
> > (unless?) we move the whole file into the new style.  Having a mix of styles
> > doesn't make sense to me.
> 
> agreed.
> 
> Please consider moving files in test_runner you touch to the content
namespace,
> rename variables and methods to match chromium style and clang format. This
> would be greatly appreciated!

I've reverted any clang-format changes. PTAL.

[jochen (gone - plz use gerrit), 2014-04-04 10:25:45]

lgtm

[dcheng, 2014-04-04 10:39:14]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-04-04 10:39:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/222973003/100001

[commit-bot: I haz the power, 2014-04-04 15:24:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/222973003/100001

[commit-bot: I haz the power, 2014-04-04 15:50:24]

Change committed as 261764