We want to land the privacy Panel in gaia-master/dev_apps.

This is the future of mobile privacy panel app. It's going into dev_apps while we test and stabilize it.

Hi Vivien, kgrandon said you should be the one to quickly review this. this is the result of the collaboration between Mozilla and DT to build an app that manages the new privacy features going into the phone (e.g. user settable location, no location, remote privacy management, etc). There is a small gecko piece that is also about to submitted for review. We implemented this as a separate app for some very specific reasons. If you'd like to know more, talk to Kevin Hu.

I want to point out that there is placeholder art and lorem ipsum text in the walkthrough portion of the app. We're just waiting for the assets and will add them ASAP.

Please tidy up the pull request and either squash everything into a single commit, or make sure all commits have a reviewer listed and in proper format. All commits also need to link to a bug number. My recommendation is to squash them into a single commit and mark Vivien as the reviewer, just due to the fact that there are some misformatted commits without bug numbers.

Comment on attachment 8492315 [details] future of mobile privacy panel app Hi Dave, Thank you for the implementation! Looks overall great! Thank you! There are some layout issues, please refer to my attachment: #1 OK button should be the round & gray one, and "Forgot/change my passphrase" should be a hyper link style. #2 Tap on back button should go back to #1 instead of jumping back to initial page. OK button should be the round & gray one. #3 It should be a cross button on the top left. #4 The current country should be defined by default. (As we agree in last con-call) #5 Mike & I just found that in this page we cannot know which app changes its accuracy. So we suggest to add a status for certain app so that users can see what app has been modified. I've made some screen for you to understand what I say, hope it's helpful and not adding too much effort to you! Thanks.

Screen attached.

Comment on attachment 8495964 [details] The second attempt with fixes Hi Marta, The patch looks no change in ux layout...

Hey, maybe the confusion comes from the outdated flowchart on bugzilla. Here is the new one.

Comment on attachment 8497474 [details] 140930 privacy panel flowchart.pdf I will look at the patch, but the flow chart is really handy so thanks for that.

The current pull request is closed and the branch is deleted. Is there a current place I should be looking for the code at?

Kevin, just submitted a new version, not sure why the current was canceled

Comment on attachment 8498268 [details] Pull request for Privacy Panel I am starting to step through this again, and in order to provide feedback/review I need to see this in a working state. When trying to access the privacy panel through settings it currently doesn't work. Maybe this is because someone forgot to add the privacy_panel.js file?

Please re-flag me for feedback if addressed. We can use the same pull request here, just push to the branch, thanks.

Comment on attachment 8498279 [details] Privacy Panel pull request Arthur - Could you review the settings portion of this? Feel free to give other parts a pass also if you have time, but mainly wanted your feedback on Settings. Thanks!

Comment on attachment 8498279 [details] Privacy Panel pull request Kevin, I'll let EJ take a look at the patch first due to my long review queue. EJ, could you help review the patch? Thanks.

Comment on attachment 8498279 [details] Privacy Panel pull request Left some comments on github and please go check it ! The main thing that needs to be changed is that we have to use AMD style in Settings for future panels. Please check the reference link on Github to understand more about this and feel free to set r? on me after things are done. BTW, I have one question about this panel. After checking the UX spec, it seems that the panel is designed to be a standalone app and get embedded from Settings app. If this is the right flow as what I thought, I think we may have to file another follow-up bug to remove the `app.lunch()` part. In Settings app, we have designed a way to embed apps inside an iframe. The reason why we made this is because BT app wants to do the same thing as privacy app (It can be launched by itself and also from Settings app). But sadly, due to some Gecko issues about OOP, the change for this part in BT app were backed out (But the mechanism in Settings is still there and is intact). So, based on our offline discussions with Gecko gurus, OOP part may get fixed in 2.2 and we would bring this back if possible. Right now, it's okay to lunch the app from Settings app but please help to file a follow-up to make sure we will remove this part and get integrated in Settings app. Thanks !

This is a final version of Privacy Panel. @EJ: I have replied to your comment in github, not sure if you've seen it. We are rebasing to 2.1 so can we leave it without the AMD style for now? We have hard deadline tonight and it will take some time to fix it

