Position autofill popup widgets correctly under --site-per-process

Position autofill popup widgets correctly under --site-per-process

When a AutofillHostMsg_QueryFormFieldAutofill message is sent from
an out-of-process iframe, its screen coordinates are relative to the
subframe's rect and not the top-level frame's viewport.

This CL exposes a facility in the content API to perform the necessary
transformation, particularly:
- it promotes GetView() from RenderFrameHostImpl to RenderFrameHost, so the RenderWidgetHostView becomes available to the embedder, and
- it promotes TransformPointToRootCoordSpace from RenderWidgetHostViewBase to RenderWidgetHostView, so that chrome/ or components/ code that directly receives coordinates from a renderer process can ensure they properly transformed

Finally, it causes the ContentAutofillDriver to use the exposed methods to transform received autofill popup coordinates in order for them to display in the correct position.

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

Committed: https://crrev.com/015b5b5829406934f9863c73f931815ae5007e8f
Cr-Commit-Position: refs/heads/master@{#371519}

[kenrb, 2016-01-20 21:25:38]

Description was changed from

==========
Position autofill popup widgets correctly under --site-per-process

When a AutofillHostMsg_QueryFormFieldAutofill message is sent from an
out-of-process iframe, its screen coordinates are relative to the
subframe's rect and not the top-level frame's viewport.

This CL provides a facility in the content API to perform the necessary
transformation, and transforms the received coordinates in the browser
process in order for autofill popups to display in the correct position.

BUG=554119
==========

to

==========
Position autofill popup widgets correctly under --site-per-process

When a AutofillHostMsg_QueryFormFieldAutofill message is sent from
an out-of-process iframe, its screen coordinates are relative to the
subframe's rect and not the top-level frame's viewport.

This CL exposes a facility in the content API to perform the necessary
transformation, particularly:
- it promotes GetView() from RenderFrameHostImpl to RenderFrameHost, so the
RenderWidgetHostView becomes available to the embedder, and
- it promotes TransformPointToRootCoordSpace from RenderWidgetHostViewBase to
RenderWidgetHostView, so that chrome/ or components/ code that directly receives
coordinates from a renderer process can ensure they properly transformed

Finally, it causes the ContentAutofillDriver to use the exposed methods to
transform received autofill popup coordinates in order for them to display in
the correct position.

BUG=554119
==========

[kenrb, 2016-01-20 21:25:38]

kenrb@chromium.org changed reviewers:
+ creis@chromium.org, isherman@chromium.org

[kenrb, 2016-01-20 21:38:43]

PTAL?
- Ilya for components/autofill and the autofill driver
- Charlie for content/, in particular the API changes

I haven't added a test, it doesn't seem like an easy thing to do because popup
coordinates are generated by the renderer process and the transformation code I
am adding here is all in the browser. The transform code within content has
tests with regard to context menus. I haven't found tests doing anything similar
for autofill.

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:157: orig_point,
&transformed_point);
There is a loss of precision here, is this a real problem?

The gfx::Transform class only operates on Point and Point3_f, not PointF, so
there isn't an easy way to plumb transformation all the way through the hit
testing code for PointF. Accordingly, we are transforming a discrete Point
rather than a PointF, and converting back and forth. It's not clear to me that
this has any noticeable effect on the resulting position of the popup.

[Ilya Sherman, 2016-01-20 23:12:15]

Thanks for the CL!

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:157: orig_point,
&transformed_point);
On 2016/01/20 21:38:43, kenrb wrote:
> There is a loss of precision here, is this a real problem?
> 
> The gfx::Transform class only operates on Point and Point3_f, not PointF, so
> there isn't an easy way to plumb transformation all the way through the hit
> testing code for PointF. Accordingly, we are transforming a discrete Point
> rather than a PointF, and converting back and forth. It's not clear to me that
> this has any noticeable effect on the resulting position of the popup.

I'm not sure -- I'd recommend checking with someone super familiar with the gfx
system.

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:157: orig_point,
&transformed_point);
nit: Can TransformPointToRootCoordSpace just return the transformed point? 
That's a slightly simpler API.

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_driver.h:98: virtual gfx::RectF
TransformBoundingBox(const gfx::RectF& bounding_box) = 0;
I think it might be nice to name this somehow so that it more clearly indicates
what sort of transformation is happening, i.e. what coordinate space is being
transformed from and to.  Perhaps naming it "TransformToScreenCoordinates" would
be clearer?  I'm not really sure what sorts of conventions there are for this
sort of thing, but I'm sure there are some.

[Charlie Reis, 2016-01-21 00:33:03]

