Propagate window coordinates to out-of-process iframes renderers.

Propagate window coordinates to out-of-process iframes renderers.

This change also adds a Page message IPC class, which can be
used by an interface in
WebContentsImpl::SendPageMessage(IPC::Message*) to send an IPC
message to every renderer in the FrameTree.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[lfg, 2016-02-10 19:37:13]

Description was changed from

==========
WIP
==========

to

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

[nasko, 2016-02-11 18:21:27]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2016-02-11 18:21:28]

A drive-by turned full review :).

https://codereview.chromium.org/1685213002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1685213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:795: int
WebContentsImpl::SendToAllViews(IPC::Message* msg) {
Sending to all "Views" sounds like the wrong abstraction. We are sending a Page
message and it is an implementation detail that we can have multiple objects for
the same Page in different processes. SendPageMsg?

https://codereview.chromium.org/1685213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:798:
frame_tree_.root()->render_manager()->GetAllProxyHosts();
I'd avoid exposing the proxies from RFHM. This method here can just be a caller
of RFHM::SendToAll or something similar, which then will hide the implementation
details of RFPH map and the rest.

https://codereview.chromium.org/1685213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:809:
frame_tree_.root()->render_manager()->current_frame_host();
If we have a pending/speculative RFH, shouldn't we be sending it to that one as
well?

https://codereview.chromium.org/1685213002/diff/40001/content/common/page_mes...
File content/common/page_messages.h (right):

https://codereview.chromium.org/1685213002/diff/40001/content/common/page_mes...
content/common/page_messages.h:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
styleL: No "(c)".

https://codereview.chromium.org/1685213002/diff/40001/content/common/page_mes...
content/common/page_messages.h:7: // IPC messages for page-level concepts.
nit: s/concepts/actions/

https://codereview.chromium.org/1685213002/diff/40001/content/public/browser/...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/1685213002/diff/40001/content/public/browser/...
content/public/browser/site_instance.h:77: virtual int32_t GetId() const = 0;
Per content/ API design, no const usage in the public API.

https://codereview.chromium.org/1685213002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1685213002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.h:35: #include "ipc/ipc_message_macros.h"
This shouldn't be needed.

https://codereview.chromium.org/1685213002/diff/40001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1685213002/diff/40001/ipc/ipc_message_macros....
ipc/ipc_message_macros.h:405: #define IPC_MESSAGE_CLASS(message)
IPC_MESSAGE_ID_CLASS((message).type())
Why is this change needed?

[lfg, 2016-02-11 20:25:58]

Description was changed from

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

to

==========
Propagate window coordinates to out-of-process iframes renderers.

This change also adds a Page message IPC class, which can be
used by an interface in
WebContentsImpl::SendPageMsg(IPC::Message*) to send an IPC
message to every renderer in the FrameTree.

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

[lfg, 2016-02-11 20:26:09]

Nasko, please take a look.

https://codereview.chromium.org/1685213002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1685213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:795: int
WebContentsImpl::SendToAllViews(IPC::Message* msg) {
On 2016/02/11 18:21:28, nasko wrote:
> Sending to all "Views" sounds like the wrong abstraction. We are sending a
Page
> message and it is an implementation detail that we can have multiple objects
for
> the same Page in different processes. SendPageMsg?

Done.

https://codereview.chromium.org/1685213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:798:
frame_tree_.root()->render_manager()->GetAllProxyHosts();
On 2016/02/11 18:21:28, nasko wrote:
> I'd avoid exposing the proxies from RFHM. This method here can just be a
caller
> of RFHM::SendToAll or something similar, which then will hide the
implementation
> details of RFPH map and the rest.

Done.

https://codereview.chromium.org/1685213002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:809:
frame_tree_.root()->render_manager()->current_frame_host();
On 2016/02/11 18:21:28, nasko wrote:
> If we have a pending/speculative RFH, shouldn't we be sending it to that one
as
> well?

Yes, we should, because if/when we commit, we need that information to be up to
date. Fixed that.

https://codereview.chromium.org/1685213002/diff/40001/content/common/page_mes...
File content/common/page_messages.h (right):

https://codereview.chromium.org/1685213002/diff/40001/content/common/page_mes...
content/common/page_messages.h:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
On 2016/02/11 18:21:28, nasko wrote:
> styleL: No "(c)".

Done.

https://codereview.chromium.org/1685213002/diff/40001/content/common/page_mes...
content/common/page_messages.h:7: // IPC messages for page-level concepts.
On 2016/02/11 18:21:28, nasko wrote:
> nit: s/concepts/actions/

Done.

https://codereview.chromium.org/1685213002/diff/40001/content/public/browser/...
File content/public/browser/site_instance.h (right):

https://codereview.chromium.org/1685213002/diff/40001/content/public/browser/...
content/public/browser/site_instance.h:77: virtual int32_t GetId() const = 0;
On 2016/02/11 18:21:28, nasko wrote:
> Per content/ API design, no const usage in the public API.

Done.

https://codereview.chromium.org/1685213002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1685213002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.h:35: #include "ipc/ipc_message_macros.h"
On 2016/02/11 18:21:28, nasko wrote:
> This shouldn't be needed.

Done.

https://codereview.chromium.org/1685213002/diff/40001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/1685213002/diff/40001/ipc/ipc_message_macros....
ipc/ipc_message_macros.h:405: #define IPC_MESSAGE_CLASS(message)
IPC_MESSAGE_ID_CLASS((message).type())
On 2016/02/11 18:21:28, nasko wrote:
> Why is this change needed?

Since '.' has precedence over '*', without this I can't write
IPC_MESSAGE_CLASS(*msg), I would have to write IPC_MESSAGE_CLASS((*msg)). I
believe this is worth it since it improves readability.

[nasko, 2016-02-12 18:14:08]

Looks good overall, but I'd rather not change the proxy_hosts_ container type in
this CL, as it is orthogonal to what you are doing here.

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

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1019:
proxy_hosts_[site_instance_id] = make_scoped_ptr(proxy_host);
Why was this rewrite necessary?

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2488:
DCHECK(IPC_MESSAGE_CLASS(*msg) == PageMsgStart);
Should this method return if the IPC is not a PageMsg one?

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2491:
DCHECK(!frame_tree_node_->parent());
Same as above. What happens when the DCHECK is violated, but it is a release
build?

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2507: if
(pending_render_frame_host_) {
Speculative and pending cannot exist at the same time, so it should be an
if/else case.

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

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:489: // Sends an IPC
message to every RenderView in the FrameTree.
nit: Let's drop the RenderView part, as while technically correct, this object
will soon disappear and we are actually sending it to all the frame
representations in each process that renders this frame tree.

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:490: void
SendPageMsg(IPC::Message* msg);
nit: Sorry, should've been more correct. SendPageMessage (shouldn't be
abbreviated).

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:763:
std::unordered_map<int32_t, scoped_ptr<RenderFrameProxyHost>> proxy_hosts_;
Why make this change in this CL?

https://codereview.chromium.org/1685213002/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1685213002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:5276: int root_value = 1;
Are the tests always positioned the same way? I'm wary of this becoming a flaky
test. Can we explicitly position the window first, then test the expectations?

https://codereview.chromium.org/1685213002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1685213002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:976: void
SendPageMsg(IPC::Message* msg);
nit: Same as in RFHM - SendPageMessage.

https://codereview.chromium.org/1685213002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1685213002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:1356: if ((IPC_MESSAGE_CLASS(msg) ==
PageMsgStart) && render_view())
If there is no render_view(), this will deliver the PageMsg IPC to observers
below, right? That sounds wrong and I think it should just return in that case.