FInal UI flowchart

Comment on attachment 8500308 [details] 141006 privacy panel flowchart.pdf Hi Marta, Thank you for the flowchart & the patch. I will give you review+ for the flowchart, but just one suggestion about it. The attention windows(ALA-5 & RPP-5) would be better if you use our Building Blocks standard confirmation dialog (https://developer.mozilla.org/en-US/Apps/Tools_and_frameworks/Building_Blocks/Confirmation) Also, about the patch, it seems to have a few bugs, but I believe it a easy fix: 1. The X button on Location Accuracy should be < button 2. The handle of slider in Location Accuracy seems to be missing 3. For RPP, if user press "Take me there" in RPP-5, it will just take the user to Settings main page and not to Screen Lock settings as the flowchart defined. 4. The X button in the Guided Tour seems to be blocked by the tour image and is not visible.

Comment on attachment 8500308 [details] 141006 privacy panel flowchart.pdf Left the ui-review to Harley

Comment on attachment 8500273 [details] Final version of Privacy Panel. Hi Marta, because this code would get merged into our codebase, please did address my comments on Github to make Settings app robust. I did leave some comments on previous PR but I can't see any change there, so I can't let this code merge into Gaia. After discussing with Arthur & Bruce, we came up some problems about this patch and please help us clarify / fix them. 1. This patch would be landed into Gaia, so this means that all future devices would be influenced with this patch. And right now, based on this patch, we can notice that this app is put under dev_apps which means if there is any device not flashed with developer build, they would all see this menu item on Settings app. In order to fix this, it would be better to use one mozSettings key under developer tools to make sure this item would be shown by design. You can name it like this "developer.privacy-panel.enabled". Reference : https://github.com/mozilla-b2g/gaia/blob/master/build/config/common-settings.json#L37 2. I did leave couple of questions on Github last time but no one got answered or fixed, so please go ahead to check them instead of making a new PR without change. BTW, because the new PR doesn't have any of my comments on previous PR, please create a second commit to make sure you did address them there and I can review them more easily. Don't worry, we will squash them into one before merging. Reference(previous PR) : https://github.com/mozilla-b2g/gaia/pull/24620 3. Make the code AMD won't take you more time than 1. and 2., so it would be quick to take a little time to change them to AMD style with less efforts. You have to make that privacy panel as one panelItem below panels/root and you can manipulate its visibility in root.js Referenece : https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/root/root.js -- I think my comments are clear enough (with references) for you to help you revise your patch more easily. If this is still not acceptable for you or may mess up with deadline, I am okay and can accept it if you want to find others to review this patch. Hope this information helps you ! Thanks :)

EJ: I have addressed your comments on github, still no response.

HI Marta, there are so many bugs and mails here, so if you want me to review or feedback, please remember to set r? or ni? and I will come back to help with your patch. -- (In reply to marta from comment #28) > EJ: I have addressed your comments on github, still no response. For this comment, I did check github and you just left one comment without doing any change no matter on old PR or new PR. So ... I don't think you did "address" my comments with related changes like AMD, inconsistent indentations, wrong implementations. Based on your comment, I did grep settings app and I finally realized that your code is copying from settings/js/developer.js. We will definitely refer some existed patches to make a new patch to make sure the coding style, concepts ... etc are consistent with the others. But this doesn't mean you can directly copy & paste them without modifying. For example, you change `alert(navigator.mozL10n.get('no-ftu'));` to `alert(navigator.mozL10n.get('no-settings'));` just because there is one existed line doing the same thing. But, you didn't realize how mozL10n work in the shadow that you have to add one more l10n change in locales folder. If you are not familiar with this (so did I when I came here in the beginning), it's ok and that's how review process work because we can share the knowledge together to make sure you can get involved and join us to make this OS better. But sadly ... After checking your comments on Github, I can only see that you have no passion to discuss more and just left "as above" on all my other comments and this really disappoints me a lot. If the deadline is urgent for you, based on my offline discussion with Arthur, you can land your app first without changes in Settings app and let's take time to fix them later, otherwise, please fix them. Feel free to set r? or ni? when you do finish them (or want to discuss/clarify something), thanks !

EJ: Here is the new pull request. I really wanted to talk to you before leaving my very vague comments on the github, but couldn't find you on irc. Sorry for the miscommunication. We have done everything you've asked for. The new pull request: https://github.com/mozilla-b2g/gaia/pull/24940 Hope now code matches your expectations.

Comment on attachment 8501685 [details] Pull request Thanks Marta, this looks like a good start ! I just left some comments on github with details to make sure you can follow the steps to write in the same way. If you have any question, please feel free to ask and I am glad to help. And about irc, I have been absent for few days because i want to focus on works on my side, in order to help you, I would be there sometimes and I think you can find me there in the following days (:eragonj) Thanks.

And by the way, every time when I want to test you patch on my device with `make clean && DEBUG=1 make rest-gaia`, my phone would become a brick and can't work. Should I do anything special to make this patch work on my flame ? Any information / tips would be grateful.

(In reply to EJ Chen [:eragonj][:小龍哥][ni? if you need me] from comment #27) > 1. This patch would be landed into Gaia, so this means that all future > devices would be influenced with this patch. And right now, based on this > patch, we can notice that this app is put under dev_apps which means if > there is any device not flashed with developer build, they would all see > this menu item on Settings app. > > In order to fix this, it would be better to use one mozSettings key under > developer tools to make sure this item would be shown by design. You can > name it like this "developer.privacy-panel.enabled". > > Reference : > https://github.com/mozilla-b2g/gaia/blob/master/build/config/common-settings. > json#L37 I almost forgot this comment, please remember to add this so that this menuItem would be shown only if this mozSetting is on, otherwise, other devices using latest Gaia would set a menuItem for privacy panel but can't work at all. Please refer https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/elements/developer.html#L80 to add one more option there.

Re: GT-3 in attachment 8500308 [details] This creates a high risk that users will see this as a way to protect their location. We can't do that, so we shouldn't make statements like: "you are not disclosing the path you are on". This is demonstrably false.

(In reply to Martin Thomson [:mt] from comment #35) > Re: GT-3 in attachment 8500308 [details] > > This creates a high risk that users will see this as a way to protect their > location. We can't do that, so we shouldn't make statements like: "you are > not disclosing the path you are on". This is demonstrably false. Created bug 1080969 to cover the work to address this.

Dave/Marta, I am a bit confused about this bug - why are there any changes being made to the settings app here? The title of this bug is "land the privacy panel in /dev_apps" - wasn't this the plan here? Why are there changes being made to the settings app?

(In reply to Paul Theriault [:pauljt] from comment #37) > Dave/Marta, I am a bit confused about this bug - why are there any changes > being made to the settings app here? > > The title of this bug is "land the privacy panel in /dev_apps" - wasn't this > the plan here? Why are there changes being made to the settings app? Ah nevermind - I see from the patch that these changes are just for integration, and they will be disabled on production builds where /dev_apps are not present.

Updated version of the flowchart to communicate the threats to the user.

Comment on attachment 8503054 [details] Updated flowchart with closeup for ALA-2 and GT-3 The explanation here is a little strange: "[...], so some services might still use your exact position" How can they know? "Location Adjustment influences the GPS coordinates. It will not affect your IP-address or locale settings, so some services might still use your exact position." This is not solely about GPS, I hope: "Location Adjustment attempts to obscure your location my reducing the accuracy of the position given to applications." Don't hyphenate "IP address", and make sure that you acknowledge other sources of information that can be used. "You will still expose your IP address and other information to applications, such as your preferred language and locale. Applications could use other methods to recover your location with improved accuracy, including matching your reported position to a map." And I think that you need a separate paragraph: "The most effective way to obscure your location from an application is to not share your position." GT-3 is awkwardly hyphenated. The grammar is a little odd in the first sentence: "Not each app". Try "Not all applications require your exact location ..."

Martin: will be changed accordingly.

The feature definition was already approved during the overall flowchart UI review. This file was uploaded for reasons of completeness. see https://bugzilla.mozilla.org/attachment.cgi?id=8500308&action=edit for the overall flowchart.

Comment on attachment 8503253 [details] Changed as requested In latest PR, you didn't address stuffs I talked to you online on IRC. 1. Remove changes in main.js and load this script in root/panel.js like how the other menuItem did 2. Don't load script from index.html 3. Hide the menuItem by default and use mozSettings (developer.xxx.yyy) to make sure this menuItem would only be shown if related developer option is enabled 4. Cache getAll() and manifestURL in privacy_panel.js 5. *** Don't keep creating new PR because this would make reviewing process more harder and I have to keep finding out comments I left on old PRs. Marta, please don't keep settings r? on non-finished PR because this would definitely waste each other's time and I am sure I did tell you what/where to update these things on IRC.

EJ, I' very sorry I used the wrong patch (we develop on 2.1 but were told that to land it to dev_apps we need to rebase to master every time, so I messed the rebase up). Now I have added to the current pull request the proper files. I am so sorry, was sure that everything is done - that's why I r? you...

It looks like you marked that as review+. Are you sure you meant to do that without the settings guys taking another look?

Comment on attachment 8503253 [details] Changed as requested Sorry! Typo! Of course I need EJ to sign off! Thanks Kevin

Comment on attachment 8503253 [details] Changed as requested Few notes below : 1. Add two missing tests in unit tests 2. Settings app can't open so please make sure your patch does work and doesn't break anything. 3. I can't run your unit test (see next note) 4. I did check your CI result and it looks like Gb, Gij, Gu, Li are all marked as red - https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=c94e21f4e1ec 5. I left some comments on Github about other minor problems and please check it

Comment on attachment 8503054 [details] Updated flowchart with closeup for ALA-2 and GT-3 I am not sure your string is strong enough. What about something like: Remember: Location Fuzzing doesn't always work. Your location can been determined by an determined adversary or even by your IP-address. Use with caution. Thoughts?

Comment #48 and #49 - Marta please could you have a look at the items and make changes as appropriate please.

@Doug - the text was changed @EJ - new pull request is there and waiting for you:)

Why is that landing in dev_apps/ and not apps/ ? Landing in apps/ will not make it automagically part of user builds, it still needs to be added to build/config/phone/apps-production.list

We were told by Wilfred and Dave that the right way to do it is to first land it in dev_apps. But yes, you are right - we will need to land it in apps in the end. Could you have a look at the code and tell me if something will need to be changed for apps?

The reason for this was likely the fact that we used to include *everything* from the apps folder for phone builds. This has only recently changed.

So what should I do now??

so the recommendation was we land this is dev_apps to make things work easier and then do a review and land it in apps. dave feel free to correct me.. but things have evolved since then!

(In reply to Wilfred Mathanaraj [:WDM] from comment #56) > so the recommendation was we land this is dev_apps to make things work > easier and then do a review and land it in apps. dave feel free to correct > me.. but things have evolved since then! That makes no sense. dev_apps/ is not a place where we can land anything without review and afterward move things to apps/. If you need a sandbox, use a branch or a forked repo.

Comment on attachment 8503253 [details] Changed as requested r+ with nits on github for settings part. -- And here comes few notes for Settings part below : 1. Remember to file a follow-up bug to remove developer checkbox and the cache part in Settings when this app is ready for apps/* 2. You have to rebase again for this patch because there are some conflicts on Settings app. 3. Make sure CI is all green before merge. Thanks.

(In reply to Fabrice Desré [:fabrice] from comment #57) > That makes no sense. dev_apps/ is not a place where we can land anything > without review and afterward move things to apps/. If you need a sandbox, > use a branch or a forked repo. Let me copied my words for Marta on IRC : -- 18:27 (EragonJ) if this is going to be laned in apps, there must be resources like owner, peers for this component and may have to follow some formal process but I am not quite sure how this happen and you may have to ask someone else -- As what I said on IRC, I am not quite sure how this happen, but if this feature is going to be landed on apps/, I agreed with Fabrice, we need a formal process including owner/peer for it.

(In reply to Fabrice Desré [:fabrice] from comment #57) > (In reply to Wilfred Mathanaraj [:WDM] from comment #56) > > so the recommendation was we land this is dev_apps to make things work > > easier and then do a review and land it in apps. dave feel free to correct > > me.. but things have evolved since then! > > That makes no sense. dev_apps/ is not a place where we can land anything > without review and afterward move things to apps/. If you need a sandbox, > use a branch or a forked repo. So we need to be able to make builds that non-technical people can flash to devices and have the privacy panel app installed. We can't expect the people reviewing this to make builds from a source tree. So the thinking was to put it in dev_apps while it gets reviewed and tweaked. And once all interal/external stakeholders are happy, we can move the privacy panel into apps/.

If you put the app in apps/ it will end up in engineering builds (because of https://github.com/mozilla-b2g/gaia/blob/master/build/config/phone/apps-engineering.list#L1), but if you put it in dev_apps/ you'll have to update gaia/build/config/phone/apps-engineering.list Also I don't understand why we can't expect reviewers to make builds...

Fabrice can you make a statement - should I file a new bug to land it in the apps? Or should I wait? If wait then for what? Can we push it to dev_apps and start merging to apps?

EJ gave r+ to the current app, so I don't see any reason not to just land it in apps/. You just need to update your PR to move the app to apps/ and remove the changes to build/config/phone/apps-engineering.list

(In reply to Fabrice Desré [:fabrice] from comment #63) > EJ gave r+ to the current app, so I don't see any reason not to just land it > in apps/. > > You just need to update your PR to move the app to apps/ and remove the > changes to build/config/phone/apps-engineering.list No, app don't get included in the build automatically event with production build. We whitelist apps in phone/apps-production.list. EJ's review only covers changes in Settings app. It has nothing to do with dev_apps/privacy. I also object landing privacy app in apps/ and/or enable it in production build before we could do a proper review of the app itself.

Fabrice, if you would like to review the app and enable the app for engineering build *only*, I have no problem with that except for build size concern. We can figure out other non-technical questions elsewhere.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #64) > (In reply to Fabrice Desré [:fabrice] from comment #63) > > EJ gave r+ to the current app, so I don't see any reason not to just land it > > in apps/. > > > > You just need to update your PR to move the app to apps/ and remove the > > changes to build/config/phone/apps-engineering.list > > No, app don't get included in the build automatically event with production > build. We whitelist apps in phone/apps-production.list. That's exactly what I said, but whatever. > EJ's review only covers changes in Settings app. It has nothing to do with > dev_apps/privacy. Then it's unfortunate that he is marked as r+ on the PR that covers everything. Let's split the settings part out the app. > I also object landing privacy app in apps/ and/or enable it in production > build before we could do a proper review of the app itself. I agree for the review part, my assumption was that EJ reviewed everything.

who should we ask to review the privacy panel app?

Comment on attachment 8503253 [details] Changed as requested I think it's great that we already have a review for the settings portion, and although not ideal, probably find that it's in the same patch. I have already asked that this be reviewed by Tim or Vivien before landing, regardless if it lands in dev_apps, or apps. I think that we should probably have a gaia owner do this for every new app that comes in.

(In reply to Fabrice Desré [:fabrice] from comment #66) > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from > comment #64) > > (In reply to Fabrice Desré [:fabrice] from comment #63) > > > EJ gave r+ to the current app, so I don't see any reason not to just land it > > > in apps/. > > > > > > You just need to update your PR to move the app to apps/ and remove the > > > changes to build/config/phone/apps-engineering.list > > > > No, app don't get included in the build automatically event with production > > build. We whitelist apps in phone/apps-production.list. > > That's exactly what I said, but whatever. > Yeah I read that up in the previous comments before I write mine, sorry about it.

It would be good to add a few integration tests to this pull request to validate the end-to-end flow. Looking at some of the pre-existing settings tests[1] would be a good start most likely. [1] https://github.com/mozilla-b2g/gaia/tree/master/apps/settings/test/marionette

Just in case. I have started to do a *basic* review of this block of code. I still have around 1500 lines of pure js to review. I hope to finish it by tomorrow. Then I will likely need to do a second and deeper review to understand what exactly this block of code is trying to do - as reviewing a complete app in one PR is always a bit tricky.

(In reply to Kevin Grandon :kgrandon from comment #68) > I think that we should probably > have a gaia owner do this for every new app that comes in. I'm fine with this. We likely need to define a process where new app can be checked in but splitted into logical pieces of code (commits) to set up the root of the app and make it clear the interaction piece by piece.

Vivien, just a heads up: I gave it a quick skim for security/privacy bugs that will have to be addressed in the future. If you notice things during your reviews, they may already exist and block bug 1083953,

The pull request containing most fixes requested by security review (missing the web crypto): https://github.com/mozilla-b2g/gaia/pull/25036#issuecomment-59691674 Try server: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=9705e4e6b6c5

Vivien, could you please give us some comments, if you have them already? We have resources to start fixing now, instead of getting whole huge bucket of comments. Also - it might be helpful to know that we are almost ready with refactoring of the code - https://github.com/Wojtek-Lakomy/gaia/tree/pp-refactor here is the branch with the refactoring close to being done. So we won't have one huge file any more, and everything should be more mozilla-style, hopefully

I know Vivien has a *long* file with review notes, but in the meantime you can start with * cleaning up the unused code in style/ * as suggested by Kevin in Comment 70, add a basic end-to-end marionette test coverage, another source of inspiration [1] [1] https://github.com/mozilla-b2g/gaia/blob/51ab1cfc1c4361c3761dd68b5c27644dcd9263a9/apps/findmydevice/test/marionette/findmydevice_test.js

Comment on attachment 8503253 [details] Changed as requested Look like Etienne is helping out. I can do the final passes if Etienne would like me to do that.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #77) > Comment on attachment 8503253 [details] > Changed as requested > > Look like Etienne is helping out. I can do the final passes if Etienne would > like me to do that. Sounds like a good plan! (me helping out then you doing the final review passes)

Comment on attachment 8503253 [details] Changed as requested Sorry, I just found a timing issue that may cause problem on Settings. Please check my comments on Github for them (with some tiny nits) thanks.

I have corrected everything and submitted a new pull request on the other bug - I suggest we close this bug and move to the /apps bug.

I am closing the bug. We have the new bug Bug 1083953 that makes much more sense in the current context. Keeping two bugs makes everyone confused. The current pull request addresses all the fixes suggested here (In comment #79, #76, Tests from comment #70 are still being added).

Comment on attachment 8503253 [details] Changed as requested Ricky, can you give us some feedback on the apps/privacy-panel/build part of the patch? (can we get away with less custom build code?)

Comment on attachment 8503253 [details] Changed as requested we get a *lot* of Privacy Panel(22880): Content JS WARN: mozL10n: A non-existing entity requested So flagging Stas for a quick l10n sanity check.

So, this will not work at all because it uses the old "locales.ini" approach to loading localization resources. Instead, it should use the new manifest.webapp + URL templates - https://github.com/mozilla-b2g/gaia/blob/2ca5fcfada83cb41eba76aa8e4976c434caa1ac8/apps/clock/index.html#L52-L55 I'll review further when this is fixed. Also, this bug seems to be marked as invalid. Should we review it in bug 1083953?

yes please. please move to bug 1083953 . marta could you look at this comment and make the changes in bug 1083953?

Comment on attachment 8503253 [details] Changed as requested The apps-engineering.list looks fine to me. But I didn't see any apps/privacy-panel/build part here.

(In reply to Ricky Chien [:rickychien] from comment #86) > Comment on attachment 8503253 [details] > Changed as requested > > The apps-engineering.list looks fine to me. > But I didn't see any apps/privacy-panel/build part here. Moving PR, it was probably not needed and removed before you got there, sorry. Thanks for taking a look!

Comment on attachment 8503253 [details] Changed as requested As Zibi explained in comment 84, if this is going to land on master (2.2), please remove the locales.ini file and instead move to using URL templates for resource linking, like here in Settings: https://github.com/mozilla-b2g/gaia/blob/a64a15961e62cbc832e2886a3d4bc7b3e9d3b1c9/apps/settings/index.html#L54-L64

For some reasons I was unable to finish the review as soon as I have expected. Here is the state of where I was on the 16th of October. I know some comments may already have been addressed by the various back and forth with other people, but I'm pretty sure there are still some valid concerns here. Please take a look at it.