Closed Bug 1063669 Opened 5 years ago Closed 5 years ago

disable e10s when IME is being used

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35
Tracking Status
e10s m2+ ---

People

(Reporter: blassey, Assigned: ally)

References

Details

Attachments

(4 files, 2 obsolete files)

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:
Duplicate of this bug: 1063846
IME does not support e10s yet, so we'd (temporarily) like to disable e10s if IME is enabled.

Is this possible? Can IME be toggled on/off while Firefox is running?
Blocks: 1063680
OS: Mac OS X → All
Hardware: x86 → All
This bug should be fixed for the e10s M2 milestone.
does it have to actually be turned off or just inaccessible?
Assignee: nobody → ally
Attached file win32imetest.zip
Looks like we're going to black list languages

   HKL locales[30];
   int count = 30;
   int result = 0;
   result = GetKeyboardLayoutList(count, locales);
   printf ("ANAAKTGE result is %d\n", result);
   for (int i=0; i<result; i++) {
      printf ("0x%x isIME = %d \n",locales[i],  ImmIsIME(locales[i]));
       //http://msdn.microsoft.com/en-us/library/windows/desktop/dd318693%28v=vs.85%29.aspx
       // maybe blacklist cjk
       // last 4 hex, last 8 bits  & 0xFF
       // fold into patch for felipe
   }
Whenever you want to input with IME, you can use it in any locales. So, I don't understand what you want to do in this bug.

We need to fix all bugs which blocks users to use Nightly comfortable before enabling e10s in the default settings...
This bug is a giant/hack bandaid for the crash-landing e10s on Nightly for a few days *this* week (orders came from on high). Then it'll get flipped off.

The idea is that if we detect any of this set of languages that need imes (cjk), to then start the browser in single process firefox where ime works.

Other, better ideas are welcome (this is afterall a very sucky hack), but we have til friday.

Yes, that means bad things if a user switches on ime during a running e10s firefox, bad things happen.

I had sent you some mail with related questions earlier this week.
Attached patch horrifying testpatch (obsolete) — Splinter Review
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/6 - 9/10, JST) from comment #7)
> Whenever you want to input with IME, you can use it in any locales. So, I
> don't understand what you want to do in this bug.
> 
> We need to fix all bugs which blocks users to use Nightly comfortable before
> enabling e10s in the default settings...

Masayuki: Andreas would like to enable e10s for a couple days of testing very soon (perhaps next week). We know major features (like IME, a11y, and printing) will be broken during the test days, so we are trying to disable e10s for configurations that we know won't work.(In reply to :ally Allison Naaktgeboren from comment #6)


(In reply to :ally Allison Naaktgeboren from comment #6)
> Looks like we're going to black list languages
>        // maybe blacklist cjk
>        // last 4 hex, last 8 bits  & 0xFF
>        // fold into patch for felipe

Ally: for these test days, would it be safer to whitelist e10s just for, say, en-* locales?
(In reply to :ally Allison Naaktgeboren from comment #8)
> Yes, that means bad things if a user switches on ime during a running e10s
> firefox, bad things happen.

So, this is very bad. And please note that currently, TSF is enabled in Nightly. It's a new IME framework of Windows and also used for hand-writing system and speech input. Similarly, on GTK build, IME is used for hand-writing system of tablet.

> I had sent you some mail with related questions earlier this week.

Yeah, I'll check it. I'm back from summer vacation right now.

(In reply to Chris Peterson (:cpeterson) from comment #10)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/6 - 9/10,
> JST) from comment #7)
> > Whenever you want to input with IME, you can use it in any locales. So, I
> > don't understand what you want to do in this bug.
> > 
> > We need to fix all bugs which blocks users to use Nightly comfortable before
> > enabling e10s in the default settings...
> 
> Masayuki: Andreas would like to enable e10s for a couple days of testing
> very soon (perhaps next week). We know major features (like IME, a11y, and
> printing) will be broken during the test days, so we are trying to disable
> e10s for configurations that we know won't work.(In reply to :ally Allison
> Naaktgeboren from comment #6)

IIRC, fixing bug 975260 is temporarily enough Nightly testers to test e10s mode with IME. Why don't you ping him in it?

> (In reply to :ally Allison Naaktgeboren from comment #6)
> > Looks like we're going to black list languages
> >        // maybe blacklist cjk
> >        // last 4 hex, last 8 bits  & 0xFF
> >        // fold into patch for felipe
> 
> Ally: for these test days, would it be safer to whitelist e10s just for,
> say, en-* locales?

How about tablet PCs?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/6 - 9/10, JST) from comment #11)
> IIRC, fixing bug 975260 is temporarily enough Nightly testers to test e10s
> mode with IME. Why don't you ping him in it?

Thanks. I'll ping Makoto.


> > Ally: for these test days, would it be safer to whitelist e10s just for,
> > say, en-* locales?
> 
> How about tablet PCs?

Good point. The ideal solution is to postpone our e10s test until these bugs are all fixed, but the faster "fix" might be to programatically detect tablet PCs so we can disable e10s. <:)
(In reply to Chris Peterson (:cpeterson) from comment #10)
> Ally: for these test days, would it be safer to whitelist e10s just for,
> say, en-* locales?

Bill & I talked about that, plus european languages, but decided against it when the list got too long.

Like I said, I'm open to better ideas, but the deadline is fast approaching.
:blassey indicates that tablets are  not a concern for the m2 crashland
I realized that GTK and Mac use IME for implementing dead keys. Does it work on both platforms?
If fixing NS_QUERY_TEXT_RECT bug 975260 is good enough for Nightly testing, then this bug doesn't need to block our M2 milestone.
Depends on: 975260
fwiw, I've been told to finish the patch as a backup. I fervently hope we don't have to land it.
so has the landing of 975270 fix m-c enough that we can wontfix this one?
Flags: needinfo?(masayuki)
Attached patch horriblehack (obsolete) — Splinter Review
icky backup plan patch
Attachment #8487614 - Attachment is obsolete: true
Attachment #8489551 - Flags: review?(jmathies)
Comment on attachment 8489551 [details] [diff] [review]
horriblehack

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

::: toolkit/xre/nsAppRunner.cpp
@@ +4542,5 @@
>      bool prefEnabled = Preferences::GetBool("browser.tabs.remote.autostart", false);
>      bool disabledForA11y = Preferences::GetBool("browser.tabs.remote.autostart.disabled-because-using-a11y", false);
> +    //http://msdn.microsoft.com/en-us/library/windows/desktop/dd318693%28v=vs.85%29.aspx
> +    HKL locales[30];
> +    int count = 10;

30 and 10? Match these up. Maybe we should query GetKeyboardLayoutList for the actual number, then allocation the buffer dynamically?

@@ +4557,5 @@
> +         isIME = true;
> +      break;
> +      }
> +    }
> +  

