On OSX, whenever I have focus in a text field in the child, my console fills up with: [Child 32173] WARNING: NS_ENSURE_TRUE(mKeyboardLayout) failed: file /Users/mrbkap/work/tree2/mozilla/widget/cocoa/TextInputHandler.mm, line 642 [Child 32173] WARNING: NS_ENSURE_TRUE(UCKey) failed: file /Users/mrbkap/work/tree2/mozilla/widget/cocoa/TextInputHandler.mm, line 416 [Child 32173] WARNING: NS_ENSURE_TRUE(ret) failed: file /Users/mrbkap/work/tree2/mozilla/widget/cocoa/TextInputHandler.mm, line 756 This appears to be because the APIs we try to use on OSX don't work in the child process, only in the parent. We should proxy those calls to the parent (or just transfer the state at the proper time). I don't have a Windows machine to test on, but it's likely that the same problem exists there.

Hey Bill, would the right fix here be to create a new sub-protocol of PContent and use that to forward this request to the parent process, and wait until it responds with the bidi information of the currently active keyboard layout?

I don't know anything about this code. However, we usually only create a new protocol if we want to associate some data with the request--a callback, or something like that. It doesn't sound like we need that here. It sounds like we could just add extra messages to PContent itself. Also, what data is actually being passed back? And how often do we need to update it? It seems like mKeyboardLayout is an opaque type from the OS, so I guess we can't serialize that. Would the idea be to do a sync call every time GetUCKeyboardLayout() or one of the other methods is called? That would probably work, but it sounds like these methods are called a lot. Is there some way to cache the data we need in the child?

My guess from looking at nsTextInputHandler.mm is that we don't get notifications for things like "the input language just changed" but at the same time, we're caching our answer to this question, so hopefully that means that we at least get a notification of some sort? I'm holding out hope that we can simply pass this information down from the parent to the children and not have to do the round trip sync message every time the caret blinks.

Well, the short-term fix is to run with MOZ_IGNORE_WARNINGS=1. That's what I do :-).

> I'm holding out hope that we can simply pass this information down > from the parent to the children and not have to do the round trip > sync message every time the caret blinks. Frankly I don't know whether you can or not. But if we can't already, we should try to make it so. For example, whenever the BIDI state of the keyboard changes, we should pass this from the parent to the child, hopefully asynchronously.

Masayuki knows more about this than I do.

I don't understand why nsChildView works in child process? In my understanding, child process's widget never has focus and it must be PuppetWidget... FYI: The path runs a lot of time. For example, perhaps, command + a printable key may cause the warning every time.

The relevant code runs far more often than that! Here's the relevant bit from the backtrace that I'm seeing: frame #2: 0x0000000101a47549 XUL`mozilla::widget::TISInputSourceWrapper::IsForRTLLanguage(this=0x00000001073eb650) + 105 at TextInputHandler.mm:760 frame #3: 0x0000000101a67a5d XUL`nsBidiKeyboard::IsLangRTL(this=0x000000010ff986c0, aIsRTL=0x00007fff5fbfbebf) + 29 at nsBidiKeyboard.mm:34 frame #4: 0x0000000102cd6550 XUL`nsCaret::UpdateCaretRects(this=0x0000000119c9d280, aFrame=0x000000011b7f57c8, aFrameOffset=0) + 288 at nsCaret.cpp:1062 frame #5: 0x0000000102cd5c5e XUL`nsCaret::DrawAtPositionWithHint(this=0x0000000119c9d280, aNode=0x000000011b7bc220, aOffset=0, aFrameHint=HINTRIGHT, aBidiLevel='\0', aInvalidate=true) + 654 at nsCaret.cpp:672 Masayuki, it seems like we might need to replace the bidi keyboard in the child to talk to the parent. Do you know the answer to my question in comment 3 about getting notified by the system when the keyboard direction changes?

Just wanted to mention that the work in bug 966467 may be helpful here. It would allow us to replace this code in the content process pretty easily.

