999293 - [e10s] Make charset menu work

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The View > Character Encoding menu is currently broken.

Comment 1

[Harry Kaimenas [:hkaimenas]]

As far as I can tell the part that prevents this to work is the call to docShell here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableWidgets.jsm#730

How can we reach the docShell functions from remote enabled browsers? I'm trying to work on this. Can you point me to any documentation/code?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, I was thinking of the actual menu in the menubar, but I guess we need to fix the hamburger menu too.

It looks like there are a couple places that might be problems. One you pointed out (the mayEnableCharacterEncodingMenu thing). There's another function in there, updateCurrentCharset, that will also need to be fixed.

The other problems are all right around here in browser.js:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5152
Namely, BrowserSetForcedCharacterSet, UpdateCurrentCharset, and charsetLoadListener.

Looking at the code, I'm not sure why charsetLoadListener is there. It sets two variables, gPrevCharset and gLastBrowserCharset. Neither of them seem to be very useful. All that we ever do with them is uncheck a menu item in UpdateCurrentCharset. However, it seems like it would be fine to uncheck all the menu items in that case. So maybe we can just eliminate charsetLoadListener?

The remaining functions can be broken into two types: functions that get the current charset and functions that set it. You can access the current charset in e10s by getting the .characterSet property of any <xul:browser> element. To see an example of how that's used, see bug 930863.

To set the character set, we should be able to send a message using the message manager. There's documentation on that here: https://developer.mozilla.org/en-US/docs/The_message_manager
You would probably want to add code to browser/base/content/content.js, which runs in the child process. It would use addMessageListener to listen for a message from the parent, and change the charset based on that. The parent would use the browser.sendAsyncMessage to send the message to change the character set.

To see an example of how this works, you can look at bug 961529, specifically https://bugzilla.mozilla.org/attachment.cgi?id=8364540&action=diff.

Comment 3

[Alex Bardas :alexbardas]

Attachment: bug999293_[e10s]_make_charset_menu_work.diff

It works with both e10s on and off. I've also applied the recommended changes from the comment. All the tests I've ran passed, but I haven't ran all of them yet. If it looks ok, I'll push it to try server.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8468919 [details] [diff] [review]
bug999293_[e10s]_make_charset_menu_work.diff

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

Nice job!

One thing I'm realizing here is that the existing code works a little strangely. When we display the charset menu, I think we get the character set of the currently focused frame. When we change the charset, we're changing the charset of the top-level frame. With the changes I'm suggesting, we do everything for the top-level frame. That makes more sense to me and it's also easier.

Also, I'm not a frontend peer, so I'll pass this review to someone who is.