nit - some white space here
(In reply to :ally Allison Naaktgeboren from comment #18)
> so has the landing of 975270 fix m-c enough that we can wontfix this one?

I'll try to check this today or tomorrow. But if you will add the patch, it's better.

> +    for (int i=0; i<result; i++) {
> +      int kb = (unsigned)locales[i] & 0xFFFF;
> +      if (kb == 0x0411 ||  // japanese
> +          kb == 0x0412 ||  // korean
> +          kb == 0x0C04 ||  // HK Chinese
> +          kb == 0x0804 || kb == 0x0004 || // Hans Chinese
> +          kb == 0x7C04 || kb ==  0x0404)  { //Hant Chinese
> +         isIME = true;
> +      break;
> +      }
> +    }

Hmm, this check cannot help for some specific environments. When we need to check if a keyboard layout is an IME, I *guess* we should use ITfInputProcessorProfileMgr. If you want an utility method for it, I'll *try* to create it and check the behavior.
Comment on attachment 8489551 [details] [diff] [review]
horriblehack

r- due to the feedback we're waiting on from masayuki, plus review nits.
Attachment #8489551 - Flags: review?(jmathies) → review-
Jim: IIUC, in comment 21, I think Masayuki recommended landing Ally's patch until the fix in bug 975270 is shown to be adequate.
On Windows, TSF mode doesn't work fine. Bug 966157? Looks like IMM mode almost works.

On GTK (Ubuntu 14.04), both iBus and SCIM work fine to me.

On Mac (Mavericks), Japanese IMEs work fine.
Flags: needinfo?(masayuki)
Ok, sounds like we still need this on Windows. Ally, can we get an updated patch with review nits addressed?
Flags: needinfo?(ally)
damn. ok
Flags: needinfo?(ally)
I am ashamed to put my name & Jim's on this patch.

Jim, I can't dynamically allocate the buffer because I have to give the buffer to GetKeyboardLayoutList to get the number of keyboard layouts(int result in this patch). This is clearly very old C style.
Attachment #8489551 - Attachment is obsolete: true
Attachment #8490908 - Flags: review?(jmathies)
Comment on attachment 8490908 [details] [diff] [review]
horriblehack+nits

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

You need to 

#if defined(XP_WIN)
#endif

this code block and move isIME above so it's valid on all platforms for use in the final check.
Attachment #8490908 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
Hi, could you provide a try link for this change, thanks!
Flags: needinfo?(ally)
Keywords: checkin-needed
This is probably bit rotted by the patch in bug Bug 1068199, but it should be easy to touch up. Also, I forgot to mention in the review, in the updated patch please add a comment above the main block explaining what we're doing here.
Can we wait a few hours before landing this? It occurred to me last night that this patch will make it impossible for anyone who has any CJK keyboard layouts installed to test e10s, even if they know what they're getting into. I don't want to lose Alice0775 White as a tester :-).

I'll post something in a bit.
Attached patch disable-imeSplinter Review
This patch makes it so that:

- If we detect IME, we don't ask the user to enable e10s on startup (although the damage is probably mostly already done here...)
- If the browser.tabs.remote.autostart.1 pref is set (see bug 1058358) then we won't actually enable e10s if IME is enabled.

However, the user can still turn on e10s via browser.tabs.remote.autostart.

One problem with this patch, which it shares with the a11y disabling patch, is that the "Enable E10S" box in prefs is checked if browser.tabs.remote.autostart.1 is checked, even if IME is present. I don't think this is a huge concern. It would be kind of a pain to fix, since the behavior of the checkbox is already pretty complex.
Attachment #8491591 - Flags: review?(jmathies)
Comment on attachment 8491591 [details] [diff] [review]
disable-ime

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

We have too many freaking e10s prefs, and this crash land thing is just making it worse. :(

::: xpcom/system/nsIXULRuntime.idl
@@ +105,5 @@
>    readonly attribute boolean accessibilityEnabled;
>  
>    /**
> +   * This returns a very rough approximation of whether IME is likely
> +   * to be used for the browser session. DO NOT USE!

might be good to really push this point:

"DO NOT USE! This is temporary and will be removed."
Attachment #8491591 - Flags: review?(jmathies) → review+
looks like there is no need for this NEEDINFO anymore
Flags: needinfo?(ally)
https://hg.mozilla.org/mozilla-central/rev/474174a2f4cb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
I landed a trivial fixup for win64, where GCC (mingw) considers casts from pointer to integer of a different size an error:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6a1ccab452
You need to log in before you can comment on or make changes to this bug.