The method WebappRegistry#isWebApkInstalled now reuses InstallerDelegate#isInstalled so as not to r…

The method WebappRegistry#isWebApkInstalled now reuses InstallerDelegate#isInstalled so as not to repeat code.

BUG=639428
Review-Url: https://codereview.chromium.org/2632693002
Cr-Commit-Position: refs/heads/master@{#447233}
Committed: https://chromium.googlesource.com/chromium/src/+/63066a0f4c6ac09670cac21748cc88f3bc14a36a

[gonzalon, 2017-01-13 15:37:28]

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

[commit-bot: I haz the power, 2017-01-13 15:37:42]

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-01-13 16:12:41]

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

[commit-bot: I haz the power, 2017-01-13 16:12:42]

Dry run: This issue passed the CQ dry run.

[gonzalon, 2017-01-20 15:34:08]

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

[commit-bot: I haz the power, 2017-01-20 15:35:06]

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

[gonzalon, 2017-01-20 15:38:02]

gonzalon@chromium.org changed reviewers:
+ hartmanng@chromium.org, pkotwicz@chromium.org

[gonzalon, 2017-01-20 15:38:02]

Hi, just a small bug I found on the Tracker.

[hartmanng, 2017-01-20 15:46:02]

lgtm

[commit-bot: I haz the power, 2017-01-20 16:11:42]

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

[commit-bot: I haz the power, 2017-01-20 16:11:43]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-01-20 18:23:41]

Can you double check whether the new code is faster or slower than the old code.
Android API calls can be pretty slow.

If there is no significant speed difference l-g-t-m

[gonzalon, 2017-01-23 21:11:22]

On 2017/01/20 18:23:41, pkotwicz wrote:
> Can you double check whether the new code is faster or slower than the old
code.
> Android API calls can be pretty slow.
> 
> If there is no significant speed difference l-g-t-m

Just tried the code and one implementation uses 2-3ms while the other ~15ms.
15ms doesn't sound like much, but I still swapped it to use the faster
implementation in case some other performance-sensitive code uses this function
in the future.

[pkotwicz, 2017-01-24 05:38:52]

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

[pkotwicz, 2017-01-24 05:38:52]

LGTM

+Dan for insight as to whether the new implementation of
InstallerDelegate#isInstalled() is correct. It looks correct to me, but I am
most likely missing something

[pkotwicz, 2017-01-24 05:54:33]

I am not sure if the "new way" will work for APKs without an activity. I don't
know if we case about those though

[pkotwicz, 2017-01-24 06:07:34]

+Dan for insight as to whether the new implementation of
InstallerDelegate#isInstalled() is correct. It looks correct to me, but I am
most likely missing something

Correction, it looks like the new implementation of
InstallerDelegate#isInstalled() works for APKs without activities

[gone, 2017-01-24 23:07:23]

On 2017/01/24 06:07:34, pkotwicz wrote:
> +Dan for insight as to whether the new implementation of
> InstallerDelegate#isInstalled() is correct. It looks correct to me, but I am
> most likely missing something
> 
> Correction, it looks like the new implementation of
> InstallerDelegate#isInstalled() works for APKs without activities

Eh, how does that work when the flag says it only returns GET_ACTIVITIES?

[gone, 2017-01-24 23:10:01]

On 2017/01/24 23:07:23, dfalcantara (load balance plz) wrote:
> On 2017/01/24 06:07:34, pkotwicz wrote:
> > +Dan for insight as to whether the new implementation of
> > InstallerDelegate#isInstalled() is correct. It looks correct to me, but I am
> > most likely missing something
> > 
> > Correction, it looks like the new implementation of
> > InstallerDelegate#isInstalled() works for APKs without activities
> 
> Eh, how does that work when the flag says it only returns GET_ACTIVITIES?

I guess PackageManager returns an empty array in that case and doesn't throw an
exception?

[gonzalon, 2017-01-24 23:14:11]

On 2017/01/24 23:10:01, dfalcantara (load balance plz) wrote:
> On 2017/01/24 23:07:23, dfalcantara (load balance plz) wrote:
> > On 2017/01/24 06:07:34, pkotwicz wrote:
> > > +Dan for insight as to whether the new implementation of
> > > InstallerDelegate#isInstalled() is correct. It looks correct to me, but I
am
> > > most likely missing something
> > > 
> > > Correction, it looks like the new implementation of
> > > InstallerDelegate#isInstalled() works for APKs without activities
> > 
> > Eh, how does that work when the flag says it only returns GET_ACTIVITIES?
> 
> I guess PackageManager returns an empty array in that case and doesn't throw
an
> exception?

Yes, I wondered the same, the documentation
(https://developer.android.com/reference/android/content/pm/PackageManager.htm...,
int) ) says that the activities information is returned on the activities field,
and that an exception is thrown if the package is not found. If no activities
are found the list would just be empty. 

I wonder though if it's necessary to pass a flag, I think passing 0 might be
enough, I'll check.

[gonzalon, 2017-01-24 23:31:19]

