Refactor ChromeActivity references out of TabModelSelectorImpl

Refactor ChromeActivity references out of TabModelSelectorImpl

BUG=653182
Committed: https://crrev.com/dcff60d80166f6e2a9a4d5518c7a94c18b70befe
Cr-Commit-Position: refs/heads/master@{#425784}

[mthiesse, 2016-10-06 22:30:18]

mthiesse@chromium.org changed reviewers:
+ tedchoc@chromium.org

[mthiesse, 2016-10-06 22:30:19]

PTAL,

I haven't written the design doc yet for moving VR to a separate activity, but
these refactors seems like The Right Thing regardless.


My current thinking is that the VR Activity will extend
AsyncInitializationActivity, and implement things like TabCreatorManager
directly because ChromeActivity does a ton of stuff we don't want at all in VR,
given that we have no (visible) java UI.

How the tab state is actually shared is TBD, but it's clear that the VR activity
will have to manage tabs to some extent, so abstracting away ChromeActivity
dependencies seems like the first step.

[mthiesse, 2016-10-11 19:19:30]

On 2016/10/06 22:30:19, mthiesse wrote:
> PTAL,
> 
> I haven't written the design doc yet for moving VR to a separate activity, but
> these refactors seems like The Right Thing regardless.
> 
> 
> My current thinking is that the VR Activity will extend
> AsyncInitializationActivity, and implement things like TabCreatorManager
> directly because ChromeActivity does a ton of stuff we don't want at all in
VR,
> given that we have no (visible) java UI.
> 
> How the tab state is actually shared is TBD, but it's clear that the VR
activity
> will have to manage tabs to some extent, so abstracting away ChromeActivity
> dependencies seems like the first step.

Friendly ping, in case this was missed.

[Ted C, 2016-10-12 00:04:40]

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
(right):

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:68:
assert (tabCreatorManager instanceof Activity);
not really a fan of this assertion.  if we still need an activity, let's just
pass a vanilla Activity and a tab creator manager separately.

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:435:
public void setFullscreenManager(FullscreenManager fullscreenManager) {
all public methods should have javadoc.

Now...to go down this rabbit hole a little further.  How hard is it move
fullscreen manager creation early enough to pass it in via the constructor.  I
think that would be much nicer to not have to do this lazy initialization.

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
(right):

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:56:
private Map<TabCreatorManager, TabModelSelector> mAssignments = new HashMap<>();
Activity makes more sense here conceptually since that is where we expect to
have tabs assigned (for now).

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:74:
*            index of the {@link TabModelSelector} returned.
should be aligned with "The index" above

[mthiesse, 2016-10-12 18:45:18]

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
(right):

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:68:
assert (tabCreatorManager instanceof Activity);
On 2016/10/12 00:04:40, Ted C wrote:
> not really a fan of this assertion.  if we still need an activity, let's just
> pass a vanilla Activity and a tab creator manager separately.

Done.

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:435:
public void setFullscreenManager(FullscreenManager fullscreenManager) {
On 2016/10/12 00:04:40, Ted C wrote:
> all public methods should have javadoc.
> 
> Now...to go down this rabbit hole a little further.  How hard is it move
> fullscreen manager creation early enough to pass it in via the constructor.  I
> think that would be much nicer to not have to do this lazy initialization.

So passing via the constructor is difficult because there's a circular
dependency, ChromeFullscreenManger needs a TabModelSelectorImpl, and
TabModelSelectorImpl needs a ChromeFullscreenManger. I've instead moved setting
the fullscreen manager to immediately after creating the TabModelSelector and
FullscreenManager, and asserting it's not null when used.

getFullscreenManager also now asserts the fullscreenManager isn't null.

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
(right):

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:56:
private Map<TabCreatorManager, TabModelSelector> mAssignments = new HashMap<>();
On 2016/10/12 00:04:40, Ted C wrote:
> Activity makes more sense here conceptually since that is where we expect to
> have tabs assigned (for now).

Done.

https://codereview.chromium.org/2394373002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:74:
*            index of the {@link TabModelSelector} returned.
On 2016/10/12 00:04:40, Ted C wrote:
> should be aligned with "The index" above

Done.

[Ted C, 2016-10-13 00:34:16]

https://codereview.chromium.org/2394373002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1312:
assert mFullscreenManager != null;
I think this breaks EmbedContentViewActivity.  It doesn't support fullscreen, so
this will be null and is valid.

I wonder if this would be better done more explicitly in ChromeActivity.
would be better off adding a couple functions in postInflationStartup

Basically, ChromeActivity would have two methods.

protected TabModelSelector initializeTabModelSelector();
protected ChromeFullscreenManager initializeFullscreenManager(ControlContainer);

Both of these would be called at the end of postInflationStartup in
ChromeActivity.

initializeFullscreenManager would set itself on the model.

Granted, I think we could also get rid of TabModelSelector from
FullscreenManager entirely.  Right now, requestToShowTab sets the
fullscreenmanager for a tab.  I think we could also have FullscreenManager have
a method that sets the active tab and calls tab with itself.  Then
fullscreenmanager only holds it's current tab and does that clean up.

If we do that, then FullscreenManager could be created first and not have this
cyclical dependency you described.

I put together a patch that I can try to get reviewed tomorrow that does this
and could help with your cleanup:
https://codereview.chromium.org/2414913004

Assuming I land that patch, you could then pass the fullscreen manager to the
tab model selector in the constructor of initializeTabModelSelector and all is
good in the world.

Then instead of having an assert here that you are != null.  We would have a
boolean mPostInflationStartupComplete set in postInflationStartup.  And the
assert would be that mPostInflationStartupComplete == true when
getFullscreenManager is called.

https://codereview.chromium.org/2394373002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
(right):

https://codereview.chromium.org/2394373002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:82:
if (mAssignments.get(tabCreatorManager) != null) {
should still use the Activity

[mthiesse, 2016-10-13 23:03:16]

https://codereview.chromium.org/2394373002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1312:
assert mFullscreenManager != null;
On 2016/10/13 00:34:16, Ted C wrote:
> I think this breaks EmbedContentViewActivity.  It doesn't support fullscreen,
so
> this will be null and is valid.
> 
> I wonder if this would be better done more explicitly in ChromeActivity.
> would be better off adding a couple functions in postInflationStartup
> 
> Basically, ChromeActivity would have two methods.
> 
> protected TabModelSelector initializeTabModelSelector();
> protected ChromeFullscreenManager
initializeFullscreenManager(ControlContainer);
> 
> Both of these would be called at the end of postInflationStartup in
> ChromeActivity.
> 
> initializeFullscreenManager would set itself on the model.
> 
> Granted, I think we could also get rid of TabModelSelector from
> FullscreenManager entirely.  Right now, requestToShowTab sets the
> fullscreenmanager for a tab.  I think we could also have FullscreenManager
have
> a method that sets the active tab and calls tab with itself.  Then
> fullscreenmanager only holds it's current tab and does that clean up.
> 
> If we do that, then FullscreenManager could be created first and not have this
> cyclical dependency you described.
> 
> I put together a patch that I can try to get reviewed tomorrow that does this
> and could help with your cleanup:
> https://codereview.chromium.org/2414913004
> 
> Assuming I land that patch, you could then pass the fullscreen manager to the
> tab model selector in the constructor of initializeTabModelSelector and all is
> good in the world.
> 
> Then instead of having an assert here that you are != null.  We would have a
> boolean mPostInflationStartupComplete set in postInflationStartup.  And the
> assert would be that mPostInflationStartupComplete == true when
> getFullscreenManager is called.

Thanks for the patch.

I don't like the assertion mPostInflationStartupComplete == true, because the
FullscreenManager doesn't necessarily have to be created then; subclasses could
(and do) create it later (or earlier).

I fixed the EmbedContentViewActivity case by overriding getFullscreenManager().

https://codereview.chromium.org/2394373002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java
(right):

https://codereview.chromium.org/2394373002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabWindowManager.java:82:
if (mAssignments.get(tabCreatorManager) != null) {
On 2016/10/13 00:34:16, Ted C wrote:
> should still use the Activity

Done.

[Ted C, 2016-10-15 00:28:55]

Sadly, I'm not convinced this is a net win anymore.

I think it's probably best to go back to the version that just sets the
fullscreen manager outside of the constructor of the tab model and then leaves
all of the existing fullscreen manager initialization alone.  I'd rather have a
longer term vision of where we want to go with this than try to make these
incremental and potentially conflicting changes going forward.

I think we need to get a better handle on how we want to untangle these
dependencies before we start doing these one off changes that won't end up being
cohesive in the end.

https://codereview.chromium.org/2394373002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1312:
assert mFullscreenManager != null;
On 2016/10/13 23:03:16, mthiesse wrote:
> On 2016/10/13 00:34:16, Ted C wrote:
> > I think this breaks EmbedContentViewActivity.  It doesn't support
fullscreen,
> so
> > this will be null and is valid.
> > 
> > I wonder if this would be better done more explicitly in ChromeActivity.
> > would be better off adding a couple functions in postInflationStartup
> > 
> > Basically, ChromeActivity would have two methods.
> > 
> > protected TabModelSelector initializeTabModelSelector();
> > protected ChromeFullscreenManager
> initializeFullscreenManager(ControlContainer);
> > 
> > Both of these would be called at the end of postInflationStartup in
> > ChromeActivity.
> > 
> > initializeFullscreenManager would set itself on the model.
> > 
> > Granted, I think we could also get rid of TabModelSelector from
> > FullscreenManager entirely.  Right now, requestToShowTab sets the
> > fullscreenmanager for a tab.  I think we could also have FullscreenManager
> have
> > a method that sets the active tab and calls tab with itself.  Then
> > fullscreenmanager only holds it's current tab and does that clean up.
> > 
> > If we do that, then FullscreenManager could be created first and not have
this
> > cyclical dependency you described.
> > 
> > I put together a patch that I can try to get reviewed tomorrow that does
this
> > and could help with your cleanup:
> > https://codereview.chromium.org/2414913004
> > 
> > Assuming I land that patch, you could then pass the fullscreen manager to
the
> > tab model selector in the constructor of initializeTabModelSelector and all
is
> > good in the world.
> > 
> > Then instead of having an assert here that you are != null.  We would have a
> > boolean mPostInflationStartupComplete set in postInflationStartup.  And the
> > assert would be that mPostInflationStartupComplete == true when
> > getFullscreenManager is called.
> 
> Thanks for the patch.
> 
> I don't like the assertion mPostInflationStartupComplete == true, because the
> FullscreenManager doesn't necessarily have to be created then; subclasses
could
> (and do) create it later (or earlier).

But I argue that flexibility is confusing and a source of a lot of these
underlying pain points you are seeing.

Having a base class have an assertion for non-null but allowing a subclass go
against that seems like an extremely dangerous thing to do.  Working on this
code, I would now assume that it could never be null in normal circumstances and
like break the EmbedContentViewActivity.

If we want to enforce this form of null checking, we should create an
dummy/empty implementation in EmbedContentViewActivity and ensure it can't be
null ever.

> 
> I fixed the EmbedContentViewActivity case by overriding
getFullscreenManager().

https://codereview.chromium.org/2394373002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1369:
protected void createFullscreenManager(ControlContainer controlContainer,
This should probably be initializeFullscreenManager now.

https://codereview.chromium.org/2394373002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:569:
super.createFullscreenManager(controlContainer, false);
This seems quite odd to me.  Having it take a param that isn't used for really
the one place that sets it different from the default doesn't seem like the best
api design.

The old way seemed better to me.

[mthiesse, 2016-10-17 17:00:42]

Alright, I think I've come up with a solution that addresses your concerns.
PTAL.

https://codereview.chromium.org/2394373002/diff/120001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1369:
protected void createFullscreenManager(ControlContainer controlContainer,
On 2016/10/15 00:28:55, Ted C wrote:
> This should probably be initializeFullscreenManager now.

Acknowledged.

https://codereview.chromium.org/2394373002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:569:
super.createFullscreenManager(controlContainer, false);
On 2016/10/15 00:28:55, Ted C wrote:
> This seems quite odd to me.  Having it take a param that isn't used for really
> the one place that sets it different from the default doesn't seem like the
best
> api design.
> 
> The old way seemed better to me.

Done.

[Ted C, 2016-10-17 17:56:44]

lgtm w/ the null checks added a nit for the comment

But, ChromeActivity is also pretty strongly tied into Tab.java, so I think we'll
need a way to untangle that.

Let's hold off on any other refactorings before we figure out if VR will be a
ChromeActivity or not.  What parts of it isn't needed, etc..., and have a game
plan ready.

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1365: *
Note: This is called during post inflation startup, so native code will not have
been
s/post inflation startup/{@link #postInflationStartup}

And it is "technically" more correct to say, "so there is no guarantee that
native code will have been initialized yet".

Another activity could have initialized native before this activity is run, but
it is just something you can not count on.

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
(right):

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:373:
mFullscreenManager.setTab(null);
this can be null, so we do need checks here still

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:85:
getFullscreenManager().setTab(getActivityTab());
same thing...need null checks here.

https://codereview.chromium.org/2394373002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/2394373002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:47:
private static boolean sNativeSingletonReady;
this seems unrelated to this change

[mthiesse, 2016-10-17 18:13:59]

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1365: *
Note: This is called during post inflation startup, so native code will not have
been
On 2016/10/17 17:56:44, Ted C wrote:
> s/post inflation startup/{@link #postInflationStartup}
> 
> And it is "technically" more correct to say, "so there is no guarantee that
> native code will have been initialized yet".
> 
> Another activity could have initialized native before this activity is run,
but
> it is just something you can not count on.

Done.

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java
(right):

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelSelectorImpl.java:373:
mFullscreenManager.setTab(null);
On 2016/10/17 17:56:44, Ted C wrote:
> this can be null, so we do need checks here still

Done.

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java
(right):

https://codereview.chromium.org/2394373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:85:
getFullscreenManager().setTab(getActivityTab());
On 2016/10/17 17:56:44, Ted C wrote:
> same thing...need null checks here.

Done.

https://codereview.chromium.org/2394373002/diff/160001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/2394373002/diff/160001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:47:
private static boolean sNativeSingletonReady;
On 2016/10/17 17:56:44, Ted C wrote:
> this seems unrelated to this change

It isn't. getContentVideoView() is called in ChromeFullscreenManager, and
crashes if native hasn't been initialized yet, which it may well not be. So in
order to initialize ChromeFullscreenManager before native is ready we need to
add this check.

[Ted C, 2016-10-17 18:24:42]

https://codereview.chromium.org/2394373002/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/2394373002/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:363:
if (!sNativeSingletonReady) return null;
LibraryLoader.isInitialized() is probably better to use in that case.

But if anything, I think the check should be in fullscreenmanager to check this
and not here.

[mthiesse, 2016-10-17 18:44:01]

https://codereview.chromium.org/2394373002/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java
(right):

https://codereview.chromium.org/2394373002/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentVideoView.java:363:
if (!sNativeSingletonReady) return null;
On 2016/10/17 18:24:42, Ted C wrote:
> LibraryLoader.isInitialized() is probably better to use in that case.
> 
> But if anything, I think the check should be in fullscreenmanager to check
this
> and not here.

Done.

[mthiesse, 2016-10-17 18:44:51]

Patchset #11 (id:200001) has been deleted

[mthiesse, 2016-10-17 19:35:56]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-10-17 19:35:57]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2394373002/#ps220001
(title: "Address comments")

[commit-bot: I haz the power, 2016-10-17 19:36:27]

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-10-17 19:40:13]

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

[commit-bot: I haz the power, 2016-10-17 19:40:14]

Exceeded global retry quota

[mthiesse, 2016-10-17 19:58:26]

The CQ bit was checked by mthiesse@chromium.org

[mthiesse, 2016-10-17 19:58:26]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2394373002/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-17 19:58:51]

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-10-17 21:39:15]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-10-17 21:40:56]

Description was changed from

==========
Refactor ChromeActivity references out of TabModelSelectorImpl

BUG=653182
==========

to

==========
Refactor ChromeActivity references out of TabModelSelectorImpl

BUG=653182

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

[commit-bot: I haz the power, 2016-10-17 21:40:57]

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