Mostly nits, with one sanity check question about the API.

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
File components/autofill/core/browser/test_autofill_driver.cc (right):

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
components/autofill/core/browser/test_autofill_driver.cc:75: gfx::RectF
TestAutofillDriver::TransformBoundingBox(
Is there any way to test the new API, either here or in content/?

https://codereview.chromium.org/1526263003/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1526263003/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:252: RenderWidgetHostView*
GetView() override;
nit: This needs to move into the RenderViewHost override section.  Also, this
comment would be great for the public API.

https://codereview.chromium.org/1526263003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1526263003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:91: // This only
needs to be overridden by RenderWidgetHostView subclasses
Do you mean RenderWidgetHostViewBase subclasses?

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_frame_host.h:192: virtual RenderWidgetHostView*
GetView() = 0;
nit: Add comment, probably taken from render_frame_host_impl.h.  Also move this
up near the other getters (maybe below GetProcess).

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_widget_host_view.h:70: // that of another
RenderWidgetHost), this provides a facility to convert
Maybe drop "that of"?

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_widget_host_view.h:71: // a point from its own
coordinate space to that of the root frame.
Is this a no-op if you call it on the root RWHV?  (Maybe mention that.)

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_widget_host_view.h:72: virtual void
TransformPointToRootCoordSpace(
Sanity check: I don't suppose it would be possible to hide this detail and only
expose transformed points from RenderWidgetHost's APIs?  It seems like we'll be
playing whack-a-mole with all the calls to APIs that expose points until we
transform them all, and we'll likely miss future call sites.

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_widget_host_view.h:74: gfx::Point*
transformed_point) = 0;
+1 to returning the result if possible, rather than having an out parameter.

[kenrb, 2016-01-22 18:22:04]

Description was changed from

==========
Position autofill popup widgets correctly under --site-per-process

When a AutofillHostMsg_QueryFormFieldAutofill message is sent from
an out-of-process iframe, its screen coordinates are relative to the
subframe's rect and not the top-level frame's viewport.

This CL exposes a facility in the content API to perform the necessary
transformation, particularly:
- it promotes GetView() from RenderFrameHostImpl to RenderFrameHost, so the
RenderWidgetHostView becomes available to the embedder, and
- it promotes TransformPointToRootCoordSpace from RenderWidgetHostViewBase to
RenderWidgetHostView, so that chrome/ or components/ code that directly receives
coordinates from a renderer process can ensure they properly transformed

Finally, it causes the ContentAutofillDriver to use the exposed methods to
transform received autofill popup coordinates in order for them to display in
the correct position.

BUG=554119
==========

to

==========
Position autofill popup widgets correctly under --site-per-process

When a AutofillHostMsg_QueryFormFieldAutofill message is sent from
an out-of-process iframe, its screen coordinates are relative to the
subframe's rect and not the top-level frame's viewport.

This CL exposes a facility in the content API to perform the necessary
transformation, particularly:
- it promotes GetView() from RenderFrameHostImpl to RenderFrameHost, so the
RenderWidgetHostView becomes available to the embedder, and
- it promotes TransformPointToRootCoordSpace from RenderWidgetHostViewBase to
RenderWidgetHostView, so that chrome/ or components/ code that directly receives
coordinates from a renderer process can ensure they properly transformed

Finally, it causes the ContentAutofillDriver to use the exposed methods to
transform received autofill popup coordinates in order for them to display in
the correct position.

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

[kenrb, 2016-01-22 18:33:59]

Thanks for the reviews. Updated... PTAL?

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:157: orig_point,
&transformed_point);
On 2016/01/20 23:12:15, Ilya Sherman wrote:
> nit: Can TransformPointToRootCoordSpace just return the transformed point? 
> That's a slightly simpler API.

Done.

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:157: orig_point,
&transformed_point);
On 2016/01/20 23:12:15, Ilya Sherman wrote:
> On 2016/01/20 21:38:43, kenrb wrote:
> > There is a loss of precision here, is this a real problem?
> > 
> > The gfx::Transform class only operates on Point and Point3_f, not PointF, so
> > there isn't an easy way to plumb transformation all the way through the hit
> > testing code for PointF. Accordingly, we are transforming a discrete Point
> > rather than a PointF, and converting back and forth. It's not clear to me
that
> > this has any noticeable effect on the resulting position of the popup.
> 
> I'm not sure -- I'd recommend checking with someone super familiar with the
gfx
> system.

I talked to vollick@, and he thinks it should be fine since subframes and parent
frames will always have the same zoom level, but says we the values need to be
rounded rather than truncated. Accordingly, I have uploaded a patch that adds a
TransformPointToRootCoordSpaceF API that does this.

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
File components/autofill/core/browser/autofill_driver.h (right):

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_driver.h:98: virtual gfx::RectF
TransformBoundingBox(const gfx::RectF& bounding_box) = 0;
On 2016/01/20 23:12:15, Ilya Sherman wrote:
> I think it might be nice to name this somehow so that it more clearly
indicates
> what sort of transformation is happening, i.e. what coordinate space is being
> transformed from and to.  Perhaps naming it "TransformToScreenCoordinates"
would
> be clearer?  I'm not really sure what sorts of conventions there are for this
> sort of thing, but I'm sure there are some.

