1118946 - API to provide localized App name

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Components, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

We currently have multiple places and algorithms to get localized App name.

Example: https://github.com/mozilla-b2g/gaia/blob/b3beac2b9fff23fcae3fe2811c33ed640f818625/shared/elements/gaia_grid/js/items/mozapp.js#L176-L179

On top of that, we will now have localized app names coming from Language packs and the current design of Langpack API does not provide a way for homescreen app to retrieve the name of an app (especially once we get to langpack-per-app).

The logic should be as follows:

1) If App manifest contains locales[locale].name, return it
2) If not, if there is a langpack for the app, return app's name from it
3) If not, return manifest's name field

wrt. 2)

We currently store localized App name and description in .properties file. Example: http://hg.mozilla.org/gaia-l10n/ta/file/baa920d9eebe/apps/sms/manifest.properties

I believe that we should instead write a JSON with those two fields either into langpack manifest, or into app's directory (for example /ta/apps/sms/manifest.json) so that this API does not have to parse .properties file.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Does the desc represent what you'd like to get?

Comment 2

[Kevin Grandon :kgrandon]

Yup, I think this sounds good to me. Fabrice do you have any concerns here?

If we are shipping langpacks in 2.2, and also replaceable home screens, then we should try to get this in as well.

Comment 3

[[:fabrice] Fabrice Desré]

Why is that not good enough to retrieve a json file with getLocalizationResource? That's mostly what a new api would do behind the scene anyway.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Why is that not good enough to retrieve a json file with
> getLocalizationResource? That's mostly what a new api would do behind the
> scene anyway.

If I understand correctly, the API only operates on self. Which means that homescreen can't use the API to ask for a json from sms or settings app. Right?

Comment 5

[Kevin Grandon :kgrandon]

There's also a fair amount of boilerplate code in order to fetch the current app name. Langpacks exacerbate the matter by requiring additional logic. I think we should make this easier for third party developers since we are shipping replaceable home screens.

Comment 6

[[:fabrice] Fabrice Desré]

(In reply to Zibi Braniecki [:gandalf] from comment #4)
> If I understand correctly, the API only operates on self. Which means that
> homescreen can't use the API to ask for a json from sms or settings app.
> Right?

Yes but we could easily expose it also on the app object.

(In reply to Kevin Grandon :kgrandon from comment #5)
> There's also a fair amount of boilerplate code in order to fetch the current
> app name. Langpacks exacerbate the matter by requiring additional logic. I
> think we should make this easier for third party developers since we are
> shipping replaceable home screens.

A good homescreen is complicated anyway no? Simple ones can just do the simple thing, and others go through the pain of copy/pasting boiler plate. Adding special apis for single properties looks like a very slippery slope to me (when will you ask for the short name? ;) ). Or maybe we should bundle that with the getIcon() api and turn it into getStuffYouNeedForHomescreens()...

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

I think I'm ok with not pushing for an API yet.

We'll need an extension to Langpack API to get a resource from another app/locale/ver.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Fabrice, would you prefer to use this bug for API extension to allow for getting resources from other apps, or should I file a new one?

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Zibi Braniecki [:gandalf] from comment #8)
> Fabrice, would you prefer to use this bug for API extension to allow for
> getting resources from other apps, or should I file a new one?

Let's use this bug. I'm not sure what solution we should implement. adding getAdditionalResource() to the app object is simple and would work, at the expense of needing some post processing in l10n.js.

Comment 10

[Kevin Grandon :kgrandon]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> A good homescreen is complicated anyway no? Simple ones can just do the
> simple thing, and others go through the pain of copy/pasting boiler plate.
> Adding special apis for single properties looks like a very slippery slope
> to me (when will you ask for the short name? ;) ). Or maybe we should bundle
> that with the getIcon() api and turn it into
> getStuffYouNeedForHomescreens()...

For most things I agree that generally giving developers the lower-level API is good, but for something so common as displaying the properly localized app name I think we're creating a very poor experience for third party developers.

