Closed Bug 956657 Opened 10 years ago Closed 10 years ago

Share more charset menu code between the view source window and the browser

Categories

(Toolkit :: View Source, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(2 files, 3 obsolete files)

As of bug 940907, the view source window now has a character encoding menu again. However only some of the code is shared with the browser window (there was more code sharing prior to bug 805374). It may be possible to refactor the code so that more of it is shared through CharsetMenu.jsm (the detector code looks like an obvious starting point).
Attached patch WIP Patch (obsolete) — Splinter Review
Just the toolkit changes.
I don't think there's anything in BrowserSetCharacterSet that I could share.
I tweaked the detector menu code to simplify it a little.
I didn't bother with try/catch because I wasn't sure it was worth while.
I added the lastChild null-check to give me the option of reusing this code for the compose window, which doesn't have a detector submenu.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8358996 - Flags: feedback?(bmcbride)
Attached patch WIP Patch (obsolete) — Splinter Review
I overlooked the need to reload after changing the detector.
Attachment #8358996 - Attachment is obsolete: true
Attachment #8358996 - Flags: feedback?(bmcbride)
Attachment #8366314 - Flags: feedback?(bmcbride)
Comment on attachment 8366314 [details] [diff] [review]
WIP Patch

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

The patch in bug 936442 changes some of this so CharsetMenu also builds the detector parts of the menu - will need to update this to work with that (sorry, that's had to have review priority as it's blocking Australis). I think the patches are complementary though.
Attachment #8366314 - Flags: feedback?(bmcbride) → feedback-
Depends on: 936442
Attached patch WIP Patch (obsolete) — Splinter Review
Still without the browser changes so there's still some cruft.
Attachment #8366314 - Attachment is obsolete: true
Attachment #8371298 - Flags: feedback?(bmcbride)
Comment on attachment 8371298 [details] [diff] [review]
WIP Patch

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

Looks good :)

::: toolkit/modules/CharsetMenu.jsm
@@ +88,5 @@
>  ];
>  
>  kPinned.forEach(x => kEncodings.delete(x));
>  
> +function CharsetComparator(a, b) {

Nit: Move these to be _ prefixed methods on CharsetMenu?

@@ +98,5 @@
> +  // "shift_jis" first.
> +  return titleA.localeCompare(titleB) || b.value.localeCompare(a.value);
> +}
> +
> +function SetDetector(aEvent) {

Nit: Rest of the file isn't using 'a' prefix for arguments (general consensus is we're moving away from that style)

@@ +196,5 @@
>        label: this._getCharsetLabel(charset),
>        accesskey: this._getCharsetAccessKey(charset),
> +      name: "charset",
> +      value: charset
> +    } for (charset of charsets)];

This is much nicer :)

