1042882 - Explicitely track the app kind in the dom registry

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

Attachment: explicit-app-type.patch

We have various places where we use indirect ways to decide if an app is packaged, hosted or hosted with an appcache and to act accordingly. Since we will soon have a 4th kind, I'm adding an explicit field to track that.

try build at https://tbpl.mozilla.org/?tree=Try&rev=856ed3c518ec

Comment 2

[Marco Castelluccio [:marco]]

We could have another kind for "core apps" to avoid the ugly checks we are currently doing with the basePath.

Comment 3

[[:fabrice] Fabrice Desré]

We could do better for the core apps, but I don't think this should be an app kind. They are all packaged and we would end up doing |if (app.kind == "local-package" || app.kind == "core")| at every "is this a package app" test. That may or may not be an issue...

Maybe app.kind should be a integer and we would do bit tests, such as:

const kPackagedApp = 1 << 0;
const kCoreApp = 1 << 1;
app.kind = kPackagedApp | kCoreApp;
if (app.kind & kPackageApp) {...}

the only drawback is that we would serialize a rather opaque integer.

Comment 4

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

(In reply to Fabrice Desré [:fabrice] from comment #3)
> the only drawback is that we would serialize a rather opaque integer.

Indeed.  I would identify core apps separately, via a property like a boolean isCoreApp, and reserve this "kind" property for differentiating between packaging/distribution models.

Comment 5

[[:fabrice] Fabrice Desré]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> (In reply to Fabrice Desré [:fabrice] from comment #3)
> > the only drawback is that we would serialize a rather opaque integer.
> 
> Indeed.  I would identify core apps separately, via a property like a
> boolean isCoreApp, and reserve this "kind" property for differentiating
> between packaging/distribution models.

Ok, fine with me, that's what the current patch is doing (I think we can do the isCoreApp in another bug).

Comment 6

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

Comment on attachment 8461061 [details] [diff] [review]
explicit-app-type.patch

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

Mostly good.  There's a minor conflict in toolkit/devtools/server/actors/webapps.js, which also needs review from a Devtools peer.  Other issues below.

::: dom/apps/src/Webapps.jsm
@@ +150,5 @@
>  // store even by error.
>  const STORE_ID_PENDING_PREFIX = "#unknownID#";
>  
> +// Constants for the different application kinds.
> +const kLocalPackage = "local-package";

Nit: it seems like we've been calling this simply "packaged" for forever, so that's what I would expect this to be.  The word "local" makes me think it's something more than simply packaged, f.e. a "core" app that comes with the device.

@@ +272,1 @@
>          }

init > loadAndUpdateApps > registerAppsHandlers reads all the manifests, so we could do this there, where we can know whether or not each app uses an appcache.

@@ +1698,5 @@
>      }
>  
>      // If the app is packaged and its manifestURL has an app:// scheme,
>      // then we can't have an update.
> +    if (app.kind == "kLocalPackage" &&

This needs to compare app.kind to the kLocalPackage const rather than the string "klocalPackage".

@@ +2391,5 @@
>        appObject.downloadAvailable = false;
>        appObject.downloading = false;
>        appObject.readyToApplyDownload = false;
> +    } else {
> +      dump("-*- Webapps.jsm : Unknown app kind: " + appObject.kind);

Nit: dump -> debug and remove the prefix, which debug adds by default.

::: toolkit/devtools/server/actors/webapps.js
@@ +259,5 @@
> +        // We need the manifest to set the app kind for hosted apps,
> +        // because of appcache.
> +        if (aApp.kind == undefined) {
> +          aApp.kind = manifest.appcache_path ? "hosted-appcache" : "hosted";
> +        }

Nit: I'd move the k* consts to properties of the DOMApplicationRegistry object, so this code can access them instead of using string literals.  You can still make them "const" by defining them as getters:

this.DOMApplicationRegistry = {
  get kHosted() "hosted",
  get kHostedAppcache() "hosted-appcache",
  get kLocalPackage() "local-package",
  …

@@ +537,5 @@
>              installOrigin: origin,
>              manifestURL: manifestURL,
>              appStatus: appType,
>              receipts: aReceipts,
> +            kind: "local-package"

Nit: I would append a trailing comma, so the next addition to this object doesn't have to touch this line (just like your addition didn't have to touch the line above it).

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: explicit-app-type.patch v2

Comments addressed, try build at https://tbpl.mozilla.org/?tree=Try&rev=97715528290c

It seems that :ochameau is in PTO, and I verified the sideloading cases. Let's ask for forgiveness if something goes wrong there ;)

Comment 8

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/4ac0059e424e

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4ac0059e424e