I think the new name is accurate.

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
File components/autofill/core/browser/test_autofill_driver.cc (right):

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
components/autofill/core/browser/test_autofill_driver.cc:75: gfx::RectF
TestAutofillDriver::TransformBoundingBox(
On 2016/01/21 00:33:03, Charlie Reis wrote:
> Is there any way to test the new API, either here or in content/?

The API is tested implicitly by SitePerProcessBrowserTest.CreateContextMenuTest,
which uses the TransformPointToRootCoordSpace in the same way as we use it here.
Context menus are easier to test this with because the browser can provide the
coordinates of the mouse click that generates the context menu (and then verify
that they have been converted back to the original coords), whereas for popups
the coordinates come from the page content and I have had trouble finding a way
to know what the correct values are.

https://codereview.chromium.org/1526263003/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1526263003/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:252: RenderWidgetHostView*
GetView() override;
On 2016/01/21 00:33:03, Charlie Reis wrote:
> nit: This needs to move into the RenderViewHost override section.  Also, this
> comment would be great for the public API.

Done.

https://codereview.chromium.org/1526263003/diff/120001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/1526263003/diff/120001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:91: // This only
needs to be overridden by RenderWidgetHostView subclasses
On 2016/01/21 00:33:03, Charlie Reis wrote:
> Do you mean RenderWidgetHostViewBase subclasses?

Done.

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_frame_host.h:192: virtual RenderWidgetHostView*
GetView() = 0;
On 2016/01/21 00:33:03, Charlie Reis wrote:
> nit: Add comment, probably taken from render_frame_host_impl.h.  Also move
this
> up near the other getters (maybe below GetProcess).

Done.

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_widget_host_view.h:70: // that of another
RenderWidgetHost), this provides a facility to convert
On 2016/01/21 00:33:03, Charlie Reis wrote:
> Maybe drop "that of"?

I ended up rewriting to make the whole thing clearer. PTAL.

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_widget_host_view.h:71: // a point from its own
coordinate space to that of the root frame.
On 2016/01/21 00:33:03, Charlie Reis wrote:
> Is this a no-op if you call it on the root RWHV?  (Maybe mention that.)

Done.

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_widget_host_view.h:72: virtual void
TransformPointToRootCoordSpace(
On 2016/01/21 00:33:03, Charlie Reis wrote:
> Sanity check: I don't suppose it would be possible to hide this detail and
only
> expose transformed points from RenderWidgetHost's APIs?  It seems like we'll
be
> playing whack-a-mole with all the calls to APIs that expose points until we
> transform them all, and we'll likely miss future call sites.

It would be nice if there was a more generic way to do this, but this doesn't
seem feasible when components handle their own IPCs -- the coordinates can't be
transformed in the content layer because content classes cannot see the IPC
message definitions. On the bright side, the renderer sending point coordinates
to the browser process seems to be relatively uncommon.

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_widget_host_view.h:74: gfx::Point*
transformed_point) = 0;
On 2016/01/21 00:33:03, Charlie Reis wrote:
> +1 to returning the result if possible, rather than having an out parameter.

Done.

[Charlie Reis, 2016-01-22 23:04:54]

content/ LGTM once the cast is fixed.

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
File components/autofill/core/browser/test_autofill_driver.cc (right):

https://codereview.chromium.org/1526263003/diff/120001/components/autofill/co...
components/autofill/core/browser/test_autofill_driver.cc:75: gfx::RectF
TestAutofillDriver::TransformBoundingBox(
On 2016/01/22 18:33:58, kenrb wrote:
> On 2016/01/21 00:33:03, Charlie Reis wrote:
> > Is there any way to test the new API, either here or in content/?
> 
> The API is tested implicitly by
SitePerProcessBrowserTest.CreateContextMenuTest,
> which uses the TransformPointToRootCoordSpace in the same way as we use it
here.
> Context menus are easier to test this with because the browser can provide the
> coordinates of the mouse click that generates the context menu (and then
verify
> that they have been converted back to the original coords), whereas for popups
> the coordinates come from the page content and I have had trouble finding a
way
> to know what the correct values are.

Acknowledged.

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
File content/public/browser/render_widget_host_view.h (right):

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_widget_host_view.h:70: // that of another
RenderWidgetHost), this provides a facility to convert
On 2016/01/22 18:33:59, kenrb wrote:
> On 2016/01/21 00:33:03, Charlie Reis wrote:
> > Maybe drop "that of"?
> 
> I ended up rewriting to make the whole thing clearer. PTAL.

Acknowledged.

