Additional window features are not passed as a parameter to new popup windows

Added wrapper interface, WindowFeatures, that allows additional window features to be passed over IPC.

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

[dreijer, 2013-11-06 05:16:29]

Hey guys,

I just submitted a patch that properly includes additional window features when
new popup windows are created. The corresponding issue is #315428.

I hope you have the time to take a quick look to prevent the patch from straying
too far from trunk.

I've highlighted the files and requested reviewers below:

tsepez@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 

jam@chromium.org:
 content/public/common/window_features.h  
 content/public/common/window_features.cc 
 content/public/common/common_param_traits_macros.h 

jochen@chromium.org:
 content/renderer/render_view_impl.cc

[jochen (gone - plz use gerrit), 2013-11-06 11:00:49]

thanks for the patch.

Can you explain what problem this is solving? And add a test that demonstrates
that the problem is solved indeed.

blink ignores most window features, and in your patch you're not passing the
extended features back to the renderer when creating the popup, so I'm not sure
why you need this code

[dreijer, 2013-11-06 15:58:56]

On 2013/11/06 11:00:49, jochen wrote:
> thanks for the patch.
> 
> Can you explain what problem this is solving? And add a test that demonstrates
> that the problem is solved indeed.
> 
> blink ignores most window features, and in your patch you're not passing the
> extended features back to the renderer when creating the popup, so I'm not
sure
> why you need this code

I'm using CEF (http://code.google.com/p/chromiumembedded). Whenever a new popup
is created, CEF passes all the window features to my application and I can use
them to show the proper type of window. However, by not including additional
features, which are very much a part of the WebWindowFeatures interface, I'm
unable to key off of any additional features in my application.

As is, Blink currently considers a feature to be an additional feature when it
doesn't match any of the predefined types, and it has a key with no value, or
the value is "yes" (see WindowFeatures::setWindowFeature). (For what it's worth,
I've patched that in my application to allow for any custom window features
(such as 'moo=foo') to be parsed by Blink since it allows for greater
flexibility, but a patch (https://codereview.chromium.org/16358008) adding
support for this was rejected by the Blink team earlier in the year because it's
non-standard, which I guess makes sense.)

So, long story short: I believe my patch resolves two issues: 1) allowing
additional features to be forwarded (after all, they *are* being parsed by
Blink), and 2) it avoids leaking the Blink-specific interface to the rest of
Chromium.

I'd happily write a test for this, but since I'm new to Chromium development,
I'm not quite sure how to. Pointers? :)

[jochen (gone - plz use gerrit), 2013-11-06 16:02:36]

John, what do you think about this?

[jam, 2013-11-06 17:44:27]

On 2013/11/06 16:02:36, jochen wrote:
> John, what do you think about this?

if additionalFeatures is needed in the browser, we can do that.

one premise of this change though is that it's bad to "leak" webkit structs in
content. that's not the case, as the alternative is to duplicate a struct which
is not great.

we didn't pass additionalFeatures to the browser because WebKit::WebString are
not thread-safe and can't be used in IPCs. what about leaving the code as is,
and adding an std::vector<std::string> to ViewHostMsg_CreateWindow_Params to
contain the strings?

[dreijer, 2013-11-06 17:53:50]

On 2013/11/06 17:44:27, jam wrote:
> On 2013/11/06 16:02:36, jochen wrote:
> > John, what do you think about this?
> 
> if additionalFeatures is needed in the browser, we can do that.

Awesome!

> one premise of this change though is that it's bad to "leak" webkit structs in
> content. that's not the case, as the alternative is to duplicate a struct
which
> is not great.
> 
> we didn't pass additionalFeatures to the browser because WebKit::WebString are
> not thread-safe and can't be used in IPCs. what about leaving the code as is,
> and adding an std::vector<std::string> to ViewHostMsg_CreateWindow_Params to
> contain the strings?

That works too, I guess. I just received feedback previously
(https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d...)
that it was preferred to mirror the type. Personally, I like the mirror approach
because it allows me to follow the Chromium coding standards rather than leaking
Webkit's naming scheme in to Chromium (e.g. additionalFeatures vs
additional_features), but seeing as I'm just scratching the surface of Chromium
dev here, I could be wrong on that. :)

Let me know which you'd prefer and I'll get that updated in the changeset.

[jam, 2013-11-07 18:01:03]