The lookup and caching of this is a problem that *every* third party developer is forced to solve. From a performance standpoint, it seems we might have to potentially cache the localized name in the home screen, in settings, and in the system app. I think it would make a lot more sense to cache this in the platform, and I really think this should be part of the mozapp api.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

If we want the API, we need it in 2.2.

Comment 12

[Stephany Wilkes]

Triage loves Langpacks, and agrees this is very important, but this feature is technically not committed for 2.2, so we can't reasonably block the release on it. Sorry. :(

It would be a huge win and we should try to fix this, so let's try to reach agreement on this API work in comment #10 and comment #11.

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Based on the conversation with Jonas, Fabrice and Kevin, I believe the proposal would be:

A new Gecko API: app.getLocalizedValue('manifest_object_path', locale_code, app_version) which does the following steps:

 - if there's a langpack registered for the given code/version:
    - take JSON object from a file referenced in the path in manifest.webapp of the langpack
    - create an ID based on path to the key from app's manifest
    - if the ID is present in the object, return it
 - otherwise check if there key is present in app's manifest locales[code]
 - otherwise return the key from app's manifest

So, for 2.2 we basically will only use it for App's name.
Example for 'fr', '2.2':

code: app.getLocalizedValue('name', 'fr', '2.2');

1) checks if there's a langpack 'fr' for '2.2' registered
2) if yes:
2.1) looks into langpack's manifest.webapp for `languages-provided['fr'].manifest` which is a path inside the langpack. For example: /fr/apps/calendar/manifest.json
2.2) inside it, it looks for field "name"
2.3) if it exists, the value of that field is returned
3) if no:
3.1) looks into app's manifest for field `locales['fr'].name`. If it exists the value of that field is returned
3.2) if not, the field `name` from app's manifest is returned

Fabrice, Kevin, Sicking, Stas - does it sound like a good solution?

Fabrice - will you have time to write this? If so, what would be the ETA? We need the estimation for 2.2 drivers

Kevin - there seem to be short_name field as well (bug 1121466). Not sure how this fits into this proposal. Is:

var name = app.getLocalizedValue('short_name', 'fr', '2.2') || app.getLocalizedValue('name', 'fr' , '2.2');

enough?

I'd like to point out that this is the last blocker for langpacks experience in 2.2 and we're on a very tight schedule here. If we cannot resolve it in a matter of days, we should look for a dirtyhack solution or revisit langpacks in 2.2.

Comment 14

[Kevin Grandon :kgrandon]

Regarding app_version, how would that work if we need to display the app name inside of a different app, like settings? Can we read the version from the settings app?

Is it possible to use the manifest version field here, and read that by the platform to automatically determine the version?

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Kevin Grandon :kgrandon from comment #14)
> Regarding app_version, how would that work if we need to display the app
> name inside of a different app, like settings? Can we read the version from
> the settings app?

We would need this app's object if understand correctly. The app version is for 2.2 shared between apps, but in the future we would need to know app version to get this.

> Is it possible to use the manifest version field here, and read that by the
> platform to automatically determine the version?

Probably yes. Fabrice?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If we're talking about the app's version, then I think the only authoritative source is the app manifest. Which means that there's no reason to pass in the version as an argument, the function can get that directly from the manifest.

All that said, I'd really like to hear from Fabrice that this is a reasonable solution. Since almost all of the implementation complexity falls on him.

Comment 17

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf] from comment #13)

>  - otherwise check if there key is present in app's manifest locales[code]
>  - otherwise return the key from app's manifest

This should good, but we should also make it possible to extend this machanism in the future to support localization of manifest fields defined as objects:

  https://github.com/w3c/manifest/issues/211

