Closed Bug 940907 Opened 11 years ago Closed 10 years ago

Re-introduce the Character Encoding menu to the View Source window

Categories

(Toolkit :: View Source, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox28 - fixed
firefox29 - fixed

People

(Reporter: hsivonen, Assigned: neil)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Bug 805374 removes the Character Encoding submenu of the View menu from the View Source window.  Per review request of there, this is the bug for tracking the re-introduction of that submenu. (Personally, I don't think it's particularly important to do this,  since View Source  picks up the encoding from the browser window, so this menu is only needed if the user decides that an override is necessary after viewing source.)
Blocks: 943632
Keywords: regression
(In reply to Henri Sivonen from comment #0)
> I don't think it's particularly important to do this, since View Source
> picks up the encoding from the browser window, so this menu is only needed
> if the user decides that an override is necessary after viewing source.

That's assuming the browser window is the only entry point into view source. Obviously you other Toolkit applications to think of, including the Error Console which links to the view source window, while DOM Inspector is an example of an extension that links to the view source window.
I recommend that someone nominate for tracking or uplift based on what release milestone this gets targeted for but for now I don't think we will track this.
Attached patch Proposed patch (obsolete) — Splinter Review
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8351727 - Flags: review?(bmcbride)
Comment on attachment 8351727 [details] [diff] [review]
Proposed patch

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -9,17 +9,17 @@
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/NotificationDB.jsm");
> Cu.import("resource:///modules/RecentWindow.jsm");
> Cu.import("resource://gre/modules/WindowsPrefSync.jsm");
> 
> XPCOMUtils.defineLazyModuleGetter(this, "Task",
>                                   "resource://gre/modules/Task.jsm");
> XPCOMUtils.defineLazyModuleGetter(this, "CharsetMenu",
>-                                  "resource:///modules/CharsetMenu.jsm");
>+                                  "resource://gre/modules/CharsetMenu.jsm");

Doesn't your addition to toolkit/content/charsetOverlay.js make this redundant?

>--- a/toolkit/locales/en-US/chrome/global/charsetOverlay.dtd
>+++ b/toolkit/locales/en-US/chrome/global/charsetOverlay.dtd
>@@ -1,17 +1,27 @@
> <!-- 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/. -->
> 
> <!-- extracted from charsetOverlay.xul -->
> <!ENTITY charsetMenu.label             "Character Encoding">
> <!ENTITY charsetMenu.accesskey         "C">
> <!ENTITY charsetMenuAutodet.label      "Auto-Detect">
>-<!ENTITY charsetMenuAutodet.accesskey  "a">
>+<!ENTITY charsetMenuAutodet.accesskey  "D"><!-- A reserved for Arabic -->
>+
>+<!ENTITY charsetMenuAutodet.off.label     "(Off)">
>+<!ENTITY charsetMenuAutodet.off.accesskey "O">
>+<!ENTITY charsetMenuAutodet.ja.label      "Japanese">
>+<!ENTITY charsetMenuAutodet.ja.accesskey  "J">
>+<!ENTITY charsetMenuAutodet.ru.label      "Russian">
>+<!ENTITY charsetMenuAutodet.ru.accesskey  "R">
>+<!ENTITY charsetMenuAutodet.uk.label      "Ukranian">
>+<!ENTITY charsetMenuAutodet.uk.accesskey  "U">

Does charsetMenuAutodet.accesskey need to be renamed since other locales could have similar conflicts? Or is this business as usual because you're adding items to the menu, which should always make localizers check their access keys?
Component: Menus → View Source
Product: Firefox → Toolkit
Version: unspecified → Trunk
(In reply to Dão Gottwald from comment #5)
> (From update of attachment 8351727 [details] [diff] [review])
> > XPCOMUtils.defineLazyModuleGetter(this, "CharsetMenu",
> >-                                  "resource:///modules/CharsetMenu.jsm");
> >+                                  "resource://gre/modules/CharsetMenu.jsm");
> 
> Doesn't your addition to toolkit/content/charsetOverlay.js make this redundant?

That's quite possible, I only checked that I didn't break anything.

> >-<!ENTITY charsetMenuAutodet.accesskey  "a">
> >+<!ENTITY charsetMenuAutodet.accesskey  "D"><!-- A reserved for Arabic -->
> 
> Does charsetMenuAutodet.accesskey need to be renamed since other locales
> could have similar conflicts? Or is this business as usual because you're
> adding items to the menu, which should always make localizers check their
> access keys?

I know access keys are always tricky. My preference is to post in .l10n to warn localisers of potential access key changes. In this case I was planning to let localisers know that these were moved from charsetMenu.dtd in the first place.
Comment on attachment 8351727 [details] [diff] [review]
Proposed patch

>-<!ENTITY % charsetDTD SYSTEM "chrome://browser/locale/charsetMenu.dtd" >
>+<!ENTITY % charsetDTD SYSTEM "chrome://global/locale/charsetOverlay.dtd" >

Folding this stuff back into charsetOverlay.dtd makes it harder to stop shipping dead code in Firefox and harder to stop keeping non-Firefox code in mozilla-central in bug 943252. I'd prefer keeping the browser-oriented charsetMenu.dtd (etc.) separate from the legacy charserOverlay stuff, which should no longer ship in Firefox and should, IMO, move out of m-c.

>+<!ENTITY charsetMenuAutodet.uk.label      "Ukranian">

Please watch out for re-introducing bug 952736 if its fix lands first.

> <!ENTITY charsetMenuMore.label         "More Encodings">
> <!ENTITY charsetMenuMore.accesskey     "m">
> <!ENTITY charsetMenuMore1.label        "West European">
> <!ENTITY charsetMenuMore1.accesskey    "w">
> <!ENTITY charsetMenuMore2.label        "East European">
> <!ENTITY charsetMenuMore2.accesskey    "E">
> <!ENTITY charsetMenuMore3.label        "East Asian">
> <!ENTITY charsetMenuMore3.accesskey    "A">

Having these in the same .dtd means re-shipping these dead UI strings in Firefox. :-(
(In reply to Dão Gottwald [:dao] from comment #5)
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
[...]
> Doesn't your addition to toolkit/content/charsetOverlay.js make this
> redundant?

No, charsetOverlay isn't used in Firefox.
Comment on attachment 8351727 [details] [diff] [review]
Proposed patch

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

What Henri said in comment 7 needs resolved.
Attachment #8351727 - Flags: review?(bmcbride) → review-
(In reply to Henri Sivonen from comment #7)
> Folding this stuff back into charsetOverlay.dtd makes it harder to stop
> shipping dead code in Firefox and harder to stop keeping non-Firefox code in
> mozilla-central in bug 943252.

OK, so if I just move rather than rename the dtd and the properties, import CharsetMenu.jsm from the view source window, and paste the charsetMenu into viewSource.xul, does that cover everything?

> Having these in the same .dtd means re-shipping these dead UI strings in
> Firefox. :-(

(You're already shipping those strings, unless you wanted to do some funky #ifdef in toolkit's jar.mn files. Anyway, you already covered that above.)
(In reply to neil@parkwaycc.co.uk from comment #10)
> (In reply to Henri Sivonen from comment #7)
> > Folding this stuff back into charsetOverlay.dtd makes it harder to stop
> > shipping dead code in Firefox and harder to stop keeping non-Firefox code in
> > mozilla-central in bug 943252.
> 
> OK, so if I just move rather than rename the dtd and the properties, import
> CharsetMenu.jsm from the view source window, and paste the charsetMenu into
> viewSource.xul, does that cover everything?

The reason I didn't do that in the first place is that the charset menu depends on JS from browser.js, so you'll probably need to refactor that code into CharsetMenu.jsm.
Attachment #8351727 - Attachment is obsolete: true
Attachment #8355911 - Flags: review?(bmcbride)
(In reply to Henri Sivonen (:hsivonen) from comment #11)
> The reason I didn't do that in the first place is that the charset menu
> depends on JS from browser.js, so you'll probably need to refactor that code
> into CharsetMenu.jsm.

Could you file a followup bug, Neil?
Attachment #8355911 - Flags: review?(bmcbride) → review+
Why isn't browser-charsetmenu.inc included into viewSource.xul but copied and pasted?
Blocks: 956657
https://hg.mozilla.org/mozilla-central/rev/98660dab432f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment on attachment 8355911 [details] [diff] [review]
Addressed review comments

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 805374
User impact if declined: 
Testing completed (on m-c, etc.): Landed on m-c
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch: Two locale files moved, this makes it possible to fix further uses of the charset menu.
Attachment #8355911 - Flags: approval-mozilla-beta?
Attachment #8355911 - Flags: approval-mozilla-aurora?
Why approval-mozilla-beta? Firefox 27 should be unaffected, right?
Comment on attachment 8355911 [details] [diff] [review]
Addressed review comments

Sorry, I thought beta was 28 for some reason.
Attachment #8355911 - Flags: approval-mozilla-beta?
Attachment #8355911 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Moving l10n files around is a massive string freeze break.

Can we please back this out from aurora?
Flags: needinfo?(lsblakk)
Does this have in-testsuite coverage?
Flags: needinfo?(neil)
Flags: in-testsuite?
I don't think the menu itself has ever had any tests. (The underlying functionality might do, but I assume that's not relevant here. I don't see any test annotations on the bug that removed the menu either.)
Flags: needinfo?(neil)
comm-central has/had a basic test for it which we should now enable again - http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/content-policy/test-view-source.js#31
Ryan - can you please back this out of 28?
Flags: needinfo?(lsblakk) → needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-aurora/rev/f8613903c6b1

Setting the status back to affected. Please change to wontfix if necessary.
Flags: needinfo?(ryanvm)
Thank you Ryan, given the l10n impact we will indeed wontfix on 28 and let this ride the trains.
Attachment #8355911 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
With string moves faked using repackaging and merged to branch.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 805374
User impact if declined: 
Testing completed (on m-c, etc.): Baked on m-c
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch: NONE!
Attachment #8365337 - Flags: feedback?(l10n)
Attachment #8365337 - Flags: approval-mozilla-aurora?
Comment on attachment 8365337 [details] [diff] [review]
Branch-safe patch

Thanks, this looks good from an l10n pov.
Attachment #8365337 - Flags: feedback?(l10n) → feedback+
Attachment #8365337 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.