On 2013/11/06 17:53:50, dreijer wrote:
> On 2013/11/06 17:44:27, jam wrote:
> > On 2013/11/06 16:02:36, jochen wrote:
> > > John, what do you think about this?
> > 
> > if additionalFeatures is needed in the browser, we can do that.
> 
> Awesome!
> 
> > one premise of this change though is that it's bad to "leak" webkit structs
in
> > content. that's not the case, as the alternative is to duplicate a struct
> which
> > is not great.
> > 
> > we didn't pass additionalFeatures to the browser because WebKit::WebString
are
> > not thread-safe and can't be used in IPCs. what about leaving the code as
is,
> > and adding an std::vector<std::string> to ViewHostMsg_CreateWindow_Params to
> > contain the strings?
> 
> That works too, I guess. I just received feedback previously
>
(https://groups.google.com/a/chromium.org/forum/?fromgroups=#%21topic/chromium...)
> that it was preferred to mirror the type. Personally, I like the mirror
approach
> because it allows me to follow the Chromium coding standards rather than
leaking
> Webkit's naming scheme in to Chromium (e.g. additionalFeatures vs
> additional_features), but seeing as I'm just scratching the surface of
Chromium
> dev here, I could be wrong on that. :)
> 
> Let me know which you'd prefer and I'll get that updated in the changeset.

sorry for giving conflicting messages. This can go both ways. The reason I
suggested sending the strings separately is that it'll be a much smaller change,
and also will reduce the cognitive overhead of adding yet another struct
(especially one that would have to be added to the public API)

[dreijer, 2013-11-07 18:42:10]

On 2013/11/07 18:01:03, jam wrote:
> On 2013/11/06 17:53:50, dreijer wrote:
> > On 2013/11/06 17:44:27, jam wrote:
> > > On 2013/11/06 16:02:36, jochen wrote:
> > > > John, what do you think about this?
> > > 
> > > if additionalFeatures is needed in the browser, we can do that.
> > 
> > Awesome!
> > 
> > > one premise of this change though is that it's bad to "leak" webkit
structs
> in
> > > content. that's not the case, as the alternative is to duplicate a struct
> > which
> > > is not great.
> > > 
> > > we didn't pass additionalFeatures to the browser because WebKit::WebString
> are
> > > not thread-safe and can't be used in IPCs. what about leaving the code as
> is,
> > > and adding an std::vector<std::string> to ViewHostMsg_CreateWindow_Params
to
> > > contain the strings?
> > 
> > That works too, I guess. I just received feedback previously
> >
>
(https://groups.google.com/a/chromium.org/forum/?fromgroups=#%2521topic/chromi...)
> > that it was preferred to mirror the type. Personally, I like the mirror
> approach
> > because it allows me to follow the Chromium coding standards rather than
> leaking
> > Webkit's naming scheme in to Chromium (e.g. additionalFeatures vs
> > additional_features), but seeing as I'm just scratching the surface of
> Chromium
> > dev here, I could be wrong on that. :)
> > 
> > Let me know which you'd prefer and I'll get that updated in the changeset.
> 
> sorry for giving conflicting messages. This can go both ways. The reason I
> suggested sending the strings separately is that it'll be a much smaller
change,
> and also will reduce the cognitive overhead of adding yet another struct
> (especially one that would have to be added to the public API)

Okiedokey, I'll get that updated shortly. Stay tuned!

[dreijer, 2013-11-07 20:52:40]

On 2013/11/07 18:42:10, dreijer wrote:
> On 2013/11/07 18:01:03, jam wrote:
> > On 2013/11/06 17:53:50, dreijer wrote:
> > > On 2013/11/06 17:44:27, jam wrote:
> > > > On 2013/11/06 16:02:36, jochen wrote:
> > > > > John, what do you think about this?
> > > > 
> > > > if additionalFeatures is needed in the browser, we can do that.
> > > 
> > > Awesome!
> > > 
> > > > one premise of this change though is that it's bad to "leak" webkit
> structs
> > in
> > > > content. that's not the case, as the alternative is to duplicate a
struct
> > > which
> > > > is not great.
> > > > 
> > > > we didn't pass additionalFeatures to the browser because
WebKit::WebString
> > are
> > > > not thread-safe and can't be used in IPCs. what about leaving the code
as
> > is,
> > > > and adding an std::vector<std::string> to
ViewHostMsg_CreateWindow_Params
> to
> > > > contain the strings?
> > > 
> > > That works too, I guess. I just received feedback previously
> > >
> >
>
(https://groups.google.com/a/chromium.org/forum/?fromgroups=#%252521topic/chro...)
> > > that it was preferred to mirror the type. Personally, I like the mirror
> > approach
> > > because it allows me to follow the Chromium coding standards rather than
> > leaking
> > > Webkit's naming scheme in to Chromium (e.g. additionalFeatures vs
> > > additional_features), but seeing as I'm just scratching the surface of
> > Chromium
> > > dev here, I could be wrong on that. :)
> > > 
> > > Let me know which you'd prefer and I'll get that updated in the changeset.
> > 
> > sorry for giving conflicting messages. This can go both ways. The reason I
> > suggested sending the strings separately is that it'll be a much smaller
> change,
> > and also will reduce the cognitive overhead of adding yet another struct
> > (especially one that would have to be added to the public API)
> 
> Okiedokey, I'll get that updated shortly. Stay tuned!

