494210 - dialog.width entity should not be shared between the two sanitize dialogs

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Drew Willcoxon :adw]

browser/components/preferences/sanitize.xul (a dialog involved with the privacy prefpane) and browser/base/content/sanitize.xul (the clear recent history dialog) take their widths from the same dialog.width entity (in browser/locales/en-US/chrome/browser/sanitize.dtd), but with bug 480169 landed the two dialogs are now quite different.  A good width for one can be a bad width for the other; see bug 490655 comment 19 and bug 493485.  Localizers should be able to specify independent widths for the two.

Comment 1

[Stefan Plewako [:stef]]

Attachment: patch

Comment 2

[:Gijs (he/him)]

Comment on attachment 8562944 [details] [diff] [review]
patch

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

Can we fix all the entity names here instead of perpetuating the lousy naming? There's no way to know that "dialog.width2" is going to be the one dialog and "window.width" the other (and in particular, which one is which). At least the inContent version is unambiguous, but we should just fix the other ones.

::: browser/locales/en-US/chrome/browser/sanitize.dtd
@@ +67,4 @@
>  <!ENTITY inContentDialog.width         "34em">
>  <!ENTITY inContentColumn.width         "24em">
> +<!-- LOCALIZATION NOTE (window.width): width of the Clear Recent History dialog -->
> +<!ENTITY window.width "34em">

Nit: the value of the entity should line up with the previous lines.

Comment 3

[Stefan Plewako [:stef]]

Attachment: v2

Comment 4

[:Gijs (he/him)]

Comment on attachment 8562964 [details] [diff] [review]
v2

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

::: browser/locales/en-US/chrome/browser/sanitize.dtd
@@ +20,2 @@
>  <!ENTITY sanitizeDialog2.title         "Clear Recent History">
> +<!-- LOCALIZATION NOTE (window.width): width of the Clear Recent History dialog -->

This ID is still the old one.

Comment 5

[Stefan Plewako [:stef]]

Attachment: v2 + updated entity id in l10n note

Missed that one, sorry.

Comment 6

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/4cafbb821d67

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4cafbb821d67

Comment 8

[Francesco Lodolo [:flod]]

We need to back this out or either fix it quickly.

The Settings… button doesn't work anymore in the Privacy panel, if you switch incontent prefs to false you'll see the yellow screen of death (broken XML, unknown entity &column.width2;)

https://hg.mozilla.org/mozilla-central/file/4cafbb821d67/browser/components/preferences/sanitize.xul#l85

Comment 9

[Stefan Plewako [:stef]]

Attachment: v2 followup

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/7f9d625a5734

Comment 11

[Francesco Lodolo [:flod]]

I should have cleared in my comment that I didn't test or check the first patch.

I did a search on the 4 strings removed in it, and there's still one reference right below the one just fixed
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/sanitize.xul#86

Comment 12

[:Gijs (he/him)]

(In reply to Francesco Lodolo [:flod] from comment #11)
> I should have cleared in my comment that I didn't test or check the first
> patch.
> 
> I did a search on the 4 strings removed in it, and there's still one
> reference right below the one just fixed
> http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/
> sanitize.xul#86

This is confusing; this is the one "just fixed" in

https://hg.mozilla.org/integration/fx-team/rev/7f9d625a5734

right?

Comment 13

[Francesco Lodolo [:flod]]

(In reply to :Gijs Kruitbosch from comment #12)
> This is confusing; this is the one "just fixed" in
> https://hg.mozilla.org/integration/fx-team/rev/7f9d625a5734

Yes, I'm dumber on Saturday. I assumed you fixed only the one I reported (column.width2) and didn't check the follow-up.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f9d625a5734

Comment 15

[Alice0775 White]

No automate test.........