#25658: Activity 2.1: Improve user understanding and user control by clarifying Tor Browser's security features -------------------------------------------------+------------------------- Reporter: isabela | Owner: | antonela Type: project | Status: | needs_revision Priority: High | Milestone: Component: Applications/Tor Browser | Version: Severity: Normal | Resolution: Keywords: ux-team, GeorgKoppen201812, | Actual Points: TorBrowserTeam201903R, tbb-8.5 | Parent ID: | Points: Reviewer: | Sponsor: | Sponsor17 -------------------------------------------------+------------------------- Changes (by mcs):
* status: needs_review => needs_revision Comment: Here are our comments on the tor-browser changes (by the way, thanks for writing code that is nicely organized and easy to read): High level questions for Georg and others: 1. Should we add copyright notices to the `browser/components/securitylevel/` files? 2. Should we rename the `security_slider` pref and migrate the old value at this time, or later, or never? Maybe use the name `torbrowser.security_level` and migrate to contiguous values (1-3). Commit message: 1. The first line is very long. Maybe shorten to `Bug 25658: Replace security slider with security level UI`. 2. s/hangar/hanger/ (two occurrences). browser/app/profile/000-tor-browser.js - there are some additional NoScript and HTTPS-E references that should be removed from the `browser.uiCustomization.state` value: * https-everywhere-button * https-everywhere_eff_org-browser-action * _73a6fe31-595d-460b-a920-fcc0f8843232_-browser-action securityLevelPanel.css - please combine all of the margin-related rules into one within each CSS selector block, or at least put them on adjacent lines so it is easy to spot the margin things. securityLevel.js - SecurityLevelStrings: 1. The `intialize our strings with en-US defaults...` comment seems a little backwards. Maybe replace with `read localized strings from Torbutton; use hard-coded en-US strings as fallback` 2. Is it worth it to rely on the `getlocale()` function from Torbutton? 3. There is one `/` missing after `https:` in the URL. It still works though! securityLevel.js - SecurityLevelPrefs: 1. The `_str` suffix for the pref name constants is a little confusing. Maybe use `_pref` instead. securityLevel.js - SecurityLevelButton: 1. Consider renaming `securitySlider` to `securityLevel` (and maybe rename the pref getter too). 2. Kathy and I think the function name `_readPrefsInternal` could be better, e.g., replace all occurrences with `_configUIFromPrefs()`. securityLevel.js - SecurityLevelPanel: 1. In the `panel` getter, `_panel` is set but not used anywhere else. Maybe you meant to cache that value? 2. Remove the comment inside `openAdvancedSecuritySettings()`. 3. For `_populateXUL()`: if you have time and can do it cleanly, consider adding a helper function that sets the strings within each of the three radio sections (the code is very similar for each). securityLevel.js - SecurityLevelPreferences: 1. _populateXUL(): remove blank line before closing brace within `vboxStandard` section. securityLevel.js - miscellaneous typos: s/abotu/about/ s/hangar/hanger/ s/lable/label/ s/re-render/re-renders/ s/scurity/security/ s/upate/update/ s/if(/if (/ securityLevel.js - remove blank lines at the beginning of function bodies: SecurityLevelButton.init() SecurityLevelButton.onCustomizeEnd() SecurityLevelPreferences.init() SecurityLevelPreferences.selectSecurityLevel() -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25658#comment:87> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs