Closed Bug 1142572 Opened 6 years ago Closed 6 years ago

Use displayName from ManifestHelper to abstract short_name vs. name properties

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-master fixed)

RESOLVED FIXED
2.2 S9 (3apr)
Tracking Status
b2g-master --- fixed

People

(Reporter: sfoster, Assigned: sfoster)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 2 obsolete files)

Since the patch in bug 1137501 landed, we have a displayName property to better abstract away the use of short_name vs. name from the manifest. We should go back and fix those `name || short_name` uses for consistency and simplicity's sake.
If I'm understanding this bug correctly, I think this is the reason why l10n is broken for translations for apps?
blocking-b2g: --- → 2.2?
Flags: needinfo?(sfoster)
(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #1)
> If I'm understanding this bug correctly, I think this is the reason why l10n
> is broken for translations for apps?

If the ManifestHelper or local_name changes are implicated in broken translations (which I dont *think* it is?) we should look at some of the other dependants of bug 1001861 where these changes were originally implemented. Making the changes called for by this bug shouldnt fix anything broken in those other bug; this is just some code clean up.
Flags: needinfo?(sfoster)
blocking-b2g: 2.2? → ---
Comment on attachment 8580895 [details] [review]
[gaia] sfoster:manifest-displayname-bug-1142572 > mozilla-b2g:master

This bug covers going back and consolidating our use of the ManifestHelper's displayName property instead of (short_name | name) logic. 

r? alive for system AppWindow and (Card view) Card parts. As we've pushed the short_name vs name down to the ManifestHelper, we can get rid of the AppWindow.prototype.shortName entirely. It had a short life. 

r? christian for bookmark editor
r? arthur for settings
Attachment #8580895 - Flags: review?(crdlc)
Attachment #8580895 - Flags: review?(arthur.chen)
Attachment #8580895 - Flags: review?(alive)
Comment on attachment 8580895 [details] [review]
[gaia] sfoster:manifest-displayname-bug-1142572 > mozilla-b2g:master

Looks good to me, thanks.
Attachment #8580895 - Flags: review?(arthur.chen) → review+
Comment on attachment 8580895 [details] [review]
[gaia] sfoster:manifest-displayname-bug-1142572 > mozilla-b2g:master

Good job, thanks a lot
Attachment #8580895 - Flags: review?(crdlc) → review+
Comment on attachment 8580895 [details] [review]
[gaia] sfoster:manifest-displayname-bug-1142572 > mozilla-b2g:master

Thanks!
Attachment #8580895 - Flags: review?(alive) → review+
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#pFs2EBqvSQKlJd8Wzlqrcw

The pull request failed to pass integration tests. It could not be landed, please try again.
Gaia currently closed, r+'d, tests are green, please land when it opens
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/pull/29023

Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#mi6GtyXST3eJb0S2gyNw-w

The pull request failed to pass integration tests. It could not be landed, please try again.
This patch has had a green test run on https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=dedd57b4d1917c10f6017725454077bb549daa03, but trying to land since gaia re-opened and I get a bunch of taskcluster failures, like: 

fatal: unable to access 'https://github.com/mozilla-b2g/gaia/': transfer closed with outstanding read data remaining    

I'll close and re-open the PR to give it another shot, but as I'm out on PTO could you push this over the finish line? Thanks!
Flags: needinfo?(mhenretty)
Whiteboard: [systemsfe] → [systemsfe] [checkin-needed]
Master: https://github.com/mozilla-b2g/gaia/commit/6b95562a08cd4603ec269c92938e392f0b4d1ea3
Assignee: nobody → sfoster
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(mhenretty)
Resolution: --- → FIXED
Whiteboard: [systemsfe] [checkin-needed] → [systemsfe]
Target Milestone: --- → 2.2 S9 (3apr)
Backed out for Gij1 failures on TC.

https://github.com/mozilla-b2g/gaia/commit/37cfde85cdf74142fdf1f1dec5f1329e8f8d6352

Revert commit: https://github.com/mozilla-b2g/gaia/commit/37cfde85cdf74142fdf1f1dec5f1329e8f8d6352

Adding a needinfo to myself to try to shepherd this to landing after a green run.
Status: RESOLVED → REOPENED
Flags: needinfo?(kgrandon)
Resolution: FIXED → ---
Attachment #8584751 - Attachment is obsolete: true
Flags: needinfo?(kgrandon)
Ben - it looks like Sam is out and I'm not entirely sure what the correct behavior is supposed to be here. This is a screenshot when the test times out. Does this screen look correct to you? Do you want to look at this before Sam gets back from PTO?
Flags: needinfo?(bfrancis)
I've commented on the pull request with what I think the problem is. I'm not sure when Sam is back but I can take this if he doesn't get back soon.
Flags: needinfo?(bfrancis)
blocking-b2g: --- → 2.2?
blocking-b2g: 2.2? → ---
(In reply to Ben Francis [:benfrancis] from comment #18)
> I've commented on the pull request with what I think the problem is. I'm not
> sure when Sam is back but I can take this if he doesn't get back soon.

Thanks for catching that Ben (and Gij!), it was my oversight not noticing WebManifestHelper vs. ManifestHelper. I think that should be good to go now it the tests go green.
Comment on attachment 8586337 [details] [review]
[gaia] sfoster:manifest-displayname-r2-bug-1142572 > mozilla-b2g:master

Sorry to bug you again, but attachment #8580895 [details] [review] landed then was backed out for causing Gij failures. I've removed the erroneous bookmarks app changes - the rest of the patch is unchanged from what you saw before.
Attachment #8586337 - Flags: review?(arthur.chen)
Attachment #8586337 - Flags: review?(alive)
Comment on attachment 8580895 [details] [review]
[gaia] sfoster:manifest-displayname-bug-1142572 > mozilla-b2g:master

This patch landed but backed out. Updated PR in attachment #8586337 [details] [review]
Attachment #8580895 - Attachment is obsolete: true
Attachment #8586337 - Flags: review?(arthur.chen) → review+
Comment on attachment 8586337 [details] [review]
[gaia] sfoster:manifest-displayname-r2-bug-1142572 > mozilla-b2g:master

WFM
Attachment #8586337 - Flags: review?(alive) → review+
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.