958356 - rename packageName to unambiguous name in app objects

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

Over in bug 934756, Fabrice suggested we rename the "packageName" property of app objects to "apkName" to avoid confusion over the meaning of the word "package", since packaged apps have both a "webapp package" and an "APK package".

We didn't end up making that change in the patch that landed in that bug, but I think we should do so before we enable the feature, since the potential for confusion is high.

We need to be careful about where we replace "package" with "apk", since it's appropriate to use the word "package" in Fennec's Java code, where "packageName" is the conventional term for an APK's class name.

So we should only make the change in the JavaScript app object itself, any JSON representations of it, and any messages that the DOMApplicationRegistry sends or receives regarding apps.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: apk-name.diff

This patch changes "packageName" to "apkName" and "packages" to "apks" in the following places:

1. The app objects created and stored by Webapps.jsm and AppsUtils.jsm.
2. The messages passed between those modules, WebappManager.jsm, and Java classes in Fennec.

It doesn't touch the names of variables in the Java classes, nor does it change the use of "packageName" in Intent objects that are exchanged between Fennec and webapp APKs.

Thus it carefully distinguishes between Open Web App contexts, where the word "package" could mean either Open Web App package or Native Android Package and Native Android App contexts, where "package" always means Native Android Package.

No fix will absolutely prevent us from ever making a mistake, but this seems like the best way to maximize clarity and minimize the risk of error.

Requesting review from Fabrice for the (one-line!) AppsUtils.jsm change and from WesJ for the other changes.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: apk-package-name.diff

jhugman pointed out this morning that apkName is used elsewhere to mean something different, so this patch changes the name to apkPackageName instead.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8361217 [details] [diff] [review]
apk-package-name.diff

Review of attachment 8361217 [details] [diff] [review]:
-----------------------------------------------------------------

TBH, I would rather that we used apkPackageName everywhere, including Java, to keep things consistent, but I'm happy to let that slide. One little nit though:

::: mobile/android/base/webapp/UninstallListener.java
@@ +46,5 @@
>              JSONObject message = new JSONObject();
>              JSONArray packageNames = new JSONArray();
>              try {
>                  packageNames.put(packageName);
> +                message.put("apks", packageNames);

This seems a little odd. I was thinking in the earlier cases that maybe we'd be better to just use "apk" instead of "apkPackageName", but I'd understand if you thought that was deceptive (they're not apks, the the names of apks). Here you do use "apks" though. Maybe this should be "apkPackageNames" (tbh, I'd rather we just used "apk" everywhere I think... because my brain is too small for such big words)

Comment 4

[James Hugman [:jhugman] [@jhugman]]

I agree with wesj on this one re keeping apkPackageName consistent throughout the codebase, including Java.

A variable called apk may conceivably be referring to the apk's filename, which should probably be called apkFilename.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: fixes nit

(In reply to Wesley Johnston (:wesj) from comment #3)

> TBH, I would rather that we used apkPackageName everywhere, including Java,
> to keep things consistent, but I'm happy to let that slide.

There are a bunch of references to packageName in the Java code, and not just in webapp-related code: packageName is the canonical name for such values in Android APIs.  So it seems preferable to use it whenever possible in the synthetic APK code, for consistency with Android and other Fennec Java code.

For that matter, we could also use it in the messages passed between Java code and Fennec JavaScript code (i.e. WebappManager.jsm).

The only place where it's really important to distinguish between Open Web App package and Android package is in the app records stored in DOMApplicationRegistry, which we won't be able to change easily after we enable synthetic APKs, since they're stored on-disk in persistent JSON data structures.

But there are comparatively few references to that property in those records (specifically: three), so we wouldn't lose much consistency if they were the odd men out.


> ::: mobile/android/base/webapp/UninstallListener.java
> @@ +46,5 @@
> >              JSONObject message = new JSONObject();
> >              JSONArray packageNames = new JSONArray();
> >              try {
> >                  packageNames.put(packageName);
> > +                message.put("apks", packageNames);
> 
> This seems a little odd. I was thinking in the earlier cases that maybe we'd
> be better to just use "apk" instead of "apkPackageName", but I'd understand
> if you thought that was deceptive (they're not apks, the the names of apks).
> Here you do use "apks" though. Maybe this should be "apkPackageNames" (tbh,
> I'd rather we just used "apk" everywhere I think... because my brain is too
> small for such big words)

I hear ya, and I like smaller and simpler too, even at the cost of some accuracy.  I opted to address this nit by changing the three "apks" references to "apkPackageNames" for now, because it's a smaller change from the existing patch.  But we can revisit and change these later, since they aren't stored persistently.

Here's the patch with the nit addressed.  This is the version I'll commit.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ab1a467525

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9ab1a467525