Closed Bug 999293 Opened 10 years ago Closed 10 years ago

[e10s] Make charset menu work

Categories

(Firefox :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 34
Tracking Status
e10s + ---

People

(Reporter: billm, Assigned: alexbardas)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

The View > Character Encoding menu is currently broken.
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?
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
Assignee: nobody → abardas
Status: NEW → ASSIGNED
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.
Attachment #8468919 - Flags: review?(wmccloskey)
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.
Attachment #8468919 - Flags: review?(wmccloskey)
Attachment #8468919 - Flags: review?(adw)
Attachment #8468919 - Flags: feedback+
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
I've added the changes from the code review.
Attachment #8468919 - Attachment is obsolete: true
Attachment #8468919 - Flags: review?(adw)
Attachment #8469049 - Flags: review?(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?
Attachment #8469049 - Flags: review?(adw)
I'll push on the try server as soon as it is available
Attachment #8469049 - Attachment is obsolete: true
Attachment #8472573 - Flags: review?(adw)
Comment on attachment 8472573 [details] [diff] [review]
[e10s] make charset menu_work

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

Thanks.
Attachment #8472573 - Flags: review?(adw) → review+
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. :-)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba7754e8a224
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Depends on: 1054493
(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
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!
Flags: needinfo?(wmccloskey)
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.
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.
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20140820]
Flags: needinfo?(wmccloskey)
You need to log in before you can comment on or make changes to this bug.