Last Comment Bug 737177 - Port the security pane to the in-content preferences implementation
: Port the security pane to the in-content preferences implementation
Status: VERIFIED FIXED
[testday-20120622]
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 15
Assigned To: Jon Rietveld
:
Mentors:
Depends on: 949589
Blocks: 718011
  Show dependency treegraph
 
Reported: 2012-03-19 13:49 PDT by Jon Rietveld
Modified: 2013-12-12 11:06 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
in-content security pane (13.07 KB, patch)
2012-03-19 13:51 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
security in-content pane (12.32 KB, patch)
2012-03-22 16:15 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
security in-content pane (12.48 KB, patch)
2012-03-24 20:10 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
security in-content pane (12.86 KB, patch)
2012-03-25 23:10 PDT, Jon Rietveld
jaws: feedback+
blair: feedback+
Details | Diff | Splinter Review
security in-content patch (12.69 KB, patch)
2012-04-08 22:54 PDT, Jon Rietveld
jaws: feedback+
Details | Diff | Splinter Review
Security with updated data-category (12.71 KB, patch)
2012-04-10 11:09 PDT, MSU Capstone Team
no flags Details | Diff | Splinter Review
security in-content patch for new landing patch (12.72 KB, patch)
2012-04-12 12:40 PDT, Jon Rietveld
no flags Details | Diff | Splinter Review
security in-content patch for new landing patch (12.82 KB, patch)
2012-04-12 13:20 PDT, Jon Rietveld
jaws: review+
blair: review-
jaws: feedback+
Details | Diff | Splinter Review
security in-content patch (12.71 KB, patch)
2012-05-07 11:12 PDT, Jon Rietveld
blair: review-
Details | Diff | Splinter Review
security in-content patch (12.76 KB, patch)
2012-05-08 16:47 PDT, Jon Rietveld
blair: review+
Details | Diff | Splinter Review

Description Jon Rietveld 2012-03-19 13:49:01 PDT
move the security pane from preferences into in-content UI preference pane
Comment 1 Jon Rietveld 2012-03-19 13:51:06 PDT
Created attachment 607297 [details] [diff] [review]
in-content security pane

Moved Security into in-content UI. In the JavaScript file made changes at like 56-59, 104-107, 190-191, 201-202, and 212-214. All dialog box changes.
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-03-22 11:37:28 PDT
Comment on attachment 607297 [details] [diff] [review]
in-content security pane

Review of attachment 607297 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/security.xul
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +   - You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +    <preferences id="securityPreferences">

This whole file is indented by four characters. Can you please remove this extra indentation?
Comment 3 Jon Rietveld 2012-03-22 16:15:58 PDT
Created attachment 608526 [details] [diff] [review]
security in-content pane

fixed indentation and based off of new landing patch
Comment 4 Jon Rietveld 2012-03-24 20:10:42 PDT
Created attachment 609066 [details] [diff] [review]
security in-content pane

added consistent title
Comment 5 Jon Rietveld 2012-03-25 23:10:08 PDT
Created attachment 609227 [details] [diff] [review]
security in-content pane

included search vbox's, and fixed title
Comment 6 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-04-03 22:17:46 PDT
Comment on attachment 609227 [details] [diff] [review]
security in-content pane

Review of attachment 609227 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/security.xul
@@ +38,5 @@
> +</vbox>
> +
> +<!-- addons, forgery (phishing) UI -->
> +<vbox data-category="security" hidden="true">
> +  <groupbox id="addonsPhishingGroup">

No need to wrap the <groupbox> in a <vbox> if the <groupbox> is the only child. Just have the data-category and hidden attributes on the <groupbox>, and get rid of the <vbox>.

We really should be doing the same for the heading too - the outer <vbox> is rather redundant :\

@@ +65,5 @@
> +</vbox>
> +
> +<!-- Passwords -->
> +<vbox data-category="security" hidden="true">
> +  <groupbox id="passwordsGroup" orient="vertical">

Ditto.
Comment 7 Jon Rietveld 2012-04-08 22:54:31 PDT
Created attachment 613234 [details] [diff] [review]
security in-content patch

fixed blairs feedback
Comment 8 MSU Capstone Team 2012-04-10 11:09:45 PDT
Created attachment 613686 [details] [diff] [review]
Security with updated data-category
Comment 9 Jon Rietveld 2012-04-12 12:40:34 PDT
Created attachment 614506 [details] [diff] [review]
security in-content patch for new landing patch
Comment 10 Jon Rietveld 2012-04-12 13:20:37 PDT
Created attachment 614539 [details] [diff] [review]
security in-content patch for new landing patch
Comment 11 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-05-06 23:49:46 PDT
Comment on attachment 614539 [details] [diff] [review]
security in-content patch for new landing patch

Review of attachment 614539 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure why, but the master password checkbox is broken - it doesn't update when a master password is added or removed.
Comment 12 Jon Rietveld 2012-05-07 11:12:51 PDT
Created attachment 621674 [details] [diff] [review]
security in-content patch

changed openDialog in changeMasterPassword and _removeMasterPassword to add the feature dlg, which made the dialog open in blocking mode which is needed for the code to function properly
Comment 13 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-05-07 22:06:38 PDT
Comment on attachment 621674 [details] [diff] [review]
security in-content patch

Review of attachment 621674 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/security.js
@@ +185,5 @@
> +                          bundle.getString("pw_change2empty_in_fips_mode"));
> +    }
> +    else {
> +      openDialog("chrome://mozapps/content/preferences/removemp.xul",
> +                 "dlg", "modal=yes", null);

"dlg" is not the most awesomest of names. That parameter is used as a sort of ID for the dialog, so it should be unique and descriptive.

So how about "Toolkit:RemoveMasterPassword" for this, and "Toolkit:ChangeMasterPassword" for changemp.xul below.
Comment 14 Jon Rietveld 2012-05-08 16:47:11 PDT
Created attachment 622209 [details] [diff] [review]
security in-content patch

made the changes in comment 13
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2012-05-08 22:41:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbf6059c293
Comment 16 Ed Morley [:emorley] 2012-05-09 07:43:40 PDT
https://hg.mozilla.org/mozilla-central/rev/0fbf6059c293
Comment 17 Alfredos-Panagiotis Damkalis [:fredy] 2012-06-22 04:39:49 PDT
I verify fixed this bug, tested at Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120621 Firefox/15.0a2.

Note You need to log in before you can comment on or make changes to this bug.