Similar to Jonas' comment 16, I also don't think passing app version should be required.  It seems like if we already have a reference to the App object, we could retrieve it's version from the manifest?  The only trouble I see with this approach is that the "version" in the manifest is a semver's "x.y.z" while the whole langpack API expects "x.y".

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Staś Małolepszy :stas from comment #17)
> (In reply to Zibi Braniecki [:gandalf] from comment #13)
> 
> >  - otherwise check if there key is present in app's manifest locales[code]
> >  - otherwise return the key from app's manifest
> 
> This should good, but we should also make it possible to extend this
> machanism in the future to support localization of manifest fields defined
> as objects:
> 
>   https://github.com/w3c/manifest/issues/211


This is just supposed to replace locales['fr'].name with name['fr']. That's an update to how we handle that in the API, not an extension, so I don't think it changes anything here.

If we decide to switch in the future, we'll have to update the API code the same way as right now we have to update it all around our codebase. With this API function it will at least be centralized.

Comment 19

[[:fabrice] Fabrice Desré]

I can implement that, but that will need to be an async method (since we need to fetch additional resources) returning a promise. So you won't be able to do var name = app.getLocalizedValue('short_name', 'fr'); 

I'll try to get something ready this week.

Comment 20

[[:fabrice] Fabrice Desré]

Attachment: l10n-prop.patch

The only failure case is when we ask for an unknown property. In all other cases, we end up getting the value (localized or not) from the manifest.

Comment 21

[Staś Małolepszy :stas]

Comment on attachment 8569484 [details] [diff] [review]
l10n-prop.patch

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

::: dom/apps/Langpacks.jsm
@@ +203,5 @@
> +    }
> +
> +    function getValueFromLangpack(aItem, aManifest) {
> +      debug("Getting value from langpack at " + aItem.url + "/manifest.json")
> +      let href = aItem.url + "/manifest.json";

Zibi, this probably means that we'll need to include manifest.json instead of manifest.properties in the langpacks, right?

@@ +255,5 @@
> +        }
> +
> +        // Check that we have the langpack for the right app version.
> +        let item = this._data[aData.manifestURL].langs[aData.lang];
> +        if (item.target == manifest.version) {

IIRC, item.target is 'x.y' whereas manifest.version is 'x.y.z' (usually), so this might not work as intended?

Comment 22

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas from comment #21)

> Zibi, this probably means that we'll need to include manifest.json instead
> of manifest.properties in the langpacks, right?

https://github.com/zbraniecki/langpack-builder/pull/4

Comment 23

[Staś Małolepszy :stas]

I tried the patch with a langpack installed and I ran into a problem where we don't actually have the version of the current app :/

Similar to Jonas (comment 16), I thought we can take the version from the app manifest, but it turns out Gaia system apps don't even use the "version" field (it's not required by the W3C spec either).

https://github.com/mozilla-b2g/gaia/blob/c5b086914cba6eb98961724423a0563d00d1f312/apps/settings/manifest.webapp

We worked around this before by adding <meta name="appVersion"> to HTML files of each app, but in this case, we can't do that same thing because we don't have access to those HTML files.  All we have is the manifest of the app that we want to show the name of.  The version of the app from which we're running this code isn't helpful (although coincidentally in Gaia, all of those apps will share the same version, but that's not something that's going to scale).

Could we start adding the "version" field to our apps?  This could be easily done as a buildtime step using the moz.b2g.version setting we added in bug 1130632.

Comment 24

[[:fabrice] Fabrice Desré]

Stas, I hardcoded manifest.json but I don't mind changing that. I just believe we need to use a well known name, and not make it configurable.

About the version number... I don't know. We can add version numbers to gaia's manifests if needed. We can also improve the version check if for instance we want to consider 2.2 == 2.2.1

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Fabrice Desré [:fabrice] from comment #24)
> Stas, I hardcoded manifest.json but I don't mind changing that. I just
> believe we need to use a well known name, and not make it configurable.

Why is that? In my proposal in comment 13, step 2.1 I made this path configurable so that we can point to a manifest.json in the app path and we can be flexible with the paths inside the langpack.