::: browser/base/content/browser.js
@@ +5358,4 @@
>      if ((window == wnd) || (wnd == null)) wnd = window.content;
>  
> +    for (let menuItem of target.getElementsByTagName("menuitem")) {
> +      let isSelected = menuItem.getAttribute("charset") === CharsetMenu.foldCharset(wnd.document.characterSet);

This code is actually using a special compatibility layer (called a CPOW) to fetch the current character set from the child process. However, the fetching happens synchronously, so the browser UI is blocked while we wait for the child process to respond with the character set. It would be better if we used the cached character set here. You can replace wnd.document.characterSet with gBrowser.selectedBrowser.characterSet. And then you can remove the "let wnd = ..." stuff since we no longer need wnd.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8468919 [details] [diff] [review]
bug999293_[e10s]_make_charset_menu_work.diff

> function updateCharacterEncodingMenuState()
> {
>   let charsetMenu = document.getElementById("charsetMenu");
>   // gBrowser is null on Mac when the menubar shows in the context of
>   // non-browser windows. The above elements may be null depending on
>   // what parts of the menubar are present. E.g. no app menu on Mac.
>   if (gBrowser &&
>-      gBrowser.docShell &&
>-      gBrowser.docShell.mayEnableCharacterEncodingMenu) {
>+      gBrowser.selectedBrowser &&
>+      gBrowser.selectedBrowser.mayEnableCharacterEncodingMenu) {

gBrowser.selectedBrowser cannot be null.

>+      menuItem.setAttribute('checked', isSelected ? 'true' : 'false');

menuItem.setAttribute("checked", isSelected);

>@@ -730,18 +730,18 @@ const CustomizableWidgets = [{
>     id: "characterencoding-button",
>     type: "view",
>     viewId: "PanelUI-characterEncodingView",
>     tooltiptext: "characterencoding-button.tooltiptext2",
>     defaultArea: CustomizableUI.AREA_PANEL,
>     maybeDisableMenu: function(aDocument) {
>       let window = aDocument.defaultView;
>       return !(window.gBrowser &&
>-               window.gBrowser.docShell &&
>-               window.gBrowser.docShell.mayEnableCharacterEncodingMenu);
>+               window.gBrowser.selectedBrowser &&
>+               window.gBrowser.selectedBrowser.mayEnableCharacterEncodingMenu);

again, window.gBrowser.selectedBrowser cannot be null

Comment 6

[Alex Bardas :alexbardas]

Attachment: bug999293_[e10s]_make_charset_menu_work.diff

I've added the changes from the code review.

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 8469049 [details] [diff] [review]
bug999293_[e10s]_make_charset_menu_work.diff

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

As we talked about, calling docshell.gatherCharsetMenuTelemetry() in the child doesn't actually do anything right now because telemetry doesn't work there.  I'll file a follow-up bug about that.

::: browser/base/content/browser.js
@@ +5353,5 @@
>  function UpdateCurrentCharset(target) {
> +  for (let menuItem of target.getElementsByTagName("menuitem")) {
> +    let isSelected = menuItem.getAttribute("charset") ===
> +                     CharsetMenu.foldCharset(gBrowser.selectedBrowser.characterSet);
> +    menuItem.setAttribute('checked', isSelected);

Nit: double-quotes, "checked"

Also please add a comment saying that since these menu items are in a radio group (type="radio"), checking the selected item automatically unchecks all the others.

@@ -5363,5 @@
> -    var wnd = document.commandDispatcher.focusedWindow;
> -    if ((window == wnd) || (wnd == null)) wnd = window.content;
> -
> -    // Uncheck previous item
> -    if (gPrevCharset) {

You need to remove the gPrevCharset declaration at the top of browser.js since it's not being used anymore.

::: toolkit/content/widgets/browser.xml
@@ +427,5 @@
>                  readonly="true"/>
>  
>        <property name="characterSet"
>                  onget="return this.contentDocument.characterSet;"
> +                readonly="true">

You need to remove readonly since you're adding a setter.

Also, I see that BrowserSetForcedCharacterSet used to set the character set by setting gBrowser.docShell.charset, and you copied that here.  But now that the characterSet property has both a getter and setter, it's strange that the getter uses this.contentDocument.characterSet but the setter uses this.docShell.charset.  So could you please change the getter to return this.docShell.charset?

Comment 8

[Alex Bardas :alexbardas]

Attachment: [e10s] make charset menu_work

I'll push on the try server as soon as it is available

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8472573 [details] [diff] [review]
[e10s] make charset menu_work

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

Thanks.

Comment 10

[Alex Bardas :alexbardas]

https://tbpl.mozilla.org/?tree=Try&rev=cbc1dde5f7a3

Is it good for checkin?

Comment 11

[Drew Willcoxon :adw]

Yes, you can add checkin-needed to the Keywords field yourself after your patch has been r+'ed and, if appropriate, you've posted a new patch that addressed the reviewer's comments, and try server looks good. :-)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7754e8a224

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba7754e8a224

Comment 14

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #7)
> As we talked about, calling docshell.gatherCharsetMenuTelemetry() in the
> child doesn't actually do anything right now because telemetry doesn't work
> there.  I'll file a follow-up bug about that.

bug 1054493

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

I just tried this in 34.0a1 (2014-08-19) and the Character encoding menu still was greyed out. Bill is that what was broken? Can you describe the problem a bit more and how we should be testing/verifying it? Thanks!

Comment 16

[Alex Bardas :alexbardas]

In about:home, about:newtab pages it is greyed out. If you visit https://www.mozilla.org/en-US/ for example, you should be able to change the character encoding in an e10s window.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I just opened a new e10s window, visited this page, and View > Character Encoding is not grayed out. Can you describe your STR more? It's possible that this bug is not completely fixed.

Comment 18

[Ricardo Maçãs [:Ricmacas]]

Tested on old build 33.0a1 (2014-07-21) and I can reproduce the bug.

(In reply to Bill McCloskey (:billm) from comment #17)
> I just opened a new e10s window, visited this page, and View > Character
> Encoding is not grayed out. Can you describe your STR more? It's possible
> that this bug is not completely fixed.

I cannot reproduce your problem in the latest nightly 34.0a1 (2014-08-20), Linux x86_64, since I've checked that after opening a e10s window, the option is grayed out in about:home and about:newtab, and not in a regular website, as per Alex Bardas.
Thus I'm marking this as VERIFIED. I'll leave the needinfo for you to confirm this.