Ah, I understand. It uses TextInputHandler directly. http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsBidiKeyboard.mm#34 Um, I think that child process should create PuppetBidiKeyboard like PuppetWidget. And it should cache the parent process's information, indeed. (FYI: even if child process can access the API, the value may be different from the parent process's.) At least on Windows, Mac OS X and Linux-GTK, widget can dispatch a WidgetEvent on focused widget. Then, PresShell or ESM should send a notification to child process and the child process should store the new value in PuppetBidiKeyboard. We can know the timing is, Windows: mozilla::widget::KeyboardLayout::OnLayoutChange() Mac: mozilla::widget::IMEInputHandler::OnCurrentTextInputSourceChange() Linux: mozilla::widget::KeymapWrapper::OnKeysChanged() It's difficult to look for the widget for dispatching the event, though...

Ehsan, did you say that you might be interested in looking into this?

I am still interested, but to be honest I don't understand much from the comments here. I'm not sure if I have a clear idea on what I should do to fix this...

Comment on attachment 8448348 [details] [diff] [review] Bug 966395 - Fixed bug by replacing the nsBidiKeyboard in the child with a PuppetBidiKeyboard Review of attachment 8448348 [details] [diff] [review]: ----------------------------------------------------------------- I have a bunch of small comments to address, but this looks really good overall. I'm going to flag Masayuki to review to review the widget/cocoa code just for process. ::: dom/ipc/ContentChild.cpp @@ +938,4 @@ > return false; > } > > +bool ContentChild::RecvBidiKeyboardNotify(const bool& aIsLangRTL) Same nit about indentation/line breaks as for PuppetBidiKeyboard.cpp. @@ +941,5 @@ > +bool ContentChild::RecvBidiKeyboardNotify(const bool& aIsLangRTL) > +{ > + // bidi is always of type PuppetBidiKeyboard* (because in the child, the only > + // possible implementation of nsIBidiKeyboard is PuppetBidiKeyboard). > + PuppetBidiKeyboard* bidi = (PuppetBidiKeyboard*) nsContentUtils::GetBidiKeyboard(); Please use static_cast. ::: widget/cocoa/TextInputHandler.mm @@ +2321,5 @@ > } > #endif // #ifdef PR_LOGGING > + > + /** > + * When a change is made, all the children are notified Please file followup bugs to add similar code on Windows and Linux (pointing to comment 10 here). ::: widget/cocoa/nsWidgetFactory.mm @@ +67,5 @@ > #include "nsBidiKeyboard.h" > NS_GENERIC_FACTORY_CONSTRUCTOR(nsBidiKeyboard) > > +#include "PuppetBidiKeyboard.h" > +NS_GENERIC_FACTORY_CONSTRUCTOR(PuppetBidiKeyboard) This code should go in widget/xpwidget/nsContentProcessWidgetFactory.cpp ::: widget/xpwidgets/PuppetBidiKeyboard.cpp @@ +11,5 @@ > +NS_IMPL_ISUPPORTS(PuppetBidiKeyboard, nsIBidiKeyboard) > + > +PuppetBidiKeyboard::PuppetBidiKeyboard() : nsIBidiKeyboard() > +{ > + Reset(); This can go away. No need to call empty functions. @@ +18,5 @@ > +PuppetBidiKeyboard::~PuppetBidiKeyboard() > +{ > +} > + > +NS_IMETHODIMP PuppetBidiKeyboard::Reset() More nits: For C++ methods, we prefer: ReturnType Class::MethodName(...) { ....; } ::: widget/xpwidgets/PuppetBidiKeyboard.h @@ +8,5 @@ > +#define PuppetBidiKeyboard_h_ > + > +#include "nsIBidiKeyboard.h" > + > +class PuppetBidiKeyboard : public nsIBidiKeyboard Another nit: For non ns-prefixed classes please put the class in |namespace mozilla { namespace widget {|. @@ +10,5 @@ > +#include "nsIBidiKeyboard.h" > + > +class PuppetBidiKeyboard : public nsIBidiKeyboard > +{ > + bool mIsLangRTL; Nit: Please specify private:.

