Last Comment Bug 860970 - Need to sync new security prefs from bug 842191
: Need to sync new security prefs from bug 842191
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.20
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 842191
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-11 15:50 PDT by neil@parkwaycc.co.uk
Modified: 2013-04-14 15:29 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
unaffected
fixed
fixed


Attachments
Proposed patch (1.21 KB, patch)
2013-04-11 15:51 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Addressed review comments (1.46 KB, patch)
2013-04-12 00:30 PDT, neil@parkwaycc.co.uk
jh: review+
iann_bugzilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2013-04-11 15:50:34 PDT
Bug 842191 added four preferences to pref-ssl.xul but didn't sync them.
Comment 1 neil@parkwaycc.co.uk 2013-04-11 15:51:58 PDT
Created attachment 736541 [details] [diff] [review]
Proposed patch
Comment 2 Philip Chee 2013-04-11 21:46:42 PDT
Neil don't you think we should also do this: http://hg.mozilla.org/mozilla-central/rev/60b08f643863

> 1.12 +// Block insecure active content on https pages
> 1.13 +pref("security.mixed_content.block_active_content", true);
Comment 3 Jens Hatlak (:InvisibleSmiley) 2013-04-12 00:22:17 PDT
Comment on attachment 736541 [details] [diff] [review]
Proposed patch

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

Please sort the prefs.

::: suite/browser/browser-prefs.js
@@ +930,5 @@
>  pref("services.sync.prefs.sync.security.warn_submit_insecure", true);
>  pref("services.sync.prefs.sync.security.warn_viewing_mixed", true);
> +pref("services.sync.prefs.sync.security.warn_mixed_active_content", true);
> +pref("services.sync.prefs.sync.security.mixed_content.block_active_content", true);
> +pref("services.sync.prefs.sync.security.warn_mixed_display_content", false);

What's the point of adding a pref that should not be synced?
Comment 4 neil@parkwaycc.co.uk 2013-04-12 00:27:11 PDT
(In reply to Philip Chee from comment #2)
> > 1.12 +// Block insecure active content on https pages
> > 1.13 +pref("security.mixed_content.block_active_content", true);
Sure, but in a separate bug.

(In reply to Jens Hatlak from comment #3)
> (From update of attachment 736541 [details] [diff] [review])
> Please sort the prefs.
Sorry, I hadn't noticed.

> > +pref("services.sync.prefs.sync.security.warn_mixed_display_content", false);
> What's the point of adding a pref that should not be synced?
Oops, too much copy & paste.
Comment 5 neil@parkwaycc.co.uk 2013-04-12 00:30:45 PDT
Created attachment 736697 [details] [diff] [review]
Addressed review comments
Comment 6 neil@parkwaycc.co.uk 2013-04-12 15:47:50 PDT
Pushed comm-central changeset aadbe43b0aa3.
Comment 7 neil@parkwaycc.co.uk 2013-04-12 15:49:04 PDT
Comment on attachment 736697 [details] [diff] [review]
Addressed review comments

[Approval Request Comment]
Regression caused by (bug #): (New preferences - 842191)
User impact if declined: Some SSL preferences don't sync
Testing completed (on m-c, etc.): Landed on c-c
Risk to taking this patch (and alternatives if risky): Low
String changes made by this patch: None
Comment 8 neil@parkwaycc.co.uk 2013-04-12 15:50:48 PDT
One of these days I'll get these flags right first time...
Comment 9 Jens Hatlak (:InvisibleSmiley) 2013-04-13 00:51:52 PDT
(In reply to neil@parkwaycc.co.uk from comment #8)
> One of these days I'll get these flags right first time...

Actually you don't need to set flags for the version that matches the TM. ;-)

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