cc: Plumbing for BeginFrameSource based on Surfaces

cc: Plumbing for BeginFrameSource based on Surfaces

This patch makes a stable decision about which Display a Surface
belongs to and notifies the corresponding SurfaceFactoryClient
of the BeginFrameSource belonging to that Display.

The stable decision is based on the sorted order of Display
pointers that the Surface currently belongs to.

This is only plumbing - the actual endpoints (BeginFrameSource
to use and what to do with that BeginFrameSource) still need
to be hooked up.

R=jbauman,mithro
BUG=401331, 471411
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/877996b0753f32b67fac2835756d23f476e72f10
Cr-Commit-Position: refs/heads/master@{#355140}

[brianderson, 2015-09-04 21:56:51]

https://codereview.chromium.org/1304063014/diff/1/cc/surfaces/surface_aggrega...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1304063014/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:453: void
SurfaceAggregator::RemoveUnreferencedChildren() {
@jbauman: We are using this function to notify the Surface class of Displays
that it is newly associated with and removed from. Is what we are doing sane?
Are there any corner cases we'd need to be careful about?

[jbauman, 2015-09-04 23:58:36]

https://codereview.chromium.org/1304063014/diff/1/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/1304063014/diff/1/cc/surfaces/surface.h#newco...
cc/surfaces/surface.h:83: void AddDisplay(Display* display) {
Probably best to use BeginFrameSource* everywhere here.

https://codereview.chromium.org/1304063014/diff/1/cc/surfaces/surface.h#newco...
cc/surfaces/surface.h:84: if (!base::ContainsValue(currently_displayed_on_, 
display)) {
DCHECK this?

https://codereview.chromium.org/1304063014/diff/1/cc/surfaces/surface.h#newco...
cc/surfaces/surface.h:86: factory_->client()->SetBeginFrameSource(surface_id_,
static_cast<BeginFrameSource*>(getPrimaryDisplay()));
Be careful, as factory_ can be an invalidated weak pointer if the factory has
been destroyed.

Also, we should have factory destruction reset the BeginFrameSource for its
client.

https://codereview.chromium.org/1304063014/diff/1/cc/surfaces/surface_aggrega...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1304063014/diff/1/cc/surfaces/surface_aggrega...
cc/surfaces/surface_aggregator.cc:453: void
SurfaceAggregator::RemoveUnreferencedChildren() {
On 2015/09/04 21:56:51, brianderson wrote:
> @jbauman: We are using this function to notify the Surface class of Displays
> that it is newly associated with and removed from. Is what we are doing sane?
> Are there any corner cases we'd need to be careful about?

This generally makes sense, though I think we'd want to have some sort of
SurfaceAggregatorClient and do client_->AddSurface(Surface*) or
client_->RemoveSurface(Surface*).

Also make sure to remove it on surface aggregator destruction.

[brianderson, 2015-09-23 02:09:39]

Addressed John's comments and made it compile. Also got rid of some of the logic
in DelegatedFrameHost that we can put in a separate patch.

Still need to add unit tests, but ptal!

[brianderson, 2015-09-23 21:44:03]

Fixed existing unit tests, but still need to add new unit tests.

[brianderson, 2015-09-24 02:02:24]

Added a bunch of unit tests. Only SurfaceAggregator unittests left. PTAL!

[brianderson, 2015-09-24 23:40:30]

Needs a rebase, but I think this patch is ready to go otherwise. ptal!

I updated the SurfaceAggregator unit tests with SurfaceAggregatorClient
expectations, which revealed a bug in SurfaceAggregator (see inline comment).

Writing unit tests also revealed a few dumb bugs on my part, which I've fixed.

https://codereview.chromium.org/1304063014/diff/140001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1304063014/diff/140001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:154: return full_rect;
I changed this method to be const and return the full_rect here if the surface
didn't exist.

The previous version of this method was inadvertently inserting elements into
previous_contained_surfaces_, which was throwing off the
AddSurface/RemoveSurface logic.

Not sure if this may have been a source of any other issues...

https://codereview.chromium.org/1304063014/diff/140001/cc/surfaces/surface_fa...
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/1304063014/diff/140001/cc/surfaces/surface_fa...
cc/surfaces/surface_factory.cc:56: if (most_recently_created_surface_id_ !=
surface_id)
I had to use an if here instead of a DCHECK since a number of the LTHI tests and
other unit tests were hitting it.

John: Are you sure this shouldn't get hit? Am I doing something wrong with the
way I'm calling Add/RemoveSurface in the SurfaceAggregator?

[brianderson, 2015-09-24 23:46:12]

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

[brianderson, 2015-09-24 23:46:13]

+sievers for owners review of content/browser
+sky for owners review of components/view_manager

Note: The only modifications in those directories are new empty stub functions
right now.

[sky, 2015-09-24 23:58:20]

Is there any harm in not implementing these in the viewmanager now? By that I
mean will rendering still work?

https://codereview.chromium.org/1304063014/diff/140001/components/view_manage...
File components/view_manager/server_view.cc (right):

https://codereview.chromium.org/1304063014/diff/140001/components/view_manage...
components/view_manager/server_view.cc:314: // TODO(tansell): Implement this.
NOTIMPLEMENTED if it won't be too spammy.

https://codereview.chromium.org/1304063014/diff/140001/components/view_manage...
File components/view_manager/surfaces/top_level_display_client.cc (right):

https://codereview.chromium.org/1304063014/diff/140001/components/view_manage...
components/view_manager/surfaces/top_level_display_client.cc:112: //
TODO(tansell): Implement this.
Same comment here.

[brianderson, 2015-09-25 00:35:47]

On 2015/09/24 23:58:20, sky wrote:
> Is there any harm in not implementing these in the viewmanager now? By that I
> mean will rendering still work?

Rendering will still work since this patch only adds plumbing for the new code
path without disabling the old code path yet. We will need to hook these up
eventually before we can remove the old code path, but that will come later.

> 
>
https://codereview.chromium.org/1304063014/diff/140001/components/view_manage...
> File components/view_manager/server_view.cc (right):
> 
>
https://codereview.chromium.org/1304063014/diff/140001/components/view_manage...
> components/view_manager/server_view.cc:314: // TODO(tansell): Implement this.
> NOTIMPLEMENTED if it won't be too spammy.

It will be spammy since I expect these methods to be called. There's no harm in
ignoring them right now.

I could put the notification behind a flag at the sources, but it would increase
the test coverage needed and I'd need to check and pass around the flag to a
number of places including several destructors. I could disable the notification
behind a flag just before it reaches a sink such as this one, but that doesn't
buy much more than just ignoring the call.

> 
>
https://codereview.chromium.org/1304063014/diff/140001/components/view_manage...
> File components/view_manager/surfaces/top_level_display_client.cc (right):
> 
>
https://codereview.chromium.org/1304063014/diff/140001/components/view_manage...
> components/view_manager/surfaces/top_level_display_client.cc:112: //
> TODO(tansell): Implement this.
> Same comment here.

[brianderson, 2015-09-25 01:00:58]

brianderson@chromium.org changed reviewers:
+ sievers@chromium.org

[brianderson, 2015-09-25 01:00:58]

Actually +sievers.

[sky, 2015-09-25 15:05:38]

LGTM

[brianderson, 2015-09-25 22:42:47]

brianderson@chromium.org changed reviewers:
+ boliu@chromium.org

[brianderson, 2015-09-25 22:42:47]

+boliu for android_webview/

[boliu, 2015-09-25 22:48:46]

android_webview rs lgtm

[no sievers, 2015-09-28 22:15:05]

content lgtm though you might want to put NOTIMPLEMENTED()

[brianderson, 2015-09-29 02:16:57]

All unit tests pass now. Tim: Can you take a look and see what you think? John
is out till 10/12.

[mithro-old, 2015-10-01 03:00:24]

Hi Brian,

LGTM.

As mentioned via chat, everything in this CL looks pretty good. There are a
couple of minor nits but otherwise I think it should be good to go.

I do think we should create bugs for the following things (and link them from
the TODO items);
 * Making a better choice for the BeginFrameSource dependent on coverage
information.
 * The work for me to hook up the BeginFrameSource in the render widget /
delegate frame stuff.

Thanks for all your work on this and sorry about the slow review.

Tim 'mithro' Ansell

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
File cc/surfaces/display.cc (right):

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/display.cc:159: // DisplayScheduler. crbug.com/476676
nit: Can you put http:// before this URL so it becomes clickable?

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
File cc/surfaces/display_unittest.cc (right):

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/display_unittest.cc:152: // Verify Display responsds to
SurfaceAggregatorClient methods properly.
nit: s/responsds/responds/

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
File cc/surfaces/onscreen_display_client.h (right):

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/onscreen_display_client.h:58: // depends on TaskRunner.
Can we use asserts/dchecks to make sure the destruction happens in the right
order?

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
File cc/surfaces/surface.cc (right):

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/surface.cc:18: template <typename Collection, typename Value>
nit: Could we get a comment about why this is here.

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
File cc/surfaces/surface.h (right):

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/surface.h:100: std::vector<BeginFrameSource*> begin_frame_sources_;
Can we just use an order set type here rather then a vector?

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
File cc/surfaces/surface_aggregator.cc (right):

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/surface_aggregator.cc:56: SurfaceAggregator::~SurfaceAggregator() {
I feel like this should be calling ProcessAddedAndRemovedSurfaces() after
resetting contained_surfaces to null? Would that work?

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
File cc/surfaces/surface_aggregator.h (right):

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/surface_aggregator.h:40: SurfaceAggregator(SurfaceAggregatorClient*
client,
I *think* it makes sense that a SurfaceAggregator only has a single client. Can
you think of any cases were we might want multiple clients?

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
File cc/surfaces/surface_aggregator_unittest.cc (right):

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/surface_aggregator_unittest.cc:226:
EXPECT_FALSE(surface_aggregator_client_.HasSurface(embedded_surface));
Thanks for adding these!

I wonder if would be a good idea to have some type of macro here? It doesn't
seem worth it for two EXPECT_FALSE / EXPECT_TRUE but might be if there are more
in the future?

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
File cc/surfaces/surface_factory.cc (right):

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/surface_factory.cc:56: if (most_recently_created_surface_id_ !=
surface_id)
nit: Can you add a comment about why this check is needed?

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
File cc/surfaces/surface_unittest.cc (right):

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/surface_unittest.cc:59: 
This test is a bit weird, but I'm unsure how to make it easier to understand.

I wonder if you are better off having 6 separate surfaces (you could wrap the
setup in a class or macro) rather than doing the RemoveBeginFrameSource bit...

I feel like these EXPECT_EQ should be under each the AddBeginFrameSource bits
too.

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/surface_unittest.cc:114: EXPECT_NE(nullptr, bfs_order1);
Should this be an ASSERT_NE?

https://chromiumcodereview-hr.appspot.com/1304063014/diff/240001/cc/surfaces/...
cc/surfaces/surface_unittest.cc:116: &bfs3 == bfs_order1);
I'm not sure what the above EXPECT_TRUE is checking?

[jbauman, 2015-10-02 19:21:40]

lgtm except for one change.

https://codereview.chromium.org/1304063014/diff/280001/cc/surfaces/surface_fa...
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/1304063014/diff/280001/cc/surfaces/surface_fa...
cc/surfaces/surface_factory.cc:62:
client_->SetBeginFrameSource(begin_frame_source);
I'd prefer to pass the SurfaceId to the client and have it do this check.

[brianderson, 2015-10-07 20:54:49]

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/display.cc
File cc/surfaces/display.cc (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/display.cc...
cc/surfaces/display.cc:159: // DisplayScheduler. crbug.com/476676
On 2015/10/01 03:00:23, mithro wrote:
> nit: Can you put http:// before this URL so it becomes clickable?

Done.

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/display_un...
File cc/surfaces/display_unittest.cc (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/display_un...
cc/surfaces/display_unittest.cc:152: // Verify Display responsds to
SurfaceAggregatorClient methods properly.
On 2015/10/01 03:00:23, mithro wrote:
> nit: s/responsds/responds/

Done.

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/onscreen_d...
File cc/surfaces/onscreen_display_client.h (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/onscreen_d...
cc/surfaces/onscreen_display_client.h:58: // depends on TaskRunner.
On 2015/10/01 03:00:23, mithro wrote:
> Can we use asserts/dchecks to make sure the destruction happens in the right
> order?

Trying to think of how to do that, but it's difficult since the classes here are
given non-owning pointers and are not notified that something they depend on is
going to be destroyed.
 
The trybots caught it, so I'm not too worried about this regressing.

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:18: template <typename Collection, typename Value>
On 2015/10/01 03:00:23, mithro wrote:
> nit: Could we get a comment about why this is here.

No longer needed with a std::set.

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface.h#...
cc/surfaces/surface.h:100: std::vector<BeginFrameSource*> begin_frame_sources_;
On 2015/10/01 03:00:23, mithro wrote:
> Can we just use an order set type here rather then a vector?

Done.

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:56: SurfaceAggregator::~SurfaceAggregator() {
On 2015/10/01 03:00:23, mithro wrote:
> I feel like this should be calling ProcessAddedAndRemovedSurfaces() after
> resetting contained_surfaces to null? Would that work?

+jbauman: wdyt? Doing that sounds like a good idea, but it would also release
resources of the previously_contained_surfaces. Is there already code elsewhere
that releases resources before we get to this destructor?

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.h (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.h:40: SurfaceAggregator(SurfaceAggregatorClient*
client,
On 2015/10/01 03:00:23, mithro wrote:
> I *think* it makes sense that a SurfaceAggregator only has a single client.
Can
> you think of any cases were we might want multiple clients?

It is owned by the Display (which is the SurfaceAggregatorClient), which will
handle the the distribution to multiple Surface classes - so there isn't a need
for SurfaceAggregator itself to have multiple clients.

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator_unittest.cc (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator_unittest.cc:226:
EXPECT_FALSE(surface_aggregator_client_.HasSurface(embedded_surface));
On 2015/10/01 03:00:24, mithro wrote:
> Thanks for adding these!
> 
> I wonder if would be a good idea to have some type of macro here? It doesn't
> seem worth it for two EXPECT_FALSE / EXPECT_TRUE but might be if there are
more
> in the future?
> 

Might do it as a followup patch since I can't think of a good macro that isn't
overly complicated right now.

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_fa...
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_fa...
cc/surfaces/surface_factory.cc:56: if (most_recently_created_surface_id_ !=
surface_id)
On 2015/10/01 03:00:24, mithro wrote:
> nit: Can you add a comment about why this check is needed?

Done.

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_un...
File cc/surfaces/surface_unittest.cc (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:59: 
On 2015/10/01 03:00:24, mithro wrote:
> This test is a bit weird, but I'm unsure how to make it easier to understand.
> 
> I wonder if you are better off having 6 separate surfaces (you could wrap the
> setup in a class or macro) rather than doing the RemoveBeginFrameSource bit...

Given each order would also need a SurfaceFactoryClient and SurfaceFactory,
there is a lot of boilerplate to do that. Callin REmoveBeginFrameSource a bunch
of times seems like the easiest way forward. I added some checks after the
sources are removed to make sure they were actually removed.

> 
> I feel like these EXPECT_EQ should be under each the AddBeginFrameSource bits
> too.

Done.

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:114: EXPECT_NE(nullptr, bfs_order1);
On 2015/10/01 03:00:24, mithro wrote:
> Should this be an ASSERT_NE?

nullptr could be a valid result if the surface doesn't belong to any display.

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:116: &bfs3 == bfs_order1);
On 2015/10/01 03:00:24, mithro wrote:
> I'm not sure what the above EXPECT_TRUE is checking?

Added a comment. It checks that one of the sources provided was actually chosen.

https://codereview.chromium.org/1304063014/diff/280001/cc/surfaces/surface_fa...
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/1304063014/diff/280001/cc/surfaces/surface_fa...
cc/surfaces/surface_factory.cc:62:
client_->SetBeginFrameSource(begin_frame_source);
On 2015/10/02 19:21:40, jbauman (ooo 9-26 to 10-12) wrote:
> I'd prefer to pass the SurfaceId to the client and have it do this check.

Done.

https://codereview.chromium.org/1304063014/diff/300001/cc/surfaces/surface_fa...
File cc/surfaces/surface_factory_client.h (right):

https://codereview.chromium.org/1304063014/diff/300001/cc/surfaces/surface_fa...
cc/surfaces/surface_factory_client.h:27: // set by this function should be
invalidated.
Is the contract defined in this comment okay?
I wasn't sure what sure what SurfaceId to pass from the of SurfaceFactory's
destructor.

[jbauman, 2015-10-13 20:23:20]

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_ag...
File cc/surfaces/surface_aggregator.cc (right):

https://codereview.chromium.org/1304063014/diff/240001/cc/surfaces/surface_ag...
cc/surfaces/surface_aggregator.cc:56: SurfaceAggregator::~SurfaceAggregator() {
On 2015/10/07 20:54:48, brianderson wrote:
> On 2015/10/01 03:00:23, mithro wrote:
> > I feel like this should be calling ProcessAddedAndRemovedSurfaces() after
> > resetting contained_surfaces to null? Would that work?
> 
> +jbauman: wdyt? Doing that sounds like a good idea, but it would also release
> resources of the previously_contained_surfaces. Is there already code
elsewhere
> that releases resources before we get to this destructor?

I think that should work. I just checked and the destructor order happening with
the cc::Display makes sense.

[brianderson, 2015-10-20 00:56:23]

Description was changed from

==========
cc: Plumbing for BeginFrameSource based on Surfaces

This patch makes a stable decision about which Display a Surface
belongs to and notifies the corresponding SurfaceFactoryClient
of the BeginFrameSource belonging to that Display.

The stable decision is based on the sorted order of Display
pointers that the Surface currently belongs to.

This is only plumbing - the actual endpoints (BeginFrameSource
to use and what to do with that BeginFrameSource) still need
to be hooked up.

R=jbauman,mithro
BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Plumbing for BeginFrameSource based on Surfaces

This patch makes a stable decision about which Display a Surface
belongs to and notifies the corresponding SurfaceFactoryClient
of the BeginFrameSource belonging to that Display.

The stable decision is based on the sorted order of Display
pointers that the Surface currently belongs to.

This is only plumbing - the actual endpoints (BeginFrameSource
to use and what to do with that BeginFrameSource) still need
to be hooked up.

R=jbauman,mithro
BUG=401331, 471411
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[brianderson, 2015-10-20 19:13:34]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-10-20 19:13:35]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
boliu@chromium.org, sievers@chromium.org, mithro@mithis.com,
jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/1304063014/#ps380001
(title: "fix webview")

[commit-bot: I haz the power, 2015-10-20 19:14:48]

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

[commit-bot: I haz the power, 2015-10-20 20:35:44]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-10-20 20:36:46]

Patchset 20 (id:??) landed as
https://crrev.com/877996b0753f32b67fac2835756d23f476e72f10
Cr-Commit-Position: refs/heads/master@{#355140}