1033483 - [e10s][RTL] Do bidi state detection in the parent process on Linux

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P1)

Description

[atifrea]

For information about this bug see bug 966395 and especially the comment at [1].
The issue for OSX has already been fixed.

[1] - https://bugzilla.mozilla.org/show_bug.cgi?id=966395#c10

Comment 1

[atifrea]

Attachment: Bug 1033483: Added a PuppetBidiKeyboard object that replaces the nsBidiKeyboard in the child

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8471237 [details] [diff] [review]
Bug 1033483: Added a PuppetBidiKeyboard object that replaces the nsBidiKeyboard in the child

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

Karl, are you the right reviewer for the gtk nsBidiKeyboard.cpp changes?

::: widget/gtk/nsGtkKeyUtils.cpp
@@ +565,5 @@
> +         * current keyboard is a bidi keyboard; we can tell if the keyboard is
> +         * bidi by looking at the result of `IsLangRTL` (i.e. rv == NS_OK if the
> +         * keyboard is bidi).
> +         */
> +        if (old_rv == NS_OK && new_rv == NS_OK && oldIsLangRTL != newIsLangRTL) {

Nit: Please use NS_SUCCEEDED instead of == NS_OK.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8471237 [details] [diff] [review]
Bug 1033483: Added a PuppetBidiKeyboard object that replaces the nsBidiKeyboard in the child

Can you explain your intentions with the different bug reports here please?
This patch includes parts that are reviewed in bug 966395, but not the cocoa
parts from that bug report.  Is the intention to use this bug to track landing
of the XP code and use bug 966395 to track landing only the cocoa parts?
Can this patch safely land without the cocoa parts or should this all be tracked on one report?  (Or perhaps it is possible to split up the patch differently to permit separate landings.)

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp

There's no checkin comment in the patch, but the attachment description is
helpful, thanks.  Usually check comments use the imperative mood:  "Add a
PuppetBidiKeyboard" ...

Has the xpwidgets code been reviewed?  Please indicate in the checkin comment.

>-    if (GdkKeymapHaveBidiLayouts)
>-        mHaveBidiKeyboards = (*GdkKeymapHaveBidiLayouts)(nullptr);
>+    // TODO: I think the problem is more complicated than just this;
>+    // the case when GTK3 is used should be treated separately (see the 'else'
>+    // below); see if GTK3 is used; if GTK3 is used then see why MOZ_WIDGET_GTK
>+    // is not updated accordingly; if GTK2 is used, then see why using
>+    // gdk_key_map_have_bidi_layouts(NULL) doesn't work
>+    if (GdkKeymapHaveBidiLayouts) {
>+        GdkDisplay* display = gdk_display_get_default();
>+        mHaveBidiKeyboards = gdk_keymap_get_for_display(display);
>+    }
> #else
>     mHaveBidiKeyboards = gdk_keymap_have_bidi_layouts(gdk_keymap_get_default());
> #endif

This is not right and will break the existing use cases.
Whether there are bidi keyboards is not related whether there is a keymap on
the display, which I suspect there always is.

What problem are you trying to solve here?
Is it possible to fix this in a separate patch, please?

The GTK2/3 distinction is (or at least should be) at compile time, except for
child plugin container processes, which should not be using this code.

>+        bool oldIsLangRTL, newIsLangRTL;
>+        nsresult old_rv = sBidiKeyboard->IsLangRTL(&oldIsLangRTL);
>+
>         sBidiKeyboard->Reset();
>+
>+        nsresult new_rv = sBidiKeyboard->IsLangRTL(&oldIsLangRTL);
>+        /**
>+         * When the direction is changed, all the children are notified, if the
>+         * current keyboard is a bidi keyboard; we can tell if the keyboard is
>+         * bidi by looking at the result of `IsLangRTL` (i.e. rv == NS_OK if the
>+         * keyboard is bidi).
>+         */
>+        if (old_rv == NS_OK && new_rv == NS_OK && oldIsLangRTL != newIsLangRTL) {
>+            nsTArray<dom::ContentParent*> children;
>+            dom::ContentParent::GetAll(children);
>+            for (uint32_t i = 0; i < children.Length(); i++) {
>+                unused << children[i]->SendBidiKeyboardNotify(newIsLangRTL);
>+            }
>+        }

How will the children know the initial state of sBidiKeyboard?

Do we know that functions sending async messages will not process any incoming messages (which might cause other children to be unref'd)?

Comment 4

[atifrea]

Attachment: Bug 1033483 Patch v2: Added a PuppetBidiKeyboard object that replaces the nsBidiKeyboard in the child

Hi,

I decided to include in the patches for this bug chunks of code from bug 966395. The reason for this is that I had a xpcshell test failure for the patch for bug 966395 so I decided to land the patch for this bug first (including the xpwidgets changes) and then adjust the patch for bug 966395 accordingly so that it only introduces the changes for cocoa.

The xpwidget code has been reviewed by both Blake and Masayuki.

You were right about the children not knowing what the initial state is. I chose to cache the value for IsLangRTL and initialize it to false. I did exactly the same for the OSX bug, but I forgot about that when writing the patch for the Linux bug.

As for the GTK issue, I made a few changes in this second version of the patch in order to make the xpcshell test pass. I hope that they won't interfere with the current use cases.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8477765 [details] [diff] [review]
Bug 1033483 Patch v2: Added a PuppetBidiKeyboard object that replaces the nsBidiKeyboard in the child

Please add more diff context and commit messages to future patches.

There is some documentation about adding more context at
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
though I wouldn't necessarily recommend using mq.

(In reply to atifrea from comment #4)
> You were right about the children not knowing what the initial state is. I
> chose to cache the value for IsLangRTL and initialize it to false. I did
> exactly the same for the OSX bug, but I forgot about that when writing the
> patch for the Linux bug.

OnKeysChanged() is called when the keymap changes, so if the keymap doesn't
change, SendBidiKeyboardNotify will not be called.  Also,
sCachedIsForRTLLanguage is never set.

However, I think the answer to my question was that your GetXPCOMProcessAttributes change already handles providing the initial state for children.

It seems that nsBidiKeyboard::Reset() would be a better place to
SendBidiKeyboardNotify()?  It already knows the previous state.

> As for the GTK issue, I made a few changes in this second version of the
> patch in order to make the xpcshell test pass. I hope that they won't
> interfere with the current use cases.

Please explain what was going wrong with the old nsBidiKeyboard::Reset() code
and how the rest of this patch triggers the associated xpcshell test failure when the GdkKeymap test in nsBidiKeyboard::Reset() is not present.

Gecko now only runs on versions of GTK+2 that are know to have the
gdk_keymap_have_bidi_layouts symbol, so the GTK+3 code here can be used for
GTK+2 also, but please make such changes in a separate patch, which might apply before other changes here.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Bug 1033483 covers the last paragraph of comment 5.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

That was bug 1152066, rather.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Non-working WIP

I was trying to make this work the other day and I wasn't successful. I don't know if it matters, but I'm running i3 on Ubuntu with English and Hebrew keyboards installed. It's possible that my not being able to make this work is due to a configuration issue specific to my setup.

First, I never see gdk_keymap_have_bidi_layouts return true, even when the keyboard itself is typing in Hebrew. Then, when (as in this patch), I removed the mHaveBidiKeyboards check from nsBidiKeyboard::IsLangRTL, gdk_keymap_get_direction seems to return the *opposite* of what I'm expecting, so my typing direction indicator is backwards of what it should be.

Karl, do you have any ideas? This isn't high priority, but as I have the partial patch, it'd be good to get it in the tree.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8721050 [details] [diff] [review]
Non-working WIP

gdk_keymap_have_bidi_layouts() depends on having left and right languages on
two different groups of the keyboard config concurrently, but IIRC some DEs
may only activate one layout on one group at a time, in which case this would never return true.

I expected gdk_keymap_get_direction() to be more reliable, but it uses
heuristics, so it could be wrong sometimes.  Getting it wrong for an English
keyboard sounds unlikely though.

@throws NS_ERROR_FAILURE if no right-to-left keyboards are installed

>+         * When the direction is changed, all the children are notified, if the
>+         * current keyboard is a bidi keyboard; we can tell if the keyboard is
>+         * bidi by looking at the result of `IsLangRTL` (i.e. rv == NS_OK if the
>+         * keyboard is bidi). No need to treat the initial case separately

If you change nsBidiKeyboard::IsLangRTL() too much, then this comment will
need to change, but IsLangRTL() "@throws NS_ERROR_FAILURE if no right-to-left
keyboards are installed".

Perhaps nsBidiKeyboards could keep track of whether it has ever seen an RTL
keyboard and IsLangRTL() would start returning successfully after that point.

It seems that nsBidiKeyboard::Reset() would be a better place to
SendBidiKeyboardNotify()?  It already knows the previous state.

Do we know that functions sending async messages will not process any
incoming messages (which might cause other children to be unref'd)?

Comment 10

[Makoto Kato [:m_kato]]

KeymapWrapper listens keys-changed signal.  When signaling, KeymapWrapper::OnKeysChanged is called.

We should send RTL information to child process at this method.

Comment 11

[Makoto Kato [:m_kato]]

When I use ibus and Debian/sid, even if switching en to ar, keys-changed isn't signal on GTK3.
But 'direction-changed' is signal, but maybe, it isn't useful.  Because this signal is fired per event :-<.  (Maybe, gdk's bug).

Comment 12

[Makoto Kato [:m_kato]]

filed https://bugzilla.gnome.org/show_bug.cgi?id=763853 for direction-changed signal

Comment 13

[Makoto Kato [:m_kato]]

When using Fedora 23 with ibus, there is another issue.  But ibus developer says "by design."

https://github.com/ibus/ibus/issues/1848

Comment 14

[Makoto Kato [:m_kato]]

When using fcitx with gnome-shell, both ibus and fcitx are used.  But GTK_IM_MODULE will be fcitx.
When using this environment, https://github.com/fcitx/fcitx/issues/257 occurs.

When using ibus only, GTK_IM_MODULE is ibus
When using this environment, https://github.com/ibus/ibus/issues/1848 occurs

Comment 15

[Jim Mathies [:jimm]]

Getting this on platform triage radar. We want to get this fixed since it prevent some linux users from using e10s.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

The ibus maintainer asked several questions on the ibus issue (1848). Kato, can you check whether you need to follow up there?

Comment 17

[Makoto Kato [:m_kato]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #16)
> The ibus maintainer asked several questions on the ibus issue (1848). Kato,
> can you check whether you need to follow up there?

I will do it.  But it isn't critical issue on ibus for this bug.  When using fcitx (default on Ubuntu 16.04 TLS now), direction-changed signal is occurred about 100-200 times by changing IM state or keyboard layout with invalid state  It is issue 257 of fcitx.  So I am looking for better workaround...

When using ibus, direction-changed signal doesn't occur when changing from AR to any CJK such as ibus-anthy or ibus-pinyin.  I mean that caret style isn't updated well on CJK.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

It looks like we can't fix this bug without fixing ibus/fcitx first. Kato, do you think we can land something that would work for most Linux users?

What do we do in non-e10s setup, does it work?

Comment 19

[Makoto Kato [:m_kato]]

(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #18)
> It looks like we can't fix this bug without fixing ibus/fcitx first. Kato,
> do you think we can land something that would work for most Linux users?

When using ibus, Bidi caret doesn't work even if on non-e10s.  When using fcitx, it has performance problem when using direction-changed signal.  So I am investigating a wokraround for that performance regression when changing keyboard.

I will check IM_MODULE env to detect fcitx for workaround...  And I think that most Bidi user doesn't use CJK IM such as fcitx-mozc / fcitx-pinyin etc...  So most users might not hit that performance regression.

> What do we do in non-e10s setup, does it work?

We don't use signal on non-e10s.  Of course, we use sync IPC from content process, it can work.  But it causes a lot of IPC with bidi setting.

Comment 20

[Makoto Kato [:m_kato]]

Attachment: Bug 1033483 - Send bidi keyboard Information on direction-changed signal.

Usgin direction-changed signal, we detect keyboard change for bidi.

Also, when system uses fcitx's IM and ibus's arabic keyboard layout, a lot of this signal might fired by switing layout and gdk_keymap_get_direction might return invalid bidi infromation.  But I think that this is rare issue.  Most users don't use Firefox Arabic version (it means that bidi.browser.ui = true) with ibus arabic layout and fcitx CJK IM.  Since there is no GTK3 API to get current IM module, I cannot find workaround for this.

Review commit: https://reviewboard.mozilla.org/r/63056/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63056/

Comment 21

[Makoto Kato [:m_kato]]

Attachment: Bug 1033483 - Part 2. Don't trust direction-changed signal when X event queue has next XkbStateNotify

To avoid IM's bugs and design, I will add prefs check to reduce unnecessary IPC.

Review commit: https://reviewboard.mozilla.org/r/63058/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63058/

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://reviewboard.mozilla.org/r/63058/#review59980

::: widget/WidgetUtils.cpp:122
(Diff revision 1)
> +  if (!Preferences::GetBool("bidi.browser.ui")) {
> +    // Don't need sync bidi information when tuned off prefs.
> +    return;
> +  }

Hmm, I don't think that this is good behavior for the other platforms on which there is some APIs to retrieve if current input is RTL.

So, I mean, cannot we ignore this check after the user activates RTL keyboard layout once?

But I guess that setting current input's language information to the method's argument causes big change. How about to add same check only in nsGtkKeyUtils.cpp for now?

Or do you want to kill the notification on the other platforms even if the user is using RTL keyboard layout on LTR locales?

Comment 23

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #22)
> https://reviewboard.mozilla.org/r/63058/#review59980
> 
> ::: widget/WidgetUtils.cpp:122
> (Diff revision 1)
> > +  if (!Preferences::GetBool("bidi.browser.ui")) {
> > +    // Don't need sync bidi information when tuned off prefs.
> > +    return;
> > +  }
> 
> Hmm, I don't think that this is good behavior for the other platforms on
> which there is some APIs to retrieve if current input is RTL.
> 
> So, I mean, cannot we ignore this check after the user activates RTL
> keyboard layout once?

On Linux system, layouts can be added / removed dynamically, so it isn't good by once check.

> But I guess that setting current input's language information to the
> method's argument causes big change. How about to add same check only in
> nsGtkKeyUtils.cpp for now?
> 
> Or do you want to kill the notification on the other platforms even if the
> user is using RTL keyboard layout on LTR locales?

I think that GTK only (and MOZ_X11) is better.  OSX and Windows can detect correctly for Bidi layout.  And most users doesn't usually change keyboard layout...   GTK with X will need a workaround to reduce unnecessary IPC.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> On Linux system, layouts can be added / removed dynamically, so it isn't good by once check.

I think that it's okay to start to notify content process when the user uses RTL keyboard layout at first time. Even after it's removed from system settings, it's okay to keep sending them.

> I think that GTK only (and MOZ_X11) is better.  OSX and Windows can detect correctly for Bidi
> layout.  And most users doesn't usually change keyboard layout...   GTK with X will need a
> workaround to reduce unnecessary IPC.

I worry about Arabic/Hebrew language users who live in English or something Western language using countries. They may use English or Western language system and same locale's localized Firefox.

Comment 25

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24)

> I worry about Arabic/Hebrew language users who live in English or something
> Western language using countries. They may use English or Western language
> system and same locale's localized Firefox.

When bidi.browser.ui=false, layout etc doesn't use nsIBidiKeyboard.  If it is addon, it can get correct value from chrome process.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Makoto Kato [:m_kato] from comment #25)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24)
> 
> > I worry about Arabic/Hebrew language users who live in English or something
> > Western language using countries. They may use English or Western language
> > system and same locale's localized Firefox.
> 
> When bidi.browser.ui=false, layout etc doesn't use nsIBidiKeyboard.  If it
> is addon, it can get correct value from chrome process.

Well, I confirmed that. nsIBidiKeyboard::IsRTL() is also used only after checking the pref. So, your change must make sense.

However, on the other hand, that means that nsIBidiKeyboard layout isn't available on child process if the pref is false. I think that nsIBidiKeyboard should return error if the pref is false and in child process. In that case, we need to be careful for the pref value. The pref may be changed anytime but in such timing, we don't send the information from chrome process to content process. So, nsIBidiKeyboard instance and WidgetUtils should store the pref value at almost same timing.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

https://reviewboard.mozilla.org/r/63056/#review61146

::: widget/gtk/nsGtkKeyUtils.cpp:560
(Diff revision 1)
> +                                  KeymapWrapper* aKeymapWrapper)
> +{
> +    // XXX
> +    // A lot of diretion-changed signal might be fired on switching bidi
> +    // keyboard when using both ibus (with arabic layout) and fcitx (with IME).
> +    // See https://github.com/fcitx/fcitx/issues/257

Drive-by question: is it possible to avoid sending an IPC message if this is a spurious direction-changed signal (e.g. by seeing if the value of isRTL changed and ignoring it if so)?

Comment 28

[Makoto Kato [:m_kato]]

(In reply to Blake Kaplan (:mrbkap) from comment #27)
> https://reviewboard.mozilla.org/r/63056/#review61146
> 
> ::: widget/gtk/nsGtkKeyUtils.cpp:560
> (Diff revision 1)
> > +                                  KeymapWrapper* aKeymapWrapper)
> > +{
> > +    // XXX
> > +    // A lot of diretion-changed signal might be fired on switching bidi
> > +    // keyboard when using both ibus (with arabic layout) and fcitx (with IME).
> > +    // See https://github.com/fcitx/fcitx/issues/257
> 
> Drive-by question: is it possible to avoid sending an IPC message if this is
> a spurious direction-changed signal (e.g. by seeing if the value of isRTL
> changed and ignoring it if so)?

gdk_keymap_get_direction() returns both true and false per direction-changed signal even if keyboard isn't changed yet.  When changing keyboard layout on this situation (as long as I test on a configuration), direction-changed signal occurs 100-200 times.  So when during changing keyboard layout, gdk_keymap_get_direction() is incredible.

Comment 29

[Makoto Kato [:m_kato]]

Comment on attachment 8769081 [details]
Bug 1033483 - Part 2. Don't trust direction-changed signal when X event queue has next XkbStateNotify

clear review to write another way.

Comment 30

[Makoto Kato [:m_kato]]

Comment on attachment 8769080 [details]
Bug 1033483 - Send bidi keyboard Information on direction-changed signal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63056/diff/1-2/

Comment 31

[Makoto Kato [:m_kato]]

Comment on attachment 8769081 [details]
Bug 1033483 - Part 2. Don't trust direction-changed signal when X event queue has next XkbStateNotify

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63058/diff/1-2/

Comment 32

[Jim Mathies [:jimm]]

Karl, any chance you can get these reviews knocked out? We're trying to get e10s turned on for 100% of release users as soon as possible.

Comment 33

[Karl Tomlinson (:karlt)]

Comment on attachment 8769080 [details]
Bug 1033483 - Send bidi keyboard Information on direction-changed signal.

https://reviewboard.mozilla.org/r/63056/#review71910

Looks good, thanks.

> Bug 1033483 - Part 1. Send bidi keyboard Information by direction-changed signal. r?karlt

lower case "information", s/by/on.

i.e. ... "information on direction-changed" ...

> Usgin direction-changed signal, we detect keyboard change for bidi.

"Using"

> Also, when system uses fcitx's IM and ibus's arabic keyboard layout, a lot of this signal might fired by switing layout and gdk_keymap_get_direction might return invalid bidi infromation.

s/a lot of this signal might fired/this signal might fire often/
s/by switing/when switching/

s/Also, w/W/ because "Also" implies that this is describing an additional change in this patch, which is not what this paragraph is doing.

::: widget/gtk/nsGtkKeyUtils.h:256
(Diff revision 2)
>       * IsAutoRepeatableKey() returns true if the key supports auto repeat.
>       * Otherwise, false.
>       */
>      bool IsAutoRepeatableKey(guint aHardwareKeyCode);
>  
> +    static void ResetBidiKeyboard();

Make this internal linkage / file static, as it doesn't need private access to KeymapWrapper, and so need not be in the header file.

Comment 34

[Karl Tomlinson (:karlt)]

Comment on attachment 8769080 [details]
Bug 1033483 - Send bidi keyboard Information on direction-changed signal.

https://reviewboard.mozilla.org/r/63056/#review61146

> Drive-by question: is it possible to avoid sending an IPC message if this is a spurious direction-changed signal (e.g. by seeing if the value of isRTL changed and ignoring it if so)?

I don't think that would achieve much.  GDK already checks whether the direction has changed before sending this signal.

Comment 35

[Karl Tomlinson (:karlt)]

Comment on attachment 8769081 [details]
Bug 1033483 - Part 2. Don't trust direction-changed signal when X event queue has next XkbStateNotify

https://reviewboard.mozilla.org/r/63058/#review72232

I don't think we should do this.  The next XkbStateNotify may be a different kind of state change and so may not trigger a direction-changed signal, in which case the direction-change will not be acted on at all.  Sending an async message on each keypress is not too much of a problem.  When there are dozens of content processes, that could become more significant, but that can addressed separately if and when required, as that issue would exist even with regular direction changes.

Comment 36

[Jim Mathies [:jimm]]

Hey Makoto checking in, when do you think you'll have some time to finish this work up?

Comment 37

[Makoto Kato [:m_kato]]

Hi, Karl.

Although I discuss with masayuki about whether we need part 2's workaround, we think that this part 2's fix is unnecessary.  Because this configuration that requires workaround is too rare environment like the following.

- Use gnome-shell as desktop shell
- ibus is installed, then gnome-shell uses ibus (gconf has ibus entry).
- User usually use ibus for RTL layout
- User sets fcitx via GTK_IMMODULE.

Is it OK to land part 1 fix only without part 2?  Part 2's code is for workaround and part 2 is too rare issue and I think that it might fix by fcitx.  If OK, I will file a new bug for this issue.

Comment 38

[Karl Tomlinson (:karlt)]

Yes, please land part 1 without part 2, thanks.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64c6e5121479
Send bidi keyboard Information on direction-changed signal. r=karlt

Comment 40

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3935a59efc38d4ae4fcbed9e9beca6f4bd25f88e for unexpected "ASSERTION: Recursive GetService!: 'Error'" failures like https://treeherder.mozilla.org/logviewer.html#?job_id=36962685&repo=mozilla-inbound in what so far looks like it will be "nearly all test suites."

Comment 41

[Makoto Kato [:m_kato]]

Comment on attachment 8769080 [details]
Bug 1033483 - Send bidi keyboard Information on direction-changed signal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63056/diff/2-3/

Comment 42

[Makoto Kato [:m_kato]]

Comment on attachment 8769080 [details]
Bug 1033483 - Send bidi keyboard Information on direction-changed signal.

Fixed recursive call.

Comment 43

[Makoto Kato [:m_kato]]

ah, I cannot reset review flag status on reviewboard :-<.

Karl, could you review again to fix assertion of comment #40?

Comment 44

[Karl Tomlinson (:karlt)]

Comment on attachment 8769080 [details]
Bug 1033483 - Send bidi keyboard Information on direction-changed signal.

https://reviewboard.mozilla.org/r/63056/#review83048

> When system uses fcitx's IM and ibus's arabic keyboard layout, this signal might fire often when switing layout

s/switing/switching/

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b920f1c673a
Send bidi keyboard Information on direction-changed signal. r=karlt

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b920f1c673a

Comment 47

[Jim Mathies [:jimm]]

Thanks for getting out the door!

Comment 48

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Tested this feature on Ubuntu 16.04 x64 and on Ubuntu 14.04 x86 using Firefox 52 aurora.
No new issues were encountered, the results are the expected ones. Here are the detailed testing results https://public.etherpad-mozilla.org/p/Manual_QA_for_1033483_(Fx52).