Convert mustash use surface references.

Convert mustash use surface references.

FrameGenerator uses surface references instead of sequences with this
patch. When mus-ws learns about a new surface, it adds a reference from
the embedding surface to the new surface. If there an existing surface
for the FrameSink, the reference to the existing surface is removed when
the next CompositorFrame is submitted.

FrameGenerator owns the display root surface here and plays the role of
the embedding client. The display root surface gets a reference from the
top level root. All embedded surfaces get a reference from the display
root surface.

When a new surface is created in the gpu process, DisplayCompositor
immediately adds a temporary reference. This is so the surface doesn't
get deleted before the mus-ws process can add the correct reference.

BUG=659227
Committed: https://crrev.com/a9194624d84580648fb7967c00c2800a880a9939
Cr-Commit-Position: refs/heads/master@{#433590}

[kylechar, 2016-11-09 17:04:40]

kylechar@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-11-10 13:53:02]

lgtm + nits:

This is super cool but super easy to forget this subtlety. This needs a design
doc for future reference.

https://codereview.chromium.org/2489003002/diff/1/services/ui/surfaces/displa...
File services/ui/surfaces/display_compositor.cc (right):

https://codereview.chromium.org/2489003002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_compositor.cc:11: namespace {}  // namespace
delete?

https://codereview.chromium.org/2489003002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_compositor.cc:29: std::find(refs.begin(),
refs.end(), child_id.local_frame_id());
I was initially confused about what this is doing. A comment might be helpful.

https://codereview.chromium.org/2489003002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_compositor.cc:39: if (parent_id !=
cc::SurfaceManager::kRootSurfaceId)
nit: Braces.

https://codereview.chromium.org/2489003002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_compositor.cc:76: // Add a temporary reference from
the top level root to this surface.
A short description of why this temporary reference is necessary would be
useful.

https://codereview.chromium.org/2489003002/diff/1/services/ui/ws/frame_genera...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/1/services/ui/ws/frame_genera...
services/ui/ws/frame_generator.cc:309: for (auto& ref : dead_references_) {
nit: no braces necessary.

[kylechar, 2016-11-14 19:05:38]

https://codereview.chromium.org/2489003002/diff/1/services/ui/surfaces/displa...
File services/ui/surfaces/display_compositor.cc (right):

https://codereview.chromium.org/2489003002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_compositor.cc:11: namespace {}  // namespace
On 2016/11/10 13:53:02, Fady Samuel wrote:
> delete?

Done.

https://codereview.chromium.org/2489003002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_compositor.cc:29: std::find(refs.begin(),
refs.end(), child_id.local_frame_id());
On 2016/11/10 13:53:02, Fady Samuel wrote:
> I was initially confused about what this is doing. A comment might be helpful.

Done.

https://codereview.chromium.org/2489003002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_compositor.cc:39: if (parent_id !=
cc::SurfaceManager::kRootSurfaceId)
On 2016/11/10 13:53:02, Fady Samuel wrote:
> nit: Braces.

Done.

https://codereview.chromium.org/2489003002/diff/1/services/ui/surfaces/displa...
services/ui/surfaces/display_compositor.cc:76: // Add a temporary reference from
the top level root to this surface.
On 2016/11/10 13:53:02, Fady Samuel wrote:
> A short description of why this temporary reference is necessary would be
> useful.

Done.

