Support custom window features, such as 'moo=foo'.

Support custom window features, such as 'moo=foo'.

This patch is part of a bigger patch for #110510, which allows custom window features on popup windows to be propagated to other handlers, such as ContentBrowserClient::CanCreateWindow(). This is particularly helpful in frameworks such as CEF, which allows third-parties to integrate with Chromium and provide custom handling for specific window features.

A separate code review (https://codereview.chromium.org/16358008) has been created for the Blink portions of this bug.

BUG=110510

[commit-bot: I haz the power, 2013-06-04 15:53:17]

No comments yet.

[dreijer, 2013-06-04 18:30:24]

I believe this patch is ready for review time!

[Tom Sepez, 2013-06-04 18:54:50]

Messages LGTM.

https://codereview.chromium.org/15994016/diff/10003/content/public/common/win...
File content/public/common/window_container_type.h (right):

https://codereview.chromium.org/15994016/diff/10003/content/public/common/win...
content/public/common/window_container_type.h:17: float x;
Nit: you could avoid having holes in your structure if you put the bools
together up-front, and the floats together thereafter.

[dreijer, 2013-06-04 19:05:03]

On 2013/06/04 18:54:50, Tom Sepez wrote:
> Messages LGTM.
> 
>
https://codereview.chromium.org/15994016/diff/10003/content/public/common/win...
> File content/public/common/window_container_type.h (right):
> 
>
https://codereview.chromium.org/15994016/diff/10003/content/public/common/win...
> content/public/common/window_container_type.h:17: float x;
> Nit: you could avoid having holes in your structure if you put the bools
> together up-front, and the floats together thereafter.

Removed holes.

[sky, 2013-06-04 21:08:32]

When you add multiple reviewers please indicate what files they are
expected to review. Otherwise no one is likely to respond.

On Tue, Jun 4, 2013 at 12:05 PM,  <dreijerbit@gmail.com> wrote:
> On 2013/06/04 18:54:50, Tom Sepez wrote:
>>
>> Messages LGTM.
>
>
>
>
https://codereview.chromium.org/15994016/diff/10003/content/public/common/win...
>>
>> File content/public/common/window_container_type.h (right):
>
>
>
>
https://codereview.chromium.org/15994016/diff/10003/content/public/common/win...
>>
>> content/public/common/window_container_type.h:17: float x;
>> Nit: you could avoid having holes in your structure if you put the bools
>> together up-front, and the floats together thereafter.
>
>
> Removed holes.
>
> https://codereview.chromium.org/15994016/

[dreijer, 2013-06-04 21:11:47]

On 2013/06/04 21:08:32, sky wrote:
> When you add multiple reviewers please indicate what files they are
> expected to review. Otherwise no one is likely to respond.
> 
> On Tue, Jun 4, 2013 at 12:05 PM,  <mailto:dreijerbit@gmail.com> wrote:
> > On 2013/06/04 18:54:50, Tom Sepez wrote:
> >>
> >> Messages LGTM.
> >
> >
> >
> >
>
https://codereview.chromium.org/15994016/diff/10003/content/public/common/win...
> >>
> >> File content/public/common/window_container_type.h (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/15994016/diff/10003/content/public/common/win...
> >>
> >> content/public/common/window_container_type.h:17: float x;
> >> Nit: you could avoid having holes in your structure if you put the bools
> >> together up-front, and the floats together thereafter.
> >
> >
> > Removed holes.
> >
> > https://codereview.chromium.org/15994016/

Noted. This is my very first Chromium patch. I just used the OWNERS that the gcl
tool suggested and I thought the reviewers would then know which files were
their responsibility (since they're in the OWNERS file in the first place).

Would you like me to list out which files I'd like you to review, sky?

[sky, 2013-06-04 21:19:40]

On Tue, Jun 4, 2013 at 2:11 PM,  <dreijerbit@gmail.com> wrote:
> On 2013/06/04 21:08:32, sky wrote:
>>
>> When you add multiple reviewers please indicate what files they are
>> expected to review. Otherwise no one is likely to respond.
>
>
>> On Tue, Jun 4, 2013 at 12:05 PM,  <mailto:dreijerbit@gmail.com> wrote:
>> > On 2013/06/04 18:54:50, Tom Sepez wrote:
>> >>
>> >> Messages LGTM.
>> >
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/15994016/diff/10003/content/public/common/win...
>>
>> >>
>> >> File content/public/common/window_container_type.h (right):
>> >
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/15994016/diff/10003/content/public/common/win...
>>
>> >>
>> >> content/public/common/window_container_type.h:17: float x;
>> >> Nit: you could avoid having holes in your structure if you put the
>> >> bools
>> >> together up-front, and the floats together thereafter.
>> >
>> >
>> > Removed holes.
>> >
>> > https://codereview.chromium.org/15994016/
>
>
> Noted. This is my very first Chromium patch. I just used the OWNERS that the
> gcl
> tool suggested and I thought the reviewers would then know which files were
> their responsibility (since they're in the OWNERS file in the first place).
>
> Would you like me to list out which files I'd like you to review, sky?

Yes.

> https://codereview.chromium.org/15994016/

[dreijer, 2013-06-04 21:28:34]

List of files that need review. Thanks in advance:

sky:
chrome/browser/ui/browser.h 
chrome/browser/ui/browser.cc 
content/browser/browser_plugin/browser_plugin_guest.h 
content/browser/browser_plugin/browser_plugin_guest.cc
content/browser/renderer_host/render_message_filter.cc 
content/browser/web_contents/web_contents_impl.cc 

darin@chromium.org
content/common/view_messages.h 

joi@chromium.org
content/public/browser/content_browser_client.h 
content/public/browser/content_browser_client.cc
content/public/browser/web_contents_delegate.h 
content/public/browser/web_contents_delegate.cc

jamesr@chromium.org
content/renderer/render_view_impl.cc

[sky, 2013-06-04 23:08:33]

https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.cc:457: WebContents*
source_contents,
4 space indent here too.

https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_p...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_p...
content/browser/browser_plugin/browser_plugin_guest.h:200: WebContents*
source_contents,
nit: change this to 4 space indent (same as 195).

https://codereview.chromium.org/15994016/diff/14001/content/public/common/win...
File content/public/common/window_container_type.h (right):

https://codereview.chromium.org/15994016/diff/14001/content/public/common/win...
content/public/common/window_container_type.h:13: // Converts the Blink-specific
WebWindowFeatures struct to a local
You should look at the the C++ style guide. This suffers the following:

. Don't indent code in namespaces (see line 74).
. unix_hacker_style for fields.
. Methods before members.
. ',' in member initializer list belongs on previous line.
. AFAICT you don't need a copy constructor here (compiler generated one is
fine).
. indentation for member initializer list is 4.

[dreijer, 2013-06-04 23:18:57]

On 2013/06/04 23:08:33, sky wrote:
>
https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_p...
> File content/browser/browser_plugin/browser_plugin_guest.cc (right):
> 
>
https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_p...
> content/browser/browser_plugin/browser_plugin_guest.cc:457: WebContents*
> source_contents,
> 4 space indent here too.

You mean indent all the function parameters with 4 spaces from the left rather
than align to the right?

>
https://codereview.chromium.org/15994016/diff/14001/content/public/common/win...
> File content/public/common/window_container_type.h (right):
> 
>
https://codereview.chromium.org/15994016/diff/14001/content/public/common/win...
> content/public/common/window_container_type.h:13: // Converts the
Blink-specific
> WebWindowFeatures struct to a local
> You should look at the the C++ style guide. This suffers the following:
> 
> . AFAICT you don't need a copy constructor here (compiler generated one is
> fine).
The copy contructor is required because it converts from
WebKit::WebWindowFeatures (from Blink) to WebKit::WindowFeatures.

[sky, 2013-06-04 23:23:31]

On Tue, Jun 4, 2013 at 4:18 PM,  <dreijerbit@gmail.com> wrote:
> On 2013/06/04 23:08:33, sky wrote:
>
>
https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_p...
>>
>> File content/browser/browser_plugin/browser_plugin_guest.cc (right):
>
>
>
>
https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_p...
>>
>> content/browser/browser_plugin/browser_plugin_guest.cc:457: WebContents*
>> source_contents,
>> 4 space indent here too.
>
>
> You mean indent all the function parameters with 4 spaces from the left
> rather
> than align to the right?

That's right. See 548 for an example.

>
>
>
>
https://codereview.chromium.org/15994016/diff/14001/content/public/common/win...
>>
>> File content/public/common/window_container_type.h (right):
>
>
>
>
https://codereview.chromium.org/15994016/diff/14001/content/public/common/win...
>>
>> content/public/common/window_container_type.h:13: // Converts the
>
> Blink-specific
>>
>> WebWindowFeatures struct to a local
>> You should look at the the C++ style guide. This suffers the following:
>
>
>> . AFAICT you don't need a copy constructor here (compiler generated one is
>> fine).
>
> The copy contructor is required because it converts from
> WebKit::WebWindowFeatures (from Blink) to WebKit::WindowFeatures.

Ah, my mistake. I missed that it takes a WebWindowFeature and not a
WindowFeature (subtle naming).

  -Scott

>
>
>
> https://codereview.chromium.org/15994016/

[dreijer, 2013-06-04 23:42:16]

On 2013/06/04 23:23:31, sky wrote:
> On Tue, Jun 4, 2013 at 4:18 PM,  <mailto:dreijerbit@gmail.com> wrote:
> > On 2013/06/04 23:08:33, sky wrote:
> >
> >
>
https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_p...
> >>
> >> File content/browser/browser_plugin/browser_plugin_guest.cc (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_p...
> >>
> >> content/browser/browser_plugin/browser_plugin_guest.cc:457: WebContents*
> >> source_contents,
> >> 4 space indent here too.
> >
> >
> > You mean indent all the function parameters with 4 spaces from the left
> > rather
> > than align to the right?
> 
> That's right. See 548 for an example.
> 
> >
> >
> >
> >
>
https://codereview.chromium.org/15994016/diff/14001/content/public/common/win...
> >>
> >> File content/public/common/window_container_type.h (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/15994016/diff/14001/content/public/common/win...
> >>
> >> content/public/common/window_container_type.h:13: // Converts the
> >
> > Blink-specific
> >>
> >> WebWindowFeatures struct to a local
> >> You should look at the the C++ style guide. This suffers the following:
> >
> >
> >> . AFAICT you don't need a copy constructor here (compiler generated one is
> >> fine).
> >
> > The copy contructor is required because it converts from
> > WebKit::WebWindowFeatures (from Blink) to WebKit::WindowFeatures.
> 
> Ah, my mistake. I missed that it takes a WebWindowFeature and not a
> WindowFeature (subtle naming).
> 
>   -Scott
> 
> >
> >
> >
> > https://codereview.chromium.org/15994016/

Thanks! I uploaded a new changeset.

Questions:

 * I googled around for unix_hacker_style for fields, but wasn't able to find
anything useful. Do you have a link?
 * Re: indentation of member initializer list: I wasn't sure where to put the
initial colon, e.g. on the previous line, or inline and then indent everything
else appropriately. I looked around the source and picked the style that I saw
several other places were using (although I noticed like 5 different styles :)

[jamesr1, 2013-06-04 23:46:03]

On Tue, Jun 4, 2013 at 4:42 PM, <dreijerbit@gmail.com> wrote:

> On 2013/06/04 23:23:31, sky wrote:
>
>  On Tue, Jun 4, 2013 at 4:18 PM,  <mailto:dreijerbit@gmail.com> wrote:
>> > On 2013/06/04 23:08:33, sky wrote:
>> >
>> >
>>
>
> https://codereview.chromium.**org/15994016/diff/14001/**
>
content/browser/browser_**plugin/browser_plugin_guest.cc<https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_plugin/browser_plugin_guest.cc>
>
>> >>
>> >> File content/browser/browser_**plugin/browser_plugin_guest.cc (right):
>> >
>> >
>> >
>> >
>>
>
> https://codereview.chromium.**org/15994016/diff/14001/**
>
content/browser/browser_**plugin/browser_plugin_guest.**cc#newcode457<https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_plugin/browser_plugin_guest.cc#newcode457>
>
>> >>
>> >> content/browser/browser_**plugin/browser_plugin_guest.**cc:457:
>> WebContents*
>> >> source_contents,
>> >> 4 space indent here too.
>> >
>> >
>> > You mean indent all the function parameters with 4 spaces from the left
>> > rather
>> > than align to the right?
>>
>
>  That's right. See 548 for an example.
>>
>
>  >
>> >
>> >
>> >
>>
>
> https://codereview.chromium.**org/15994016/diff/14001/**
>
content/public/common/window_**container_type.h<https://codereview.chromium.org/15994016/diff/14001/content/public/common/window_container_type.h>
>
>> >>
>> >> File content/public/common/window_**container_type.h (right):
>> >
>> >
>> >
>> >
>>
>
> https://codereview.chromium.**org/15994016/diff/14001/**
>
content/public/common/window_**container_type.h#newcode13<https://codereview.chromium.org/15994016/diff/14001/content/public/common/window_container_type.h#newcode13>
>
>> >>
>> >> content/public/common/window_**container_type.h:13: // Converts the
>> >
>> > Blink-specific
>> >>
>> >> WebWindowFeatures struct to a local
>> >> You should look at the the C++ style guide. This suffers the following:
>> >
>> >
>> >> . AFAICT you don't need a copy constructor here (compiler generated
>> one is
>> >> fine).
>> >
>> > The copy contructor is required because it converts from
>> > WebKit::WebWindowFeatures (from Blink) to WebKit::WindowFeatures.
>>
>
>  Ah, my mistake. I missed that it takes a WebWindowFeature and not a
>> WindowFeature (subtle naming).
>>
>
>     -Scott
>>
>
>  >
>> >
>> >
>> >
https://codereview.chromium.**org/15994016/<https://codereview.chromium.org/1...
>>
>
> Thanks! I uploaded a new changeset.
>
> Questions:
>
>  * I googled around for unix_hacker_style for fields, but wasn't able to
> find
> anything useful. Do you have a link?
>

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Naming



>  * Re: indentation of member initializer list: I wasn't sure where to put
> the
> initial colon, e.g. on the previous line, or inline and then indent
> everything
> else appropriately. I looked around the source and picked the style that I
> saw
> several other places were using (although I noticed like 5 different
> styles :)
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...


>
>
https://codereview.chromium.**org/15994016/<https://codereview.chromium.org/1...
>

[dreijer, 2013-06-04 23:58:39]

On 2013/06/04 23:46:03, jamesr1 wrote:
> On Tue, Jun 4, 2013 at 4:42 PM, <mailto:dreijerbit@gmail.com> wrote:
> 
> > On 2013/06/04 23:23:31, sky wrote:
> >
> >  On Tue, Jun 4, 2013 at 4:18 PM,  <mailto:dreijerbit@gmail.com> wrote:
> >> > On 2013/06/04 23:08:33, sky wrote:
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.**org/15994016/diff/14001/**
> >
>
content/browser/browser_**plugin/browser_plugin_guest.cc<https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_plugin/browser_plugin_guest.cc>
> >
> >> >>
> >> >> File content/browser/browser_**plugin/browser_plugin_guest.cc (right):
> >> >
> >> >
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.**org/15994016/diff/14001/**
> >
>
content/browser/browser_**plugin/browser_plugin_guest.**cc#newcode457<https://codereview.chromium.org/15994016/diff/14001/content/browser/browser_plugin/browser_plugin_guest.cc#newcode457>
> >
> >> >>
> >> >> content/browser/browser_**plugin/browser_plugin_guest.**cc:457:
> >> WebContents*
> >> >> source_contents,
> >> >> 4 space indent here too.
> >> >
> >> >
> >> > You mean indent all the function parameters with 4 spaces from the left
> >> > rather
> >> > than align to the right?
> >>
> >
> >  That's right. See 548 for an example.
> >>
> >
> >  >
> >> >
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.**org/15994016/diff/14001/**
> >
>
content/public/common/window_**container_type.h<https://codereview.chromium.org/15994016/diff/14001/content/public/common/window_container_type.h>
> >
> >> >>
> >> >> File content/public/common/window_**container_type.h (right):
> >> >
> >> >
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.**org/15994016/diff/14001/**
> >
>
content/public/common/window_**container_type.h#newcode13<https://codereview.chromium.org/15994016/diff/14001/content/public/common/window_container_type.h#newcode13>
> >
> >> >>
> >> >> content/public/common/window_**container_type.h:13: // Converts the
> >> >
> >> > Blink-specific
> >> >>
> >> >> WebWindowFeatures struct to a local
> >> >> You should look at the the C++ style guide. This suffers the following:
> >> >
> >> >
> >> >> . AFAICT you don't need a copy constructor here (compiler generated
> >> one is
> >> >> fine).
> >> >
> >> > The copy contructor is required because it converts from
> >> > WebKit::WebWindowFeatures (from Blink) to WebKit::WindowFeatures.
> >>
> >
> >  Ah, my mistake. I missed that it takes a WebWindowFeature and not a
> >> WindowFeature (subtle naming).
> >>
> >
> >     -Scott
> >>
> >
> >  >
> >> >
> >> >
> >> >
>
https://codereview.chromium.**org/15994016/%3Chttps://codereview.chromium.org...>
> >>
> >
> > Thanks! I uploaded a new changeset.
> >
> > Questions:
> >
> >  * I googled around for unix_hacker_style for fields, but wasn't able to
> > find
> > anything useful. Do you have a link?
> >
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Naming
> 
> 
> 
> >  * Re: indentation of member initializer list: I wasn't sure where to put
> > the
> > initial colon, e.g. on the previous line, or inline and then indent
> > everything
> > else appropriately. I looked around the source and picked the style that I
> > saw
> > several other places were using (although I noticed like 5 different
> > styles :)
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constructor_In...
> 
> 
> >
> >
>
https://codereview.chromium.**org/15994016/%3Chttps://codereview.chromium.org...>
> >

Aaaand another changeset uploaded with proper field names. Looks like I already
had the initializer list conforming to the style formatting.

[Jói, 2013-06-07 15:29:28]

jam@ should review for content/ I think, because of the interface change.

[jam, 2013-06-10 18:13:56]

https://codereview.chromium.org/15994016/diff/36017/content/public/common/win...
File content/public/common/window_container_type.h (right):

https://codereview.chromium.org/15994016/diff/36017/content/public/common/win...
content/public/common/window_container_type.h:9: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebWindowFeatures.h"
see crbug.com/237267, people are working on removing webkit includes from
content/common, so this is taking us a step back.

presumably this code is only needed by code in src/content/renderer, so it
should just be there?

https://codereview.chromium.org/15994016/diff/36017/content/public/common/win...
content/public/common/window_container_type.h:15: struct WindowFeatures {
only classes defined in webkit itself can be in the WebKit namespace.

also, per content api conventions, every class needs to be in its own file. see
http://www.chromium.org/developers/content-module/content-api

[dreijer, 2013-07-15 15:51:37]

On 2013/06/10 18:13:56, jam wrote:
>
https://codereview.chromium.org/15994016/diff/36017/content/public/common/win...
> File content/public/common/window_container_type.h (right):
> 
>
https://codereview.chromium.org/15994016/diff/36017/content/public/common/win...
> content/public/common/window_container_type.h:9: #include
> "third_party/WebKit/Source/WebKit/chromium/public/WebWindowFeatures.h"
> see crbug.com/237267, people are working on removing webkit includes from
> content/common, so this is taking us a step back.
> 
> presumably this code is only needed by code in src/content/renderer, so it
> should just be there?
> 
>
https://codereview.chromium.org/15994016/diff/36017/content/public/common/win...
> content/public/common/window_container_type.h:15: struct WindowFeatures {
> only classes defined in webkit itself can be in the WebKit namespace.
> 
> also, per content api conventions, every class needs to be in its own file.
see
> http://www.chromium.org/developers/content-module/content-api

My apologies for the slow reply. I'll get these changes incorporated asap and
push a new patch set.

[dreijer, 2013-07-19 16:15:36]

https://codereview.chromium.org/15994016/diff/36017/content/public/common/win...
File content/public/common/window_container_type.h (right):

https://codereview.chromium.org/15994016/diff/36017/content/public/common/win...
content/public/common/window_container_type.h:9: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebWindowFeatures.h"
On 2013/06/10 18:13:57, jam wrote:
> see crbug.com/237267, people are working on removing webkit includes from
> content/common, so this is taking us a step back.
> 
> presumably this code is only needed by code in src/content/renderer, so it
> should just be there?

Well, so the problem here is that this class (WindowFeatures) converts a
Webkit::WebWindowFeatures to a type that can be used all across Chromium.
Regardless of where I declare WindowFeatures, I still need to include the Webkit
header so I can do the conversion.

Since you'd like me to put WindowFeatures in to its own file, where would you
suggest I create that file?