All WIP, but basic IME operations are working

For testing proper zoom/panning when focusing text field.

Comment on attachment 473767 [details] [diff] [review] (1/7) Backout Android a1-specific fixes > #ifdef MOZ_IPC > #ifdef ANDROID > mozilla::dom::PBrowserParent* > nsEventStateManager::GetCrossProcessTarget() > { >- nsCOMPtr<nsFrameLoader> fl = nsContentUtils::GetActiveFrameLoader(); >- NS_ENSURE_TRUE(fl, nsnull); >- return fl->GetRemoteBrowser(); >+ return nsnull; > } > > PRBool > nsEventStateManager::IsTargetCrossProcess(nsGUIEvent *aEvent) > { >- nsQueryContentEvent stateEvent(PR_TRUE, NS_QUERY_CONTENT_STATE, aEvent->widget); >- nsContentEventHandler handler(mPresContext); >- handler.OnQueryContentState(&stateEvent); >- return !stateEvent.mSucceeded; >+ return PR_FALSE; > } > #endif > #endif why not back out this chunk entirely? Is it used anywhere? also, http://mxr.mozilla.org/mozilla-central/source/widget/public/nsGUIEvent.h#1247 got added for a1. Is that still being used? r+ assuming those two pieces are still needed for some reason (in which case, please comment here) or they get removed.