Comment on attachment 8449100 [details] [diff] [review] Bug 966395 Patch v2 - Fixed bug by replacing the nsBidiKeyboard in the child with a PuppetBidiKeyboard Review of attachment 8449100 [details] [diff] [review]: ----------------------------------------------------------------- One more comment to address. ::: widget/cocoa/nsWidgetFactory.mm @@ -64,5 @@ > #include "nsMenuBarX.h" > NS_GENERIC_FACTORY_CONSTRUCTOR(nsNativeMenuServiceX) > > -#include "nsBidiKeyboard.h" > -NS_GENERIC_FACTORY_CONSTRUCTOR(nsBidiKeyboard) Sorry, I was unclear here :(. This bidi keyboard stuff should stay here (especially because it'd be weird for a MAIN_PROCESS_ONLY to be in the ContentProcess file), only the puppet widget should be in the other init file.

Comment on attachment 8449726 [details] [diff] [review] Bug 966395 Patch v3 - Fixed bug by replacing the nsBidiKeyboard in the child with a PuppetBidiKeyboard Review of attachment 8449726 [details] [diff] [review]: ----------------------------------------------------------------- This looks great to me. We still need Masayuki's stamp for the widget/* stuff, but hopefully that should be a formality.

Comment on attachment 8449726 [details] [diff] [review] Bug 966395 Patch v3 - Fixed bug by replacing the nsBidiKeyboard in the child with a PuppetBidiKeyboard >diff --git a/widget/cocoa/TextInputHandler.mm b/widget/cocoa/TextInputHandler.mm >@@ -2318,6 +2320,15 @@ IMEInputHandler::OnCurrentTextInputSourceChange(CFNotificationCenterRef aCenter, > sLastTIS = newTIS; > } > #endif // #ifdef PR_LOGGING >+ >+ /** >+ * When a change is made, all the children are notified >+ */ >+ nsTArray<dom::ContentParent*> children; >+ dom::ContentParent::GetAll(children); >+ for (uint32_t i = 0; i < children.Length(); i++) { >+ unused << children[i]->SendBidiKeyboardNotify(tis.IsForRTLLanguage()); >+ } > } This looks pretty bad for performance. I think that it's enough to do this only when the direction is actually changed. So, let's cache the last selected keyboard layout's direction with a static variable and perform this only when tis.IsForRTLLanguage() is different from the cached value. >diff --git a/widget/nsWidgetsCID.h b/widget/nsWidgetsCID.h >index 5c69f97..de63495 100644 >--- a/widget/nsWidgetsCID.h >+++ b/widget/nsWidgetsCID.h >@@ -104,6 +104,10 @@ > #define NS_BIDIKEYBOARD_CID \ > { 0x9f1800ab, 0xf428, 0x4207, { 0xb4, 0x0c, 0xe8, 0x32, 0xe7, 0x7b, 0x01, 0xfc } } > >+// {9f1800ab-f428-4207-b40c-e832e77b01fc} >+#define PUPPETBIDIKEYBOARD_CID \ >+{ 0x689e2586, 0x0344, 0x40b2, {0x83, 0x75, 0x13, 0x67, 0x2d, 0x3b, 0x71, 0x9a } } >+ > #define NS_SCREENMANAGER_CID \ > { 0xc401eb80, 0xf9ea, 0x11d3, { 0xbb, 0x6f, 0xe7, 0x32, 0xb7, 0x3e, 0xbe, 0x7c } } What's the comment? It looks like the value of NS_BIDIKEYBOARD_CID. >diff --git a/widget/xpwidgets/PuppetBidiKeyboard.cpp b/widget/xpwidgets/PuppetBidiKeyboard.cpp >new file mode 100644 >index 0000000..a59b74b >--- /dev/null >+++ b/widget/xpwidgets/PuppetBidiKeyboard.cpp >@@ -0,0 +1,46 @@ >+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ c-basic-offset should be 2. Other files in widget/xpwidgets are: > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* vim: set sw=4 ts=8 et tw=80 : */ >+/* >+ * This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#include "PuppetBidiKeyboard.h" >+ >+using namespace mozilla::widget; >+ >+NS_IMPL_ISUPPORTS(PuppetBidiKeyboard, nsIBidiKeyboard) >+ >+PuppetBidiKeyboard::PuppetBidiKeyboard() : nsIBidiKeyboard() >+{ >+} >+ >+PuppetBidiKeyboard::~PuppetBidiKeyboard() >+{ >+} >+ >+NS_IMETHODIMP >+PuppetBidiKeyboard::Reset() >+{ >+ return NS_OK; >+} So, the indent should be 2 ASCII while spaces. >+ >+NS_IMETHODIMP >+PuppetBidiKeyboard::IsLangRTL(bool *aIsRTL) Please use |bool* aIsRTL| style. >diff --git a/widget/xpwidgets/PuppetBidiKeyboard.h b/widget/xpwidgets/PuppetBidiKeyboard.h >new file mode 100644 >index 0000000..8096a54 >--- /dev/null >+++ b/widget/xpwidgets/PuppetBidiKeyboard.h >@@ -0,0 +1,33 @@ >+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ >+/* vim: set sw=4 ts=8 et tw=80 : */ >+/* >+ * This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+#ifndef PuppetBidiKeyboard_h_ Should be |mozilla_widget_PuppetBidiKeyboard_h_|. Although, only the file name must be enough safe. >+#define PuppetBidiKeyboard_h_ >+ >+#include "nsIBidiKeyboard.h" >+ >+namespace mozilla { >+namespace widget { >+class PuppetBidiKeyboard : public nsIBidiKeyboard Could you insert an empty line between |namespace widet {| and |class Puppet...|? >+{ >+private: >+ bool mIsLangRTL; >+ Could you move this to bottom of the class? (i.e., below |void SetIsLangRTL()|. >+public: >+ NS_DECL_ISUPPORTS >+ NS_DECL_NSIBIDIKEYBOARD >+ >+ PuppetBidiKeyboard(); >+ virtual ~PuppetBidiKeyboard(); >+ >+ void SetIsLangRTL(bool aIsLangRTL); >+}; >+ >+} // namespace widget >+} // namespace mozilla >+ >+#endif // PuppetBidiKeyboard_h_