On 2017/01/24 23:14:11, gonzalon wrote:
> On 2017/01/24 23:10:01, dfalcantara (load balance plz) wrote:
> > On 2017/01/24 23:07:23, dfalcantara (load balance plz) wrote:
> > > On 2017/01/24 06:07:34, pkotwicz wrote:
> > > > +Dan for insight as to whether the new implementation of
> > > > InstallerDelegate#isInstalled() is correct. It looks correct to me, but
I
> am
> > > > most likely missing something
> > > > 
> > > > Correction, it looks like the new implementation of
> > > > InstallerDelegate#isInstalled() works for APKs without activities
> > > 
> > > Eh, how does that work when the flag says it only returns GET_ACTIVITIES?
> > 
> > I guess PackageManager returns an empty array in that case and doesn't throw
> an
> > exception?
> 
> Yes, I wondered the same, the documentation
>
(https://developer.android.com/reference/android/content/pm/PackageManager.htm...,
> int) ) says that the activities information is returned on the activities
field,
> and that an exception is thrown if the package is not found. If no activities
> are found the list would just be empty. 
> 
> I wonder though if it's necessary to pass a flag, I think passing 0 might be
> enough, I'll check.

Just checked, it works fine without passing in any flags. Removed the activity
one :)

[gone, 2017-01-24 23:44:47]

On 2017/01/24 23:31:19, gonzalon wrote:
> On 2017/01/24 23:14:11, gonzalon wrote:
> > On 2017/01/24 23:10:01, dfalcantara (load balance plz) wrote:
> > > On 2017/01/24 23:07:23, dfalcantara (load balance plz) wrote:
> > > > On 2017/01/24 06:07:34, pkotwicz wrote:
> > > > > +Dan for insight as to whether the new implementation of
> > > > > InstallerDelegate#isInstalled() is correct. It looks correct to me,
but
> I
> > am
> > > > > most likely missing something
> > > > > 
> > > > > Correction, it looks like the new implementation of
> > > > > InstallerDelegate#isInstalled() works for APKs without activities
> > > > 
> > > > Eh, how does that work when the flag says it only returns
GET_ACTIVITIES?
> > > 
> > > I guess PackageManager returns an empty array in that case and doesn't
throw
> > an
> > > exception?
> > 
> > Yes, I wondered the same, the documentation
> >
>
(https://developer.android.com/reference/android/content/pm/PackageManager.htm...,
> > int) ) says that the activities information is returned on the activities
> field,
> > and that an exception is thrown if the package is not found. If no
activities
> > are found the list would just be empty. 
> > 
> > I wonder though if it's necessary to pass a flag, I think passing 0 might be
> > enough, I'll check.
> 
> Just checked, it works fine without passing in any flags. Removed the activity
> one :)

Cool, lgtm if it still works for WebAPK.

[gonzalon, 2017-01-26 19:46:19]

The CQ bit was checked by gonzalon@chromium.org

[gonzalon, 2017-01-26 19:46:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
hartmanng@chromium.org, pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2632693002/#ps60001
(title: "")

[commit-bot: I haz the power, 2017-01-26 19:46:54]

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-01-26 21:46:38]

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

[commit-bot: I haz the power, 2017-01-26 21:46:39]

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...)

[gonzalon, 2017-01-27 20:13:41]

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

[commit-bot: I haz the power, 2017-01-27 20:14:19]

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-01-28 00:41:37]

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

[commit-bot: I haz the power, 2017-01-28 00:41:38]

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...)

[gonzalon, 2017-01-30 18:31:41]

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

[commit-bot: I haz the power, 2017-01-30 18:32:37]

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-01-30 20:15:36]

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

[commit-bot: I haz the power, 2017-01-30 20:15:37]

Dry run: This issue passed the CQ dry run.

[gonzalon, 2017-01-31 15:04:20]

The CQ bit was checked by gonzalon@chromium.org

[gonzalon, 2017-01-31 15:04:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, dfalcantara@chromium.org, hartmanng@chromium.org
Link to the patchset: https://codereview.chromium.org/2632693002/#ps100001
(title: "The method WebappRegistry#isWebApkInstalled now reuses
InstallerDelegate#isInstalled so as not to r…")

[commit-bot: I haz the power, 2017-01-31 15:04:35]

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-01-31 15:08:26]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485875060068350,
"parent_rev": "90f3f9f959c9a43aa9dc7666380a5cffb11ca52c", "commit_rev":
"63066a0f4c6ac09670cac21748cc88f3bc14a36a"}

[commit-bot: I haz the power, 2017-01-31 15:09:02]

Description was changed from

==========
The method WebappRegistry#isWebApkInstalled now reuses
InstallerDelegate#isInstalled so as not to repeat code.

BUG=639428
==========

to

==========
The method WebappRegistry#isWebApkInstalled now reuses
InstallerDelegate#isInstalled so as not to repeat code.

BUG=639428

Review-Url: https://codereview.chromium.org/2632693002
Cr-Commit-Position: refs/heads/master@{#447233}
Committed:
https://chromium.googlesource.com/chromium/src/+/63066a0f4c6ac09670cac21748cc...
==========

[commit-bot: I haz the power, 2017-01-31 15:09:03]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/63066a0f4c6ac09670cac21748cc...