Alright, new changeset uploaded. Comments/suggestions?

[jochen (gone - plz use gerrit), 2013-11-08 11:53:50]

lgtm

[dreijer, 2013-11-08 15:32:04]

On 2013/11/08 11:53:50, jochen wrote:
> lgtm

Hmm, it looks like part of the patchset failed to get applied on the try server.
I'm working off of CEF's trunk, which I guess isn't using the very newest trunk
of Chromium.

If I do a clean checkout of Chromium and manually apply my changes, is there a
way to reopen this issue rather than creating a new one?

[jochen (gone - plz use gerrit), 2013-11-08 15:33:21]

you can associate a branch on a git checkout with an issue using the command

git cl issue <number>

thne you can just upload

[dreijer, 2013-11-09 06:30:48]

On 2013/11/08 15:33:21, jochen wrote:
> you can associate a branch on a git checkout with an issue using the command
> 
> git cl issue <number>
> 
> thne you can just upload

Thanks, got it working.

I've submitted a new changeset against the latest trunk. Comments?

[jam, 2013-11-11 17:07:18]

https://codereview.chromium.org/61503003/diff/440001/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/61503003/diff/440001/content/common/view_mess...
content/common/view_messages.h:367: // The additional window features to use for
the new view.
nit: add a comment to explain that although this is part of
blink::WebWindowFeatures, we send it separately because we can't send WebStrings
over IPCs

https://codereview.chromium.org/61503003/diff/440001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/61503003/diff/440001/content/public/browser/c...
content/public/browser/content_browser_client.h:465: const
std::vector<string16>& additional_features,
we should keep the implementation details of no-WebString-over-IPC hidden from
this API. i.e. when you dispatch the IPC from the renderer, create a new
blink::WebWindowFeatures and set the additionalFeatures member to this array

[dreijer, 2013-11-11 23:01:22]

On 2013/11/11 17:07:18, jam wrote:
>
https://codereview.chromium.org/61503003/diff/440001/content/common/view_mess...
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/61503003/diff/440001/content/common/view_mess...
> content/common/view_messages.h:367: // The additional window features to use
for
> the new view.
> nit: add a comment to explain that although this is part of
> blink::WebWindowFeatures, we send it separately because we can't send
WebStrings
> over IPCs
> 
>
https://codereview.chromium.org/61503003/diff/440001/content/public/browser/c...
> File content/public/browser/content_browser_client.h (right):
> 
>
https://codereview.chromium.org/61503003/diff/440001/content/public/browser/c...
> content/public/browser/content_browser_client.h:465: const
> std::vector<string16>& additional_features,
> we should keep the implementation details of no-WebString-over-IPC hidden from
> this API. i.e. when you dispatch the IPC from the renderer, create a new
> blink::WebWindowFeatures and set the additionalFeatures member to this array

Changes incorporated. Comments?

[Tom Sepez, 2013-11-11 23:07:58]

view_messages.h LGTM with nits.

https://codereview.chromium.org/61503003/diff/330002/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/61503003/diff/330002/AUTHORS#newcode313
AUTHORS:313: Soren Dreijer <dreijerbit@gmail.com>
nit: alphabetize.

https://codereview.chromium.org/61503003/diff/330002/content/common/view_mess...
File content/common/view_messages.h (right):

https://codereview.chromium.org/61503003/diff/330002/content/common/view_mess...
content/common/view_messages.h:367: // The additional window features to use for
the new view. We pass it
nit: "we pass these" since features is plural.

https://codereview.chromium.org/61503003/diff/330002/content/common/view_mess...
content/common/view_messages.h:368: // separately from 'features' above because
we cannot serialize WebStrings
nit: |features|, per chromium standard for delimiting symbols in comments.