Your approach will not work for multiple apps per langpack if I'm correct.

Comment 26

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf] from comment #25)

> Your approach will not work for multiple apps per langpack if I'm correct.

The code will look for manifest.json in the directory which has other l10n resources for a particular app and language, so this should work correctly. I don't have an opinion yet on whether this name should be configurable.

Comment 27

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1118946-app-names-from-api > mozilla-b2g:master

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

Started working on the gaia side. One thing I noticed is that we need to add another step to Fabrice's API:

 - check entry_points['dialer'].locales[code]['name']
 - check entry_points['dialer']['name']

and in manifest.json:
 - check entry_points_dialer_name

to deal with: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/manifest.webapp

at least for 2.2 until we remove the support for that in bug 1135340.

Comment 29

[Zibi Braniecki [:zbraniecki][:gandalf]]

Actually, since entryPoint is available in gaiaGrid, we may have to pass it to the API: getLocalizedValue('short_name', 'fr', 'dialer') ?

Comment 30

[Kevin Grandon :kgrandon]

(In reply to Zibi Braniecki [:gandalf] from comment #29)
> Actually, since entryPoint is available in gaiaGrid, we may have to pass it
> to the API: getLocalizedValue('short_name', 'fr', 'dialer') ?

Yes, I think that's correct. This would match what we do in the other mozapp APIs, like app.launch(entryPoint).

Comment 31

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Zibi Braniecki [:gandalf] from comment #28)
> Started working on the gaia side. One thing I noticed is that we need to add
> another step to Fabrice's API:
> 
>  - check entry_points['dialer'].locales[code]['name']
>  - check entry_points['dialer']['name']
> 
> and in manifest.json:
>  - check entry_points_dialer_name

the reason I suggested this is because sicking sided with manifest.json being flat and having a key that represents the path to the key in manifest.webapp, so: {x:{y:{z:'val'}}} in manifest.webapp is localized by {'x_y_z': 'val'} in manifest.json.

The problem here is that "_" is already present in entry_points key name so this serialization will make it ambiguous - do we mean entry:{points:'val'} or entry_points: 'val'.

We can't use `.` because it is meaningful in our properties.

Sicking, do you have an opinion?

Comment 32

[Staś Małolepszy :stas]

I think the most future- and feature-proof way of dealing with nested manifest fields would be to replicate the nesting structure one-to-one in manifest.json in the langpack.  This is consistent with the most simple case of:

{
  "name": "Localized name"
}

and is easily extendable:

{
  "entry_points": {
    "dialer": {
      "name": "Localized name"
    }
  }
}

For example, I just discovered bug 930371 in which the "inputs" field was added.  While the addition itself may be disputable, it makes me think that we should be agnostic to the structure of the manifest.

Could we implement something like:

  getLocalizedValue('name', 'fr')
  getLocalizedValue('entry_points.dialer.name', 'fr')
  getLocalizedValue('inputs.number.name', 'fr')

?

Comment 33

[Zibi Braniecki [:zbraniecki][:gandalf]]

This was my original suggestion, but jonas aimed at making the l10n object a flat one for simplicity reasons. Because of inputs ans entry_points and some other localizable fields in the future, maybe it's worth going for nested object structure?

The only question is how will we expose it to localizers. Especially in l20n format...

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll defer to you guys on this.

Comment 35

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok. Updates to the patch:

1) path/manifest.json is now 1-1 matching manifest.webapp structure overlaying localizable fields. It means that it may look like this:

{
  "entry_points": {
    "dialer": {
      "name": "foo"
    }
  },
  "name": "foo"
}

2) the API should take one more optional argument - entryPoint:

app.getLocalizedValue('short_name', 'fr', 'dialer');

If entryPoint is given, there's an additional step in retrieving the name both from manifest.json and manifest.webapp: entry_points[entryPoint]

Comment 36

[Staś Małolepszy :stas]

