Fix WebAPK crashes on launch on Jellybean.

Fix WebAPK crashes on launch on Jellybean.

The crash is caused by using getId() instead of mWebappInfo.id() when registering
WebAPK. WebApkManagedActivity is launched on Jellybean instead of WebApkActivity,
and getId() in WebApkManagedActivity returns the index of the managered activity,
rather than the Webapp id.

BUG=640434
Committed: https://crrev.com/be21c85ff9c6e897496e1731a76e9ef24af600ac
Cr-Commit-Position: refs/heads/master@{#418949}

[Xi Han, 2016-09-13 19:42:41]

Description was changed from

==========
Fix crashes on Jellybean.

BUG=640434
==========

to

==========
Fix WebAPK crashes on launch on Jellybean.

The crash is caused by using getId() instead of mWebappInfo.id() when
registering
WebAPK. WebApkManagedActivity is launched on Jellybean instead of
WebApkActivity,
and getId() in WebApkManagedActivity returns the index of the managered
activity,
rather than the Webapp id.

BUG=640434
==========

[Xi Han, 2016-09-13 19:43:42]

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

[Xi Han, 2016-09-13 19:43:43]

Hi Peter, could you please take a look? Thanks!

[pkotwicz, 2016-09-13 19:51:33]

We should add a warning for WebappActivity#getId() that subclasses overwrite the
method.

I would be tempted to rename WebappActivity#getId() too but I can't think of a
good name

[Xi Han, 2016-09-13 20:38:47]

Hi Peter, PTAL, thanks!

[pkotwicz, 2016-09-13 21:34:46]

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

[pkotwicz, 2016-09-13 21:34:46]

+dfalcantara@ for his thoughts

https://codereview.chromium.org/2339623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/2339623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:511:
protected String getId() {
I recommend renaming getId() to getDirectoryId() to make things super clear.
Adding dfalcantara@ as a reviewer in case he has a better name in mind.

[gone, 2016-09-13 21:38:27]

https://codereview.chromium.org/2339623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/2339623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:511:
protected String getId() {
On 2016/09/13 21:34:46, pkotwicz wrote:
> I recommend renaming getId() to getDirectoryId() to make things super clear.
> Adding dfalcantara@ as a reviewer in case he has a better name in mind.

This does more than return an ID for directories; the code that uses it
differently is even called out above WebApkActivity.java.

[pkotwicz, 2016-09-13 22:10:17]

https://codereview.chromium.org/2339623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/2339623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:511:
protected String getId() {
Dan what do you suggest renaming this function to?

It looks like the result of getId() is only used in:
- WebappDirectoryManager#cleanUpDirectories()
- WebappDirectoryManager#getWebappDirectory()

What call sites am I missing?

[gone, 2016-09-13 22:24:13]

https://codereview.chromium.org/2339623002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/2339623002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:511:
protected String getId() {
On 2016/09/13 22:10:17, pkotwicz wrote:
> Dan what do you suggest renaming this function to?
> 
> It looks like the result of getId() is only used in:
> - WebappDirectoryManager#cleanUpDirectories()
> - WebappDirectoryManager#getWebappDirectory()
> 
> What call sites am I missing?

After this CL, those are the remaining callsites.  The question is why you're
changing semantics.  The ID has a specific definition that is tied to the
ActivityAssigner in WebappLauncherActivity -- getId() returns what the ID of the
Activity is in the Intent  to launch the WebappActivity.  That's not directory
related.

[pkotwicz, 2016-09-14 14:28:03]

I am suggesting renaming WebappActivity#getId() to make it harder to confuse
WebappInfo#id() and WebapActivity#getId(). I am open to any name at all. I agree
that WebappActivity#getDirectoryId() is a bad name.

[gone, 2016-09-14 15:23:50]

On 2016/09/14 14:28:03, pkotwicz wrote:
> I am suggesting renaming WebappActivity#getId() to make it harder to confuse
> WebappInfo#id() and WebapActivity#getId(). I am open to any name at all. I
agree
> that WebappActivity#getDirectoryId() is a bad name.

Eh, what about WebappActivity#getActivityId() then?

[Xi Han, 2016-09-14 15:45:50]

On 2016/09/14 15:23:50, dfalcantara wrote:
> On 2016/09/14 14:28:03, pkotwicz wrote:
> > I am suggesting renaming WebappActivity#getId() to make it harder to confuse
> > WebappInfo#id() and WebapActivity#getId(). I am open to any name at all. I
> agree
> > that WebappActivity#getDirectoryId() is a bad name.
> 
> Eh, what about WebappActivity#getActivityId() then?

It looks good to me.

[Xi Han, 2016-09-15 13:55:14]

Rename the function getId() to getActivityId(). PTAL, thanks!

[pkotwicz, 2016-09-15 15:37:05]

LGTM with nits

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

https://codereview.chromium.org/2339623002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:507:
* Note: do not call this function when you need |mWebappInfo.id()|. Subclasses
like
Nits:
|mWebappInfo.id()| -> {@link WebappInfo#id()}
WebappManageredActivity spelled inkorrektlee 😃 -> WebappManagedActivity

[Xi Han, 2016-09-15 16:13:33]

Hi Dan, PTAL, thanks!

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

https://codereview.chromium.org/2339623002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:507:
* Note: do not call this function when you need |mWebappInfo.id()|. Subclasses
like
On 2016/09/15 15:37:05, pkotwicz wrote:
> Nits:
> |mWebappInfo.id()| -> {@link WebappInfo#id()}
> WebappManageredActivity spelled inkorrektlee 😃 -> WebappManagedActivity

Korrektlee, thanks for catching it:P

[gone, 2016-09-15 17:30:24]

yar, lgtm

[Xi Han, 2016-09-15 19:42:17]

The CQ bit was checked by hanxi@chromium.org

[Xi Han, 2016-09-15 19:42:17]

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

[commit-bot: I haz the power, 2016-09-15 19:42:56]

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-09-15 20:16:40]

Description was changed from

==========
Fix WebAPK crashes on launch on Jellybean.

The crash is caused by using getId() instead of mWebappInfo.id() when
registering
WebAPK. WebApkManagedActivity is launched on Jellybean instead of
WebApkActivity,
and getId() in WebApkManagedActivity returns the index of the managered
activity,
rather than the Webapp id.

BUG=640434
==========

to

==========
Fix WebAPK crashes on launch on Jellybean.

The crash is caused by using getId() instead of mWebappInfo.id() when
registering
WebAPK. WebApkManagedActivity is launched on Jellybean instead of
WebApkActivity,
and getId() in WebApkManagedActivity returns the index of the managered
activity,
rather than the Webapp id.

BUG=640434
==========

[commit-bot: I haz the power, 2016-09-15 20:16:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-15 20:18:07]

Description was changed from

==========
Fix WebAPK crashes on launch on Jellybean.

The crash is caused by using getId() instead of mWebappInfo.id() when
registering
WebAPK. WebApkManagedActivity is launched on Jellybean instead of
WebApkActivity,
and getId() in WebApkManagedActivity returns the index of the managered
activity,
rather than the Webapp id.

BUG=640434
==========

to

==========
Fix WebAPK crashes on launch on Jellybean.

The crash is caused by using getId() instead of mWebappInfo.id() when
registering
WebAPK. WebApkManagedActivity is launched on Jellybean instead of
WebApkActivity,
and getId() in WebApkManagedActivity returns the index of the managered
activity,
rather than the Webapp id.

BUG=640434

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

[commit-bot: I haz the power, 2016-09-15 20:18:08]

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