Create a content::FrameOwnerProperties struct for IPC transport of WebFrameOwnerProperties

Create a content::FrameOwnerProperties struct for IPC transport of WebFrameOwnerProperties

blink::WebFrameOwnerProperties can't be sent over IPC because one of its members
is a WebVector which doesn't have ParamTraits defined. We introduce a new struct
in content to transfer the data.

BUG=614608
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/bba82b368739017314eac8bed4ae8c5533975bf5
Cr-Commit-Position: refs/heads/master@{#406163}

[raymes, 2016-07-14 05:54:52]

Description was changed from

==========
Transport

BUG=
==========

to

==========
Transport

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[raymes, 2016-07-14 06:27:26]

Description was changed from

==========
Transport

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Create a content::FrameOwnerProperties struct for IPC transport of
WebFrameOwnerProperties

blink::WebFrameOwnerProperties can't be sent over IPC because one of its members
is a WebVector which doesn't have ParamTraits defined. We introduce a new struct
in content to transfer the data.

BUG=614608
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[raymes, 2016-07-14 06:35:40]

raymes@chromium.org changed reviewers:
+ alexmos@chromium.org, dcheng@chromium.org

[raymes, 2016-07-14 06:35:41]

Hey guys, PTAL. I've done the most naive thing and just used the content struct
for transport but I could pull on the strings and replace the occurrences of
WebFrameOwnerProperties in content with the new struct too. WDYT?

[dcheng, 2016-07-14 11:17:48]

Blink LGTM. I don't have an opinion wrt to using FrameOwnerProperties more
widely in content. It doesn't seem like there's an immediate need to, so I think
just this patch is fine.

https://codereview.chromium.org/2146803004/diff/40001/content/common/frame_ow...
File content/common/frame_owner_properties.cc (right):

https://codereview.chromium.org/2146803004/diff/40001/content/common/frame_ow...
content/common/frame_owner_properties.cc:11:
FrameOwnerProperties::FrameOwnerProperties(const FrameOwnerProperties& other)
Nit: just default this:

FrameOwnerProperties::FrameOwnerProperties(const FrameOwnerPropertie&) =
default;

[alexmos, 2016-07-14 23:44:14]

This seems reasonable, but it does feel a bit weird to convert the properties to
blink::WebFrameOwnerProperties when they arrive into content/browser, only to
convert them right back to content::FrameOwnerProperties before sending them off
to renderers again.  Something like OnDidUpdateFrameOwnerProperties is basically
doing two extra conversions without even using the intermediate version for
anything.  So I'd vote to just use content::FrameOwnerProperties everywhere in
content/browser for consistency.  (FrameOwnerProperties is also shorter than
blink::WebFrameOwnerProperties) :)  That said, it's fine if you want to do this
in a followup, so this patch LGTM with nits.

https://codereview.chromium.org/2146803004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2146803004/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:42: #include
"third_party/WebKit/public/web/WebFrameOwnerProperties.h"
nit: can this be moved to the .cc file now?  (Same for render_frame_impl.h)

https://codereview.chromium.org/2146803004/diff/40001/content/common/frame_ow...
File content/common/frame_owner_properties.h (right):

https://codereview.chromium.org/2146803004/diff/40001/content/common/frame_ow...
content/common/frame_owner_properties.h:15: // Used for IPC transport of
WebFrameOwnerProperties.
Maybe expand this comment to explain why WebFrameOwnerProperties can't be
transported directly?

https://codereview.chromium.org/2146803004/diff/40001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/2146803004/diff/40001/content/content_common....
content/content_common.gypi:330: 'common/frame_owner_properties.h',
nit: put after frame_messages.h to preserve sort order.

[raymes, 2016-07-18 03:39:28]

Sounds good - I agree. I'll do that in a followup. Thanks!

https://codereview.chromium.org/2146803004/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2146803004/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:42: #include
"third_party/WebKit/public/web/WebFrameOwnerProperties.h"
On 2016/07/14 23:44:13, alexmos wrote:
> nit: can this be moved to the .cc file now?  (Same for render_frame_impl.h)

Done.

https://codereview.chromium.org/2146803004/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:42: #include
"third_party/WebKit/public/web/WebFrameOwnerProperties.h"
On 2016/07/14 23:44:13, alexmos wrote:
> nit: can this be moved to the .cc file now?  (Same for render_frame_impl.h)

Done.

https://codereview.chromium.org/2146803004/diff/40001/content/common/frame_ow...
File content/common/frame_owner_properties.cc (right):

https://codereview.chromium.org/2146803004/diff/40001/content/common/frame_ow...
content/common/frame_owner_properties.cc:11:
FrameOwnerProperties::FrameOwnerProperties(const FrameOwnerProperties& other)
whoa cool!

https://codereview.chromium.org/2146803004/diff/40001/content/common/frame_ow...
File content/common/frame_owner_properties.h (right):

