API to provide localized App name

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gandalf, Assigned: fabrice)

Tracking

unspecified
2.2 S8 (20mar)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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

Comment 1

4 years ago
Does the desc represent what you'd like to get?
Flags: needinfo?(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.
Flags: needinfo?(kgrandon) → needinfo?(fabrice)
(Reporter)

Updated

4 years ago
(Assignee)

Comment 3

4 years ago
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.
Flags: needinfo?(fabrice)
(Reporter)

Comment 4

4 years ago
(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?
Flags: needinfo?(fabrice)
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.
(Assignee)

Comment 6

4 years ago
(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()...
Flags: needinfo?(fabrice)
(Reporter)

Comment 7

4 years ago
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.
(Reporter)

Comment 8

4 years ago
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?
Flags: needinfo?(fabrice)
(Assignee)

Comment 9

4 years ago
(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.
Flags: needinfo?(fabrice)
(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.
(Reporter)

Comment 11

4 years ago
If we want the API, we need it in 2.2.
blocking-b2g: --- → 2.2?

Comment 12

4 years ago
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.
blocking-b2g: 2.2? → ---
(Reporter)

Comment 13

4 years ago
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.
Flags: needinfo?(stas)
Flags: needinfo?(kgrandon)
Flags: needinfo?(jonas)
Flags: needinfo?(fabrice)
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?
Flags: needinfo?(kgrandon)
(Reporter)

Comment 15

4 years ago
(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?
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.
Flags: needinfo?(jonas)
(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".
Flags: needinfo?(stas)
(Reporter)

Comment 18

4 years ago
(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.
(Assignee)

Comment 19

4 years ago
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.
Flags: needinfo?(fabrice)
(Assignee)

Comment 20

4 years ago
Posted patch l10n-prop.patch (obsolete) — Splinter Review
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.
Assignee: nobody → fabrice
Attachment #8569484 - Flags: review?(ferjmoreno)
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?
(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
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.
(Assignee)

Comment 24

4 years ago
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
(Reporter)

Comment 25

4 years ago
(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.
(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.
(Reporter)

Comment 28

4 years ago
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.
(Reporter)

Comment 29

4 years ago
Actually, since entryPoint is available in gaiaGrid, we may have to pass it to the API: getLocalizedValue('short_name', 'fr', 'dialer') ?
Flags: needinfo?(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).
Flags: needinfo?(kgrandon)
(Reporter)

Comment 31

4 years ago
(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?
Flags: needinfo?(jonas)
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')

?
(Reporter)

Comment 33

4 years ago
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...
I'll defer to you guys on this.
Flags: needinfo?(jonas)
(Reporter)

Comment 35

4 years ago
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]
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.
Flags: needinfo?(gandalf)
(Reporter)

Comment 37

4 years ago
(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.
Flags: needinfo?(gandalf)
(Assignee)

Updated

4 years ago
Attachment #8569484 - Flags: review?(ferjmoreno)
Depends on: 1138450
(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.
(Assignee)

Comment 39

4 years ago
Posted patch l10n-prop.patch v2 (obsolete) — Splinter Review
Added support for entry points, and deals with x.y or x.y.z versions (by ignoring the `z` part).
Attachment #8569484 - Attachment is obsolete: true
(Assignee)

Comment 40

4 years ago
fixed issue when there are no entry points in the manifest.
Attachment #8572856 - Attachment is obsolete: true
(Reporter)

Comment 41

4 years ago
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.
Attachment #8570135 - Flags: feedback?(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.
Attachment #8570135 - Flags: feedback?(kgrandon) → feedback+
(Assignee)

Updated

4 years ago
Attachment #8572880 - Flags: review?(ferjmoreno)
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;	
});
Attachment #8572880 - Flags: review?(ferjmoreno) → review+
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
Attachment #8572880 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/79821bfa9616
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Reporter)

Comment 47

4 years ago
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
Attachment #8572880 - Flags: approval-mozilla-b2g37?
(Reporter)

Comment 48

4 years ago
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
Flags: needinfo?(martijn.martijn)
Flags: needinfo?(kgrandon)
(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.
Flags: needinfo?(gandalf)
Martin - the failing tests are here: https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=25f6c27159e9
Flags: needinfo?(gandalf)
(Reporter)

Comment 52

4 years ago
Its the Gaia PR. The patch by fabrics has been merged into Gecko already.
(Reporter)

Comment 53

4 years ago
(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)
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(kgrandon)
(Reporter)

Comment 55

4 years ago
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...
(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.
(Reporter)

Comment 57

4 years ago
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?
(Reporter)

Comment 58

4 years ago
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.
Flags: needinfo?(martijn.martijn)
Flags: needinfo?(kgrandon)
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(kgrandon)
(Assignee)

Comment 60

4 years ago
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).
(Reporter)

Comment 61

4 years ago
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.
Attachment #8570135 - Flags: review?(kgrandon)
(Reporter)

Comment 62

4 years ago
Filed bug 1142230 to handle the conversation about updateManifest there.
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!
Attachment #8570135 - Flags: review?(kgrandon) → review+
(Reporter)

Comment 64

4 years ago
applied
Keywords: checkin-needed
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.
(Reporter)

Updated

4 years ago
Component: DOM: Apps → Gaia::Components
Product: Core → Firefox OS
Target Milestone: mozilla39 → 2.2 S8 (20mar)
(Reporter)

Comment 67

4 years ago
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.
Attachment #8570135 - Flags: approval-gaia-v2.2?(bbajaj)
Zibi, thanks for taking care of the failures! Sorry I couldn't be of more help.
Attachment #8570135 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
Attachment #8572880 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
(Reporter)

Comment 69

4 years ago
Follow up: bug 1142758
Whiteboard: [NO_UPLIFT]
You need to log in before you can comment on or make changes to this bug.