Remove untrusted sources installation flow.

Remove untrusted sources installation flow.

This model of installation involved a user having to lower the security level
of their device to allow apps to install APKs. It was only used for developer
and preview purposes. It's no longer necessary so clean things up.

BUG=708274
Review-Url: https://codereview.chromium.org/2791983003
Cr-Commit-Position: refs/heads/master@{#462462}
Committed: https://chromium.googlesource.com/chromium/src/+/a8d6359dff31bf5db91295dadac0aff2bc02182d

[Yaron, 2017-04-03 17:45:28]

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

[commit-bot: I haz the power, 2017-04-03 17:46:13]

Dry run: 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, 2017-04-03 17:49:46]

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

[commit-bot: I haz the power, 2017-04-03 17:49:47]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Yaron, 2017-04-03 17:50:15]

yfriedman@chromium.org changed reviewers:
+ dominickn@chromium.org

[Yaron, 2017-04-03 17:50:15]

https://codereview.chromium.org/2791983003/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_installer_unittest.cc (right):

https://codereview.chromium.org/2791983003/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer_unittest.cc:99:
can_install_webapks_(true) {}
note the default change which is intentional. I think it makes sense for most of
the tests to assume you can install and I added an explicit test to ensure it
fails appropriately when disabled

[Yaron, 2017-04-03 18:06:34]

On 2017/04/03 17:50:15, Yaron (limited availability) wrote:
>
https://codereview.chromium.org/2791983003/diff/1/chrome/browser/android/weba...
> File chrome/browser/android/webapk/webapk_installer_unittest.cc (right):
> 
>
https://codereview.chromium.org/2791983003/diff/1/chrome/browser/android/weba...
> chrome/browser/android/webapk/webapk_installer_unittest.cc:99:
> can_install_webapks_(true) {}
> note the default change which is intentional. I think it makes sense for most
of
> the tests to assume you can install and I added an explicit test to ensure it
> fails appropriately when disabled

you can hold off unless I ping again - need to rebase

[Yaron, 2017-04-03 18:44:24]

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

[Yaron, 2017-04-03 18:44:45]

should be better now

[commit-bot: I haz the power, 2017-04-03 18:45:39]

Dry run: 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, 2017-04-03 21:09:34]

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

[commit-bot: I haz the power, 2017-04-03 21:09:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dominickn, 2017-04-03 23:00:39]

\o/

Looks like the WebApkUpdateManagerTests are broken. Also, there are comments
referencing the unknown sources flow in
chrome/browser/android/webapk/webapk_install_service.h:31 and
chrome/browser/android/webapk/chrome_webapk_host.h:32 that would be good to
remove.

https://codereview.chromium.org/2791983003/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2791983003/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:397: if (!CanInstallWebApks())
{
Could this call be made before we've fetched the URL?

https://codereview.chromium.org/2791983003/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.h (right):

https://codereview.chromium.org/2791983003/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.h:36: // Talks to Chrome WebAPK
server to download metadata about WebApk and issue
nit: "about a WebAPK"

[dominickn, 2017-04-04 04:01:15]

Found more references to "unsigned sources" in
chrome/browser/android/shortcut_helper.cc, so that can go too. :)

[Yaron, 2017-04-04 16:00:44]

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

[commit-bot: I haz the power, 2017-04-04 16:01:04]

Dry run: 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, 2017-04-04 16:40:41]

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

[commit-bot: I haz the power, 2017-04-04 16:40:42]

Dry run: This issue passed the CQ dry run.

[Yaron, 2017-04-04 16:42:02]

https://codereview.chromium.org/2791983003/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.h (right):

https://codereview.chromium.org/2791983003/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.h:36: // Talks to Chrome WebAPK
server to download metadata about WebApk and issue
On 2017/04/03 23:00:39, dominickn wrote:
> nit: "about a WebAPK"