[lfg, 2016-03-02 18:29:08]

PTAL.

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

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1019:
proxy_hosts_[site_instance_id] = make_scoped_ptr(proxy_host);
On 2016/02/12 18:14:08, nasko wrote:
> Why was this rewrite necessary? 

Moved to another CL.

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2488:
DCHECK(IPC_MESSAGE_CLASS(*msg) == PageMsgStart);
On 2016/02/12 18:14:08, nasko wrote:
> Should this method return if the IPC is not a PageMsg one?

Done.

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2491:
DCHECK(!frame_tree_node_->parent());
On 2016/02/12 18:14:08, nasko wrote:
> Same as above. What happens when the DCHECK is violated, but it is a release
> build?

We shouldn't write code to violate the DCHECK ;)

But it's probably a good idea to just return if it's violated. Done.

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:2507: if
(pending_render_frame_host_) {
On 2016/02/12 18:14:08, nasko wrote:
> Speculative and pending cannot exist at the same time, so it should be an
> if/else case.

Done.

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

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:489: // Sends an IPC
message to every RenderView in the FrameTree.
On 2016/02/12 18:14:08, nasko wrote:
> nit: Let's drop the RenderView part, as while technically correct, this object
> will soon disappear and we are actually sending it to all the frame
> representations in each process that renders this frame tree.

Done.

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:490: void
SendPageMsg(IPC::Message* msg);
On 2016/02/12 18:14:08, nasko wrote:
> nit: Sorry, should've been more correct. SendPageMessage (shouldn't be
> abbreviated).

