Last Comment Bug 835134 - Modify Privacy & Security Preferences UI to reflect recent backend additions
: Modify Privacy & Security Preferences UI to reflect recent backend additions
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.18
Assigned To: rsx11m
:
:
Mentors:
https://wiki.mozilla.org/Phishing_Pro...
Depends on: 477718 939481
Blocks: 631566
  Show dependency treegraph
 
Reported: 2013-01-27 06:38 PST by Philip Chee
Modified: 2013-11-16 19:42 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (hg cp/mv) (22.01 KB, patch)
2013-01-29 09:43 PST, rsx11m
no flags Details | Diff | Splinter Review
Proposed patch (hg add/rm) (26.76 KB, patch)
2013-01-29 09:47 PST, rsx11m
no flags Details | Diff | Splinter Review
Screenshot (Windows 7) (18.36 KB, image/png)
2013-01-29 09:50 PST, rsx11m
no flags Details
Screenshot (comment #9) (4.39 KB, image/png)
2013-01-29 20:28 PST, rsx11m
no flags Details
Screenshot variants (8.00 KB, image/png)
2013-01-30 06:24 PST, rsx11m
no flags Details
Proposed patch (v2) (25.22 KB, patch)
2013-01-31 07:45 PST, rsx11m
neil: ui‑review+
Details | Diff | Splinter Review
Screenshots (v2) (20.32 KB, image/png)
2013-01-31 07:50 PST, rsx11m
no flags Details
Proposed patch (v2b) (25.22 KB, patch)
2013-01-31 14:12 PST, rsx11m
rsx11m.pub: ui‑review+
Details | Diff | Splinter Review
Proposed patch (v2c) (25.21 KB, patch)
2013-02-01 07:42 PST, rsx11m
iann_bugzilla: review+
rsx11m.pub: ui‑review+
Details | Diff | Splinter Review
Final patch [ui-r=Neil r=IanN check-in comment 41] (25.19 KB, patch)
2013-02-03 06:20 PST, rsx11m
rsx11m.pub: review+
rsx11m.pub: ui‑review+
Details | Diff | Splinter Review

Description Philip Chee 2013-01-27 06:38:38 PST
From Bug 477718 Comment 47:

> We should probably expose the preferences for enabling/debugging within the
> UI somewhere.
> Debug probably under the Debug pref pane.
> Enabling somewhere under Browser, Privacy & Security or Advanced?
I'd say Privacy & Security.

Firefox places these prefs in their Security Pref pane:
http://hg.mozilla.org/mozilla-central/annotate/47684913d63d/browser/components/preferences/security.xul#l65

      <checkbox id="blockAttackSites"
                label="&blockAttackSites.label;"
                accesskey="&blockAttackSites.accesskey;"
                preference="browser.safebrowsing.malware.enabled" />
      <checkbox id="blockWebForgeries"
                label="&blockWebForgeries.label;"
                accesskey="&blockWebForgeries.accesskey;"
                preference="browser.safebrowsing.enabled" />
Comment 1 rsx11m 2013-01-27 07:22:53 PST
(In reply to Philip Chee)
> I'd say Privacy & Security.

I'd agree with that suggestion - that pane is rather crowed already, though.

At least with my default settings, there wouldn't be enough vertical space for two more checkboxes. However, If at least "Cache" and "Cookies" in the Private Data group could move into a second column next to "Browsing/Location Bar History", the first group could be expanded to "User Tracking and Phishing" or similar.

Or, move "Private Data" into its own pane.

In contrast, plenty of space in the "Advanced" main pane, but it would be a bit hidden there and doesn't quite fit to the other (general) options offered there.
Comment 2 Philip Chee 2013-01-28 04:36:15 PST
> Or, move "Private Data" into its own pane.
I would prefer this myself.
Comment 3 rsx11m 2013-01-28 19:32:07 PST
I'll look into it, give me a few days. This should be a nice exercise to learn how to split and move files around in the tree.  ;-)
Comment 4 rsx11m 2013-01-29 09:43:36 PST
Created attachment 707695 [details] [diff] [review]
Proposed patch (hg cp/mv)

This was actually fairly straight-forward, I hope I've got the jar-file handling right and nothing is missing (works for me). This splits off "Private Data" into its own pane as suggested, just leaving the "User Tracking" checkbox in "Privacy & Security". The new "Phishing Protection" group was taken right from the Firefox implementation, including labels and access keys.

The new "Private Data" pane contains a single "Clear Private Data" group which is the same as was previously offered in the "Privacy & Security" pane. I've put it above the Cookies pane as it's more general than the other panes.

As a drive-by fix, I've corrected askBeforeClear.accesskey as it collided with itemCache.accesskey (both were 'a').

Obviously, this goes on top of bug 477718 which hasn't checked in yet
Comment 5 rsx11m 2013-01-29 09:47:22 PST
Created attachment 707701 [details] [diff] [review]
Proposed patch (hg add/rm)

This as an alternative version of attachment 707695 [details] [diff] [review] producing exactly the same code, but created in a different way. Initially I was using "hg cp" and "hg mv" to derive the new files, to retain the files' histories. If the first patch doesn't apply, please try this one which was only using "hg add" and "hg rm" to account for the added or moved files.
Comment 6 rsx11m 2013-01-29 09:50:08 PST
Created attachment 707704 [details]
Screenshot (Windows 7)

Here the screenshot of the patch in action. Since I haven't applied Phil's patch, the prefs are initially undefined, thus the boxes not checked by default.
Comment 7 rsx11m 2013-01-29 09:54:22 PST
Bug 631566 has been filed already to take care of the Help updates.
Comment 8 rsx11m 2013-01-29 12:46:51 PST
Comment on attachment 707695 [details] [diff] [review]
Proposed patch (hg cp/mv)

> copy from suite/common/pref/pref-security.xul
> copy to suite/common/pref/pref-privdata.xul
> --- a/suite/common/pref/pref-security.xul
> +++ b/suite/common/pref/pref-privdata.xul
> @@ -2,30 +2,26 @@

> -<!ENTITY % prefSecurityDTD SYSTEM "chrome://communicator/locale/pref/pref-security.dtd">
> +<!ENTITY % prefSecurityDTD SYSTEM "chrome://communicator/locale/pref/pref-privdata.dtd">
>  %prefSecurityDTD;

Oops, that was supposed to be

> -<!ENTITY % prefSecurityDTD SYSTEM "chrome://communicator/locale/pref/pref-security.dtd">
> -%prefSecurityDTD;
> +<!ENTITY % prefPrivDataDTD SYSTEM "chrome://communicator/locale/pref/pref-privdata.dtd">
> +%prefPrivDataDTD;

I'll change the entity name with the next patch after any comments from you (no impact on function).
Comment 9 rsx11m 2013-01-29 15:03:19 PST
Some more thoughts on the main Privacy & Security page:

Should the group label "Phishing Protection" be "Safe Browsing" instead, (a) to match the preference name, and (b) to reflect that this both about phishing and malicious web sites?

Also, since we have a bit more space than Firefox in the preference pane, maybe add a short <description> similar to various other panes? My suggestions:

Track Users - "SeaMonkey can ask web sites not to track how you are using their site to maintain privacy. However, it is up to that site whether or not to honor your request."

Safe Browsing - "SeaMonkey can block web sites which have been reported to contain malicious content or try to obtain your login information for legitimate sites (phishing)."

Or, should those go into the help information per bug 631566? In that case, I can remove %brandDTD from pref-security.xul as well.

Please let me know if you want me to consider any of those in the next iteration.
Comment 10 neil@parkwaycc.co.uk 2013-01-29 16:34:47 PST
Comment on attachment 707695 [details] [diff] [review]
Proposed patch (hg cp/mv)

>+<!ENTITY % prefSecurityDTD SYSTEM "chrome://communicator/locale/pref/pref-privdata.dtd">
> %prefSecurityDTD;
> <!ENTITY % prefSanitizeDTD SYSTEM "chrome://communicator/locale/sanitize.dtd">
> %prefSanitizeDTD;
[Perhaps we should just put everything in sanitize.dtd?]

>+  <prefpane id="privdata_pane" label="&pref.privdata.title;"
>+            script="chrome://communicator/content/tasksOverlay.js chrome://communicator/content/pref/pref-privdata.js">
>+    <preferences id="privdata_preferences">
> 
>       <!-- Clear Private Data -->
Nit: lose the blank line here

>     <!-- Do Not Track -->
>     <groupbox id="doNotTrackGroup">
>       <caption label="&tracking.label;"/>
> 
>       <hbox id="doNotTrackBox" align="center">
>         <checkbox id="privacyDoNotTrackPrefs"
>                   flex="1"
>                   label="&doNotTrack.label;"
>                   accesskey="&doNotTrack.accesskey;"
>                   preference="privacy.donottrackheader.enabled"/>
>       </hbox>
>     </groupbox>
[So, it looks as if this got copied from the clear private data...]

>-      <hbox id="clearDataBox" align="center">
>-        <checkbox id="alwaysClear" flex="1"
>-                  preference="privacy.sanitize.sanitizeOnShutdown"
>-                  label="&alwaysClear.label;"
>-                  accesskey="&alwaysClear.accesskey;"/>
>-      </hbox>
>-      <hbox id="askClearBox">
>-        <checkbox id="askBeforeClear" flex="1"
>-                  preference="privacy.sanitize.promptOnSanitize"
>-                  label="&askBeforeClear.label;"
>-                  accesskey="&askBeforeClear.accesskey;"/>
>-        <button id="clearDataNow" icon="clear"
>-                labelDialog="&clearDataDialog.label;"
>-                labelSilent="&clearDataSilent.label;"
>-                accesskey="&clearDataDialog.accesskey;"
>-                oncommand="clearPrivateDataNow();"/>
>-      </hbox>
[... which has this combination of checkbox and button that needed an hbox. And unfortunately the align="center" got set on the wrong hbox too. Oops.]

>+      <vbox id="phishingProtectionBox" align="left">
Now, this is an interesting approach, but you can in fact put the align on the groupbox itself.

>+        <checkbox id="blockAttackSites"
>+                  flex="1"
And you don't need the flex here, again that was only for the checkbox button combo. (And I think the other sanitise items flex for consistency, otherwise it would be unnecessary there too.)
Comment 11 rsx11m 2013-01-29 19:08:04 PST
Neil, thanks for your comments and the quick turnaround.

> [Perhaps we should just put everything in sanitize.dtd?]

Actually, I was thinking of pref-privdata.dtd being a superset of sanitize.dtd given that the latter is also used for the "Clear Private Data" dialog coming from the menus. It seems to be cleaner to keep the pane-specific entities parallel to the panes, thus I'd tend to keep it.

> >     <!-- Do Not Track -->
> [So, it looks as if this got copied from the clear private data...]

Maybe, but I didn't program that, I'm just carrying it along...  ;-)
I've made the phishing group following those examples, though, that's true.

