Vector-of-structs (instead of struct-of-vectors) in "savable resources" IPC.

Vector-of-structs (instead of struct-of-vectors) in "savable resources" IPC.

It is cleaner to send a vector-of-structs rather than a struct-of-vectors
(i.e. no need to verify the size of the vectors is the same, iteration
is easier as it doesn't require an index to look into both vectors).

This CL changes how "subframes" are sent in
FrameHostMsg_SavableResourceLinksResponse.  They used to be sent as 2
vectors (of URLs and of routing_ids) and now they are sent as a single
vector (of a struct that contains URLs and routing_ids).

The same struct (content::SavableSubframe) is reused to return results
out of GetSavableResourceLinksForFrame in savable_resources.cc.  The
decision to reuse the same struct necessitates
1) sharing/reusing GetRoutingIdForFrameOrProxy across
   render_frame_impl.cc and savable_resources.cc and (so
   GetRoutingIdForFrameOrProxy has been moved to web_frame_utils
   compilation unit)
2) introducing a content::SavableSubframe struct that can be shared
   via A) IPC handlers in browser and renderer and B) by savable_resources.cc
   (and addressing B seemed better with an explicit struct with a short
   name, rather than introducing an implicit struct via IPC_STRUCT_BEGIN).

Note that this CL keeps using a struct-of-vectors for reporting savable
resources (URL + referrer).  This will be addressed in a separate CL
(crrev.com/1425013004).

BUG=526786
Committed: https://crrev.com/779a08f834625617d79a14be44871bbc84ab2574
Cr-Commit-Position: refs/heads/master@{#357852}

[Łukasz Anforowicz, 2015-10-28 17:02:28]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org, nick@chromium.org

[Łukasz Anforowicz, 2015-10-28 17:02:28]

Daniel + Nick - could you please take a look?

[Łukasz Anforowicz, 2015-10-28 17:02:40]

Description was changed from

==========
Vector-of-structs (instead of struct-of-vectors) in "savable resources" IPC.

It is cleaner to send a vector-of-structs rather than a struct-of-vectors
(i.e. no need to verify the size of the vectors is the same, iteration
is easier as it doesn't require an index to look into both vectors).

This CL changes how "subframes" are sent in
FrameHostMsg_SavableResourceLinksResponse.  They used to be sent as 2
vectors (of URLs and of routing_ids) and now they are sent as a single
vector (of a struct that contains URLs and routing_ids).

The same struct (content::SavableSubframe) is reused to return results
out of GetSavableResourceLinksForFrame in savable_resources.cc.  The
decision to reuse the same struct necessitates
1) sharing/reusing GetRoutingIdForFrameOrProxy across
   render_frame_impl.cc and savable_resources.cc and (so
   GetRoutingIdForFrameOrProxy has been moved to web_frame_utils
   compilation unit)
2) introducing a content::SavableSubframe struct that can be shared
   via A) IPC handlers in browser and renderer and B) by savable_resources.cc
   (and addressing B seemed better with an explicit struct with a short
   name, rather than introducing an implicit struct via IPC_STRUCT_BEGIN).

Note that this CL keeps using a struct-of-vectors for reporting savable
resources (URL + referrer).  This will be addressed in a separate CL.

BUG=526786
==========

to

==========
Vector-of-structs (instead of struct-of-vectors) in "savable resources" IPC.

It is cleaner to send a vector-of-structs rather than a struct-of-vectors
(i.e. no need to verify the size of the vectors is the same, iteration
is easier as it doesn't require an index to look into both vectors).

This CL changes how "subframes" are sent in
FrameHostMsg_SavableResourceLinksResponse.  They used to be sent as 2
vectors (of URLs and of routing_ids) and now they are sent as a single
vector (of a struct that contains URLs and routing_ids).

The same struct (content::SavableSubframe) is reused to return results
out of GetSavableResourceLinksForFrame in savable_resources.cc.  The
decision to reuse the same struct necessitates
1) sharing/reusing GetRoutingIdForFrameOrProxy across
   render_frame_impl.cc and savable_resources.cc and (so
   GetRoutingIdForFrameOrProxy has been moved to web_frame_utils
   compilation unit)
2) introducing a content::SavableSubframe struct that can be shared
   via A) IPC handlers in browser and renderer and B) by savable_resources.cc
   (and addressing B seemed better with an explicit struct with a short
   name, rather than introducing an implicit struct via IPC_STRUCT_BEGIN).

Note that this CL keeps using a struct-of-vectors for reporting savable
resources (URL + referrer).  This will be addressed in a separate CL.

