329865 - Charset Menu Code Needs Love

Status: NEW

(Firefox :: General, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Take this bug in three parts:

1. move the charset menu's template into its own .inc file for sanity. I hate having to scroll past this thing every time I want to change another menu. 
2. move all of the js functions into their own object so we're not polluting the global scope with functions called "CreateMenu"... give them better names while we're at it. MultiplexHandler for a command handler? o_O..

3. maybe a separate pass: refactor the back end. It appears that the datasource is lazily instantiated for performance reasons by waiting for the user to open the menu once. As the popup is shown, the CreateMenu function sends an observer notification which the charset menu receives, and asserts a bunch of things into the datasource which causes the menu to populate. 

What this means is that no one can ever use the charset list anywhere else where there aren't perf concerns (a dialog box) without doing this weird init step first. If the code wants to lazily init, it should set the datasources attribute onpopupshowing, rather than telling the ds to populate itself. Using this approach, If the ds is never referenced in the xul, the charset menu will never even be created until it is required.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

4. there's lots of code in the charset menu code that looks to be specific to other apps. what's the reason for this? can this be ifdef'ed or consolidated? 

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: cleanup patch for browser fe code

not fully tested. this moves the charset menu into its own .js file, #included in optimized builds and <script>-included in debug builds. #includes the template code in the menus for ease of reading of both. Makes the code look nicer and have better names in CharsetMenu object to avoid namespace collisions.

Comment 3

[Jungshik Shin]

(In reply to comment #1)
> 4. there's lots of code in the charset menu code that looks to be specific to
> other apps. what's the reason for this? can this be ifdef'ed or consolidated? 

See bug 288850

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Hi Jungshik, do you know if there are test suites/cases for the charset menu code anywhere that I could run my patch with?

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to Ben Goodger (use ben at mozilla dot org for email) from comment #0)
> Take this bug in three parts:
> 
> 1. 
...
> 3. 

Bug 805374.

> 4. 

Bug 943252.

> 2. 

Still left, but might happen as part of bug 936442.