Zibi, what do you think about my proposal in comment 32?  To repeat it here:

  getLocalizedValue('entry_points.dialer.name', 'fr')
  getLocalizedValue('inputs.number.name', 'fr')

This would make the API work not only for entry_points, but also other nested fields.

Comment 37

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Staś Małolepszy :stas from comment #36)
> Zibi, what do you think about my proposal in comment 32?  To repeat it here:
> 
>   getLocalizedValue('entry_points.dialer.name', 'fr')
>   getLocalizedValue('inputs.number.name', 'fr')
> 
> This would make the API work not only for entry_points, but also other
> nested fields.

Not a fan. entry_points is a deprecated feature that we're trying to remove. I'd prefer not to have to chain `entry_points[entryPoint].short_name || entry_points[entryPoint].name || short_name || name`.

For inputs, we don't care because we will not localize them in 2.2.

Comment 38

[Staś Małolepszy :stas]

(In reply to Fabrice Desré [:fabrice] from comment #24)
> Stas, I hardcoded manifest.json but I don't mind changing that. I just
> believe we need to use a well known name, and not make it configurable.

I think hardcoding manifest.json is fine for now, just like we hardcoded *.gaiamobile.org.

> About the version number... I don't know. We can add version numbers to
> gaia's manifests if needed. We can also improve the version check if for
> instance we want to consider 2.2 == 2.2.1

I filed bug 1138450 to add "version" to manifests of Gaia apps.  Let's see what the outcome of the discussion re. version syntax (x.y vs. x.y.z) is in that bug.  In any case, using something like manifest.version.split('.').slice(0, 2).join('.') in the current code should be safe.

Comment 39

[[:fabrice] Fabrice Desré]

Attachment: l10n-prop.patch v2

Added support for entry points, and deals with x.y or x.y.z versions (by ignoring the `z` part).

Comment 40

[[:fabrice] Fabrice Desré]

Attachment: l10n-prop.patch v3

fixed issue when there are no entry points in the manifest.

Comment 41

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8570135 [details] [review]
[gaia] zbraniecki:1118946-app-names-from-api > mozilla-b2g:master

Just to put this on your radar Kevin. That's the scope of updates we will need for Name API to work.

Let me know if you like the direction.

Comment 42

[Kevin Grandon :kgrandon]

Comment on attachment 8570135 [details] [review]
[gaia] zbraniecki:1118946-app-names-from-api > mozilla-b2g:master

Left some comments on the pull request. I'm fine with the direction here, thanks.

Comment 43

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8572880 [details] [diff] [review]
l10n-prop.patch v3

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

Thank you Fabrice. r=me with the comments addressed.

Could you also add tests for the cases where the app manifest doesn't have the version or have an x.y.z version, please?

::: dom/apps/Langpacks.jsm
@@ +17,5 @@
>                                     "nsIMessageBroadcaster");
>  
>  this.EXPORTED_SYMBOLS = ["Langpacks"];
>  
> +let debug = true || Services.prefs.getBoolPref("dom.mozApps.debug")

Revert this change before pushing, please.

@@ +206,5 @@
> +      debug("Getting value from langpack at " + aItem.url + "/manifest.json")
> +      let href = aItem.url + "/manifest.json";
> +
> +      let xhr =  Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +                 .createInstance(Ci.nsIXMLHttpRequest);