@@ +241,5 @@
> +
> +  update: function(aEvent, aCharset) {
> +    // For substantially similar encodings, treat two encodings as the same
> +    // for the purpose of the check mark.
> +    if (aCharset == "ISO-8859-8-I")

Split this off into it's own foldCharsets() method, so it's more obvious what its doing, and it's obvious what to update in the future?

@@ +245,5 @@
> +    if (aCharset == "ISO-8859-8-I")
> +      aCharset = "windows-1255";
> +    else if (aCharset == "gb18030")
> +      aCharset = "gbk";
> +    var menuitem = aEvent.target.getElementsByAttribute("charset", aCharset).item(0);

Nit: let
Attachment #8371298 - Flags: feedback?(bmcbride) → feedback+
(In reply to Blair McBride from comment #5)
> > +function CharsetComparator(a, b) {
> 
> Nit: Move these to be _ prefixed methods on CharsetMenu?

They're not actually instance methods though, and JS doesn't have anything like C++ or Java's class static methods yet.
Attached patch Proposed patchSplinter Review
Attachment #8371298 - Attachment is obsolete: true
Attachment #8376354 - Flags: review?(bmcbride)
Comment on attachment 8376354 [details] [diff] [review]
Proposed patch

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

r+ with some tweaks.

::: browser/base/content/browser.js
@@ -5170,5 @@
> -        dump("Failed to set the intl.charset.detector preference.\n");
> -    }
> -}
> -
> -function BrowserSetForcedCharacterSet(aCharset)

Retain this function name and signature, as it's used elsewhere (CustomizableWidgets.jsm, bunch of docshell tests, and at least 1 add-on) where an event isn't involved.

::: toolkit/modules/CharsetMenu.jsm
@@ +232,5 @@
>      } catch (ex) {}
>      return "";
>    },
> +
> +  fold: function FoldCharset(charset) {

Just looking at the API, CharsetMenu.fold(), I could easily mistake this for a method that folded the menu somehow. Then I would be confused. Rename to CharsetMenu.foldCharset().

Also, kill the function name. The JS engine adds those automatically these days if it's needed for things like stack traces.

@@ +234,5 @@
>    },
> +
> +  fold: function FoldCharset(charset) {
> +    // For substantially similar encodings, treat two encodings as the same
> +    // for the purpose of the check mark.

Make this a JSDoc/JavaDoc style comment.

@@ +247,5 @@
> +        return charset;
> +    }
> +  },
> +
> +  update: function UpdateCharsetMenu(event, charset) {

Ditto, kill the function name.
Attachment #8376354 - Flags: review?(bmcbride) → review+
Comment on attachment 8376354 [details] [diff] [review]
Proposed patch

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

r+ with some tweaks.

::: browser/base/content/browser-charsetmenu.inc
@@ -3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -#filter substitution
> -
> -#expand <menu id="__ID_PREFIX__charsetMenu"

Copying this from the MemoServ note I sent earlier for posterity:

[ID_PREFIX is] not used in mozilla-central anymore. It was used pre-Australis for the old app-menu: https://hg.mozilla.org/projects/holly/file/33fda6ace9dc/browser/base/content/browser-appmenu.inc#l171 (Holly is our Australis backout branch).

And it doesn't look like comm-central uses it either. So it's safe to kill.

::: browser/base/content/browser.js
@@ -5170,5 @@
> -        dump("Failed to set the intl.charset.detector preference.\n");
> -    }
> -}
> -
> -function BrowserSetForcedCharacterSet(aCharset)

Retain this function name and signature, as it's used elsewhere (CustomizableWidgets.jsm, bunch of docshell tests, and at least 1 add-on) where an event isn't involved.

::: toolkit/modules/CharsetMenu.jsm
@@ +232,5 @@
>      } catch (ex) {}
>      return "";
>    },
> +
> +  fold: function FoldCharset(charset) {

Just looking at the API, CharsetMenu.fold(), I could easily mistake this for a method that folded the menu somehow. Then I would be confused. Rename to CharsetMenu.foldCharset().

Also, kill the function name. The JS engine adds those automatically these days if it's needed for things like stack traces.

@@ +234,5 @@
>    },
> +
> +  fold: function FoldCharset(charset) {
> +    // For substantially similar encodings, treat two encodings as the same
> +    // for the purpose of the check mark.

Make this a JSDoc/JavaDoc style comment.

@@ +247,5 @@
> +        return charset;
> +    }
> +  },
> +
> +  update: function UpdateCharsetMenu(event, charset) {

Ditto, kill the function name.
https://hg.mozilla.org/mozilla-central/rev/810b9ad81b03
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Attached patch Followup fixSplinter Review
I meant to make update take a parent, not an event.
Would you mind if I changed it? Thanks.
Attachment #8380661 - Flags: review?(bmcbride)
Comment 10 said this was backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Comment on attachment 8380661 [details] [diff] [review]
Followup fix

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

Much prefer this anyway.

But, I don't see how or why this would cause/fix the failure reported in comment 10:

12:06:53     INFO -  2965 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/tree/test_combobox.xul | Different amount of expected children of ['menupopup node', address: [object XULElement], role: combobox list, address: [xpconnect wrapped nsIAccessible]]. - got 0, expected 1

Might be worth a run through Try?
Attachment #8380661 - Flags: review?(bmcbride) → review+
(In reply to Blair McBride [:Unfocused] from comment #14)
> But, I don't see how or why this would cause/fix the failure reported in
> comment 10:
> 
> 12:06:53     INFO -  2965 ERROR TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/a11y/accessible/tests/mochitest/tree/
> test_combobox.xul | Different amount of expected children of ['menupopup
> node', address: [object XULElement], role: combobox list, address:
> [xpconnect wrapped nsIAccessible]]. - got 0, expected 1
> 
> Might be worth a run through Try?

From discussion on IRC:

That was caused from a different changeset in the same push. And it had been re-landed on inbound (sans comment indicating so). So, all good.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 976613
Backout by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/6d1223f072ad
Backed out 3 changesets (bug 956657, bug 974824, bug 974258) for mochitest-other failures on a CLOSED TREE.
You need to log in before you can comment on or make changes to this bug.