And I wonder, why don't you need to implement this on Windows and Linux?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20) > And I wonder, why don't you need to implement this on Windows and Linux? I wanted to get this in sooner rather than later. Alex has already filed bug 1033483 and bug 1033488 to track Linux and Windows. We'll get to those bugs as soon as we can.

I see.

Comment on attachment 8450515 [details] [diff] [review] Bug 966395 Patch v4 - Fixed bug by replacing the nsBidiKeyboard in the child with a PuppetBidiKeyboard Review of attachment 8450515 [details] [diff] [review]: ----------------------------------------------------------------- I noticed one thing I should have seen before and there are a couple of small things to fix up. Getting closer! ::: dom/ipc/ContentChild.cpp @@ +676,5 @@ > if (NS_FAILED(svc->RegisterListener(mConsoleListener))) > NS_WARNING("Couldn't register console listener for child process"); > > + bool isOffline, isLangRTL; > + SendGetXPCOMProcessAttributes(&isOffline, &isLangRTL); You need to call RecvBidiKeyboardNotify in order to update the puppet bidi keyboard's state here. ::: dom/ipc/ContentParent.cpp @@ +2569,5 @@ > NS_ASSERTION(io, "No IO service?"); > DebugOnly<nsresult> rv = io->GetOffline(aIsOffline); > NS_ASSERTION(NS_SUCCEEDED(rv), "Failed getting offline?"); > > + nsBidiKeyboard* bidi = static_cast<nsBidiKeyboard*>(nsContentUtils::GetBidiKeyboard()); No need for the static cast, you can use the interface directly: nsIBidiKeyboard* bidi = ...; @@ +2573,5 @@ > + nsBidiKeyboard* bidi = static_cast<nsBidiKeyboard*>(nsContentUtils::GetBidiKeyboard()); > + > + *aIsLangRTL = false; > + if (bidi) { > + bidi->IsLangRTL(aIsLangRTL ); Uber-nit: nuke the space after RTL. ::: widget/cocoa/TextInputHandler.mm @@ +2323,5 @@ > #endif // #ifdef PR_LOGGING > + > + /** > + * When the direction is changed, all the children are notified. > + * No need to treat the initial case separately because You're leaving me hanging here :-) ::: widget/nsWidgetsCID.h @@ +104,4 @@ > #define NS_BIDIKEYBOARD_CID \ > { 0x9f1800ab, 0xf428, 0x4207, { 0xb4, 0x0c, 0xe8, 0x32, 0xe7, 0x7b, 0x01, 0xfc } } > > +// {689e2586-0344-40b2-8375-13672d3b719a} I might just get rid of this comment altogether. ::: widget/xpwidgets/PuppetBidiKeyboard.h @@ +12,5 @@ > + > +namespace mozilla { > +namespace widget { > + > +class PuppetBidiKeyboard : public nsIBidiKeyboard Sorry for not noticing this earlier: Please mark this class as MOZ_FINAL by putting that after the class name (but before the colon). @@ +19,5 @@ > + NS_DECL_ISUPPORTS > + NS_DECL_NSIBIDIKEYBOARD > + > + PuppetBidiKeyboard(); > + virtual ~PuppetBidiKeyboard(); The destructor can actually be private and non-virtual because the only caller is release.

