Closed Bug 1139735 Opened 7 years ago Closed 7 years ago

[Settings] "short_name" is not used in "Settings>Privacy Controls>Transparency Control>Applications"

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S9 (3apr)
Tracking Status
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: hcheng, Assigned: marta)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 1 obsolete file)

As bug 1134680, short_name should be used in Settings app. However, when I go to "Settings>Privacy Controls>Transparency Control>Applications", it shows "name" but not "short_name".
Blocks: 1134680
blocking-b2g: --- → 2.2?
Whiteboard: [systemsfe]
Wilfred, can you confirm that this is needed? Team thought privacy controls was removed in 2.2 anyway.
Flags: needinfo?(wmathanaraj)
Marta, can you take this one? Ben Francis can help out if you have any questions.
Flags: needinfo?(marta)
(In reply to Candice Serran (:cserran) from comment #1)
> Wilfred, can you confirm that this is needed? Team thought privacy controls
> was removed in 2.2 anyway.

Hi Candice,
privacy controls is not blocking 2.2 and will be fix by DT.
Thanks!
blocking-b2g: 2.2? → ---
taking
Assignee: nobody → marta
Flags: needinfo?(marta)
Comment on attachment 8578576 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

Here is a patch. I have included the manifest.short_name the way it was done in bug 1134680, as an alternative to the manifest.name
Ben, could you have a look and let me know if that was the right way?
Attachment #8578576 - Flags: feedback?(bfrancis)
Comment on attachment 8578576 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

Thanks for the patch!

This is fine, but you might consider using ManifestHelper.displayName as implemented in 1137501 instead, this allows us to keep this logic in one place. We may update all the other places to use this too.
Attachment #8578576 - Flags: feedback?(bfrancis) → feedback+
Comment on attachment 8578576 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

corrected. Ben, can you r+ it?
Attachment #8578576 - Flags: review?(bfrancis)
Comment on attachment 8578576 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

That's great, thanks.

I've just restarted some tests on Treeherder that look like infrastructure problems, then we can add checkin-needed.
Attachment #8578576 - Flags: review?(bfrancis) → review+
TreeHerder looks unhappy, you may need to rebase this pull request against master.
Flags: needinfo?(marta)
Flags: needinfo?(marta)
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.
I did it, still won't land...?
Similar problem as we don't really have a privacy panel component yet. 

Let's try landing manually for now: https://github.com/mozilla-b2g/gaia/commit/6e24aff11fbb385a6fb1651536a97263457b6c50
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Sorry about that, strange as the gaia-try has a passing Gu test. I was too optimistic here.
Flags: needinfo?(kgrandon)
.displayName is a property of the ManifestHelper, and _makeAppRepresentation is using the manifest directly. This shouldn't have passed unit tests - might need to look at those to figure out why it wasnt caught. If you grep for `new ManifestHelper` you'll find several examples of how its used; refactoring to use that may be the right thing to do here.
Hi Marta,
Could you check whether comment 17 from Sam helps?
Thanks!
Status: REOPENED → NEW
Attachment #8578576 - Attachment is obsolete: true
Flags: needinfo?(marta)
Comment on attachment 8580599 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

Sam, could you?
Attachment #8580599 - Flags: feedback?(sfoster)
Comment on attachment 8580599 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

See comments in the PR.
Attachment #8580599 - Flags: feedback?(sfoster)
Comment on attachment 8580599 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

Kevin, I know Sam is on PTO, could you please review? I addressed all Sam's remarks and fixed the tests. Now should work.
Attachment #8580599 - Flags: review?(kgrandon)
Comment on attachment 8580599 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

Looks good to me. Thanks.
Attachment #8580599 - Flags: review?(kgrandon) → review+
In master: https://github.com/mozilla-b2g/gaia/commit/70ce8d87be7df4dbeff34c3ebde787a55905de6c
Status: NEW → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
NI myself for following verification.
Flags: needinfo?(hcheng)
Whiteboard: [systemsfe] → [systemsfe]javascript:void(0);
Verified master branch
* master
Build ID               20150329160203
Gaia Revision          67ad91f3f660b1f16b354ee4c5159ddc5a74d149
Gaia Date              2015-03-28 10:02:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/385840329d91
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150329.191719
Firmware Date          Sun Mar 29 19:17:28 EDT 2015
Bootloader             L1TC100118D0
Flags: needinfo?(hcheng)
Whiteboard: [systemsfe]javascript:void(0); → [systemsfe]
Comment on attachment 8580599 [details] [review]
[gaia] martasect:Bug_1139735 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):1139735
[User impact] if declined:the issue is fixed on Master so we need backward compatibility
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: non
Attachment #8580599 - Flags: approval-gaia-v2.2?(dietrich)
Flags: needinfo?(wmathanaraj)
Attachment #8580599 - Flags: approval-gaia-v2.2?(dietrich) → approval-gaia-v2.2?(bbajaj)
Attachment #8580599 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
verified, and add test case to v2.2 on MozTrap.