Done.

https://codereview.chromium.org/1685213002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:763:
std::unordered_map<int32_t, scoped_ptr<RenderFrameProxyHost>> proxy_hosts_;
On 2016/02/12 18:14:08, nasko wrote:
> Why make this change in this CL?

Moved to another CL.

https://codereview.chromium.org/1685213002/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1685213002/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:5276: int root_value = 1;
On 2016/02/12 18:14:08, nasko wrote:
> Are the tests always positioned the same way? I'm wary of this becoming a
flaky
> test. Can we explicitly position the window first, then test the expectations?

This shouldn't be an issue. The important point of the test is testing that
window.{screenX,screenY,outerWidget,outerHeight} all return the same values
regardless if you are in the iframe or the top level page.

https://codereview.chromium.org/1685213002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/1685213002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:976: void
SendPageMsg(IPC::Message* msg);
On 2016/02/12 18:14:08, nasko wrote:
> nit: Same as in RFHM - SendPageMessage.

Done.

https://codereview.chromium.org/1685213002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1685213002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:1356: if ((IPC_MESSAGE_CLASS(msg) ==
PageMsgStart) && render_view())
On 2016/02/12 18:14:08, nasko wrote:
> If there is no render_view(), this will deliver the PageMsg IPC to observers
> below, right? That sounds wrong and I think it should just return in that
case.

Yes, you are right. Changed.

[lfg, 2016-03-02 18:36:42]

Description was changed from

==========
Propagate window coordinates to out-of-process iframes renderers.

This change also adds a Page message IPC class, which can be
used by an interface in
WebContentsImpl::SendPageMsg(IPC::Message*) to send an IPC
message to every renderer in the FrameTree.

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

to

==========
Propagate window coordinates to out-of-process iframes renderers.

This change also adds a Page message IPC class, which can be
used by an interface in
WebContentsImpl::SendPageMessage(IPC::Message*) to send an IPC
message to every renderer in the FrameTree.

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

[nasko, 2016-03-03 18:28:57]

LGTM with a few nits.

https://codereview.chromium.org/1685213002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1685213002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2541:
pending_render_frame_host_->Send(copy);
These 3 lines are repeating 3 times in this method. Why not create a local
vector of all routing ids (all proxies, current, speculative, pending) and then
iterate once?

https://codereview.chromium.org/1685213002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1685213002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:490: // Sends an IPC
message to every process in the FrameTree.
Let's mention in the comment that it is only supported for the main frame RFHM.

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/devto...
File content/renderer/devtools/render_widget_screen_metrics_emulator.h (right):

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/devto...
content/renderer/devtools/render_widget_screen_metrics_emulator.h:57: void
OnUpdateWindowScreenRectMessage(const gfx::Rect& window_screen_rect);
nit: No need for Message suffix, all IPC handlers by design are handing
messages.

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:258: return
render_view()->OnMessageReceived(msg);
Why not use the same structure as in RenderFrameImpl? In this case, if there is
no RenderView, the message will fall through the local message map. Is this
desirable?

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/rende...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/rende...
content/renderer/render_view_impl.h:679: void OnUpdateWindowScreenRect(gfx::Rect
window_screen_rect);
Let's move this towards the end of this section and put a comment that the ones
that follow are page-level messages. Ideally, only those will stay in this
object in the future.