Comment on attachment 8450681 [details] [diff] [review] Bug 966395 Patch v5 - Fixed bug by replacing the nsBidiKeyboard in the child with a PuppetBidiKeyboard Review of attachment 8450681 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Masayuki, would you mind looking at this again? Thanks!

Backed out for build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=43477873&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/07520461d3aa

Comment on attachment 8454257 [details] [diff] [review] Bug 966395 Patch v6 - Fixed bug by replacing the nsBidiKeyboard in the child with a PuppetBidiKeyboard I've pushed this patch to try at: <https://tbpl.mozilla.org/?tree=Try&rev=bc78c252e516>.

Trying again: https://hg.mozilla.org/integration/mozilla-inbound/rev/51ff7710ccbc

Backed out for the assertion errors during the build in https://hg.mozilla.org/integration/mozilla-inbound/rev/cab972b60028 https://tbpl.mozilla.org/php/getParsedLog.php?id=44544212&tree=Mozilla-Inbound

This also appears to have broken xpcshell tests on linux: https://tbpl.mozilla.org/php/getParsedLog.php?id=44546348&tree=Mozilla-Inbound

I think the patch for bug 1033483 should fix the test failure problem. I moved the more general xpwidgets stuff from this patch over to the patch for the Linux bug in order to be able to land that first. I will soon have a new patch for this bug containing only the changes for cocoa.

Move old M2's low-priority bugs to M6 milestone.

Blake, it looks like bug 1033483 stalled out a few months ago. Do you want to pick this up?

A bunch of merge conflicts showed up in the past few months, but nothing major. The meat of the patch is the same.

I'm battling GDK in bug 1033483 trying to get this feature working on Linux, so in the meantime I was wondering if it would be possible to get this patch landed on OSX. It turned out that the xpcshell test orange was simple to fix: we can't assume that there's a default keymap. In that case, we should just do something sane and not crash.

related, same bug on linux is bug 1033483.

This fix doesn't work well. But I will fix it by bug 1033488.