> [... which has this combination of checkbox and button that needed an hbox.
> And unfortunately the align="center" got set on the wrong hbox too. Oops.]

Yes, the plenty of alignments and flexes irritated me as well, thus I've left it alone. I can try to clean it up a bit.

> >+      <vbox id="phishingProtectionBox" align="left">
> Now, this is an interesting approach, but you can in fact put the align on
> the groupbox itself.

... and it probably should be align="start" anyway, if needed at all, given that it has to be RTL safe, I assume.
Comment 12 rsx11m 2013-01-29 19:09:21 PST
What about these suggestions?

(In reply to rsx11m from comment #9)
> Should the group label "Phishing Protection" be "Safe Browsing" instead,

> Also, since we have a bit more space than Firefox in the preference pane,
> maybe add a short <description> similar to various other panes?
Comment 13 rsx11m 2013-01-29 20:28:10 PST
Created attachment 707989 [details]
Screenshot (comment #9)

Ok, so I could safely get rid of the additional boxes and alignments without any effect on appearance, looks much cleaner now. I took the opportunity to create a mockup of the brief descriptions for each group, with slightly shortened labels as proposed to make them fit into two lines. If you don't like this, I'll stick with just the group headings without the introductory strings.
Comment 14 neil@parkwaycc.co.uk 2013-01-30 01:26:35 PST
(In reply to rsx11m from comment #11)
> ... and it probably should be align="start" anyway, if needed at all, given
> that it has to be RTL safe, I assume.
Good catch!

(In reply to rsx11m from comment #9)
> Some more thoughts on the main Privacy & Security page:
[Sorry, I only looked at the patch, I forgot to check out the comments]

> Should the group label "Phishing Protection" be "Safe Browsing" instead, (a)
> to match the preference name, and (b) to reflect that this both about
> phishing and malicious web sites?
Yes, I meant to say that "Phishing Protection" is wrong, since it's 50% phishing, 50% malware. "Safe Browsing" sounds fine though.

> Safe Browsing - "SeaMonkey can block web sites which have been reported to
> contain malicious content or try to obtain your login information for
> legitimate sites (phishing)."
"or try to acquire personal details by masquerading as a trustworthy site"?

> Or, should those go into the help information per bug 631566?
A one-line summary seems fine, and more detailed information goes in Help.
Comment 15 neil@parkwaycc.co.uk 2013-01-30 01:28:40 PST
Comment on attachment 707989 [details]
Screenshot (comment #9)

I'd prefer "For your privacy".
Comment 16 Philip Chee 2013-01-30 01:34:49 PST
[This is just me bikeshedding. Feel free to ignore me]

> +   content/communicator/pref/pref-privdata.js
I'd prefer pref-privatedata.js it's not that much longer.
> +  <prefpane id="privdata_pane" label="&pref.privdata.title;"
pref.privatedata.title
...etc... /privdata/privatedata/g

> +<!ENTITY blockAttackSites.label           "Block reported attack sites">
> +<!ENTITY blockAttackSites.accesskey       "k">
> +<!ENTITY blockWebForgeries.label          "Block reported web forgeries">
> +<!ENTITY blockWebForgeries.accesskey      "B">

I'd say the access key for blocked sites should be "B" and the phishing sites should be "k". I know you copied the access keys from Firefox, but the ordering there is a historical accident, where the phishing preference was created first and then only later was the malware preference was inserted.
Comment 17 neil@parkwaycc.co.uk 2013-01-30 01:40:46 PST
"f" for phishing perhaps ;-)
Comment 18 Philip Chee 2013-01-30 01:43:29 PST
> "f" for phishing perhaps ;-)
isth there a listhp?

> Track Users - "SeaMonkey can ask web sites not to track how you are using their site
> to maintain privacy. However, it is up to that site whether or not to honor your
> request."
[FYI: Bug 765398 - Implement three-state UI for DNT (do not track me, okay to track me, decline to state).]
Comment 19 rsx11m 2013-01-30 06:24:16 PST
Created attachment 708106 [details]
Screenshot variants

(In reply to neil@parkwaycc.co.uk from comment #15)
> I'd prefer "For your privacy".
(In reply to neil@parkwaycc.co.uk from comment #14)
> "or try to acquire personal details by masquerading as a trustworthy site"?

I've changed the 2-line descriptions accordingly, see the upper screenshot.

> > Or, should those go into the help information per bug 631566?
> A one-line summary seems fine, and more detailed information goes in Help.

There is not much room to explain a lot in a single line, thus I've moved some details into the "Block..." checkbox labels in this version (lower screenshot).

(In reply to Philip Chee from comment #16)
> I'd prefer pref-privatedata.js it's not that much longer.

Yes, that should be an easy change, and there are other long file names in pref/.

> I'd say the access key for blocked sites should be "B" and the phishing
> sites should be "k".
(In reply to neil@parkwaycc.co.uk from comment #17)
> "f" for phishing perhaps ;-)

I've used "B" for blocking attach sites and "f" (or "P" for the 1-line version as we have now a matching "Phishing" in the label itself).

So, pick your preferred variation and I'm open for any further suggestions, then I'll update the patch respectively.
Comment 20 rsx11m 2013-01-30 06:26:54 PST
(In reply to Philip Chee from comment #18)
> [FYI: Bug 765398 - Implement three-state UI for DNT (do not track me, okay
> to track me, decline to state).]

That should be relevant for the expanded Help documentation, but shouldn't affect the checkbox label or short description in any way (they are generic enough).

This needs to be revisited if and when Toolkit introduces that change.
Comment 21 rsx11m 2013-01-30 06:30:36 PST
[oops, that landed already - bummer... thus I'll have to consider that now.]
Comment 22 rsx11m 2013-01-30 07:15:10 PST
I guess this needs to land for SM 2.18 so that the user-tracking functionality works properly. I'll try to copy-paste from the Firefox version to match their 3-radiobutton layout (which is a bit tricky as it combines two preferences now, i.e., I'll have to keep pref-security.js but replace its contents entirely with those new functions), that's hopefully sufficiently straight-forward to do.
Comment 23 neil@parkwaycc.co.uk 2013-01-30 07:23:29 PST
Don't feel you need to copy the Firefox UI; I'd be quite happy with a double checkbox approach where one disables the other.
Comment 24 rsx11m 2013-01-30 07:28:28 PST
I'll have to figure out first what these prefs are actually doing... it may be the easiest to just copy the Firefox code if it works out of the box, unless you would prefer a 2-checkbox version over the 3 radiobuttons.
Comment 25 rsx11m 2013-01-30 10:07:20 PST
Ok, so I have extracted the two functions for getTrackingPrefs() and setTrackingPrefs() and included that radiogroup. I'm wondering why the Firefox version wraps these functions into

  var gPrivacyPane = { ... };

which in turn isn't done for updateClearNowButtonLabel() and clearPrivateDataNow() in the existing code on SeaMonkey's side. Unless necessary for scoping reasons, I would simply make those get/set*() regular functions.

Another scoping oddity is that

  <prefpane ... script="chrome://communicator/content/pref/pref-security.js">

doesn't work in this case, the functions won't be found for onsync{to,from}preference. It only works when using instead

    <script type="application/javascript" 
            src="chrome://communicator/content/pref/pref-security.js"/>

before the groupbox. Aside from that, I think I've got it working.
Comment 26 rsx11m 2013-01-30 10:13:17 PST
(In reply to neil@parkwaycc.co.uk from comment #23)
> Don't feel you need to copy the Firefox UI; I'd be quite happy with a double
> checkbox approach where one disables the other.

The Firefox-style version is now:

  ( ) Don't track me
  ( ) Do track me
  ( ) Don't say anything

where the ".enabled" (bool) pref states whether or not a header is sent at all (thus, if false, that corresponds to "Don't say anything"), and ".value" (int) says whether the header should state "Don't track me" or "Do track me". A direct implementation of those prefs would look like this:

  [ ] Send the server my tracking preferences:
    ( ) Don't track me
    ( ) Do track me

which takes up the same vertical space and may be less intuitive (and some logic is required there as well to enable/disable the radiogroup based on the checkbox value).

Thus, I'll stick with the Firefox version unless there are any issues with it.
Comment 27 neil@parkwaycc.co.uk 2013-01-30 11:55:00 PST
(In reply to rsx11m from comment #25)
> Ok, so I have extracted the two functions for getTrackingPrefs() and
> setTrackingPrefs() and included that radiogroup. I'm wondering why the
> Firefox version wraps these functions into
> 
>   var gPrivacyPane = { ... };
> 
> which in turn isn't done for updateClearNowButtonLabel() and
> clearPrivateDataNow() in the existing code on SeaMonkey's side. Unless
> necessary for scoping reasons, I would simply make those get/set*() regular
> functions.
> 
> Another scoping oddity is that
> 
>   <prefpane ... script="chrome://communicator/content/pref/pref-security.js">
> 
> doesn't work in this case, the functions won't be found for
> onsync{to,from}preference. It only works when using instead
> 
>     <script type="application/javascript" 
>             src="chrome://communicator/content/pref/pref-security.js"/>
> 
> before the groupbox. Aside from that, I think I've got it working.
This is because <prefpane ... script> automatically scopes the script to the prefpane element, and the code that handles onsync*preference can't handle that, so we have to use a slightly cumbersome approach to accessing the script methods (I'm sure you can find one in one of our other pref panes as an example).

(In reply to rsx11m from comment #26)
> (In reply to comment #23)
> > Don't feel you need to copy the Firefox UI; I'd be quite happy with a double
> > checkbox approach where one disables the other.
> 
> The Firefox-style version is now:
> 
>   ( ) Don't track me
>   ( ) Do track me
>   ( ) Don't say anything
> 
> where the ".enabled" (bool) pref states whether or not a header is sent at
> all (thus, if false, that corresponds to "Don't say anything"), and ".value"
> (int) says whether the header should state "Don't track me" or "Do track
> me". A direct implementation of those prefs would look like this:
> 
>   [ ] Send the server my tracking preferences:
>     ( ) Don't track me
>     ( ) Do track me
> 
> which takes up the same vertical space and may be less intuitive
Seems as least as intiutive as "Don't say anything" to me.

> (and some logic is required there as well to
> enable/disable the radiogroup based on the checkbox value).
Hardly any logic is needed. I tried looking in pref-keynav but it's using outdated code; pref-findasyoutype has a good example though.
Comment 28 rsx11m 2013-01-30 15:46:21 PST
(In reply to neil@parkwaycc.co.uk from comment #27)
> Seems as least as intiutive as "Don't say anything" to me.

That weren't the labels Firefox used, of course, just their implications.
Here their definitions:

>  <!ENTITY dntTrackingNopref.label        "Do not tell sites anything about my tracking preferences.">
>  <!ENTITY dntTrackingNopref.accesskey    "o">
>  <!ENTITY dntTrackingNotOkay.label       "Tell sites that I do not want to be tracked.">
>  <!ENTITY dntTrackingNotOkay.accesskey   "n">
>  <!ENTITY dntTrackingOkay.label          "Tell sites that I want to be tracked.">
>  <!ENTITY dntTrackingOkay.accesskey      "t">


> Hardly any logic is needed. I tried looking in pref-keynav but it's using
> outdated code; pref-findasyoutype has a good example though.

I'll look into that (it appears to me that you'd prefer the box+radio version?).
Comment 29 rsx11m 2013-01-30 16:02:53 PST
(In reply to neil@parkwaycc.co.uk from comment #27)
> Hardly any logic is needed. I tried looking in pref-keynav but it's using
> outdated code; pref-findasyoutype has a good example though.

Thanks for the pointer. Yes, the findasyoutype version looks trivial and should be directly applicable to what's needed here. So, let's go with that one with similar labels as quoted in comment #28. This would also avoid the scoping issue.
Comment 30 neil@parkwaycc.co.uk 2013-01-30 16:32:24 PST
(In reply to rsx11m from comment #28)
> (it appears to me that you'd prefer the box+radio version?).
Yes please, I don't think the extra complexity of the three-way radio is worth it.

Mostly based on previous comments, the best labels I have come up with so far are as follows:

[X] Tell sites about my tracking preference
    ( ) I do not want to be tracked
    (o) I consent to being tracked
Comment 31 rsx11m 2013-01-30 17:45:11 PST
I like the "consent" in that label, this should make people think twice before checking that box. The default is "I do not want to be tracked" for this pref.
Comment 32 rsx11m 2013-01-31 07:45:17 PST
Created attachment 708580 [details] [diff] [review]
Proposed patch (v2)

Issues addressed:
  (comment #10) got rid of redundant empty lines, boxes, and flexes
  (comment #14) added 1-line introductory descriptions
  (comment #14) renamed "Phishing Protection" to "Safe Browsing"
  (comment #16) changed "privdata" to "privatedata" in all instances
  (comment #19) new accesskeys defined per 1-line suggestions
  (comment #30) tracking prefs are now a checkbox + dependent radiogroup

This patch uses "hg cp" again to create the new pref-privatedata.* files from the existing pref-security.* ones, this seemed to have worked.
Comment 33 rsx11m 2013-01-31 07:50:22 PST
Created attachment 708582 [details]
Screenshots (v2)

Left side: main Privacy & Security pane; not checked (top), checked (bottom).
Right side: new Private Data pane (no changes vs. the initial patch).
Comment 34 neil@parkwaycc.co.uk 2013-01-31 13:00:26 PST
Comment on attachment 708580 [details] [diff] [review]
Proposed patch (v2)

>+        <radio id="dntNoTrack" value="1" label="&dntTrackingNotOkay.label;"
>+               accesskey="&dntTrackingNotOkay.accesskey;" />
>+        <radio id="dntDoTrack" value="0" label="&dntTrackingOkay.label;"
>+               accesskey="&dntTrackingOkay.accesskey;" />
Nit: no space before />

>+<!ENTITY blockWebForgeries.label          "Block reported web forgeries (Phishing)">
>+<!ENTITY blockWebForgeries.accesskey      "P">
>+
Nit: blank line at end of file
Comment 35 rsx11m 2013-01-31 14:12:18 PST
Created attachment 708794 [details] [diff] [review]
Proposed patch (v2b)

(In reply to neil@parkwaycc.co.uk from comment #34)
> Nit: no space before />

Apparently I've copy-pasted the Firefox code a bit too literally here...

> Nit: blank line at end of file

Taken care of, thanks!

Ian, it's all yours now.
Comment 36 rsx11m 2013-02-01 07:42:13 PST
Created attachment 709054 [details] [diff] [review]
Proposed patch (v2c)

Tiny follow-up fix: "websites" is used rather than "web sites" in the labels for the other preference panes, thus removed the spaces in pref-security.dtd.

Carrying over ui-r assuming that this change is ok, otherwise let me know.
Comment 37 neil@parkwaycc.co.uk 2013-02-01 08:54:19 PST
(In reply to rsx11m from comment #36)
> Tiny follow-up fix: "websites" is used rather than "web sites" in the labels
> for the other preference panes, thus removed the spaces in pref-security.dtd.
> 
> Carrying over ui-r assuming that this change is ok, otherwise let me know.

Indeed, if memory serves me correctly the en-GB localisation uses "web sites" which is probably why I didn't notice ;-)
Comment 38 Ian Neal 2013-02-03 03:16:47 PST
Comment on attachment 709054 [details] [diff] [review]
Proposed patch (v2c)

>+++ b/suite/common/pref/pref-privatedata.xul
>+  <prefpane id="privatedata_pane" label="&pref.privatedata.title;"
>+            script="chrome://communicator/content/tasksOverlay.js chrome://communicator/content/pref/pref-privatedata.js">
You do not need to load tasksOverlay.js here anymore as it is now loaded for all prefpanes in preferences.xul when bug 588419 landed.

r=me with that fixed.
Comment 39 rsx11m 2013-02-03 06:20:25 PST
Created attachment 709466 [details] [diff] [review]
Final patch [ui-r=Neil r=IanN check-in comment 41]

(In reply to Ian Neal from comment #38)
> You do not need to load tasksOverlay.js here anymore as it is now loaded for
> all prefpanes in preferences.xul when bug 588419 landed.

Correct, it works fine with that removed in pref-privatedata.xul. I even changed the respective comment in line #27 of preferences.xul, but obviously didn't quite get its implications at that time.
Comment 40 rsx11m 2013-02-03 06:21:08 PST
Push for trunk/comm-central please.
Comment 41 Philip Chee 2013-02-04 01:58:45 PST
Comment on attachment 709466 [details] [diff] [review]
Final patch [ui-r=Neil r=IanN check-in comment 41]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/2482caee1e4f

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