https://codereview.chromium.org/1526263003/diff/120001/content/public/browser...
content/public/browser/render_widget_host_view.h:72: virtual void
TransformPointToRootCoordSpace(
On 2016/01/22 18:33:59, kenrb wrote:
> On 2016/01/21 00:33:03, Charlie Reis wrote:
> > Sanity check: I don't suppose it would be possible to hide this detail and
> only
> > expose transformed points from RenderWidgetHost's APIs?  It seems like we'll
> be
> > playing whack-a-mole with all the calls to APIs that expose points until we
> > transform them all, and we'll likely miss future call sites.
> 
> It would be nice if there was a more generic way to do this, but this doesn't
> seem feasible when components handle their own IPCs -- the coordinates can't
be
> transformed in the content layer because content classes cannot see the IPC
> message definitions. On the bright side, the renderer sending point
coordinates
> to the browser process seems to be relatively uncommon.

Ok, we can go with this approach.

https://codereview.chromium.org/1526263003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1526263003/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:693: return
(gfx::PointF)TransformPointToRootCoordSpace(
This should be a C++ style cast (static_cast), right?

[Ilya Sherman, 2016-01-23 01:06:12]

autofill LGTM -- thanks

[kenrb, 2016-01-25 22:41:40]

https://codereview.chromium.org/1526263003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1526263003/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:693: return
(gfx::PointF)TransformPointToRootCoordSpace(
On 2016/01/22 23:04:54, Charlie Reis wrote:
> This should be a C++ style cast (static_cast), right?

Thanks for catching that, it wasn't supposed to be a cast. :(

I blame coding on a Friday afternoon. It is supposed to invoke the copy
constructor for PointF, which the new PS does. PTAL?

[kenrb, 2016-01-25 22:42:28]

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

[commit-bot: I haz the power, 2016-01-25 22:44:31]

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

[commit-bot: I haz the power, 2016-01-26 00:09:21]

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

[commit-bot: I haz the power, 2016-01-26 00:09:23]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-01-26 04:51:24]

https://codereview.chromium.org/1526263003/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1526263003/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:693: return
(gfx::PointF)TransformPointToRootCoordSpace(
On 2016/01/25 22:41:40, kenrb wrote:
> On 2016/01/22 23:04:54, Charlie Reis wrote:
> > This should be a C++ style cast (static_cast), right?
> 
> Thanks for catching that, it wasn't supposed to be a cast. :(
> 
> I blame coding on a Friday afternoon. It is supposed to invoke the copy
> constructor for PointF, which the new PS does. PTAL?

Ah, that makes much more sense!  LGTM.

[kenrb, 2016-01-26 15:52:43]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2016-01-26 15:52:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1526263003/#ps200001
(title: "Fixed incorrect cast")

[commit-bot: I haz the power, 2016-01-26 15:53:06]

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

[commit-bot: I haz the power, 2016-01-26 15:58:05]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-01-26 15:59:23]

Description was changed from

==========
Position autofill popup widgets correctly under --site-per-process

When a AutofillHostMsg_QueryFormFieldAutofill message is sent from
an out-of-process iframe, its screen coordinates are relative to the
subframe's rect and not the top-level frame's viewport.

This CL exposes a facility in the content API to perform the necessary
transformation, particularly:
- it promotes GetView() from RenderFrameHostImpl to RenderFrameHost, so the
RenderWidgetHostView becomes available to the embedder, and
- it promotes TransformPointToRootCoordSpace from RenderWidgetHostViewBase to
RenderWidgetHostView, so that chrome/ or components/ code that directly receives
coordinates from a renderer process can ensure they properly transformed

Finally, it causes the ContentAutofillDriver to use the exposed methods to
transform received autofill popup coordinates in order for them to display in
the correct position.

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

to

==========
Position autofill popup widgets correctly under --site-per-process

When a AutofillHostMsg_QueryFormFieldAutofill message is sent from
an out-of-process iframe, its screen coordinates are relative to the
subframe's rect and not the top-level frame's viewport.

This CL exposes a facility in the content API to perform the necessary
transformation, particularly:
- it promotes GetView() from RenderFrameHostImpl to RenderFrameHost, so the
RenderWidgetHostView becomes available to the embedder, and
- it promotes TransformPointToRootCoordSpace from RenderWidgetHostViewBase to
RenderWidgetHostView, so that chrome/ or components/ code that directly receives
coordinates from a renderer process can ensure they properly transformed

Finally, it causes the ContentAutofillDriver to use the exposed methods to
transform received autofill popup coordinates in order for them to display in
the correct position.

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

Committed: https://crrev.com/015b5b5829406934f9863c73f931815ae5007e8f
Cr-Commit-Position: refs/heads/master@{#371519}
==========

[commit-bot: I haz the power, 2016-01-26 15:59:24]

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