nit: align .createInstance with [

@@ +215,5 @@
> +      // validating the dataType value.
> +      xhr.responseType = "json";
> +
> +      function getProperty(aProp) {
> +        let root = aData.entryPoint && xhr.response.entry_points && xhr.response.entry_points[aData.entryPoint]

nit: line too long.

@@ +221,5 @@
> +          : xhr.response;
> +        return root[aProp];
> +      }
> +
> +      xhr.addEventListener("load", function() {

Maybe we can move some of this code to a helper function and reuse it here and in  getLocalizationResource

@@ +258,5 @@
> +        // Fallback to the manifest values.
> +        if (!this._data[aData.manifestURL].langs[aData.lang]) {
> +          getValueFromManifest(manifest);
> +          return;
> +        }

You can do 

if (!this._data[aData.manifestURL] ||
    !this._data[aData.manifestURL].langs[aData.lang]) {
  getValueFromManifest(manifest);
  return;
}

@@ +263,5 @@
> +
> +        // Check that we have the langpack for the right app version.
> +        let item = this._data[aData.manifestURL].langs[aData.lang];
> +        // Only keep x.y in the manifest's version in case it's x.y.z
> +        let manVersion = manifest.version.split('.').slice(0, 2).join('.');

We should check if the manifest doesn't have the version field and in that case fallback to getValueFromManifest. It would be good to log a warning in that case as well.

@@ +271,5 @@
> +        }
> +        // Fallback on getting the value from the manifest.
> +        getValueFromManifest(manifest);
> +      })
> +      .catch(aError => { sendError("No app!", "NoSuchApp") });

nit: I think I mentioned this previously. We have an awful mixture of error strings in the WebApps API. We usually use capital SNAKE_CASE for API error values.

::: dom/apps/Webapps.jsm
@@ +4679,5 @@
> +
> +    return new Promise((aResolve, aReject) => {
> +      this.getManifestFor(aManifestURL)
> +        .then((aManifest) => {
> +          let manifest = aEntryPoint && aManifest.entry_points && aManifest.entry_points[aEntryPoint]

nit: line too long

@@ +4693,5 @@
> +          app.manifest = new ManifestHelper(manifest, app.origin, app.manifestURL);
> +          aResolve(app);
> +        })
> +        .catch(aReject);
> +    });

I believe you don't need to wrap this code with a new promise. You can probably do:

return this.getManifestFor(aManifestURL).then((aManifest) => {
    [...]
    return app;	
});

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8572880 [details] [diff] [review]
l10n-prop.patch v3

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

r=me on the Apps.webidl change

Comment 45

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/79821bfa9616

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79821bfa9616

Comment 47

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8572880 [details] [diff] [review]
l10n-prop.patch v3

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1107341
User impact if declined: app names not translated on the homescreen when using langpacks
Testing completed: master + gaia-try + devices 
Risk to taking this patch (and alternatives if risky): none, it's a new API
String or UUID changes made by this patch: none

Comment 48

[Zibi Braniecki [:zbraniecki][:gandalf]]

So, I have the gaia patch to use the gecko patch and it's fairly simple. I tested it back and forth and I use it on my Flame and Buri devices to make sure that it works properly.

Unfortunately, there are two Gip errors on gaia-try and I can't debug them because I can't get those tests to run even on master.

NI'ing :kgrandon who may be able to help me with that, and he also asked for the last person who touched the code around those tests. This person is mostly Kevin himself, but there's a change to Homescreen.app_installed made by :mwargers, so I'm CC'ing Martijn as well in case he may have a clue on why the name is not picked up

Comment 49

[Martijn Wargers (dead)]

