arc: Prevent App list popping on each app installed from batch.

arc: Prevent App list popping on each app installed from batch.

There is possibility to manually install packages in batch mode.
In this mode App list is shown on each package installation. This
is annoying for user. This CL show App launcher only for first
installed in batch app. If user does not close App list he may
observer how next apps appear in list. If he closes it, now App
list will be shown unless next batch installation is started.
This also introduces new mojo notifications to support installation
sessions from Play Store. These notifications are used in current
task and will be used in next planned task to detect the case
when default app is not available after Arc start. There is yet
another idea to implement installation progress in App launcher.

TEST=Manually on device.
TEST=unit_tests + extended browser_tests.
TBR=skuhne@chromium.org
BUG=b/33420574
BUG=674656
Committed: https://crrev.com/09a187e794db3a8cd261d7652658e51864a5a64d
Cr-Commit-Position: refs/heads/master@{#440262}

[khmel, 2016-12-15 19:42:24]

khmel@chromium.org changed reviewers:
+ skuhne@chromium.org, xiyuan@chromium.org

[khmel, 2016-12-15 19:42:24]

Hi Xiyaun and Stefan,

PTAL

[xiyuan, 2016-12-15 22:07:21]

https://codereview.chromium.org/2585503002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1283:
installing_packages_.insert(package_name);
How important it is to check IsNewPackageInSystem on them? Can we reduce
|installing_packages_| into a count? 

i.e.
installing_packages_ -> installing_packages_count_

void ArcAppListPrefs::OnInstallationStarted(...) {
  if (IsNewPackageInSystem(package_name)) {
    if (installing_packages_count_ == 0)
      ++current_batch_installation_revision_;
  }
  ++installing_packages_count_;
}

void ArcAppListPrefs::OnInstallationFinished(...) {
  --installing_packages_count_;
}

https://codereview.chromium.org/2585503002/diff/20001/components/arc/common/a...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/2585503002/diff/20001/components/arc/common/a...
components/arc/common/app.mojom:132: // Notifies that Play Store installation
has been started. |name| specifies name
nit: wrap

[khmel, 2016-12-16 00:35:30]

Thank you Xiyuan for your comments,

PTAL

https://codereview.chromium.org/2585503002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1283:
installing_packages_.insert(package_name);
On 2016/12/15 22:07:21, xiyuan wrote:
> How important it is to check IsNewPackageInSystem on them? Can we reduce
> |installing_packages_| into a count? 
> 
> i.e.
> installing_packages_ -> installing_packages_count_
> 
> void ArcAppListPrefs::OnInstallationStarted(...) {
>   if (IsNewPackageInSystem(package_name)) {
>     if (installing_packages_count_ == 0)
>       ++current_batch_installation_revision_;
>   }
>   ++installing_packages_count_;
> }
> 
> void ArcAppListPrefs::OnInstallationFinished(...) {
>   --installing_packages_count_;
> }

I think we can change in this CL, thank you for idea. Probably in next task it
will be needed but let left it when it actually required. In this case
IsNewPackageInSystem is also not required.

https://codereview.chromium.org/2585503002/diff/20001/components/arc/common/a...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/2585503002/diff/20001/components/arc/common/a...
components/arc/common/app.mojom:132: // Notifies that Play Store installation
has been started. |name| specifies name
On 2016/12/15 22:07:21, xiyuan wrote:
> nit: wrap

Done

[khmel, 2016-12-16 00:35:30]

Thank you Xiyuan for your comments,

PTAL

https://codereview.chromium.org/2585503002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1283:
installing_packages_.insert(package_name);
On 2016/12/15 22:07:21, xiyuan wrote:
> How important it is to check IsNewPackageInSystem on them? Can we reduce
> |installing_packages_| into a count? 
> 
> i.e.
> installing_packages_ -> installing_packages_count_
> 
> void ArcAppListPrefs::OnInstallationStarted(...) {
>   if (IsNewPackageInSystem(package_name)) {
>     if (installing_packages_count_ == 0)
>       ++current_batch_installation_revision_;
>   }
>   ++installing_packages_count_;
> }
> 
> void ArcAppListPrefs::OnInstallationFinished(...) {
>   --installing_packages_count_;
> }

I think we can change in this CL, thank you for idea. Probably in next task it
will be needed but let left it when it actually required. In this case
IsNewPackageInSystem is also not required.

https://codereview.chromium.org/2585503002/diff/20001/components/arc/common/a...
File components/arc/common/app.mojom (right):

https://codereview.chromium.org/2585503002/diff/20001/components/arc/common/a...
components/arc/common/app.mojom:132: // Notifies that Play Store installation
has been started. |name| specifies name
On 2016/12/15 22:07:21, xiyuan wrote:
> nit: wrap

Done

[Mr4D (OOO till 08-26), 2016-12-16 04:48:54]

https://codereview.chromium.org/2585503002/diff/30001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/30001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1272:
++installing_packages_count_;
Maybe nothing, but:

Would it be possible that a download starts multiple times? E.g. when the
android container dies and gets re-started (or another coincidence)?
Would we need to remember what got installed to properly "check in and out"?

https://codereview.chromium.org/2585503002/diff/30001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1284:
--installing_packages_count_;
Maybe adding a dcheck to test that this never goes negative?

https://codereview.chromium.org/2585503002/diff/30001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2585503002/diff/30001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:366: bool
IsNewPackageInSystem(const std::string& package_name) const;
maybe "isUnknownPackage()"?

[khmel, 2016-12-16 15:29:38]

khmel@chromium.org changed reviewers:
+ dcheng@chromium.org

[khmel, 2016-12-16 15:29:39]

Thank you Stefan for comments, I updated the code.

Also adding dcheng@ (mojom)

PTAL

https://codereview.chromium.org/2585503002/diff/30001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/30001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1272:
++installing_packages_count_;
On 2016/12/16 04:48:54, Mr4D wrote:
> Maybe nothing, but:
> 
> Would it be possible that a download starts multiple times? E.g. when the
> android container dies and gets re-started (or another coincidence)?
> Would we need to remember what got installed to properly "check in and out"?

That is good point, in this case ArcAppListPrefs::OnInstanceClosed() is called
(L 687) and we reset installing_packages_count_

https://codereview.chromium.org/2585503002/diff/30001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1284:
--installing_packages_count_;
On 2016/12/16 04:48:54, Mr4D wrote:
> Maybe adding a dcheck to test that this never goes negative?

Yep, done

https://codereview.chromium.org/2585503002/diff/30001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right):

https://codereview.chromium.org/2585503002/diff/30001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:366: bool
IsNewPackageInSystem(const std::string& package_name) const;
On 2016/12/16 04:48:54, Mr4D wrote:
> maybe "isUnknownPackage()"? 

Good name, thank you

[khmel, 2016-12-20 16:34:03]

Hi,

Gentle ping,

Thank you!

[xiyuan, 2016-12-20 16:43:39]

lgtm

Sorry for the delay. Dropped out of my radar somehow. :p

[dcheng, 2016-12-20 20:04:24]

https://codereview.chromium.org/2585503002/diff/50001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/50001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1273: void
ArcAppListPrefs::OnInstallationSetActive(const std::string& package_name) {
I guess there will be a followup CL that implements more logic here?

[khmel, 2016-12-20 20:19:05]

https://codereview.chromium.org/2585503002/diff/50001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/50001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1273: void
ArcAppListPrefs::OnInstallationSetActive(const std::string& package_name) {
On 2016/12/20 20:04:24, dcheng wrote:
> I guess there will be a followup CL that implements more logic here?

I have plan to use OnInstallationSetActive + OnInstallationProgress for
implementing installation progress in App Launcher. However for my nearest 2
tasks this is not required and I can drop these 2 from this CL.

WDYT?

[dcheng, 2016-12-21 03:32:53]

https://codereview.chromium.org/2585503002/diff/50001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/50001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1273: void
ArcAppListPrefs::OnInstallationSetActive(const std::string& package_name) {
On 2016/12/20 20:19:05, khmel wrote:
> On 2016/12/20 20:04:24, dcheng wrote:
> > I guess there will be a followup CL that implements more logic here?
> 
> I have plan to use OnInstallationSetActive + OnInstallationProgress for
> implementing installation progress in App Launcher. However for my nearest 2
> tasks this is not required and I can drop these 2 from this CL.
> 
> WDYT?

If you could do that, I would appreciate it. Thanks!

[khmel, 2016-12-21 16:15:54]

Remove unused methods,

PTAL

Thanks!

https://codereview.chromium.org/2585503002/diff/50001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/50001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1273: void
ArcAppListPrefs::OnInstallationSetActive(const std::string& package_name) {
On 2016/12/21 03:32:53, dcheng wrote:
> On 2016/12/20 20:19:05, khmel wrote:
> > On 2016/12/20 20:04:24, dcheng wrote:
> > > I guess there will be a followup CL that implements more logic here?
> > 
> > I have plan to use OnInstallationSetActive + OnInstallationProgress for
> > implementing installation progress in App Launcher. However for my nearest 2
> > tasks this is not required and I can drop these 2 from this CL.
> > 
> > WDYT?
> 
> If you could do that, I would appreciate it. Thanks!

Done.

[dcheng, 2016-12-21 20:52:25]

https://codereview.chromium.org/2585503002/diff/70001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/70001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1258: const std::string&
package_name) {
Two followup questions (sorry for noticing in the initial review):

- Do we need these parameters
- If these parameters aren't needed, can ARC++ just keep track of this, so we
can simplify this to:

OnStartedInstallingPackages
OnFinishedInstallingPackages

So we don't have to keep state on the Chrome side?

(As it stands, ARC++ could potentially send these out of order, or send some
messages twice. It's fairly innocuous, but that means things like
--installing_packages_count_ could go negative, etc)

[khmel, 2016-12-21 21:10:22]

https://codereview.chromium.org/2585503002/diff/70001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):

https://codereview.chromium.org/2585503002/diff/70001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1258: const std::string&
package_name) {
On 2016/12/21 20:52:25, dcheng wrote:
> Two followup questions (sorry for noticing in the initial review):
> 
> - Do we need these parameters
> - If these parameters aren't needed, can ARC++ just keep track of this, so we
> can simplify this to:
> 
> OnStartedInstallingPackages
> OnFinishedInstallingPackages
> 
> So we don't have to keep state on the Chrome side?
> 
> (As it stands, ARC++ could potentially send these out of order, or send some
> messages twice. It's fairly innocuous, but that means things like
> --installing_packages_count_ could go negative, etc)

In this particular CL I don't need name/package name but in next CL I use
package_name to resolve situation when default apps are not available:
https://codereview.chromium.org/2596043002. I am waiting this CL submitted to
continue work on next task.

> (As it stands, ARC++ could potentially send these out of order, or send some
> messages twice. It's fairly innocuous, but that means things like
> --installing_packages_count_ could go negative, etc)

Is it because of mojo nature? Based on logic from Android side this should not
happen. Do I understand you correctly?

[dcheng, 2016-12-21 21:21:45]

On 2016/12/21 21:10:22, khmel wrote:
>
https://codereview.chromium.org/2585503002/diff/70001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):
> 
>
https://codereview.chromium.org/2585503002/diff/70001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1258: const std::string&
> package_name) {
> On 2016/12/21 20:52:25, dcheng wrote:
> > Two followup questions (sorry for noticing in the initial review):
> > 
> > - Do we need these parameters
> > - If these parameters aren't needed, can ARC++ just keep track of this, so
we
> > can simplify this to:
> > 
> > OnStartedInstallingPackages
> > OnFinishedInstallingPackages
> > 
> > So we don't have to keep state on the Chrome side?
> > 
> > (As it stands, ARC++ could potentially send these out of order, or send some
> > messages twice. It's fairly innocuous, but that means things like
> > --installing_packages_count_ could go negative, etc)
> 
> In this particular CL I don't need name/package name but in next CL I use
> package_name to resolve situation when default apps are not available:
> https://codereview.chromium.org/2596043002. I am waiting this CL submitted to
> continue work on next task.

Would it be OK to add the params in the next CL then so they can be reviewed in
context?

> 
> > (As it stands, ARC++ could potentially send these out of order, or send some
> > messages twice. It's fairly innocuous, but that means things like
> > --installing_packages_count_ could go negative, etc)
> 
> Is it because of mojo nature? Based on logic from Android side this should not
> happen. Do I understand you correctly?

The ARC++ container is less trusted than the browser process, so ideally, the
Chrome browser process should not trust IPCs from the ARC++ container.
I'm OK with this; messing up the state means we will be a bit confused about
when installation starts/finishes, but it seems like that's it. But perhaps we
should explicitly prevent the count from going < 0.

[khmel, 2016-12-21 22:06:56]

On 2016/12/21 21:21:45, dcheng wrote:
> On 2016/12/21 21:10:22, khmel wrote:
> >
>
https://codereview.chromium.org/2585503002/diff/70001/chrome/browser/ui/app_l...
> > File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right):
> > 
> >
>
https://codereview.chromium.org/2585503002/diff/70001/chrome/browser/ui/app_l...
> > chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:1258: const
std::string&
> > package_name) {
> > On 2016/12/21 20:52:25, dcheng wrote:
> > > Two followup questions (sorry for noticing in the initial review):
> > > 
> > > - Do we need these parameters
> > > - If these parameters aren't needed, can ARC++ just keep track of this, so
> we
> > > can simplify this to:
> > > 
> > > OnStartedInstallingPackages
> > > OnFinishedInstallingPackages
> > > 
> > > So we don't have to keep state on the Chrome side?
> > > 
> > > (As it stands, ARC++ could potentially send these out of order, or send
some
> > > messages twice. It's fairly innocuous, but that means things like
> > > --installing_packages_count_ could go negative, etc)
> > 
> > In this particular CL I don't need name/package name but in next CL I use
> > package_name to resolve situation when default apps are not available:
> > https://codereview.chromium.org/2596043002. I am waiting this CL submitted
to
> > continue work on next task.
> 
> Would it be OK to add the params in the next CL then so they can be reviewed
in
> context?
> 
> > 
> > > (As it stands, ARC++ could potentially send these out of order, or send
some
> > > messages twice. It's fairly innocuous, but that means things like
> > > --installing_packages_count_ could go negative, etc)
> > 
> > Is it because of mojo nature? Based on logic from Android side this should
not
> > happen. Do I understand you correctly?
> 
> The ARC++ container is less trusted than the browser process, so ideally, the
> Chrome browser process should not trust IPCs from the ARC++ container.
> I'm OK with this; messing up the state means we will be a bit confused about
> when installation starts/finishes, but it seems like that's it. But perhaps we
> should explicitly prevent the count from going < 0.

Thank you for idea, I removed arguments and protected counter to be
non-negative.
PTAL

[dcheng, 2016-12-21 22:30:41]

mojo lgtm

[khmel, 2016-12-21 22:34:14]

Description was changed from

==========
arc: Prevent App list popping on each app installed from batch.

There is possibility to manually install packages in batch mode.
In this mode App list is shown on each package installation. This
is annoying for user. This CL show App launcher only for first
installed in batch app. If user does not close App list he may
observer how next apps appear in list. If he closes it, now App
list will be shown unless next batch installation is started.
This also introduces new mojo notifications to support installation
sessions from Play Store. These notifications are used in current
task and will be used in next planned task to detect the case
when default app is not available after Arc start. There is yet
another idea to implement installation progress in App launcher.

TEST=Manually on device.
TEST=unit_tests + extended browser_tests.
BUG=b/33420574
BUG=674656
==========

to

==========
arc: Prevent App list popping on each app installed from batch.

There is possibility to manually install packages in batch mode.
In this mode App list is shown on each package installation. This
is annoying for user. This CL show App launcher only for first
installed in batch app. If user does not close App list he may
observer how next apps appear in list. If he closes it, now App
list will be shown unless next batch installation is started.
This also introduces new mojo notifications to support installation
sessions from Play Store. These notifications are used in current
task and will be used in next planned task to detect the case
when default app is not available after Arc start. There is yet
another idea to implement installation progress in App launcher.

TEST=Manually on device.
TEST=unit_tests + extended browser_tests.
TBR=skuhne@chromium.org
BUG=b/33420574
BUG=674656
==========

[khmel, 2016-12-21 22:35:19]

Thank you for your review.
I set Stefan TBR because he is OOO. He already reviewed Android ag/1716470 and I
addressed his comments in this CL.

[khmel, 2016-12-21 22:35:36]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-12-21 22:35:37]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2585503002/#ps90001
(title: "removed unused parameters, protect counter to be negative")

[commit-bot: I haz the power, 2016-12-21 22:36:12]

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-12-21 23:35:06]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1482359736797670,
"parent_rev": "b2a72b9d049f14b7b4ed383335703736ea968ebc", "commit_rev":
"4b42dcec00bdc90a94105e7221d468a04c61e4e0"}

[commit-bot: I haz the power, 2016-12-21 23:35:36]

Description was changed from

==========
arc: Prevent App list popping on each app installed from batch.

There is possibility to manually install packages in batch mode.
In this mode App list is shown on each package installation. This
is annoying for user. This CL show App launcher only for first
installed in batch app. If user does not close App list he may
observer how next apps appear in list. If he closes it, now App
list will be shown unless next batch installation is started.
This also introduces new mojo notifications to support installation
sessions from Play Store. These notifications are used in current
task and will be used in next planned task to detect the case
when default app is not available after Arc start. There is yet
another idea to implement installation progress in App launcher.

TEST=Manually on device.
TEST=unit_tests + extended browser_tests.
TBR=skuhne@chromium.org
BUG=b/33420574
BUG=674656
==========

to

==========
arc: Prevent App list popping on each app installed from batch.

There is possibility to manually install packages in batch mode.
In this mode App list is shown on each package installation. This
is annoying for user. This CL show App launcher only for first
installed in batch app. If user does not close App list he may
observer how next apps appear in list. If he closes it, now App
list will be shown unless next batch installation is started.
This also introduces new mojo notifications to support installation
sessions from Play Store. These notifications are used in current
task and will be used in next planned task to detect the case
when default app is not available after Arc start. There is yet
another idea to implement installation progress in App launcher.

TEST=Manually on device.
TEST=unit_tests + extended browser_tests.
TBR=skuhne@chromium.org
BUG=b/33420574
BUG=674656

Review-Url: https://codereview.chromium.org/2585503002
==========

[commit-bot: I haz the power, 2016-12-21 23:35:37]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-12-21 23:38:07]