[dreijer, 2013-11-11 23:16:42]

On 2013/11/11 23:07:58, Tom Sepez wrote:
> view_messages.h LGTM with nits.
> 
> https://codereview.chromium.org/61503003/diff/330002/AUTHORS
> File AUTHORS (right):
> 
> https://codereview.chromium.org/61503003/diff/330002/AUTHORS#newcode313
> AUTHORS:313: Soren Dreijer <mailto:dreijerbit@gmail.com>
> nit: alphabetize.
> 
>
https://codereview.chromium.org/61503003/diff/330002/content/common/view_mess...
> File content/common/view_messages.h (right):
> 
>
https://codereview.chromium.org/61503003/diff/330002/content/common/view_mess...
> content/common/view_messages.h:367: // The additional window features to use
for
> the new view. We pass it
> nit: "we pass these" since features is plural.
> 
>
https://codereview.chromium.org/61503003/diff/330002/content/common/view_mess...
> content/common/view_messages.h:368: // separately from 'features' above
because
> we cannot serialize WebStrings
> nit: |features|, per chromium standard for delimiting symbols in comments.

Fixed nits.

[jam, 2013-11-11 23:25:36]

lgtm

[dreijer, 2013-11-12 21:09:16]

On 2013/11/11 23:25:36, jam wrote:
> lgtm

So, I'm seeing various try errors, but I don't know if those are related to my
changes. What's going to happen now?

[jochen (gone - plz use gerrit), 2013-11-12 21:15:44]

https://codereview.chromium.org/61503003/diff/570001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/61503003/diff/570001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:464:
features.additionalFeatures[i] = blink::WebString(
this probably needs to be
features.additionalFeatures.append(blink::WebString(..));

[dreijer, 2013-11-12 21:19:39]

On 2013/11/12 21:15:44, jochen wrote:
>
https://codereview.chromium.org/61503003/diff/570001/content/browser/renderer...
> File content/browser/renderer_host/render_message_filter.cc (right):
> 
>
https://codereview.chromium.org/61503003/diff/570001/content/browser/renderer...
> content/browser/renderer_host/render_message_filter.cc:464:
> features.additionalFeatures[i] = blink::WebString(
> this probably needs to be
> features.additionalFeatures.append(blink::WebString(..));

Doh, oops, forgot the initial size call when I was playing around with something
else. I'll get that committed in a sec.

[dreijer, 2013-11-12 23:27:39]

On 2013/11/12 21:19:39, dreijer wrote:
> On 2013/11/12 21:15:44, jochen wrote:
> >
>
https://codereview.chromium.org/61503003/diff/570001/content/browser/renderer...
> > File content/browser/renderer_host/render_message_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/61503003/diff/570001/content/browser/renderer...
> > content/browser/renderer_host/render_message_filter.cc:464:
> > features.additionalFeatures[i] = blink::WebString(
> > this probably needs to be
> > features.additionalFeatures.append(blink::WebString(..));
> 
> Doh, oops, forgot the initial size call when I was playing around with
something
> else. I'll get that committed in a sec.

Alright, fixed.

[dreijer, 2013-11-18 15:57:36]

On 2013/11/12 23:27:39, dreijer wrote:
> On 2013/11/12 21:19:39, dreijer wrote:
> > On 2013/11/12 21:15:44, jochen wrote:
> > >
> >
>
https://codereview.chromium.org/61503003/diff/570001/content/browser/renderer...
> > > File content/browser/renderer_host/render_message_filter.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/61503003/diff/570001/content/browser/renderer...
> > > content/browser/renderer_host/render_message_filter.cc:464:
> > > features.additionalFeatures[i] = blink::WebString(
> > > this probably needs to be
> > > features.additionalFeatures.append(blink::WebString(..));
> > 
> > Doh, oops, forgot the initial size call when I was playing around with
> something
> > else. I'll get that committed in a sec.
> 
> Alright, fixed.

Looks like there were try errors again, but I'm still not sure how to interpret
them. Some of the tests appear unrelated to the changes in this patch. Other
tests outright crash, but I don't know how to interpret the output to figure out
why.

[jochen (gone - plz use gerrit), 2013-11-18 16:20:29]

hum, they all look somewhat unrelated.

Let's give it a try

[commit-bot: I haz the power, 2013-11-18 16:20:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dreijerbit@gmail.com/61503003/720001

[commit-bot: I haz the power, 2013-11-18 18:40:45]

Change committed as 235770