Done.

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (left):

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:166: if
(ChromeWebApkHost::GetGooglePlayInstallState() ==
the comment was unnecessary, but I'm truthfully unsure about the conditional. We
can probably assume it's SUPPORTED if we get here?

[pkotwicz, 2017-04-04 19:06:07]

pkotwicz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[pkotwicz, 2017-04-04 19:06:08]

Some drive by comments.

Please also attach a bug number to this CL. Especially since the cleanup will
take up several CLs

https://codereview.chromium.org/2791983003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java
(right):

https://codereview.chromium.org/2791983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:49:
public static boolean areUpdatesEnabled() {
For a follow up CL: We can make this function always return true. It is now very
very hard to be in a state where a user launches a WebAPK in WebAPK mode but
updates are disabled.

If !ChromeWebApkHost#isEnabled() WebAPKs that a user has created will open in a
Chrome tab.

https://codereview.chromium.org/2791983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:71:
* Returns whether installing WebAPKs is possible.
Nit: You can make this comment one line now :)

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (left):

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:166: if
(ChromeWebApkHost::GetGooglePlayInstallState() ==
We can. The caller only calls ShortcutHelper::InstallWebApkWithSkBitmap() if
ChromeWebApkHost::CanInstallWebApk()

See AppBannerInfoBarDelegateAndroid::Create()

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:13: #include
"base/files/file_path.h"
Nit: You can remove this include

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:14: #include
"base/files/file_util.h"
Nit: You can remove this include

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:383: if (task_type_ == UPDATE
&& signed_download_url.is_empty()) {
In a separate CL, we should clean this up. Does the server send an empty WAM
token if the server does not have a newer WebAPK to update to?
Ideally we would not deal with the download URL at all

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.h (right):

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.h:19: #include
"chrome/browser/net/file_downloader.h"
Nit: You can remove this include now

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.h:170:
std::unique_ptr<FileDownloader> downloader_;
Nit: You can remove this now

[Yaron, 2017-04-04 19:44:36]

Description was changed from

==========
Remove untrusted sources installation flow.

This model of installation involved a user having to lower the security level
of their device to allow apps to install APKs. It was only used for developer
and preview purposes. It's no longer necessary so clean things up.

BUG=
==========

to

==========
Remove untrusted sources installation flow.

This model of installation involved a user having to lower the security level
of their device to allow apps to install APKs. It was only used for developer
and preview purposes. It's no longer necessary so clean things up.

BUG=708274
==========

[Yaron, 2017-04-04 19:49:08]

https://codereview.chromium.org/2791983003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java
(right):

https://codereview.chromium.org/2791983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:49:
public static boolean areUpdatesEnabled() {
On 2017/04/04 19:06:07, pkotwicz wrote:
> For a follow up CL: We can make this function always return true. It is now
very
> very hard to be in a state where a user launches a WebAPK in WebAPK mode but
> updates are disabled.
> 
> If !ChromeWebApkHost#isEnabled() WebAPKs that a user has created will open in
a
> Chrome tab.

Acknowledged.

https://codereview.chromium.org/2791983003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:71:
* Returns whether installing WebAPKs is possible.
On 2017/04/04 19:06:07, pkotwicz wrote:
> Nit: You can make this comment one line now :)

Done.

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (left):

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:166: if
(ChromeWebApkHost::GetGooglePlayInstallState() ==
On 2017/04/04 19:06:07, pkotwicz wrote:
> We can. The caller only calls ShortcutHelper::InstallWebApkWithSkBitmap() if
> ChromeWebApkHost::CanInstallWebApk()
> 
> See AppBannerInfoBarDelegateAndroid::Create()
> 
> 
> 
> 

Done.

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:13: #include
"base/files/file_path.h"
On 2017/04/04 19:06:07, pkotwicz wrote:
> Nit: You can remove this include

Done.

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:14: #include
"base/files/file_util.h"
On 2017/04/04 19:06:07, pkotwicz wrote:
> Nit: You can remove this include

Done.

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:383: if (task_type_ == UPDATE
&& signed_download_url.is_empty()) {
On 2017/04/04 19:06:07, pkotwicz wrote:
> In a separate CL, we should clean this up. Does the server send an empty WAM
> token if the server does not have a newer WebAPK to update to?
> Ideally we would not deal with the download URL at all

I know it sends an empty URL but don't know about WAM token. +Glenn

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.h (right):

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.h:19: #include
"chrome/browser/net/file_downloader.h"
On 2017/04/04 19:06:08, pkotwicz wrote:
> Nit: You can remove this include now

Done.

[hartmanng, 2017-04-04 19:53:32]

hartmanng@chromium.org changed reviewers:
+ hartmanng@chromium.org

[hartmanng, 2017-04-04 19:53:33]

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:383: if (task_type_ == UPDATE
&& signed_download_url.is_empty()) {
> In a separate CL, we should clean this up. Does the server send an empty WAM
> token if the server does not have a newer WebAPK to update to?

Yes, the server will send both an empty URL and an empty WAM token.

[Yaron, 2017-04-04 19:56:58]

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2791983003/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:383: if (task_type_ == UPDATE
&& signed_download_url.is_empty()) {
On 2017/04/04 19:53:33, hartmanng wrote:
> > In a separate CL, we should clean this up. Does the server send an empty WAM
> > token if the server does not have a newer WebAPK to update to?
> 
> Yes, the server will send both an empty URL and an empty WAM token.

Err but actually I don't expect play client to back off on update requests so we
might have to keep this

[dominickn, 2017-04-05 00:59:41]

lgtm assuming Peter is happy

[pkotwicz, 2017-04-05 03:49:12]

lgtm

[Yaron, 2017-04-06 13:51:54]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2017-04-06 13:52:06]

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, 2017-04-06 14:26:46]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491486714172870,
"parent_rev": "9ac1d89015b28214fb6876bb4a4a078f5b2dc0eb", "commit_rev":
"a8d6359dff31bf5db91295dadac0aff2bc02182d"}

[commit-bot: I haz the power, 2017-04-06 14:27:35]

Description was changed from

==========
Remove untrusted sources installation flow.

This model of installation involved a user having to lower the security level
of their device to allow apps to install APKs. It was only used for developer
and preview purposes. It's no longer necessary so clean things up.

BUG=708274
==========

to

==========
Remove untrusted sources installation flow.

This model of installation involved a user having to lower the security level
of their device to allow apps to install APKs. It was only used for developer
and preview purposes. It's no longer necessary so clean things up.

BUG=708274

Review-Url: https://codereview.chromium.org/2791983003
Cr-Commit-Position: refs/heads/master@{#462462}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8d6359dff31bf5db91295dadac0...
==========

[commit-bot: I haz the power, 2017-04-06 14:27:36]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a8d6359dff31bf5db91295dadac0...