https://codereview.chromium.org/2146803004/diff/40001/content/common/frame_ow...
content/common/frame_owner_properties.h:15: // Used for IPC transport of
WebFrameOwnerProperties.
On 2016/07/14 23:44:13, alexmos wrote:
> Maybe expand this comment to explain why WebFrameOwnerProperties can't be
> transported directly?

Done.

https://codereview.chromium.org/2146803004/diff/40001/content/content_common....
File content/content_common.gypi (right):

https://codereview.chromium.org/2146803004/diff/40001/content/content_common....
content/content_common.gypi:330: 'common/frame_owner_properties.h',
On 2016/07/14 23:44:14, alexmos wrote:
> nit: put after frame_messages.h to preserve sort order.

Done.

[raymes, 2016-07-18 03:43:25]

raymes@chromium.org changed reviewers:
+ esprehn@chromium.org, nasko@chromium.org

[raymes, 2016-07-18 03:43:26]

+OWNERS

esprehn for
third_party/WebKit/public/platform/modules/permissions/WebPermissionType.h

nasko for 
content/common/frame_messages.h
content/common/frame_owner_properties.cc 
content/common/frame_owner_properties.h

[raymes, 2016-07-18 03:43:36]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 03:43:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 03:43:46]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-18 04:14:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 04:14:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2016-07-18 04:44:37]

The CQ bit was checked by raymes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 04:44:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 04:44:45]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-18 05:56:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 05:56:56]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-07-18 21:59:15]

If we are creating a separate type for content/, I'd rather see it used
everywhere we are passing it browser side. Doing conversions in the browser
process doesn't make much sense to me.

https://codereview.chromium.org/2146803004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2146803004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1671:
properties.ToWebFrameOwnerProperties();
Can we avoid this conversion? Why not pass through FrameOwnerProperties directly
to RFHM?

https://codereview.chromium.org/2146803004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2146803004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:68: struct
WebFrameOwnerProperties;
Why do we still need this? The browser side shouldn't ever see the Web* object,
should it?

[esprehn, 2016-07-18 22:19:56]

lgtm

[nasko, 2016-07-19 00:13:01]

I was told that the "use everywhere" is a separate CL -
https://codereview.chromium.org/2156883002/. Since that's the case, this one
LGTM with the nit from my previous comment.

[raymes, 2016-07-19 00:22:00]

Thanks!

https://codereview.chromium.org/2146803004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2146803004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1671:
properties.ToWebFrameOwnerProperties();
On 2016/07/18 21:59:15, nasko wrote:
> Can we avoid this conversion? Why not pass through FrameOwnerProperties
directly
> to RFHM?

I've changed this to do that in https://codereview.chromium.org/2156883002/

https://codereview.chromium.org/2146803004/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2146803004/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:68: struct
WebFrameOwnerProperties;
On 2016/07/18 21:59:15, nasko wrote:
> Why do we still need this? The browser side shouldn't ever see the Web*
object,
> should it?

It's still used right now in OnCreateChildFrame but that will change in
https://codereview.chromium.org/2156883002/

[raymes, 2016-07-19 00:22:10]

The CQ bit was checked by raymes@chromium.org

[raymes, 2016-07-19 00:22:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexmos@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2146803004/#ps80001
(title: "Transport")

[commit-bot: I haz the power, 2016-07-19 00:23:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 00:23:11]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-19 00:42:15]

Description was changed from

==========
Create a content::FrameOwnerProperties struct for IPC transport of
WebFrameOwnerProperties

blink::WebFrameOwnerProperties can't be sent over IPC because one of its members
is a WebVector which doesn't have ParamTraits defined. We introduce a new struct
in content to transfer the data.

BUG=614608
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Create a content::FrameOwnerProperties struct for IPC transport of
WebFrameOwnerProperties

blink::WebFrameOwnerProperties can't be sent over IPC because one of its members
is a WebVector which doesn't have ParamTraits defined. We introduce a new struct
in content to transfer the data.

BUG=614608
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-07-19 00:42:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-19 00:44:32]

Description was changed from

==========
Create a content::FrameOwnerProperties struct for IPC transport of
WebFrameOwnerProperties

blink::WebFrameOwnerProperties can't be sent over IPC because one of its members
is a WebVector which doesn't have ParamTraits defined. We introduce a new struct
in content to transfer the data.

BUG=614608
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Create a content::FrameOwnerProperties struct for IPC transport of
WebFrameOwnerProperties

blink::WebFrameOwnerProperties can't be sent over IPC because one of its members
is a WebVector which doesn't have ParamTraits defined. We introduce a new struct
in content to transfer the data.

BUG=614608
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/bba82b368739017314eac8bed4ae8c5533975bf5
Cr-Commit-Position: refs/heads/master@{#406163}
==========

[commit-bot: I haz the power, 2016-07-19 00:44:33]

Patchset 5 (id:??) landed as
https://crrev.com/bba82b368739017314eac8bed4ae8c5533975bf5
Cr-Commit-Position: refs/heads/master@{#406163}