921939 - [b2gperf] App with localized name couldn't be access by gaia_app.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::UI Tests, defect, P2)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

https://github.com/mozilla-b2g/gaia/blob/106001c4e0ab9253001bed815c03f46b8dd046f0/tests/python/gaia-ui-tests/gaiatest/atoms/gaia_apps.js#L186

Currently GaiaApps in b2gperf accesses AppWindow via name comparison.
However the name property of AppWindow would be replaced each time we have a localized event.

Simple proposal:
Keep the English name in another attribute and modify gaia_apps to use that.

The name access seems unresolvable currently...if we want to use symptoms like App=System when running command line.

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/12540

This is a workaround to resolve localized event: compare manifest.name instead of app.name.
I don't really know what's the difference between the two gaia_apps.js

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks Alive, the two files will be merged in bug 909839. I'll try out your workaround and provide feedback.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 811876 [details]
https://github.com/mozilla-b2g/gaia/pull/12540

This works for apps such as Settings, but for apps with multiple entry points it fails, such as the Phone app. This is because we're launching an application with the name 'Phone' but the manifest name is 'Communications'.

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Alive: Do you have any suggestions regarding the above comment?

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

That should be a bug inside window manager because it's manifest should be updated to each entry. I'll double check.
BTW I had a discussion with Zac for we should know if an app is running by querying div.appWindow[data-localized-name] in the future.

Comment 6

[Ravi Dandu]

Is this issue still persistent. Elevating to P2

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Yes, but I don't have bandwidth right now.

Comment 8

[Zac C (:zac)]

I'll take this and fix it for v2.0+

Comment 9

[Zac C (:zac)]

Attachment: github pr

Comment 10

[Zac C (:zac)]

Comment on attachment 8423882 [details] [review]
github pr

This will now launch from es locale with en names.
and also with es locale with es names.

Expect there are other scenarios where we are not locale compatible but, one at a time!

Comment 11

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

Would it make sense to use one of the default locales instead of es (we currently have four: ar, fr, en-US and zh-TW)?

Comment 12

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf] from comment #11)
> Would it make sense to use one of the default locales instead of es (we
> currently have four: ar, fr, en-US and zh-TW)?

My plan is to remove ar, fr and zh-TW in favor of pseudolocales (bug 1011519).

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I haven't looked at the patch yet, but did notice that it's failing in Travis.

Comment 14

[Zac C (:zac)]

I chose es because of a special request for this from that country, but there's no reason why we can't use another.

I did all the testing for this on device so there must be some kind of anomaly on desktopb2g. Let me fix it up first and I'll ping y'all.

Comment 15

[Zac C (:zac)]

Well the reason is that Spanish does not exist on desktopb2g so I'll have to use French in the meantime.

Comment 16

[Zac C (:zac)]

Comment on attachment 8423882 [details] [review]
github pr

Spanish doesn't exist on desktop profile so I changed to French :)

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8423882 [details] [review]
github pr

Great to see this working. I've asked a few questions in the pull request as I'd like to understand the changes better before I implement this in b2gperf.

Comment 18

[Zac C (:zac)]

Comment on attachment 8423882 [details] [review]
github pr

I can't resolve this to the satisfaction of review without major re-factor so releasing it to someone with a bit more javascript-fu.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'll take this, I have something working locally.

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19482

Credit to :zac for the solution here, I just helped with some of the JavaScript. This will need squashing before committing, and also needs further testing.

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I've triggered an adhoc job http://selenium.qa.mtv2.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/409/ and I'm also continuing to run tests locally.

Note that this isn't the full solution as this bug is about b2gperf. We will also need to release a new version of gaiatest after this lands, and update b2gperf to use the new getAppByURL method and launchPath argument.

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8426252 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19482

Alive: Would you also be free to look over this?

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8426252 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19482

Looks like you find a solution doesn't need window manager support. Thanks.

Comment 24

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8426252 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/19482

The unit tests pass fine with these changes.

Comment 26

[Zac C (:zac)]

Merged:
https://github.com/mozilla-b2g/gaia/commit/ef5c6232898fcc41c375cad892265e511cd1977e

I don't think we need this on v2.0 right now but if requested we could uplift it.

Comment 27

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Uplifted to v2.0:
https://github.com/mozilla-b2g/gaia/commit/8277815063c22c887173287326834672d39e00e1