Description was changed from

==========
arc: Prevent App list popping on each app installed from batch.

There is possibility to manually install packages in batch mode.
In this mode App list is shown on each package installation. This
is annoying for user. This CL show App launcher only for first
installed in batch app. If user does not close App list he may
observer how next apps appear in list. If he closes it, now App
list will be shown unless next batch installation is started.
This also introduces new mojo notifications to support installation
sessions from Play Store. These notifications are used in current
task and will be used in next planned task to detect the case
when default app is not available after Arc start. There is yet
another idea to implement installation progress in App launcher.

TEST=Manually on device.
TEST=unit_tests + extended browser_tests.
TBR=skuhne@chromium.org
BUG=b/33420574
BUG=674656

Review-Url: https://codereview.chromium.org/2585503002
==========

to

==========
arc: Prevent App list popping on each app installed from batch.

There is possibility to manually install packages in batch mode.
In this mode App list is shown on each package installation. This
is annoying for user. This CL show App launcher only for first
installed in batch app. If user does not close App list he may
observer how next apps appear in list. If he closes it, now App
list will be shown unless next batch installation is started.
This also introduces new mojo notifications to support installation
sessions from Play Store. These notifications are used in current
task and will be used in next planned task to detect the case
when default app is not available after Arc start. There is yet
another idea to implement installation progress in App launcher.

TEST=Manually on device.
TEST=unit_tests + extended browser_tests.
TBR=skuhne@chromium.org
BUG=b/33420574
BUG=674656

Committed: https://crrev.com/09a187e794db3a8cd261d7652658e51864a5a64d
Cr-Commit-Position: refs/heads/master@{#440262}
==========

[commit-bot: I haz the power, 2016-12-21 23:38:09]

Patchset 6 (id:??) landed as
https://crrev.com/09a187e794db3a8cd261d7652658e51864a5a64d
Cr-Commit-Position: refs/heads/master@{#440262}