[lfg, 2016-03-03 22:09:38]

https://codereview.chromium.org/1685213002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1685213002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:2541:
pending_render_frame_host_->Send(copy);
On 2016/03/03 18:28:57, nasko wrote:
> These 3 lines are repeating 3 times in this method. Why not create a local
> vector of all routing ids (all proxies, current, speculative, pending) and
then
> iterate once?

I've switched to use a lambda, let me know what you think. There was also a leak
on the return at the top of the function that I fixed.

https://codereview.chromium.org/1685213002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1685213002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:490: // Sends an IPC
message to every process in the FrameTree.
On 2016/03/03 18:28:57, nasko wrote:
> Let's mention in the comment that it is only supported for the main frame
RFHM.

Done.

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/devto...
File content/renderer/devtools/render_widget_screen_metrics_emulator.h (right):

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/devto...
content/renderer/devtools/render_widget_screen_metrics_emulator.h:57: void
OnUpdateWindowScreenRectMessage(const gfx::Rect& window_screen_rect);
On 2016/03/03 18:28:57, nasko wrote:
> nit: No need for Message suffix, all IPC handlers by design are handing
> messages.

Done.

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:258: return
render_view()->OnMessageReceived(msg);
On 2016/03/03 18:28:57, nasko wrote:
> Why not use the same structure as in RenderFrameImpl? In this case, if there
is
> no RenderView, the message will fall through the local message map. Is this
> desirable?

Done.

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/rende...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/1685213002/diff/140001/content/renderer/rende...
content/renderer/render_view_impl.h:679: void OnUpdateWindowScreenRect(gfx::Rect
window_screen_rect);
On 2016/03/03 18:28:57, nasko wrote:
> Let's move this towards the end of this section and put a comment that the
ones
> that follow are page-level messages. Ideally, only those will stay in this
> object in the future.

Done.

[lfg, 2016-03-03 22:12:03]

lfg@chromium.org changed reviewers:
+ tsepez@chromium.org

[lfg, 2016-03-03 22:12:04]

tsepez@chromium.org: Please review changes in 

ipc/ipc_message_macros.h

[Tom Sepez, 2016-03-03 22:16:46]

IPC lgtm

[lfg, 2016-03-04 21:03:54]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-03-04 21:03:55]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/1685213002/#ps200001
(title: "properly rebasing")

[commit-bot: I haz the power, 2016-03-04 21:04:12]

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

[commit-bot: I haz the power, 2016-03-04 23:09:15]

Description was changed from

==========
Propagate window coordinates to out-of-process iframes renderers.

This change also adds a Page message IPC class, which can be
used by an interface in
WebContentsImpl::SendPageMessage(IPC::Message*) to send an IPC
message to every renderer in the FrameTree.

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

to

==========
Propagate window coordinates to out-of-process iframes renderers.

This change also adds a Page message IPC class, which can be
used by an interface in
WebContentsImpl::SendPageMessage(IPC::Message*) to send an IPC
message to every renderer in the FrameTree.

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

[commit-bot: I haz the power, 2016-03-04 23:09:16]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-04 23:10:40]

Description was changed from

==========
Propagate window coordinates to out-of-process iframes renderers.

This change also adds a Page message IPC class, which can be
used by an interface in
WebContentsImpl::SendPageMessage(IPC::Message*) to send an IPC
message to every renderer in the FrameTree.

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

to

==========
Propagate window coordinates to out-of-process iframes renderers.

This change also adds a Page message IPC class, which can be
used by an interface in
WebContentsImpl::SendPageMessage(IPC::Message*) to send an IPC
message to every renderer in the FrameTree.

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-03-04 23:10:42]

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