(In reply to comment #9) > Comment on attachment 473767 [details] [diff] [review] > (1/7) Backout Android a1-specific fixes > > r+ assuming those two pieces are still needed for some reason (in which case, > please comment here) or they get removed. Yes they are used by the new PuppetWidget code. Thanks

does one of the other patches on this bug provide a new implementation for GetCrossProcessTarget() and IsTargetCrossProcess()?

(In reply to comment #11) > does one of the other patches on this bug provide a new implementation for > GetCrossProcessTarget() and IsTargetCrossProcess()? Yeah, the patch that deals with IME events (attachment 471700 [details] [diff] [review]) has new implementations for both.

I changed PuppetWidget::DispatchEvent so that the event doesn't get dispatched to both nsView and the child puppet widget.

nsIWidget::GetIMEUpdatePreference queries native widget for what kinds of IME notification it needs. This avoids unnecessary processing in content process and also avoids e10s overhead.

Used by PuppetWidget. This lets us receive OnIMEFocusChange(PR_FALSE), but not OnIMETextChange or OnIMESelectionChange. This avoids unnecessary processing in the content process.

+#ifdef MOZ_IPC + /* + * Possible IME states for + * PBrowser::GetIMEState and PBrowser::SetIMEState + */ + enum IMEState { You don't use it in this patch, why is it here? + * aWantUpdates is PR_TRUE if OnIME*Change should be called + * aWantHints must be PR_TRUE to use cross-process query content events + */ + NS_IMETHOD GetIMEUpdatePreference(PRBool *aWantUpdates, + PRBool *aWantHints) = 0; Probably better to make a virtual method that returns a struct with two fields. Also need to rev nsIWidget IID

Comment on attachment 473855 [details] [diff] [review] (4/7) Add option to receive blur notification only Okay for me, but this patch actually changes the meaning of nsIWidget, so, I guess that this needs sr.

> #define NS_SUCCESS_IME_NO_UPDATES \ > NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_WIDGET, 1) nit, use 2 spaces for indentation in nsIWidget.h

Comment on attachment 473802 [details] [diff] [review] (2/7) PuppetWidget event handling >diff --git a/widget/public/nsIWidget.h b/widget/public/nsIWidget.h >--- a/widget/public/nsIWidget.h >+++ b/widget/public/nsIWidget.h > * be fed to it. Currently used in content processes. NULL is > * returned if puppet widgets aren't supported in this build > * config, on this platform, or for this process type. > * > * This function is called "Create" to match CreateInstance(). > * The returned widget must still be nsIWidget::Create()d. > */ > static already_AddRefed<nsIWidget> >- CreatePuppetWidget(); >+ CreatePuppetWidget(mozilla::dom::PBrowserChild *aTabChild); You can add a protected: typedef mozilla::dom::PBrowserChild PBrowserChild; to nsIWidget and then use PBrowserChild here. The forward decl doesn't depend on MOZ_IPC. >diff --git a/widget/src/xpwidgets/PuppetWidget.cpp b/widget/src/xpwidgets/PuppetWidget.cpp >--- a/widget/src/xpwidgets/PuppetWidget.cpp >+++ b/widget/src/xpwidgets/PuppetWidget.cpp >@@ -125,32 +128,33 @@ PuppetWidget::CreateChild(const nsIntRec > nsIDeviceContext *aContext, > nsIAppShell *aAppShell, > nsIToolkit *aToolkit, > nsWidgetInitData *aInitData, >@@ -238,30 +242,52 @@ PuppetWidget::Update() > } > > if (mDirtyRegion.IsEmpty()) { > return NS_OK; > } > return DispatchPaintEvent(); > } > >+void >+PuppetWidget::InitEvent(nsGUIEvent& event, nsIntPoint* aPoint) >+{ >+ if (nsnull == aPoint) { >+ event.refPoint.x = 0; >+ event.refPoint.y = 0; >+ } >+ else { >+ // use the point override if provided >+ event.refPoint.x = aPoint->x; >+ event.refPoint.y = aPoint->y; >+ } >+ event.time = PR_Now() / 1000; >+} >+ This method seems out of place. It seems better to have whatever is going to dispatch the event also initialize it. >+nsEventStatus >+PuppetWidget::DispatchEvent(nsGUIEvent* event) >+{ >+ nsEventStatus status; >+ DispatchEvent(event, status); >+ return status; >+} >+ This doesn't appear to add anything meaningful to the |DispatchEvent()| below. Please rm. >diff --git a/widget/src/xpwidgets/PuppetWidget.h b/widget/src/xpwidgets/PuppetWidget.h >--- a/widget/src/xpwidgets/PuppetWidget.h >+++ b/widget/src/xpwidgets/PuppetWidget.h >@@ -50,27 +50,33 @@ > > #include "nsBaseWidget.h" > #include "nsThreadUtils.h" > #include "nsWeakReference.h" > > class gfxASurface; > > namespace mozilla { >+namespace dom { >+class PBrowserChild; >+} >+} >+ >+namespace mozilla { You don't need the PBrowser forward decl here because nsIWidget already provides it and will typedef it in nsIWidget scope. > namespace widget { > > class PuppetWidget : public nsBaseWidget, public nsSupportsWeakReference > { > typedef nsBaseWidget Base; > > // The width and height of the "widget" are clamped to this. > static const size_t kMaxDimension; > > public: >- PuppetWidget(); >+ PuppetWidget(mozilla::dom::PBrowserChild *aTabChild); PBrowserChild >@@ -174,16 +184,18 @@ private: > public: > NS_DECL_NSIRUNNABLE > PaintTask(PuppetWidget* widget) : mWidget(widget) {} > void Revoke() { mWidget = nsnull; } > private: > PuppetWidget* mWidget; > }; > >+ // TabChild holds strong reference to PuppetWidget >+ mozilla::dom::PBrowserChild *mTabChild; PBrowserChild This would be a good place for a comment explaining how PuppetWidget and PBrowserChild cooperate, at least from the PuppetWidget perspective.

(In reply to comment #19) > Comment on attachment 473802 [details] [diff] [review] > (2/7) PuppetWidget event handling > > You can add a > > protected: > typedef mozilla::dom::PBrowserChild PBrowserChild; > > to nsIWidget and then use PBrowserChild here. The forward decl > doesn't depend on MOZ_IPC. Done. > >+void > >+PuppetWidget::InitEvent(nsGUIEvent& event, nsIntPoint* aPoint) > >+{ > > This method seems out of place. It seems better to have whatever is > going to dispatch the event also initialize it. Using an InitEvent method seems to be the convention in other widget code. http://mxr.mozilla.org/mozilla-central/ident?i=InitEvent > >+nsEventStatus > >+PuppetWidget::DispatchEvent(nsGUIEvent* event) > >+{ > > This doesn't appear to add anything meaningful to the > |DispatchEvent()| below. Please rm. Done. > > namespace mozilla { > >+namespace dom { > >+class PBrowserChild; > >+} > >+} > >+ > >+namespace mozilla { > > You don't need the PBrowser forward decl here because nsIWidget > already provides it and will typedef it in nsIWidget scope. Done > >+ // TabChild holds strong reference to PuppetWidget > >+ mozilla::dom::PBrowserChild *mTabChild; > > PBrowserChild > > This would be a good place for a comment explaining how PuppetWidget > and PBrowserChild cooperate, at least from the PuppetWidget > perspective. OK. I added an explanation.

(In reply to comment #16) > +#ifdef MOZ_IPC > + /* > + * Possible IME states for > + * PBrowser::GetIMEState and PBrowser::SetIMEState > + */ > + enum IMEState { > > You don't use it in this patch, why is it here? OK removed. Sorry about that. > + * aWantUpdates is PR_TRUE if OnIME*Change should be called > + * aWantHints must be PR_TRUE to use cross-process query content events > + */ > + NS_IMETHOD GetIMEUpdatePreference(PRBool *aWantUpdates, > + PRBool *aWantHints) = 0; > > Probably better to make a virtual method that returns a struct with two fields. Added nsIMEUpdatePreference, and made GetIMEUpdatePreference return it. GetIMEUpdatePreference will be used in another patch. > Also need to rev nsIWidget IID Done.

Comment on attachment 474293 [details] [diff] [review] (2/7) PuppetWidget event handling v2 (In reply to comment #20) > Created attachment 474293 [details] [diff] [review] > > >+void > > >+PuppetWidget::InitEvent(nsGUIEvent& event, nsIntPoint* aPoint) > > >+{ > > > > This method seems out of place. It seems better to have whatever is > > going to dispatch the event also initialize it. > > Using an InitEvent method seems to be the convention in other widget code. > http://mxr.mozilla.org/mozilla-central/ident?i=InitEvent > Shrug, ok. As a private method or translation-unit-static function this would make more sense to me, but probably best to follow convention. >diff --git a/widget/public/nsIWidget.h b/widget/public/nsIWidget.h >--- a/widget/public/nsIWidget.h >+++ b/widget/public/nsIWidget.h >@@ -184,16 +189,21 @@ enum nsTopLevelWidgetZPlacement { // for > > > /** > * The base class for all the widgets. It provides the interface for > * all basic and necessary functionality. > */ > class nsIWidget : public nsISupports { > >+#ifdef MOZ_IPC >+ protected: >+ typedef mozilla::dom::PBrowserChild PBrowserChild; >+#endif >+ Drop the extra newline after |class nsIWidget ... {|. Looks good, thanks.

+ * mWantUpdates must be true to receive text and selection updates + * mWantHints must be true for cross-process query events to work This isn't quite clear enough ... Can you say *exactly* what these do? If mWantHints just makes cross-process query events work, why wouldn't we enable it all the time? Presumably enabling it has some other effect that we don't always want?

(In reply to comment #26) > + * mWantUpdates must be true to receive text and selection updates > + * mWantHints must be true for cross-process query events to work > > This isn't quite clear enough ... Can you say *exactly* what these do? > > If mWantHints just makes cross-process query events work, why wouldn't we > enable it all the time? Presumably enabling it has some other effect that we > don't always want? If mWantUpdates is true, PuppetWidget will forward nsIWidget::OnIMETextChange and nsIWidget::OnIMESelectionChange to the chrome process. This means the content process has to install observers, calculate offsets, and also incur the overhead of sending the notifications through IPDL. So if the IME implementation on a particular platform doesn't care about OnIMETextChange and OnIMESelectionChange from content processes, they can set mWantUpdates to false to avoid these overheads. If mWantHints is true, PuppetWidget will forward the content of text fields to the chrome process to be cached. This way we return the cached content during query events. (see https://bugzilla.mozilla.org/show_bug.cgi?id=583976#c5 and #c6) However this only makes sense for IME implementations that DO use query events, otherwise there's a significant overhead. So set mWantHints to false to avoid that overhead if we don't use query events. Should I include all these information in the comments?

Yes please!

Added comments. Thanks.

Sorry, fixed some comments that were out-of-date / worded wrong

With PuppetWidget, we don't have to keep track of whether events are going to content or not anymore. We do have to reset keyboard in OnFocus though because ResetInputState is only called during compositions for efficiency.

Just kidding that patch wouldn't work. Sorry about that.

Comment on attachment 475589 [details] [diff] [review] (3/7) Add IME update preference support to nsIWidget v2.1 I can't r+sr, technically

+ * focus PR_TRUE if editable object is receiving focus So if it's false, the editable object is losing focus? + sync GetIMEState(IMEState type) returns (IMEState value); + + SetIMEState(IMEState value); I think it would make more sense to use separate methods instead of a single method with a parameter that controls what it does. + } else if (end >= mIMECacheOffset && end <= cacheEnd) { + mIMECacheText.Replace(0, end - mIMECacheOffset, aText); + mIMECacheOffset = aOffset; This can't be right. end is always <= cacheEnd. I think you should remove + if (end > cacheEnd) + end = cacheEnd; + if (aCancel) + widget->CancelIMEComposition(); + else + widget->ResetInputState(); We put {} around these. Why aren't we setting *aComposition here? Somewhere you should describe what you're optimizing --- chrome caches text, and OnIMESelectionChange tries to optimize to only send text chrome doesn't know about. Have you done profiling to show that these optimizations are actually important? In particular I wonder whether OnIMESelectionChange is really important. Why not just have OnIMESelectionChange send all the selected text up to some reasonable maximum length?

Comment on attachment 475160 [details] [diff] [review] (6/7) Support IME events from chrome to content Add "using namespace mozilla::dom;" to avoid mozilla::dom:: prefixes in .cpp files. + if (inputOffset < mIMECacheOffset) + inputOffset = mIMECacheOffset; + if (inputEnd > cacheEnd) + inputEnd = cacheEnd; {} What's the strategy for HandleQueryContentEvent? We need some documentation. In particular, I don't understand what happens if the IME queries outside the cached text, we just fail? + *aComposition = mIMECompositionText; + mIMECompositionText.Truncate(0); OK now I see where aComposition gets set. Should this have been in an earlier patch? I guess not.

Can you describe a detailed example of the problem part 7 is solving?

Comment on attachment 475160 [details] [diff] [review] (6/7) Support IME events from chrome to content minusing pending review comments being addressed

Comment on attachment 475161 [details] [diff] [review] (7/7) Suppress possible out-of-sync IME notifications minusing pending review comments being addressed

Comment on attachment 475591 [details] [diff] [review] (5/7) Support IME notifications from content to chrome v1.1 minusing pending review comments being addressed

Sorry for the delay with all the moving back home and moving back to school going on :) (In reply to comment #35) > + * focus PR_TRUE if editable object is receiving focus > > So if it's false, the editable object is losing focus? > Yes; added comment. > + sync GetIMEState(IMEState type) returns (IMEState value); > + > + SetIMEState(IMEState value); > > I think it would make more sense to use separate methods instead of a single > method with a parameter that controls what it does. OK. Done. > + } else if (end >= mIMECacheOffset && end <= cacheEnd) { > + mIMECacheText.Replace(0, end - mIMECacheOffset, aText); > + mIMECacheOffset = aOffset; > > This can't be right. end is always <= cacheEnd. I think you should remove > + if (end > cacheEnd) > + end = cacheEnd; > Right. Done. > + if (aCancel) > + widget->CancelIMEComposition(); > + else > + widget->ResetInputState(); > > We put {} around these. Why aren't we setting *aComposition here? > OK. aComposition was set in the next patch because I thought it's more relevant for that patch. > Somewhere you should describe what you're optimizing --- chrome caches text, > and OnIMESelectionChange tries to optimize to only send text chrome doesn't > know about. > Done. > Have you done profiling to show that these optimizations are actually > important? In particular I wonder whether OnIMESelectionChange is really > important. Why not just have OnIMESelectionChange send all the selected text up > to some reasonable maximum length? I don't know how to profile on Android yet (GTK code doesn't use selection updates). My reasoning was that a large part of the selection will almost always be cached already, and even for a moderately long selection it would be better to optimize than to copy the whole string over.

(In reply to comment #36) > Comment on attachment 475160 [details] [diff] [review] > (6/7) Support IME events from chrome to content > > Add "using namespace mozilla::dom;" to avoid mozilla::dom:: prefixes in .cpp > files. > Done. > + if (inputOffset < mIMECacheOffset) > + inputOffset = mIMECacheOffset; > + if (inputEnd > cacheEnd) > + inputEnd = cacheEnd; > > {} > Done. > What's the strategy for HandleQueryContentEvent? We need some documentation. In > particular, I don't understand what happens if the IME queries outside the > cached text, we just fail? > Added comments. Right, we fail if it's outside the cache (for now at least).

Updated patch. (In reply to comment #37) > Can you describe a detailed example of the problem part 7 is solving? The root cause is that the Android API specifies the ability to set the selection after committing text. So to do that, in our implementation, we get the selection first, calculate the appropriate new selection, and send the set selection event, as following: Chrome gets selection offsets Chrome commits text to Content Chrome sets calculated selection in Content Specifically, the "gets selection offsets" part is implemented by saving the selection offsets given by the NotifyIMESelection notification, which can arrive at any time. So one scenario is, Chrome gets selection, commits text #1, sets selection Content receives text #1, commits, and sets selection Content sends selection update #1 to Chrome Chrome gets selection, commits text #2, sets selection Chrome receives selection update #1 Content receives text #2, commits, and sets selection Content sends selection update #2 to Chrome Chrome receives selection update #2 Chrome gets selection, commits text #3, sets selection Note that at the "gets selection" step for text #2, chrome has the wrong selection offset, because we haven't received selection update #1 yet. So when we set selection for text #2, we are setting a wrong offset. Now text #3 will be committed at the wrong offset. So this patch simulates this selection change when committing text, and suppresses the possibly wrong selection update.

Updated patch. Carried over r+ from blassey.

Updated patch. Carried over r+ from cjones.

Updated patch. Carried over r+ from cjones and sr+ from roc.

(In reply to comment #42) > I don't know how to profile on Android yet (GTK code doesn't use selection > updates). My reasoning was that a large part of the selection will almost > always be cached already, and even for a moderately long selection it would be > better to optimize than to copy the whole string over. Sure it would be better, but is it enough better to make these caching strategies worthwhile? How about we have patches for the basic functionality with the simplest possible caching, where we update all the text every time? And then if we observe slowness (either via profiling or manual testing), add additional patches (possibly in another bug) to implement the more complex caching?

Part 6: + PRUint32 selLen = mIMESelectionAnchor > mIMESelectionFocus ? + mIMESelectionAnchor - mIMESelectionFocus : + mIMESelectionFocus - mIMESelectionAnchor; Use PR_ABS + * For NS_QUERY_TEXT_CONTENT, fail only if the cache doesn't overlap with + * the queried range. Note the difference from above. We use So in what situations does this fail in practice, and what are the consequences? I'm wondering if there are normal situations where this approach just doesn't work.

So the basic problem in part 7 is that the chrome's idea of what the current selection is could be obsolete. In particular, chrome could send a message that assumes a particular selection state S1, and that message can cross in-flight with a message from content to chrome telling it that *content* has just updated the selection. So I'm not sure that your check in OnIMESelectionChange is correct. Can't the content process just happen to change the selection while mIMESuppressNotifySel is true, in which case you just ignore that selection change?

Addressed roc's comments over irc: removed optimization for text cache.

Addressed roc's comments over irc: removed optimization for text cache.

What about comment #49?

(In reply to comment #50) > So the basic problem in part 7 is that the chrome's idea of what the current > selection is could be obsolete. In particular, chrome could send a message that > assumes a particular selection state S1, and that message can cross in-flight > with a message from content to chrome telling it that *content* has just > updated the selection. > > So I'm not sure that your check in OnIMESelectionChange is correct. Can't the > content process just happen to change the selection while mIMESuppressNotifySel > is true, in which case you just ignore that selection change? Right, theoretically we could be ignoring content-initiated selection changes, and the detection could be a lot more elaborate to avoid that. But I imagine in the vast majority of cases this would not be a problem, since this all happens while the user is typing text, and content changing the selection when the user is typing would probably not be a correct behavior. Also, even in the case of a missed selection update, the consequence is that the IME *might* get confused for a moment, but as soon as the composition ends, we send selection updates again, so the consequence is most likely short-lived.

Discussion over irc: (In reply to comment #53) > What about comment #49? (In reply to comment #49) > Part 6: > + PRUint32 selLen = mIMESelectionAnchor > mIMESelectionFocus ? > + mIMESelectionAnchor - mIMESelectionFocus : > + mIMESelectionFocus - mIMESelectionAnchor; > > Use PR_ABS > <jchen> roc: sorry didn't see your new comments. mIMESelectionAnchor and mIMESelectionFocus are unsigned? i don't think PR_ABS would work <roc> cast them to ints and use PR_ABS then <roc> well, don't bother <roc> that's a little too ugly > + * For NS_QUERY_TEXT_CONTENT, fail only if the cache doesn't overlap with > + * the queried range. Note the difference from above. We use > > So in what situations does this fail in practice, and what are the > consequences? > > I'm wondering if there are normal situations where this approach just doesn't > work. <jchen> roc: also since we're caching the entire text now, NS_QUERY_TEXT_CONTENT would not fail unless an invalid offset was passed in, in the first place <roc> jchen: ok great

Comment on attachment 477845 [details] [diff] [review] (6/7) Support IME events from chrome to content v3 Fix the comment to say why it can't fail now.

I'll look at part 7 tomorrow.

As i mentioned in the chat, i found one bug with that, in case the user has open the vkb on a website and click again into the input field, the vkb close. IME_STATE is actually disabled. I think there is something strange going on with the focusmanagers of <chrome> and <browser>. Jchen confirmed this behavior on android, I found it originally on Meego.

Addition: Its not happening for input elements in <chrome>.

Fixed comment. Carried over roc's r+.

jbos - please file new bugs.

pushed: http://hg.mozilla.org/mozilla-central/rev/e0f37aadafd1 http://hg.mozilla.org/mozilla-central/rev/834f8bf2bf08 http://hg.mozilla.org/mozilla-central/rev/ba77f120b5e9 http://hg.mozilla.org/mozilla-central/rev/beb5d04777bc http://hg.mozilla.org/mozilla-central/rev/cea9bd7e6077 http://hg.mozilla.org/mozilla-central/rev/0af6ccc89e51 http://hg.mozilla.org/mozilla-central/rev/832f91c1d5a6 http://hg.mozilla.org/mozilla-central/rev/e5aa69f235a5