https://codereview.chromium.org/2489003002/diff/1/services/ui/ws/frame_genera...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/1/services/ui/ws/frame_genera...
services/ui/ws/frame_generator.cc:309: for (auto& ref : dead_references_) {
On 2016/11/10 13:53:02, Fady Samuel wrote:
> nit: no braces necessary.

Acknowledged.

[kylechar, 2016-11-14 19:06:38]

Patchset #2 (id:20001) has been deleted

[kylechar, 2016-11-16 18:05:21]

Big changes were necessary in FrameGenerator to fix tests and everything else.
Please take another look fsamuel.

[kylechar, 2016-11-16 18:05:37]

Patchset #3 (id:60001) has been deleted

[Fady Samuel, 2016-11-18 03:46:06]

Some initial questions and comments. This really needs a design doc with
diagrams at this point. There's a lot of subtlety here.

https://codereview.chromium.org/2489003002/diff/40001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/40001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:131:
display_compositor->AddSurfaceReference(parent_id, surface_id);
I'm confused about this line. Aren't you already doing this in the line above?

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
File services/ui/surfaces/display_compositor.cc (right):

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:24: if
(temp_references_.count(child_id.frame_sink_id()) == 0) {
nit: store temp_references_.find(child_id.frame_sink_id()) in a temporary
variable instead so that we don't need to look up the table twice?

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:34: std::find(refs.begin(),
refs.end(), child_id.local_frame_id());
This looks expensive but in practice I think the size of this vector will
usually be one unless the client is faster than the window server. Maybe a
comment indicating this?

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:48: // Remove temporary references
for earlier frames.
Add a comment about the reasoning for why this is safe to do.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:54: refs.erase(refs.begin(),
++ref_iter);
This is removing the temp ref up to the |child_id| in the vector.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:74: manager_.RemoveSurfaceReference(
So this can cause multiple GCes right? I guess that's not a problem right now
but could be a problem in the future when we return multiple surface references.
Maybe a TODO somewhere?

https://codereview.chromium.org/2489003002/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:34: while (current->parent()) {
I'm worried about the correctness of this in general. Today, the display root is
the parent of all surfaces right? Can we just make the code explicit here?

https://codereview.chromium.org/2489003002/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:54: root_surface_id_(cc::FrameSinkId(),
Why isn't the root window's FrameSinkId part of the root_surface_id_?

https://codereview.chromium.org/2489003002/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:137: AddNewParentReferences(ref.child_id,
surface_id);
I'm confused. Aren't we supposed to remove parent references first?

https://codereview.chromium.org/2489003002/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:346:
display_compositor->RemoveRootSurfaceReference(ref.child_id);
That's a lot of potential IPCs in the future. I guess it would be nice to bundle
these into a vector or set or something. It would nice to have an API:

display_compositor->RemoveSurfaceReferences(std::move(dead_references));

[kylechar, 2016-11-18 17:24:11]

https://codereview.chromium.org/2489003002/diff/40001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/40001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:131:
display_compositor->AddSurfaceReference(parent_id, surface_id);
On 2016/11/18 03:46:06, Fady Samuel wrote:
> I'm confused about this line. Aren't you already doing this in the line above?

Disregard this patch. It was more WIP than anything.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
File services/ui/surfaces/display_compositor.cc (right):

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:24: if
(temp_references_.count(child_id.frame_sink_id()) == 0) {
On 2016/11/18 03:46:06, Fady Samuel wrote:
> nit: store temp_references_.find(child_id.frame_sink_id()) in a temporary
> variable instead so that we don't need to look up the table twice?

Done.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:34: std::find(refs.begin(),
refs.end(), child_id.local_frame_id());
On 2016/11/18 03:46:06, Fady Samuel wrote:
> This looks expensive but in practice I think the size of this vector will
> usually be one unless the client is faster than the window server. Maybe a
> comment indicating this?

I can add one if you really want, but I'm not sure that level of detail is
necessary here.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:48: // Remove temporary references
for earlier frames.
On 2016/11/18 03:46:06, Fady Samuel wrote:
> Add a comment about the reasoning for why this is safe to do.

Done.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:54: refs.erase(refs.begin(),
++ref_iter);
On 2016/11/18 03:46:06, Fady Samuel wrote:
> This is removing the temp ref up to the |child_id| in the vector.

Done.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/surfaces/di...
services/ui/surfaces/display_compositor.cc:74: manager_.RemoveSurfaceReference(
On 2016/11/18 03:46:06, Fady Samuel wrote:
> So this can cause multiple GCes right? I guess that's not a problem right now
> but could be a problem in the future when we return multiple surface
references.
> Maybe a TODO somewhere?

Done.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:34: while (current->parent()) {
On 2016/11/18 03:46:06, Fady Samuel wrote:
> I'm worried about the correctness of this in general. Today, the display root
is
> the parent of all surfaces right? Can we just make the code explicit here?

Done.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:54: root_surface_id_(cc::FrameSinkId(),
On 2016/11/18 03:46:06, Fady Samuel wrote:
> Why isn't the root window's FrameSinkId part of the root_surface_id_?

This is different from the display root. I changed the name to make that
clearer.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:137: AddNewParentReferences(ref.child_id,
surface_id);
On 2016/11/18 03:46:06, Fady Samuel wrote:
> I'm confused. Aren't we supposed to remove parent references first?

If something else is referencing the old parent, then we don't want to remove
references from the old parent to children because then GC would children when
they're still being used. (I'm not sure that case ever happens for
FrameGenerator but will for other clients). We do remove our reference to the
old parent, which automatically removes the references it holds (assuming
nothing else is referencing it).

This is just adding references from the new parent to the child surfaces, so
that if nothing else is referencing the old parent and it gets GCd, the children
don't get GCd too.

https://codereview.chromium.org/2489003002/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:346:
display_compositor->RemoveRootSurfaceReference(ref.child_id);
On 2016/11/18 03:46:06, Fady Samuel wrote:
> That's a lot of potential IPCs in the future. I guess it would be nice to
bundle
> these into a vector or set or something. It would nice to have an API:
> 
> display_compositor->RemoveSurfaceReferences(std::move(dead_references));

Added TODO.

[kylechar, 2016-11-18 22:24:52]

kylechar@chromium.org changed reviewers:
+ sky@chromium.org

[kylechar, 2016-11-18 22:24:53]

+sky for services/ui/ws/*.

There are some tests for DisplayCompositor in
https://codereview.chromium.org/2520513002/ that I will follow up with.

[Fady Samuel, 2016-11-18 22:27:25]

lgtm

https://codereview.chromium.org/2489003002/diff/100001/services/ui/surfaces/d...
File services/ui/surfaces/display_compositor.cc (right):

https://codereview.chromium.org/2489003002/diff/100001/services/ui/surfaces/d...
services/ui/surfaces/display_compositor.cc:45: // add a real reference to the
top level root then we do nothing. Otherwise
super nit: I don't like the word "real" here. I guess I'm not a fan of the term
temporary reference. I don't really have a better name in mind so meh.

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:110: // surfaces.
nit: specify when this can happen.

[sky, 2016-11-19 16:38:56]

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:101: DCHECK(surface_id.frame_sink_id() ==
ref.child_id.frame_sink_id());
DCHECK_EQ and NE?

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:266: return
root_window_->compositor_frame_sink_manager()->GetLatestSurfaceId(
How do you know the latest surface id corresponds to window? Comment?

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.h (right):

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.h:130: const cc::SurfaceId top_level_root_;
Can you use a more specific name, mayb top_level_root_surface_id_? I ask as the
name you have here is easily confused with root_window_.

[kylechar, 2016-11-21 14:48:23]

https://codereview.chromium.org/2489003002/diff/100001/services/ui/surfaces/d...
File services/ui/surfaces/display_compositor.cc (right):

https://codereview.chromium.org/2489003002/diff/100001/services/ui/surfaces/d...
services/ui/surfaces/display_compositor.cc:45: // add a real reference to the
top level root then we do nothing. Otherwise
On 2016/11/18 22:27:24, Fady Samuel wrote:
> super nit: I don't like the word "real" here. I guess I'm not a fan of the
term
> temporary reference. I don't really have a better name in mind so meh.
> 

I removed the word real and reworded a bit. Left temporary, because I also can't
think of something better.

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:101: DCHECK(surface_id.frame_sink_id() ==
ref.child_id.frame_sink_id());
On 2016/11/19 16:38:56, sky wrote:
> DCHECK_EQ and NE?

I don't think that works? Maybe I just don't understand those logging macros
fully but FrameSinkId and LocalFrameId don't have << operator defined for them.
It looks like DCHECK_EQ/NE fail to compile when there is no way to print out the
values being checked?

../base/logging.h:558:3: error: no matching function for call to
'MakeCheckOpValueString'
+long template compile error

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:110: // surfaces.
On 2016/11/18 22:27:24, Fady Samuel wrote:
> nit: specify when this can happen.

Done.

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:266: return
root_window_->compositor_frame_sink_manager()->GetLatestSurfaceId(
On 2016/11/19 16:38:56, sky wrote:
> How do you know the latest surface id corresponds to window? Comment?

Added comment.

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.h (right):

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.h:130: const cc::SurfaceId top_level_root_;
On 2016/11/19 16:38:56, sky wrote:
> Can you use a more specific name, mayb top_level_root_surface_id_? I ask as
the
> name you have here is easily confused with root_window_.

Done.

[sky, 2016-11-21 16:51:48]

LGTM

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:101: DCHECK(surface_id.frame_sink_id() ==
ref.child_id.frame_sink_id());
On 2016/11/21 14:48:22, kylechar wrote:
> On 2016/11/19 16:38:56, sky wrote:
> > DCHECK_EQ and NE?
> 
> I don't think that works? Maybe I just don't understand those logging macros
> fully but FrameSinkId and LocalFrameId don't have << operator defined for
them.
> It looks like DCHECK_EQ/NE fail to compile when there is no way to print out
the
> values being checked?
> 
> ../base/logging.h:558:3: error: no matching function for call to
> 'MakeCheckOpValueString'
> +long template compile error

My mistake, you are right.

[kylechar, 2016-11-21 16:54:20]

Thanks!

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.cc:101: DCHECK(surface_id.frame_sink_id() ==
ref.child_id.frame_sink_id());
On 2016/11/21 16:51:48, sky wrote:
> On 2016/11/21 14:48:22, kylechar wrote:
> > On 2016/11/19 16:38:56, sky wrote:
> > > DCHECK_EQ and NE?
> > 
> > I don't think that works? Maybe I just don't understand those logging macros
> > fully but FrameSinkId and LocalFrameId don't have << operator defined for
> them.
> > It looks like DCHECK_EQ/NE fail to compile when there is no way to print out
> the
> > values being checked?
> > 
> > ../base/logging.h:558:3: error: no matching function for call to
> > 'MakeCheckOpValueString'
> > +long template compile error
> 
> My mistake, you are right.

I talked with fsamuel about this and since SurfaceId/FrameSinkId/LocalFrameId
are all pretty widely used and debugging them is important, adding << operator
will be helpful and worth the tiny bit of extra code that is generated. I'll do
that in another CL and update here after.

[kylechar, 2016-11-21 16:54:26]

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

[commit-bot: I haz the power, 2016-11-21 16:54:53]

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

[sky, 2016-11-21 16:58:59]

SG

On Mon, Nov 21, 2016 at 8:54 AM,  <kylechar@chromium.org> wrote:
> Thanks!
>
>
>
https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
> File services/ui/ws/frame_generator.cc (right):
>
>
https://codereview.chromium.org/2489003002/diff/100001/services/ui/ws/frame_g...
> services/ui/ws/frame_generator.cc:101: DCHECK(surface_id.frame_sink_id()
> == ref.child_id.frame_sink_id());
> On 2016/11/21 16:51:48, sky wrote:
>> On 2016/11/21 14:48:22, kylechar wrote:
>> > On 2016/11/19 16:38:56, sky wrote:
>> > > DCHECK_EQ and NE?
>> >
>> > I don't think that works? Maybe I just don't understand those
> logging macros
>> > fully but FrameSinkId and LocalFrameId don't have << operator
> defined for
>> them.
>> > It looks like DCHECK_EQ/NE fail to compile when there is no way to
> print out
>> the
>> > values being checked?
>> >
>> > ../base/logging.h:558:3: error: no matching function for call to
>> > 'MakeCheckOpValueString'
>> > +long template compile error
>>
>> My mistake, you are right.
>
> I talked with fsamuel about this and since
> SurfaceId/FrameSinkId/LocalFrameId are all pretty widely used and
> debugging them is important, adding << operator will be helpful and
> worth the tiny bit of extra code that is generated. I'll do that in
> another CL and update here after.
>
> https://codereview.chromium.org/2489003002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-11-21 17:51:54]

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

[commit-bot: I haz the power, 2016-11-21 17:51:54]

Dry run: This issue passed the CQ dry run.

[kylechar, 2016-11-21 18:19:02]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-11-21 18:19:03]

The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2489003002/#ps120001
(title: "More fixes for comments.")

[commit-bot: I haz the power, 2016-11-21 18:19:37]

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-11-21 18:25:47]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1479752342801190,
"parent_rev": "a9e54f0cad599e27f3e79d98bcc9f7ab56b06ce2", "commit_rev":
"834d465fa465c656714fcc7e44fda61b7927c520"}

[commit-bot: I haz the power, 2016-11-21 18:26:15]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2016-11-21 18:28:21]

Description was changed from

==========
Convert mustash use surface references.

FrameGenerator uses surface references instead of sequences with this
patch. When mus-ws learns about a new surface, it adds a reference from
the embedding surface to the new surface. If there an existing surface
for the FrameSink, the reference to the existing surface is removed when
the next CompositorFrame is submitted.

FrameGenerator owns the display root surface here and plays the role of
the embedding client. The display root surface gets a reference from the
top level root. All embedded surfaces get a reference from the display
root surface.

When a new surface is created in the gpu process, DisplayCompositor
immediately adds a temporary reference. This is so the surface doesn't
get deleted before the mus-ws process can add the correct reference.

BUG=659227
==========

to

==========
Convert mustash use surface references.

FrameGenerator uses surface references instead of sequences with this
patch. When mus-ws learns about a new surface, it adds a reference from
the embedding surface to the new surface. If there an existing surface
for the FrameSink, the reference to the existing surface is removed when
the next CompositorFrame is submitted.

FrameGenerator owns the display root surface here and plays the role of
the embedding client. The display root surface gets a reference from the
top level root. All embedded surfaces get a reference from the display
root surface.

When a new surface is created in the gpu process, DisplayCompositor
immediately adds a temporary reference. This is so the surface doesn't
get deleted before the mus-ws process can add the correct reference.

BUG=659227

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

[commit-bot: I haz the power, 2016-11-21 18:28:22]

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