(In reply to Zibi Braniecki [:gandalf] from comment #48)
> Unfortunately, there are two Gip errors on gaia-try and I can't debug them
> because I can't get those tests to run even on master.

Which Gip tests were failing? What patch were you using?

> NI'ing :kgrandon who may be able to help me with that, and he also asked for
> the last person who touched the code around those tests. This person is
> mostly Kevin himself, but there's a change to Homescreen.app_installed made
> by :mwargers, so I'm CC'ing Martijn as well in case he may have a clue on
> why the name is not picked up

Not sure what you're referring to. There is an is_app_installed method, but I can't remember I made any changes to it.

Comment 50

[Kevin Grandon :kgrandon]

Martin - the failing tests are here: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=25f6c27159e9

Comment 51

[Martijn Wargers (dead)]

Ok, and is it the https://bug1118946.bugzilla.mozilla.org/attachment.cgi?id=8572880 patch or the https://github.com/mozilla-b2g/gaia/pull/28510 that's causing it? I guess I can try it out myself.

The tests that are failing are:
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/tests/functional/homescreen/test_homescreen_launch_app_packaged.py#41
http://mxr.mozilla.org/gaia/source/tests/python/gaia-ui-tests/gaiatest/tests/functional/homescreen/test_homescreen_delete_app_packaged.py#34

Apparently with the patch applied, the navigator.mozApps.installPackage('http://localnetwork/webapps/packaged1/manifest.webapp') method doesn't work.

Comment 52

[Zibi Braniecki [:zbraniecki][:gandalf]]

Its the Gaia PR. The patch by fabrics has been merged into Gecko already.

Comment 53

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #51)
> Apparently with the patch applied, the
> navigator.mozApps.installPackage('http://localnetwork/webapps/packaged1/
> manifest.webapp') method doesn't work.

Fabrice, do you have any clue on what might be causing that? I am able to install this app using B2G with Fabrice's patch and when I launch the gip test locally I see a banner "packagedapp1 installed" (but no icons on the homescreen, both on master and with the patch, so I can't debug it myself)

Comment 54

[Kevin Grandon :kgrandon]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #51)
> Apparently with the patch applied, the
> navigator.mozApps.installPackage('http://localnetwork/webapps/packaged1/
> manifest.webapp') method doesn't work.

Where did you get this info from? Looking at the source artifact it appears that the app is on the home screen, but just that no title is populated. I think we're either not waiting for it properly, or it's just not being set on b2g desktop. Here is the dump of the HTML of the icon:

<div class="icon test-icon-cached" data-identifier="http://127.0.0.1:46947/webapps/packaged1/manifest.webapp" data-is-draggable="true" role="link" style="transform: translate(213.333px, 1786px) scale(1); height: 120px; background-size: 88px auto; background-image: url(&quot;blob:app://verticalhome.gaiamobile.org/6a7bb3a3-9d3e-4939-a30b-fd0c3f55cb62&quot;);" data-app-state="ready" data-test-icon="app://{6c270d53-7500-4336-9f26-b718b8e920ad}/qalogo.png"><p style="margin-top: 86px;"><span class="title"></span></p><span class="remove"></span></div>

Not really sure what else I can do here. Let's see if Fabrice has any ideas, and if not we can work with QA to further debug the test. Barring that, we could see about disabling the test assuming we have a similar test in GIJ, but that should be a last resort.

Comment 55

[Zibi Braniecki [:zbraniecki][:gandalf]]

is there a chance that the test doesn't really "wait" but just synchronously queries for the title? Because we now set the title asynchronously...

Comment 56

[Martijn Wargers (dead)]

(In reply to Zibi Braniecki [:gandalf] from comment #55)
> is there a chance that the test doesn't really "wait" but just synchronously
> queries for the title? Because we now set the title asynchronously...

No, it's querying repeatedly for the title, until it finds it or times out, in this case after 30s.

Comment 57

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok, so the difference is that when Gip tests are launched, the getLocalizedValue('name') for this app rejects. It does not reject on the device.

It rejects with the following log:


*************************
A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: this._manifest is null
Full stack: this.ManifestHelper@resource://gre/modules/AppsUtils.jsm:77:282
this.DOMApplicationRegistry.updateOfflineCacheForApp/<@resource://gre/modules/Webapps.jsm:42:597
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:79:82
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:72:31
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:59:62

*************************
*************************
A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: aManifest is null
Full stack: this.DOMApplicationRegistry.appKind@resource://gre/modules/Webapps.jsm:39:277
this.DOMApplicationRegistry.updatePermissionsForApp/<@resource://gre/modules/Webapps.jsm:42:148
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:79:82
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:72:31
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:59:62

*************************
*************************
A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: this._manifest is null
Full stack: this.ManifestHelper@resource://gre/modules/AppsUtils.jsm:77:282
this.DOMApplicationRegistry.updateOfflineCacheForApp/<@resource://gre/modules/Webapps.jsm:42:597
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:79:82
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:72:31
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:59:62

*************************
*************************
A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: aManifest is null
Full stack: this.DOMApplicationRegistry.appKind@resource://gre/modules/Webapps.jsm:39:277
this.DOMApplicationRegistry.updatePermissionsForApp/<@resource://gre/modules/Webapps.jsm:42:148
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:79:82
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:72:31
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:59:62

*************************


Fabrice, do you have any idea why this app: https://github.com/mozilla-b2g/gaia/tree/master/tests/python/gaia-ui-tests/gaiatest/resources/webapps/packaged1 could cause this Promise error stack for any condition?

Comment 58

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok. I have more. In Gip test, this.app.manifest is null, and everything is in this.app.updateManifest. Not sure how is it different, but it seems that our API does not handle that. Should it?

Taking off NI from Fabrice and Martijn. Adding NI on Kevin.

Comment 59

[Kevin Grandon :kgrandon]

Zibi - Do you have a line number of where we're not handing the updateManifest? In most of gaia we use something like `this.app.manifest || this.app.updateManifest` to just get either one. If the API is not handling this, it probably should.

Comment 60

[[:fabrice] Fabrice Desré]

If you only have an app.updateManifest, this means that the app is not yet fully installed, so we could not get whatever is only in the app.manifest.
I guess that's what happens in the test: it should wait for the app to receive the ondownloadapplied event.

We could also change gecko to fallback to the updateManifest if there's no manifest but the set of properties available there is very limited (basically only the app name and the developer info).

Comment 61

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8570135 [details] [review]
[gaia] zbraniecki:1118946-app-names-from-api > mozilla-b2g:master

Ok, so what this patch will basically do is:

 - try to get short_name from manifest
 - try to get name from manifest
 - use updateManifest.short_name || updateManifest.name instead

We can discuss the updates to the API to handle updateManifest (or assume that we only take name from manifest in which case something with verticalhome is wrong that it asks for name before the app is installed), but I'd like to get the basics landed so that we can start testing langpacks for 2.2 first.

Comment 62

[Zibi Braniecki [:zbraniecki][:gandalf]]

Filed bug 1142230 to handle the conversation about updateManifest there.

Comment 63

[Kevin Grandon :kgrandon]

Comment on attachment 8570135 [details] [review]
[gaia] zbraniecki:1118946-app-names-from-api > mozilla-b2g:master

I think this seems fine to me. We should probably add a mock app with a short_name property and a test which ensures that it's populated, but we can do this in a follow-up.

I left a few comments on github which I would like you to address or respond to if you think they make sense. Thanks for sticking with this, and nice work!

Comment 64

[Zibi Braniecki [:zbraniecki][:gandalf]]

applied

Comment 65

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 66

[Zibi Braniecki [:zbraniecki][:gandalf]]

Commit: https://github.com/mozilla-b2g/gaia/commit/c70a699a9866be5af72ba9db0743c58f57f0903f
Merge: https://github.com/mozilla-b2g/gaia/commit/e12598912989b680665a066ec6b46881e7747ad5

Comment 67

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8570135 [details] [review]
[gaia] zbraniecki:1118946-app-names-from-api > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): langpacks
[User impact] if declined: app names not localized when using a langpack
[Testing completed]: Gaia try, local builds
[Risk to taking this patch] (and alternatives if risky): none known
[String changes made]: none

This is the Gaia part of this bug. With those two patches we're done with langpacks for 2.2 and we can move on to testing.

Comment 68

[Martijn Wargers (dead)]

Zibi, thanks for taking care of the failures! Sorry I couldn't be of more help.

Comment 69

[Zibi Braniecki [:zbraniecki][:gandalf]]

Follow up: bug 1142758

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/377b4a99d3c2

v2.2: https://github.com/mozilla-b2g/gaia/commit/e59ac067a1d22b7a72cbebc892ec652723f2a557