Build ID               20150412162502
Gaia Revision          cec00d643f517ffd96cde559cd3bbd43ab85816c
Gaia Date              2015-04-10 21:41:12
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5005522fd68e
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150412.200029
Firmware Date          Sun Apr 12 20:00:40 EDT 2015
Bootloader             L1TC000118D0
Status: RESOLVED → VERIFIED
Comment on attachment 8642991 [details] [review]
[gaia] albertopq:fix-privacy-test > mozilla-b2g:master

Kevin, as you reviewed the original patch, could you please take a look to this? It seems that the unit test are failing in master.
Attachment #8642991 - Flags: review?(kevingrandon)
Comment on attachment 8642991 [details] [review]
[gaia] albertopq:fix-privacy-test > mozilla-b2g:master

Oddly though, this test started passing in master once bug 1186301 was reverted - is it still needed?

Should we reland this as well as bug 1186301? Please flag again for review if needed, looks fine to me as long as tests pass.
Flags: needinfo?(dwi2)
Attachment #8642991 - Flags: review?(kevingrandon)
I cannot see the failing test as PASS in the GU13 logs after the backout. May be is running in other chunk? I'm not sure how the tests are splited in the GU.

Anyway, I it seems that with both, the test runs and passes:

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=88ca4cc0b13ec33313f051167395471cbdb6fd38
My observation is the same with :albertopq.
The failing test '/privacy-panel/test/unit/app_list_test.js' never runs in treeherder. It seems that app_list_test.js usually get skipped. I randomly pick two jobs for example:

1. 
https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/_qxsmxRnS2-cdRwglK7Y6w/0/public/logs/live_backing.log

  ...
Running tests:  [ '/music/test/unit/metadata/mp4_test.js',
  ...
  '/privacy-panel/test/unit/ala/define_custom_location_test.js',
  '/privacy-panel/test/unit/app_list_test.js',
  '/privacy-panel/test/unit/rp/auth_test.js' ]
...
TEST-START | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty
TEST-PASS | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty
TEST-END | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty took 0 ms
TEST-START | null | [privacy-panel-test/unit/rp/auth_test.js] RP Auth panels validate passphrase should validate given passphrase during register
TEST-PASS | null | [privacy-panel-test/unit/rp/auth_test.js] RP Auth panels validate passphrase should validate given passphrase during register
TEST-END | null | [privacy-panel-test/unit/rp/auth_test.js] RP Auth panels validate passphrase should validate given passphrase during register took 0 ms
...


2. 
https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/cac-zad_Tv2p9jPD3muwqg/0/public/logs/live_backing.log

...
Running tests:  [ '/music/test/unit/tabbar_test.js',
  ...
  '/privacy-panel/test/unit/ala/define_custom_location_test.js',
  '/privacy-panel/test/unit/app_list_test.js' ]
...
TEST-START | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty
TEST-PASS | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty
TEST-END | null | [privacy-panel-test/unit/ala/define_custom_location_test.js] ALA CustomLocation should validate false when longitude is empty took 0 ms
*~*~* Results *~*~*
passed: 171
failed: 0
todo: 0

  171 passing (2ms)
I also try to run app_list_test on my laptop, without patch from :albertopq. However test agent tells me no test was run:
Running tests:  [ '/privacy-panel/test/unit/app_list_test.js' ]

  0 passing (0ms)

But I got this log from nightly:

JavaScript error: app://privacy-panel.gaiamobile.org/js/vendor/alameda.js?time=1438757617501, line 537: Error: Not loaded: app://system.gaiamobile.org/shared/test/unit/mocks/mock_manifest_helper.js


My guess is that app_list_test already got skipped for some unknown reason for a long time, but my patch for bug 1186301 accidentally changes test chunk and makes app_list_test to be the first test in the chunk

Running tests:  [ '/privacy-panel/test/unit/app_list_test.js',
  '/privacy-panel/test/unit/rp/auth_test.js',
  ...

(see https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/UoL66wjDTsqXPmyiJDj9hA/2/public/logs/live_backing.log)

So I think the patch from :albertopq is necessary since this test is already broken but just accidentally got skipped for a long time.
Flags: needinfo?(dwi2)
Hi Kevin,

According to comment #33 and comment #34, could you help to review the patch? Thanks

Bugzilla did not allow me to flag review for this patch https://bugzilla.mozilla.org/attachment.cgi?id=8642991, that's why I flag NI.
Flags: needinfo?(kevingrandon)
Comment on attachment 8642991 [details] [review]
[gaia] albertopq:fix-privacy-test > mozilla-b2g:master

Alright, let's get these landed then. Thanks!
Flags: needinfo?(kevingrandon)
Attachment #8642991 - Flags: review+
You need to log in before you can comment on or make changes to this bug.