BUG=526786
==========

[ncarter (slow), 2015-10-28 22:28:39]

lgtm

https://codereview.chromium.org/1422473004/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1422473004/diff/1/content/common/frame_messag...
content/common/frame_messages.h:402:
IPC_STRUCT_TRAITS_BEGIN(content::SavableSubframe)
Just mentioning this as an option in case you hadn't considered it: you could
have done this with IPC_STRUCT_BEGIN (instead of TRAITS_BEGIN) to define the
struct without predeclaring it -- such a struct would probably be named
FrameHostMsg_SavableSubframe.

I'm fine with either approach, it's a tradeoff between "less boilerplate" and
"less ipc magic".

https://codereview.chromium.org/1422473004/diff/1/content/common/savable_subf...
File content/common/savable_subframe.h (right):

https://codereview.chromium.org/1422473004/diff/1/content/common/savable_subf...
content/common/savable_subframe.h:9: 
This file needs to be added to content_common.gypi

https://codereview.chromium.org/1422473004/diff/1/content/renderer/web_frame_...
File content/renderer/web_frame_utils.cc (right):

https://codereview.chromium.org/1422473004/diff/1/content/renderer/web_frame_...
content/renderer/web_frame_utils.cc:14: return MSG_ROUTING_NONE;
Need an #include for MSG_ROUTING_NONE

https://codereview.chromium.org/1422473004/diff/1/content/renderer/web_frame_...
content/renderer/web_frame_utils.cc:15: if (web_frame->isWebRemoteFrame())
Need an #include for WebFrame.

[ncarter (slow), 2015-10-28 22:29:34]

https://codereview.chromium.org/1422473004/diff/1/content/common/frame_messag...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1422473004/diff/1/content/common/frame_messag...
content/common/frame_messages.h:402:
IPC_STRUCT_TRAITS_BEGIN(content::SavableSubframe)
On 2015/10/28 22:28:38, ncarter wrote:
> Just mentioning this as an option in case you hadn't considered it: you could
> have done this with IPC_STRUCT_BEGIN (instead of TRAITS_BEGIN) to define the
> struct without predeclaring it -- such a struct would probably be named
> FrameHostMsg_SavableSubframe.
> 
> I'm fine with either approach, it's a tradeoff between "less boilerplate" and
> "less ipc magic".

Duh, you totally explained this in your commit comment. Disregard my comment.

[Łukasz Anforowicz, 2015-10-28 23:12:06]

Thanks for the feedback and apologies for rushing this CL a little bit (missing
header file in contents_common.gypi + includes in web_frame_utils.cc didn't
affect the build but are kind of obvious issues in retrospect...).  And I should
have built and run content_browsertests in addition to browser_tests - this
revealed that I also need to make some tweaks to fix SavebleResourcesTest
(addressed in patchset #2).

https://codereview.chromium.org/1422473004/diff/1/content/common/savable_subf...
File content/common/savable_subframe.h (right):

https://codereview.chromium.org/1422473004/diff/1/content/common/savable_subf...
content/common/savable_subframe.h:9: 
On 2015/10/28 22:28:38, ncarter wrote:
> This file needs to be added to content_common.gypi

Done.

https://codereview.chromium.org/1422473004/diff/1/content/renderer/web_frame_...
File content/renderer/web_frame_utils.cc (right):

https://codereview.chromium.org/1422473004/diff/1/content/renderer/web_frame_...
content/renderer/web_frame_utils.cc:14: return MSG_ROUTING_NONE;
On 2015/10/28 22:28:39, ncarter wrote:
> Need an #include for MSG_ROUTING_NONE

Done.

https://codereview.chromium.org/1422473004/diff/1/content/renderer/web_frame_...
content/renderer/web_frame_utils.cc:15: if (web_frame->isWebRemoteFrame())
On 2015/10/28 22:28:38, ncarter wrote:
> Need an #include for WebFrame.

Done.

[ncarter (slow), 2015-10-28 23:25:12]

ps1 vs ps3 lgtm

[dcheng, 2015-10-29 00:23:58]

lgtm

[Łukasz Anforowicz, 2015-11-04 17:26:29]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-11-04 17:26:29]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1422473004/#ps60001
(title: "Rebasing...")

[commit-bot: I haz the power, 2015-11-04 17:27:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422473004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422473004/60001

[commit-bot: I haz the power, 2015-11-04 18:27:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-04 18:27:54]

Patchset 4 (id:??) landed as
https://crrev.com/779a08f834625617d79a14be44871bbc84ab2574
Cr-Commit-Position: refs/heads/master@{#357852}