nsIDOMWindowUtils::SendCompositionEvent() and nsICompositionStringSynthesizer::DispatchEvent() were designed for automated tests which can test composition string handling in text/html editor without native IME. Therefore, these events are marked as mIsSynthesizedForTests true. This causes nsIMEStateManager emulates the requests to IME. http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp?rev=545c76a2bf6f#592 This also causes no notification being sent to Gonk widget because composition string is generated via the methods. Perhaps, they should have an optional argument which can specify mIsSynthesizedForTests value.

I'm thinking that IME which is implemented by JS should start to synthesize composition with an observer which observes notifications and requests to IME (i.e., alternate of nsIWidget::NotifyIME()). The actual idea of that is, nsIDOMWindowUtils::SendCompositionEvent() should take a pointer to the observer. It should be ignored if composition is already started. I.e., it's referred at starting composition and stored by TextComposition. This approach allows to test asynchronous commit too. So, I think this approach is what we should take. If you agree with this idea, I have an issue. What's the good interface of the observer? Should it be nsIObserver and its topic should be "notify-ime" and data should be the name of notification? Or, should I define a specific observer interface like nsICompositionObserver?

So this is about using WindowUtils and CompositionStringSynthesizer for non-testing? Could we perhaps have a separate API for non-testing case? And then possibly let that code have a special mode for testing, and drop the composition stuff from DOMWindowUtils (it is a bit sad that the originally for testing only DOMWindowUtils is being used in production code.)

(In reply to Olli Pettay [:smaug] from comment #2) > So this is about using WindowUtils and CompositionStringSynthesizer for > non-testing? Yes. > Could we perhaps have a separate API for non-testing case? And then possibly > let that code have a > special mode for testing, and drop the composition stuff from DOMWindowUtils Hmm, do you think that we should create new service? FYI: At least an addon uses them. So, I'm afraid to move APIs from DOMWindowUtils, though. https://mxr.mozilla.org/addons/search?string=createCompositionStringSynthesizer&find=&findi=&filter=^%5B^\0%5D*%24&hitlimit=&tree=addons > (it is a bit sad that the originally for testing only DOMWindowUtils is > being used in production code.) Yeah, I warned about it to B2G staff at beginning to implement IME API...

Oops, see previous comment.

Well, that addon already deals with old and new version of the API. Perhaps we could contact the addon author to add support for the new API (which would be hopefully then the most stable one). IME handling is complex enough that the JS API to deal with it certainly could have its own service, IMO.

Rough sketch: // not singleton. // If this is released while it has composition, the composition will be committed forcibly. // Instances of this must be held by IME at least during a composition. nsIInputMethodEditorService { /** * @param aIME Must not be null. */ void init(in nsIDOMWindow aWindow, in nsIInputMethodEditor aIME, in DOMString aLocale); /** * @param aIME Can be null. If this is null, TextComposition will automatically respond * to requests of commit or cancel composition. */ void initForTest(in nsIDOMWindow aWindow, in nsIInputMethodEditor aIME, in DOMString aLocale); readonly attribute boolean isComposing; // The string removed by the composition readonly attribute DOMString removedString; readonly attribute DOMString composingString; void setCompositionString(in DOMString aString); void setCaretOffset(in unsigned long aOffset); const unsigned long ATTR_RAWINPUT = 0x02; const unsigned long ATTR_SELECTEDRAWTEXT = 0x03; const unsigned long ATTR_CONVERTEDTEXT = 0x04; const unsigned long ATTR_SELECTEDCONVERTEDTEXT = 0x05; void appendClause(in unsigned long aLength, in unsigned long aAttribute); // Must be called after setCompositionString, setCaretOffset and/or appendClause // If there is a composition synthesized with another IMEService or native IME, // this will throw an exception. // compositionstart is automatically dispatched if this is called when isComposing is false. void modifyComposition(); // compositionstart is automatically dispatched if this is called when isComposing is false. void commitComposition([optional] in DOMString aCommitString); void cancelComposition(boolean aRestoreRemovedString); }; // scriptable nsIInputMethodEditor { const unsigned long NOTIFY_NOTHING = 0; const unsigned long NOTIFY_SELECTION_CHANGE = 1 << 0; const unsigned long NOTIFY_TEXT_CHANGE 1 << 1; const unsigned long NOTIFY_POSITION_CHANGE 1 << 2; const unsigned long NOTIFY_CHANGES_CAUSED_BY_COMPOSITION 1 << 6; const unsigned long NOTIFY_DURING_DEACTIVE 1 << 7; const unsigned long DEFAULT_CONDITIONS_OF_NOTIFYING_CHANGES = NOTIFY_CHANGES_CAUSED_BY_COMPOSITION; readonly attribute updatePreference(); void onRequestToCommitComposition(); // Required void onRequestToCancelComposition(); // Required void onFocus(); // Optional void onBlur(); // Optional void onSelectionChanged(in boolean aCausedByComposition); // Optional void onTextChanged(in boolean aCausedByComposition, in unsigned long aStartOffset, in unsigned long aOldOffset, in unsigned long aNewOffset); // Optional void onPositionChange(); // Optional // TODO? implement onMouseButtonEvent() };

Um, perhaps, nsIInputMethodEditorService.init() should take updatePrefernece() because "scriptable, function" is useful for JS implementer but it allows only one method for its member. So, nsIInputMethodEditor should be nsIInputMethodEditorCallback. // scriptable, function nsIInputMethodEditorCallback { /** * @return Not used for now. Perhaps, it will be used when mouse-button-event notification. */ unsigned long onNotify(nsINotificationToInputMethodEditor aNotification); }; nsINotificationToInputMethodEditor { readonly attribute DOMString type; readonly attribute boolean causedByComposition; readonly attribute unsigned long startOffset; readonly attribute unsigned long oldOffset; readonly attribute unsigned long newOffset; };

I'll post a lot of patches for this soon. I'd like to explain overview of the final design. First, each WidgetCompositionEvent is dispatched by widget::TextInputProxy (widget::TextEventManager might be better name, probably, I'll use this name). The class is refcountable. It's created and owned by each nsIWidget instance. It has methods like StartComposition(), CommitComposition(), NotifyIME()... So, it hides D3E spec from widget. In the future which should be used by each native IME handler too. Next, An interface for JS is nsIInputMethodEditor. This is similar to nsICompositionStringSynthesizer but more stateful. For example, it has StartComposition() and CommitComposition(). When this instance is initialized by JS, the instance tries to link to TextInputProxy. If the TextInputProxy is composing, it fails. Otherwise, can start composition. Additionally, nsIInputMethodEditor.init() and .initForTests() take nsIInputMethodEditorCallback. This is an observer of nsIWidget::NotifyIME(). (For init(), it's required argument, but for initForTests(), it's optional argument) This will help to fix bug 917323. I.e., JS IME should have nsIInputMethodEditorCallback interface (just a function). And it should initialize nsIInputMethodEditor instance at starting composition. If it failed, it shouldn't start composition. This protects a composition from being modified by two or more IME. Note that old APIs of nsIDOMWindowUtils and nsICompositionStringSynthesizer itself are removed by this bug. The goals of this bug are: 1. Implementing an XP level composition event dispatcher under widget 2. Implementing an interface for JS IME and tests 3. Providing a callback interface for JS IME 4. Removing current API The future plans are: 1. Using all native IME handlers use the XP level composition event dispatcher. 2. Using all native key event handlers use the XP level composition event dispatcher too. 3. Providing more notifications by the callback interface. Then, we can reduce the cost of conforming new behavior of DOM events (D3E, UI Events, etc)

> widget::TextInputProxy (widget::TextEventManager might be better name, probably, I'll use this name) I use widget::TextEventDispatcher for this. It's stateful but it's enough simple.

This is whole changes of following patches. I hope this helps you to understand my idea and review following patches. CompositionStringSynthesizer has two jobs. One is synthesizing composition events. This is copied to widget::TextEventDispatcher. The other is an interface for JS. This is copied to InputMethodEditor which inherits nsIInputMethodEditor.

First of all, we should create a helper class which dispatches composition events from widget/. I think that this should be used by native IME handlers in the future, but for now, this should be used only from JS-IME. The instance is created by nsIWidget instance and it's owned by the widget. When nsIWidget::GetTextEventDispatcher() is called, this is created if there is no instance. Note that actually, only top level widgets creates the instance because XP code always accesses top level widgets when they need to communicate with IME.

Next, CompositionStringSynthesizer should use TextEventDispatcher for dispatching composition events. With this change, CompositionStringSynthesizer becomes just an interface for JS.

While dispatching a compositionchange event, some composition event handlers may cause requesting commit or cancel the composition. Then, JS-IME may need to change composition string before committing/canceling the composition. Therefore, TextEventDispatcher should forget the pending composition string information immediately before dispatching compositionchange event. Then, nested calls must work fine. FYI: mClauses is a refcountable array. Therefore, modifying it during dispatching composition change causes nsEditor referring different clause information which is for next composition string. I.e., the dispatcher needs to forget mClauses completely before dispatching compositionchange event.

We should make utility methods in TextEventDispatcher for sharing same code with new methods which will be added by following patches.

For managing composition state in TextEventDispatcher, all composition events should be dispatched from it. This patch moves the code dispatching compositionstart from nsDOMWindowUtils to TextEventDispatcher.

For same purpose, we need to move the code dispatching compositioncommit(asis) event from nsDOMWindowUtils to TextEventDispatcher.

Then, TextEventDispatcher can be stateful for a composition. During compositionstart and compositioncommit(asis), mIsComposing should be true. However, for managing it perfectly, it needs to hook REQUEST_TO_COMMIT_COMPOSITION and REQUEST_TO_CANCEL_COMPOSITION of NotifyIME() and handle them instead of TextComposition. For that, this patch removes nsIWidget::NotifyIME() from nsWindow of each platform. Instead, adding nsBaseWidget::NotifyIMEInternal() which replaces current nsIWidget::NotifyIME() implementation. This increases a virtual call count per notification/request. However, I think that it's not a problem since the count of call isn't so high frequently.

For simpler usage of new IME API, TextEventDispatcher should start composition automatically if composition string is changed before dispatching compositionstart. This is now possible because TextEventDispatcher has mIsComposing.

Similarly, TextEventDispatcher can detect strange operation of JS-IME. In such case, it should return error. This patch makes it return error when: 1. CommitComposition() is called without composition but the commit string is empty. 2. CommitComposition() is called without composition but the commit string is null (i.e., the caller tries to commit composition with its last composing string).

Now, most automated tests don't need to synthesize compositionstart explicitly. This makes a lot of tests simpler.

Next, it is time to redesign JS-IME API. The new XPCOM interface is nsIInputMethodEditor. JS can create its instance. After creating one or more instances, JS-IME needs to call init() (or automated tests need to call initForTests()) with a DOM window. Then, mozilla::InputMethodEditor which is an implementation of nsIInputMethodEditor retrieves proper widget (top level widget) for given DOM window. And it will work with the TextEventDispatcher. Note that currently, if two or more InputMethodEditor instance tries to link with same widget, it won't work explicitly. But please ignore this fact for now. This issue will be fixed by following patches.

For removing legacy API, EventUtils.js should use nsIInputMethodEditor instead of nsIDOMWindowUtils and/or nsICompositionStringSynthesizer.

CompositionManager in forms.js (That is IME API for B2G) should use nsIInputMethodEditor with init(). This is the main purpose of this bug!

Now, we can remove legacy IME API completely.

Next, we need to try to fix exclusive issue of a TextEventDispatcher with multiple InputMethodEditor instances. My idea is that TextEventDispatcher should NOT be stolen by different InputMethodEditor instance ONLY while it has a composition. In other words, it shouldn't be problem any InputMethodEditor instance steal the right to use a TextEventDispatcher from another instance. For implementing this behavior, this patch does: 1. InputMethodEditor::Init() and InputMethodEditor::InitForTests() return error when the instance already owns the right to use the TextEventDispatcher for different purpose (for JS-IME or for tests). 2. InputMethodEditor::Init() and InputMethodEditor::InitForTests() return error when another instance is composing with the TextEventDispatcher. 3. InputMethodEditor should be notified when TextEventDispatcher is stolen by another instance. For this, it should be referred by TextEventDispatcher as a weak reference. 4. Then, InputMethodEditor can handle NotifyIME() by itself. This can be not a part of this patch, but included for reducing interface change of TextEventDispatcherListener.

Just renaming constants of composition string's clause attributes in EventUtils.js. With this change, we wouldn't need to maintain for synchronizing the clause attribute value if they would be changed.

Doing same thing for NS_TEXTRANGE_* is worthwhile.

This is the most behavior check of nsIInputMethodEditor. You can see this for understanding the design of new API.

I don't like to change nsIInputMethodEditor.init() and nsIInputMethodEditor.initForTests() with required new arguments. Therefore, I'd like to do this in this bug. When we fix bug 917323, JS-IME needs to listen all requests and notifications to IME. For doing that, init() should have an argument of pointer to the callback. This should be required. Requests should be handled by JS-IME itself. Similarly, initForTests() should take it for creating new tests which are impossible with current m-c. However, most tests don't need to such complicated system. So, it should be optional argument only for initForTests(). The callback interface is nsIInputMethodEditorCallback. It can be just a JS function. Its onNotify() has reference to nsIInputMethodEditor and nsIInputMethodEditorNotification. The former is useful for committing/canceling composition. The latter is useful when we will support selection change, text change and mouse button event notification in the future. We need to change only this interface at that time. This is the best for add-on compatibility.

With the nsIInputMethodEditorCallback, we can test asynchronous commit for a request to commit composition. That is a major case on Linux if user uses iBus. We should add more tests around this, but it should be done in another bug. That's all for this bug. Perhaps, I forget to explain a lot of details of the patches. Please tell me if something are unclear for you. And I'd like to land them next cycle (38). So, you can put off these reviews next week or later. Thanks in advance!

Ok, will review next week or so.

Except that comment 8 sounds so promising that I may want to look at the code sooner ;)

Comment on attachment 8546637 [details] [diff] [review] part.1 Create mozilla::widget::TextEventDispatcher class nsIWidget* mWidget; // [Weak] is a comment about its lifetime management.

Comment on attachment 8546665 [details] [diff] [review] part.11 Create nsIInputMethodEditor and implement it as mozilla::InputMethodEditor "+ * nsIInputMethodEditor instances should be created for each IME + * framework and each top level window." is not too clear. Does top level window mean the chrome DOM Window object in non-e10s FF? And does init()/initForTests() throw if aWindow isn't top level? + * IME does NOT need* to Why 'need*'?

Comment on attachment 8546675 [details] [diff] [review] part.15 Create TextEventDispatcherListener abstract class for listening notifications to IME >+#define NS_TEXT_INPUT_PROXY_LISTENER_IID \ >+{ 0xf2226f55, 0x6ddb, 0x40d5, \ >+ { 0x8a, 0x24, 0xce, 0x4d, 0x5b, 0x38, 0x15, 0xf0 } }; >+ >+class TextEventDispatcherListener : public nsSupportsWeakReference Actually, why this is called TextEventDispatcherListener, if it is mainly for IME. is this about "text" event or IME or what? I guess TextEventDispatcher class has same issue. >+{ >+public: >+ NS_DECLARE_STATIC_IID_ACCESSOR(NS_TEXT_INPUT_PROXY_LISTENER_IID) >+ >+ /** >+ * NotifyIME() is by from TextEventDispatcher::NotifyIME(). is called? sr- since need to think about better naming.

Comment on attachment 8546682 [details] [diff] [review] part.19 Add nsIInputMethodEditorCallback Please add a comment why nsIInputMethodEditorNotification is useful, since at the moment it could be replaced with a simple string. (But in comment 29 you explain that it will have more information later.)

Comment on attachment 8546637 [details] [diff] [review] part.1 Create mozilla::widget::TextEventDispatcher class So actually, should we call this CompositionEventDispatcher?

Comment on attachment 8546637 [details] [diff] [review] part.1 Create mozilla::widget::TextEventDispatcher class So I don't understand the setup. The class can be initialized only once, either for tests or non-tests, yet widget owns this kind of class. Though, basewidget doesn't initialize the dispatcher. But anyhow, I'd expect that if we're dealing with OS level events, widget should use not-for-tests dispatcher. (and I this the dispatcher class name could be something else, maybe CompositionEventDispatcher)

Comment on attachment 8546638 [details] [diff] [review] part.2 Make CompositionStringSynthesizer just a wrapper of TextEventDispatcher >+ nsRefPtr<TextEventDispatcher> dispatcher(widget->GetTextEventDispatcher()); >+ // XXX Ignore the result of InitForTest() for now. >+ dispatcher->InitForTests(); Ok, so perhaps some other patch will change this stuff.

Comment on attachment 8546646 [details] [diff] [review] part.3 Pending composition string data should be cleared immediately before flushing a pending composition string >+ // While this method dispatches a composition event, its or something other >+ // event handler may cause JS-IME doing something. A bit odd comment. Maybe "... event, some event handler may cause more clauses to be added. So, we should..."

Comment on attachment 8546648 [details] [diff] [review] part.4 Create utility methods of TextEventDispatcher > nsRefPtr<TextEventDispatcher> dispatcher(widget->GetTextEventDispatcher()); >- // XXX Ignore the result of InitForTest() for now. >- dispatcher->InitForTests(); >+ nsresult rv = dispatcher->GetState(); >+ if (rv == NS_ERROR_NOT_INITIALIZED) { >+ dispatcher->InitForTests(); >+ } else if (NS_WARN_IF(NS_FAILED(rv))) { >+ return rv; >+ } I really don't understand Init()/InitForTests() handling, but that issue isn't really about this patch, or at least not only about this patch. So I assume that will be sorted out separately.

Comment on attachment 8546649 [details] [diff] [review] part.5 Implement TextEventDispatcher::StartComposition() and nsDOMWindowUtils should use it >+ TextEventDispatcher* dispatcher = widget->GetTextEventDispatcher(); >+ nsresult rv = dispatcher->GetState(); Oh, GetTextEventDispatcher guarantees non-null return value. Please rename GetTextEventDispatcher() to TextEventDispatcher() (that is in patch 1)

Comment on attachment 8546655 [details] [diff] [review] part.7 TextEventDispatcher should manage if it has composition >+ if (rv == NS_ERROR_NOT_IMPLEMENTED) { >+ return rv; >+ } >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return rv; > } Why not just? if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } Do we really need to not warn about NS_ERROR_NOT_IMPLEMENTED >+PuppetWidget::NotifyIMEInternal(const IMENotification& aIMENotification) > { > switch (aIMENotification.mMessage) { > case REQUEST_TO_COMMIT_COMPOSITION: > return IMEEndComposition(false); > case REQUEST_TO_CANCEL_COMPOSITION: > return IMEEndComposition(true); So I don't understand this. nsBaseWidget::NotifyIME doesn't end up calling NotifyIMEInternal always when dealing with REQUEST_TO_COMMIT_COMPOSITION or REQUEST_TO_CANCEL_COMPOSITION. Don't we then miss IMEEndComposition(...) calls? Please explain or fix and re-ask review

Comment on attachment 8546659 [details] [diff] [review] part.9 TextEventDispatcher::CommitComposition() should return error if caller tries to commit composition non-existing composition with the last data or empty string I wonder if a warning in this case would be useful.

Comment on attachment 8546660 [details] [diff] [review] part.10 Remove unnecessary synthesizeComposition("compositionstart") from all tests >- if (aEvent.type != "text") { >+ if (aEvent.type != "text" && aEvent.type != "compositionstart") { > is(aEvent.data, composingString, "mismatch composition string"); So this change is needed because we end up dispatching compositionstart later and composingString isnt' "" at that moment anymore, but aEvent.data is, right? Could you perhaps add a check for compositionstart's .data With that, r+

Comment on attachment 8546665 [details] [diff] [review] part.11 Create nsIInputMethodEditor and implement it as mozilla::InputMethodEditor >+InputMethodEditor::StartComposition(bool* aSucceeded) >+{ >+ MOZ_RELEASE_ASSERT(aSucceeded, "aSucceeded must not be nullptr"); >+ MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()); >+ *aSucceeded = false; >+ nsRefPtr<TextEventDispatcher> dispatcher(mDispatcher); // Grab it. odd comment. Perhaps rename dispatcher to kungfuDeathGrip and remove the comment. >+InputMethodEditor::SetPendingCompositionString(const nsAString& aString) > { > MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()); >+ nsRefPtr<TextEventDispatcher> dispatcher(mDispatcher); // Grab it. ditto >+InputMethodEditor::AppendClauseToPendingComposition(uint32_t aLength, >+ uint32_t aAttribute) > { > MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()); >+ nsRefPtr<TextEventDispatcher> dispatcher(mDispatcher); // Grab it. And here. >+InputMethodEditor::SetCaretInPendingComposition(uint32_t aOffset) > { > MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()); >- return mDispatcher->SetCaretInPendingComposition(aOffset, aLength); >+ nsRefPtr<TextEventDispatcher> dispatcher(mDispatcher); // Grab it. ditto >+InputMethodEditor::FlushPendingComposition(bool* aSucceeded) > { >+ MOZ_RELEASE_ASSERT(aSucceeded, "aSucceeded must not be nullptr"); > MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()); >- NS_ENSURE_ARG_POINTER(aDefaultPrevented); >+ *aSucceeded = false; >+ nsRefPtr<TextEventDispatcher> dispatcher(mDispatcher); // Grab it. and here >+InputMethodEditor::CommitComposition(const nsAString& aCommitString, >+ uint8_t aOptionalArgc, >+ bool* aSucceeded) >+{ >+ MOZ_RELEASE_ASSERT(aSucceeded, "aSucceeded must not be nullptr"); >+ MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()); >+ nsRefPtr<TextEventDispatcher> dispatcher(mDispatcher); // Grab it. ditto >+InputMethodEditor::CancelComposition() >+{ >+ MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()); >+ nsRefPtr<TextEventDispatcher> dispatcher(mDispatcher); // Grab it. and here > SOURCES += [ > # this file doesn't like windows.h > 'MessagePort.cpp', > # Because of OS X headers. > 'nsContentUtils.cpp', > # this file doesn't like windows.h > 'nsDOMWindowUtils.cpp', >+ # Conflicts with windows.h's definition of SendMessage. >+ 'nsFrameMessageManager.cpp', How is this related to this patch? >+/** >+ * nsIInputMethodEditor instances should be created for each IME >+ * framework and each top level window. >+ */ as I said earlier, this needs some tweaking. >+ * Set new composition string. Pending composition will be flushed by >+ * a call of flushPendingComposition(). However, if new composition string s/if new/if the new/ >+ * isn't empty, you need to call appendClauseToPendingComposition() to fill >+ * all characters of aString with one or more clauses before flushing. >+ */ >+ void setPendingCompositionString(in DOMString aString); Not sure I quite understand this method. Why don't we have a method which doesn't require calling appendClauseToPendingComposition()? >+ >+ // ATTR_RAW_CLAUSE means that the clause hasn't been selected nor converted >+ // yet. >+ const unsigned long ATTR_RAW_CLAUSE = 0x02; >+ // ATTR_SELECTED_RAW_CLAUSE means that the clause hasn't been converted yet >+ // but is selected for converting to the other string. >+ const unsigned long ATTR_SELECTED_RAW_CLAUSE = 0x03; >+ // ATTR_CONVERTED_CLAUSE means that the clause has already been converted but >+ // is not selected. This does NOT mean that this clause isn't modifiable. >+ const unsigned long ATTR_CONVERTED_CLAUSE = 0x04; >+ // ATTR_SELECTED_CLAUSE means that the clause has already been converted and >+ // is selected. In other words, the clause is being converted. >+ const unsigned long ATTR_SELECTED_CLAUSE = 0x05; >+ >+ /** >+ * Append a clause to the pending composition. >+ * >+ * If you need to fill the pending composition string with a clause, you >+ * should call this once. For example: >+ * appendClauseToPendingComposition(compositionString.length, >+ * ATTR_RAW_CLAUSE); >+ * is enough. If you need to separate the pending composition string to >+ * multiple clauses, you need to call this multiple times. For example, >+ * if your pending composition string has three clauses and the second clause >+ * is being converted: >+ * appendClauseToPendingComposition(firstClauseLength, >+ * ATTR_CONVERTED_CLAUSE); >+ * appendClauseToPendingComposition(secondClauseLength, >+ * ATTR_SELECTED_CLAUSE); >+ * appendClauseToPendingComposition(thirdClauseLength, >+ * ATTR_CONVERTED_CLAUSE); >+ * Note that if sum of aLength mismatches length of the pending composition >+ * string, flushPendingComposition() will throw an exception. I.e., >+ * |firstClauseLength + secondClauseLength + thirdClauseLength| must be >+ * same as the length of pending composition string. >+ * >+ * TODO: Should be able to specify custom clause style. >+ * >+ * @param aLength Length of the clause. >+ * @param aAttribute One of ATTR_* constants. >+ */ >+ void appendClauseToPendingComposition(in unsigned long aLength, >+ in unsigned long aAttribute); I think the documentation isn't quite enough. The examples don't quite explain how setPendingCompositionString and appendClauseToPendingComposition should be used together. >+ * flushPendingComposition() must be called after >+ * setPendingComposigionString() setPendingCompositionString > and appendClauseToPendingComposition() >+ * (setCaretInPendingComposition() is optional) are called. >+ * Note that compositionstart will be automatically dispatched if this is >+ * called when there is no composition. What? Why would compositionstart be dispatched in that case? Wasn't there in some other patch a check that compositionstart isn't dispatched when not needed. >+ * >+ * @return Returns true if composition starts and composing >+ * the string normally. >+ * Otherwise, returns false because it might be >+ * canceled if composition starts by a call of this. I don't understand "canceled if composition starts by a call of this." yet before that there is "Returns true if composition starts" >+ * commitComposition() will commit composition. >+ * If there is a composition which is started by the instance, this commits >+ * the composition. Otherwise, throw an exception. throws >+ * >+ * @param aCommitString This is optional. If this is not specified, >+ * composition will be committed with the last >+ * composing string. Otherwise, committed with >+ * the specified string. >+ * @return Returns true if composition starts and commits >+ * the string normally. >+ * Otherwise, returns false because it might be >+ * canceled if composition starts by a call of this. >+ */ >+ [optional_argc] boolean commitComposition([optional] in DOMString aCommitString); Ah, when I saw setPendingCompositionString I thought it was about this one. Could you please clarify somewhere how setPendingCompositionString and commitComposition are related. I guess this file should have a documentation how a normal IME transaction is created and committed. r- mainly because of missing documentation.

Comment on attachment 8546667 [details] [diff] [review] part.12 EventUtils.js should use nsIInputMethodEditor for synthesizing composition Ah, this patch explain a lot about the API. But I still think the .idl could use some better documentation.

Comment on attachment 8546668 [details] [diff] [review] part.13 B2G should use nsIInputMethodEditor in forms.js > endComposition: function cm_endComposition(text) { > if (!this._isStarted) { > return; > } >- domWindowUtils.sendCompositionEvent('compositioncommit', text, ''); >+ this._inputMethodEditor.commitComposition(text ? text : ""); ?: shouldn't be needed. null should be converted to empty string (well, void string is also empty) by default. But please verify.

Though, perhaps ?: makes the code clearer. But maybe add a comment.

Comment on attachment 8546675 [details] [diff] [review] part.15 Create TextEventDispatcherListener abstract class for listening notifications to IME So I think TextEventDispatcherListener could be CompositionEventDispatcherListener or some such. >- // Another IME framework is using it now. >- if (rv == NS_ERROR_ALREADY_INITIALIZED) { >- return NS_OK; // Don't throw exception. >+ // And also if another instance composing with the new dispatcher, it'll fail s/composing/is composing/ > TextEventDispatcher::NotifyIME(const IMENotification& aIMENotification) > { >- switch (aIMENotification.mMessage) { >- case REQUEST_TO_COMMIT_COMPOSITION: { >- NS_ASSERTION(mIsComposing, "Why is this requested without composition?"); >- nsEventStatus status = nsEventStatus_eIgnore; >- CommitComposition(status); >- return NS_OK; >- } >- case REQUEST_TO_CANCEL_COMPOSITION: { >- NS_ASSERTION(mIsComposing, "Why is this requested without composition?"); >- nsEventStatus status = nsEventStatus_eIgnore; >- CommitComposition(status, &EmptyString()); >- return NS_OK; >- } >- default: >- return NS_ERROR_NOT_IMPLEMENTED; >+ nsCOMPtr<TextEventDispatcherListener> listener = do_QueryReferent(mListener); >+ if (!listener) { >+ return NS_ERROR_NOT_IMPLEMENTED; > } >+ nsresult rv = listener->NotifyIME(this, aIMENotification); >+ // If the listener isn't available, we should return NS_ERROR_NOT_IMPLEMENTED >+ // because it's not implement at such moment. Shouldn't the comment be above the previous 'if' > nsIWidget* mWidget; // [Weak] >+ nsWeakPtr mListener; // TextEventDispatcherListener Would be good to document somewhere that dispatcher does *not* keep the listener alive (since usually listeners and observers are kept alive by the object they are registered to). >+ * NotifyIME() is by from TextEventDispatcher::NotifyIME(). odd sentence. >+ /** >+ * OnRemovedFrom() is called when the TextEventDispatcher stops working and >+ * releasing the listener. ...and is releasing the listener (I assume the possible rename of the class may happen in a separate patch or something. And such thing might not require a review.)

Comment on attachment 8546681 [details] [diff] [review] part.18 Add tests of nsIInputMethodEditor Make sure to run these tests on all the relevant platforms several times on tryserver before pushing to m-i. (focus handling can be error prone)

Comment on attachment 8546682 [details] [diff] [review] part.19 Add nsIInputMethodEditorCallback So please add a comment to nsIInputMethodEditorNotification why it is needed, and why not just use a string param.

Comment on attachment 8546684 [details] [diff] [review] part.20 Add tests of async commit at requesting to commit a composition Why setTimeout and executeSoon? Use either one, not both. >+ is(events.length, 0, >+ "runAsyncForceCommitTest: composition events shouldn't been fired by asynchronous call of nsIInputMethodEditor.commitComposigion()"); commitComposition Once you've updated the patches, I'd like to look at the all-patches-in-one-patch again.

Thank you for your quick review! (In reply to Olli Pettay [:smaug] from comment #33) > Comment on attachment 8546637 [details] [diff] [review] > part.1 Create mozilla::widget::TextEventDispatcher class > > nsIWidget* mWidget; // [Weak] is a comment about its lifetime management. What did you mean? What should I do? (In reply to Olli Pettay [:smaug] from comment #34) > Comment on attachment 8546665 [details] [diff] [review] > part.11 Create nsIInputMethodEditor and implement it as > mozilla::InputMethodEditor > > "+ * nsIInputMethodEditor instances should be created for each IME > + * framework and each top level window." > is not too clear. Does top level window mean the chrome DOM Window object in > non-e10s FF? It means that InputMethodEditor needs to be for each top level widget if IME needs to compose IME in any window. The dispatcher belongs to its top level window. And the dispatched events cannot cross the top level widget boundary. Of course, IME can create only one instance, although, it cannot have two or more compositions at once. (E.g., on Mac and Linux, native IME can have composition in each top level widget) > And does init()/initForTests() throw if aWindow isn't top level? > > + * IME does NOT need* to > Why 'need*'? Oops, just a mistake, It must have been start of the next line. (In reply to Olli Pettay [:smaug] from comment #35) > Comment on attachment 8546675 [details] [diff] [review] > >+#define NS_TEXT_INPUT_PROXY_LISTENER_IID \ > >+{ 0xf2226f55, 0x6ddb, 0x40d5, \ > >+ { 0x8a, 0x24, 0xce, 0x4d, 0x5b, 0x38, 0x15, 0xf0 } }; > >+ > >+class TextEventDispatcherListener : public nsSupportsWeakReference > Actually, why this is called TextEventDispatcherListener, if it is mainly > for IME. Hmm, I think that the name should be the class under widget/'s and "Listener" because that's our usual rule at naming listener class. > is this about "text" event or IME or what? > > I guess TextEventDispatcher class has same issue. In my plan, TextEventDispatcher should handle WidgetKeyboardEvent too. Currently, we don't dispatch keyboard events during composition, don't dispatch keypress event only for modifier keys and etc. These rules might be changed for conforming to D3E or UI Events. For maintaining easier, I'd like to get rid of such logic from each widget and manage in the new class. Therefore, I like "TextEvent" for its name (because they are defined in "TextEvents.h"). And also it might be better to cache the content information in the new class too. Currently, for e10s, TabParent does it. However, if it can be moved to the new class and using same path both e10s and non-e10s, we can debug/test easier. So, TextEventDispatcher is only for dispatching composition events in this bug. However, it should manage more things in the future. (In reply to Olli Pettay [:smaug] from comment #37) > Comment on attachment 8546637 [details] [diff] [review] > part.1 Create mozilla::widget::TextEventDispatcher class > > So actually, should we call this CompositionEventDispatcher? Therefore, I don't like such specific name. (In reply to Olli Pettay [:smaug] from comment #42) > Comment on attachment 8546649 [details] [diff] [review] > part.5 Implement TextEventDispatcher::StartComposition() and > nsDOMWindowUtils should use it > > >+ TextEventDispatcher* dispatcher = widget->GetTextEventDispatcher(); > >+ nsresult rv = dispatcher->GetState(); > Oh, GetTextEventDispatcher guarantees non-null return value. > Please rename GetTextEventDispatcher() to TextEventDispatcher() > (that is in patch 1) I did it first... However, compiler (at least Visual C++) is confused because TextEventDispatcher() can be both the method and constructor of TextEventDispatcher. Do you have some idea to solve this? (In reply to Olli Pettay [:smaug] from comment #43) > Comment on attachment 8546655 [details] [diff] [review] > part.7 TextEventDispatcher should manage if it has composition > > >+ if (rv == NS_ERROR_NOT_IMPLEMENTED) { > >+ return rv; > >+ } > >+ if (NS_WARN_IF(NS_FAILED(rv))) { > >+ return rv; > > } > Why not just? > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > Do we really need to not warn about NS_ERROR_NOT_IMPLEMENTED Oh, I should've done so. However, currently, even on GTK, we don't return NS_ERROR_NOT_IMPLEMENTED for REQUEST_TO_CANCEL_COMPOSITION. So, we could remove the check. > >+PuppetWidget::NotifyIMEInternal(const IMENotification& aIMENotification) > > { > > switch (aIMENotification.mMessage) { > > case REQUEST_TO_COMMIT_COMPOSITION: > > return IMEEndComposition(false); > > case REQUEST_TO_CANCEL_COMPOSITION: > > return IMEEndComposition(true); > > So I don't understand this. > nsBaseWidget::NotifyIME doesn't end up calling NotifyIMEInternal always when > dealing with REQUEST_TO_COMMIT_COMPOSITION or REQUEST_TO_CANCEL_COMPOSITION. > Don't we then miss IMEEndComposition(...) calls? That's true. However, it's necessary to be called at handling native IME's composition in e10s mode (except on B2G). Looks like that the purpose of this is what committing/canceling composition should occur synchronously even in e10s mode. However, this is now guaranteed by TextComposition::RequestToCommit(). Therefore, I think that we can remove this from PuppetWidget. Anyway, PuppetWidget::NotifyIME() hasn't been called if the composition is synthesized since it's always ignored by native IME. http://mxr.mozilla.org/mozilla-central/source/dom/events/TextComposition.cpp?rev=d571e279fac4#343 So, not calling IMEEndComposition() in the new patches doesn't change anything. In the next step (in another bug), I'll make all native IME handlers use TextEventDispatcher. In this time, I'll fix this unused logic from PuppetWidget. Does this sound good to you? (In reply to Olli Pettay [:smaug] from comment #48) > Comment on attachment 8546668 [details] [diff] [review] > part.13 B2G should use nsIInputMethodEditor in forms.js > > > endComposition: function cm_endComposition(text) { > > if (!this._isStarted) { > > return; > > } > >- domWindowUtils.sendCompositionEvent('compositioncommit', text, ''); > >+ this._inputMethodEditor.commitComposition(text ? text : ""); > ?: shouldn't be needed. null should be converted to empty string (well, void > string is also empty) by default. > But please verify. Hmm, this is necessary for a case text === undefined. Then, commitComposition(undefined) causes committing composition with the last composing string. However, for keeping compatibility with current behavior, it must cause calling commitComposition(""). This might be clearer: if (typeof text === "string") { this._inputMethodEditor.commitComposition(text); } else { this._inputMethodEditor.cancelComposition(); } How about you? (In reply to Olli Pettay [:smaug] from comment #49) > Though, perhaps ?: makes the code clearer. But maybe add a comment. Yeah, anyway, I agree with this. Especially, at least for the naming issue of TextEventDispatcher, I'd like to get quick reply if you're busy because renaming the class needs a lot of work.

One of the options of naming TextEventDispatcher is that let's use CompositionEventDispatcher for now and when we implement key event dispatcher into the class, let's rename it to TextEventDispatcher or something.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #54) > Thank you for your quick review! > > (In reply to Olli Pettay [:smaug] from comment #33) > > Comment on attachment 8546637 [details] [diff] [review] > > part.1 Create mozilla::widget::TextEventDispatcher class > > > > nsIWidget* mWidget; // [Weak] is a comment about its lifetime management. > > What did you mean? What should I do? Hmm, what did I write. I think I meant "Needs a comment about its lifetime management." Like how do we guarantee that the pointer never points to a deleted object. > > In my plan, TextEventDispatcher should handle WidgetKeyboardEvent too. > Currently, we don't dispatch keyboard events during composition, don't > dispatch keypress event only for modifier keys and etc. These rules might be > changed for conforming to D3E or UI Events. For maintaining easier, I'd like > to get rid of such logic from each widget and manage in the new class. > Therefore, I like "TextEvent" for its name (because they are defined in > "TextEvents.h"). Ah then TextEventDispatcher sounds ok. But please add a comment at least for now why it is called what it is. (Since CompositionEventDispatcher would make more sense atm) > > >+ TextEventDispatcher* dispatcher = widget->GetTextEventDispatcher(); > > >+ nsresult rv = dispatcher->GetState(); > > Oh, GetTextEventDispatcher guarantees non-null return value. > > Please rename GetTextEventDispatcher() to TextEventDispatcher() > > (that is in patch 1) > > I did it first... However, compiler (at least Visual C++) is confused > because TextEventDispatcher() can be both the method and constructor of > TextEventDispatcher. Do you have some idea to solve this? Ah, I see. I wonder if you could use namespace for TextEventDispatcher type, and don't 'use' mozilla:: in the nsIWidget or something like that. But if that doesn't work, fine. > So, not calling IMEEndComposition() in the new patches doesn't change > anything. In the next step (in another bug), I'll make all native IME > handlers use TextEventDispatcher. In this time, I'll fix this unused logic > from PuppetWidget. Does this sound good to you? Fine > > >- domWindowUtils.sendCompositionEvent('compositioncommit', text, ''); > > >+ this._inputMethodEditor.commitComposition(text ? text : ""); > > ?: shouldn't be needed. null should be converted to empty string (well, void > > string is also empty) by default. > > But please verify. > > Hmm, this is necessary for a case text === undefined. Why? Doesn't undefined get also converted to "" in xpconnect. (webidl is different) > Especially, at least for the naming issue of TextEventDispatcher, I'd like > to get quick reply if you're busy because renaming the class needs a lot of > work. TextEventDispatcher is fine if you add a comment explaining its naming.

Thank you for the quick response! (In reply to Olli Pettay [:smaug] from comment #56) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #54) > > > >- domWindowUtils.sendCompositionEvent('compositioncommit', text, ''); > > > >+ this._inputMethodEditor.commitComposition(text ? text : ""); > > > ?: shouldn't be needed. null should be converted to empty string (well, void > > > string is also empty) by default. > > > But please verify. > > > > Hmm, this is necessary for a case text === undefined. > Why? Doesn't undefined get also converted to "" in xpconnect. > (webidl is different) Because commitComposition() is defined as: [optional_argc] boolean commitComposition([optional] in DOMString aCommitString); If aCommitString isn't specified, it should commit with the last composition string. This may help JS-IME implementation which don't want to store composing string. If aCommitString is called with empty string, it should commit the composition with empty string. Otherwise, it should commit composition with the specified string.

(In reply to Olli Pettay [:smaug] from comment #45) > Comment on attachment 8546660 [details] [diff] [review] > part.10 Remove unnecessary synthesizeComposition("compositionstart") from > all tests > > >- if (aEvent.type != "text") { > >+ if (aEvent.type != "text" && aEvent.type != "compositionstart") { > > is(aEvent.data, composingString, "mismatch composition string"); > > So this change is needed because we end up dispatching compositionstart > later and composingString isnt' "" at that moment anymore, but aEvent.data > is, right? data value of composition start is always removing string (if there was a selection). https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#event-type-compositionstart So, in this test, it must be empty string.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #58) > data value of composition start is always removing string (if there was a > selection). > https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#event- > type-compositionstart > > So, in this test, it must be empty string. Yes, but please add that test and don't just filter out compositionstart.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #57) > Because commitComposition() is defined as: > > [optional_argc] boolean commitComposition([optional] in DOMString > aCommitString); > > If aCommitString isn't specified, it should commit with the last composition > string. This may help JS-IME implementation which don't want to store > composing string. > > If aCommitString is called with empty string, it should commit the > composition with empty string. > > Otherwise, it should commit composition with the specified string. Sure, but the old code just passed empty string, right? I'd like to keep the old behavior unless there is a need to change it.

(In reply to Olli Pettay [:smaug] from comment #59) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #58) > > data value of composition start is always removing string (if there was a > > selection). > > https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/DOM3-Events.html#event- > > type-compositionstart > > > > So, in this test, it must be empty string. > Yes, but please add that test and don't just filter out compositionstart. Yeah, I do. (In reply to Olli Pettay [:smaug] from comment #60) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #57) > > Because commitComposition() is defined as: > > > > [optional_argc] boolean commitComposition([optional] in DOMString > > aCommitString); > > > > If aCommitString isn't specified, it should commit with the last composition > > string. This may help JS-IME implementation which don't want to store > > composing string. > > > > If aCommitString is called with empty string, it should commit the > > composition with empty string. > > > > Otherwise, it should commit composition with the specified string. > Sure, but the old code just passed empty string, right? > I'd like to keep the old behavior unless there is a need to change it. No, the patch doesn't change the behavior. The old code is just synthesizes "compositioncommit" via nsIDOMWindowUtils.SendCompositionEvent(). Even if its aData is null or something at calling, the value becomes empty string in nsDOMWindowUtils. Therefore, nsDOMWindowUtils::SendCompositionEvent("compositioncommit", EmptyString()) means "commit the composition with empty string". On the other hand, new API nsIInputMethodEditor.commitComposition()'s aCommitString is optional argument. Therefore, if it is undefined (i.e., omitted), it becomes InputMethodEditor::CommitComposition(EmptyString, 0). This equals nsIDOMWindowUtils.sendCompositionEvent("compositioncommitasis"). Therefore, it commits the composition with last composition string, not empty string. Therefore, if we pass the argument (text) directly, it may cause changing the behavior of CompositionManager.endCompodition(). Although, the behavior when the commit string is not specified is not defined by CompositionManager's document, but we shouldn't change the behavior. Therefore, we should use "" explicitly if the argument may be undefined.

I just added some comments into TextEventDispatcher.h. Initializing steps of this class is too buggy only with this patch. With part.15, it's probably perfect. TextEventDispatcherListener makes this class distinguish the callers. And TextComposition can manage if it has a composition with part.7. These two information are necessary for managing the rights to access this class.

I removed to check if rv of nsIWidget::NotifyIME() is NS_ERROR_NOT_IMPLEMENTED since there is no platform which returns the error for REQUEST_TO_*_COMPOSITION for now. And as I said, not calling IMEEndComposition() in PuppetWidget() is same as current behavior. So, we don't need to do anything for that.

Adding NS_WARN_IF(). However, I'm not 100% sure if this is what you expected. If you think that we should separate NS_WARN_IF() for null case and empty string case, let me know.

I think that this is what you want. Could you check it in test_bug697842.html again?

I added a lot of document into nsIInputMethodEditor.idl and rewrote some of them. Could you check them? And renaming a lot of |dispatcher| to |kungFuDeathGrip|. >> SOURCES += [ >> # this file doesn't like windows.h >> 'MessagePort.cpp', >> # Because of OS X headers. >> 'nsContentUtils.cpp', >> # this file doesn't like windows.h >> 'nsDOMWindowUtils.cpp', >>+ # Conflicts with windows.h's definition of SendMessage. >>+ 'nsFrameMessageManager.cpp', > How is this related to this patch? It has "SendMessage()". It causes bustage on my environment (VS2012 on Win8.1). I'm not sure the exact cause of that. However, removing from unified build fixes the bustage.

As I said in comment 61, we still need to check if |text| is undefined. I believe that this is necessary to keep current behavior of CompositionManager.endComposition().

Using the name, TextEventDispatcherListener, isn't problem. >> TextEventDispatcher::NotifyIME(const IMENotification& aIMENotification) >> { >>- switch (aIMENotification.mMessage) { >>- case REQUEST_TO_COMMIT_COMPOSITION: { >>- NS_ASSERTION(mIsComposing, "Why is this requested without composition?"); >>- nsEventStatus status = nsEventStatus_eIgnore; >>- CommitComposition(status); >>- return NS_OK; >>- } >>- case REQUEST_TO_CANCEL_COMPOSITION: { >>- NS_ASSERTION(mIsComposing, "Why is this requested without composition?"); >>- nsEventStatus status = nsEventStatus_eIgnore; >>- CommitComposition(status, &EmptyString()); >>- return NS_OK; >>- } >>- default: >>- return NS_ERROR_NOT_IMPLEMENTED; >>+ nsCOMPtr<TextEventDispatcherListener> listener = do_QueryReferent(mListener); >>+ if (!listener) { >>+ return NS_ERROR_NOT_IMPLEMENTED; >> } >>+ nsresult rv = listener->NotifyIME(this, aIMENotification); >>+ // If the listener isn't available, we should return NS_ERROR_NOT_IMPLEMENTED >>+ // because it's not implement at such moment. > Shouldn't the comment be above the previous 'if' No, if TextEventDispatcherListener::NotifyIME() returns NS_ERROR_NOT_AVAILABLE, it means that there are not proper implementation at the moment. Therefore, we should return NS_ERROR_NOT_IMPLEMENTED in this case. I rewrote the comment. And I added a lot of comments.

Added some comments. Could you check it?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #61) > No, the patch doesn't change the behavior. The old code is just synthesizes > "compositioncommit" via nsIDOMWindowUtils.SendCompositionEvent(). Even if > its aData is null or something at calling, the value becomes empty string in > nsDOMWindowUtils. Therefore, > nsDOMWindowUtils::SendCompositionEvent("compositioncommit", EmptyString()) > means "commit the composition with empty string". > > On the other hand, new API nsIInputMethodEditor.commitComposition()'s > aCommitString is optional argument. Therefore, if it is undefined (i.e., > omitted), But IIRC in .idl undefined doesn't mean omitted. It just means "". It is a special case in .webidl bindings to handle it as omitted.

... but explicitly passing "" might be clearer.

Hmm, Okay, I'll check the behavior with including it into the test.

Oh, I'm surprised at the actual behavior. If we call commitComposition() with undefined, XPC converts the argument to "undefined"... This fact means that the part.13 changes the behavior of CompositionManager.endComposition() when its argument isn't specified or is undefined. However, converting to empty string is more expected behavior, I guess. So, I think that we should use ?: for that. If you don't agree with changing the behavior of this case, please mark part.13's review as r-.

Oops, I forgot to say. runCommitCompositionTests() is the new tests which I added by the new part.18.

Comment on attachment 8548781 [details] [diff] [review] part.1 Create mozilla::widget::TextEventDispatcher class >+/** >+ * TextEventDispatcher is a helper class for dispatching widget events defined >+ * in TextEvents.h. Currently, this is a helper for dispatching >+ * WidgetCompositionEvent. However, WidgetKeyboardEvent and/or >+ * WidgetQueryContentEvent may be supported by this class in the future. >+ * An instance of this class is created by nsIWidget instance and owned by it. >+ * This is typically created by only top level widgets because only top level created only by the top level widgets because only they handle IME. >+ * SetCaretInPendingComposition() sets caret position in the pending >+ * composition string and its length. This is optional. If IME don't want s/don't/doesn't/ >+ * to show caret, it shouldn't need to call this. >+ * >+ * @param aOffset Offset of the caret in the pending composition >+ * string. This should not be larger than length of the length >+ * the pending composition string. >+ * @param aLength Caret width. If this is 0, caret will be collapsed. >+ * Note that Gecko hasn't supported wide caret yet, s/hasn't/doesn't/ >+ * FlushPendingComposition() sends the pending composition string >+ * to widget for the stored DOM window. the the widget of the store DOM Window. (or something like that) Before calling this, IME needs to >+ * sets set pending composition string with SetPendingCompositionString(), >+ * AppendClauseToPendingComposition() and/or >+ * SetCaretInPendingComposition(). >+ */ >+ nsresult FlushPendingComposition(nsEventStatus& aStatus) >+ { >+ return mPendingComposition.Flush(this, aStatus); >+ } > >+ /** >+ * GetTextEventDispatcher() returns TextEventDispatcher belonging to the >+ * widget. >+ */ >+ NS_IMETHOD_(TextEventDispatcher*) GetTextEventDispatcher() = 0; Might be good to document that this never returns null. (it wouldn't be needed if there was some way to call this TextEventDispatcher() )

Comment on attachment 8548790 [details] [diff] [review] part.7 TextEventDispatcher should manage if it has composition >+ * NotifyIME() is same as nsIWidget::NotifyIME(). See the document around it. Somewhat odd comment. Perhaps just "@see nsIWidget::NotifyIME" >+nsBaseWidget::NotifyIME(const IMENotification& aIMENotification) >+{ >+ switch (aIMENotification.mMessage) { >+ case REQUEST_TO_COMMIT_COMPOSITION: >+ case REQUEST_TO_CANCEL_COMPOSITION: >+ // If the notification is a request to IME and mTextEventDispatcher has >+ // composition, it should be handled by the mTextEventDispatcher. >+ if (mTextEventDispatcher && mTextEventDispatcher->IsComposing()) { >+ return mTextEventDispatcher->NotifyIME(aIMENotification); >+ } Do we ever execute this code without mTextEventDispatcher->IsComposing()? Perhaps worth to add some assertion

(In reply to Olli Pettay [:smaug] from comment #85) > Comment on attachment 8548790 [details] [diff] [review] > part.7 TextEventDispatcher should manage if it has composition > > >+ * NotifyIME() is same as nsIWidget::NotifyIME(). See the document around it. > Somewhat odd comment. > Perhaps just > "@see nsIWidget::NotifyIME" > > > >+nsBaseWidget::NotifyIME(const IMENotification& aIMENotification) > >+{ > >+ switch (aIMENotification.mMessage) { > >+ case REQUEST_TO_COMMIT_COMPOSITION: > >+ case REQUEST_TO_CANCEL_COMPOSITION: > >+ // If the notification is a request to IME and mTextEventDispatcher has > >+ // composition, it should be handled by the mTextEventDispatcher. > >+ if (mTextEventDispatcher && mTextEventDispatcher->IsComposing()) { > >+ return mTextEventDispatcher->NotifyIME(aIMENotification); > >+ } > Do we ever execute this code without mTextEventDispatcher->IsComposing()? Yes. If it's requested but mTextEventDispatcher doesn't have composition, native IME has composition. In this case, this needs to call NotifyIMEInternal(). > Perhaps worth to add some assertion Therefore, this causes a spam (or crash).

Comment on attachment 8548794 [details] [diff] [review] part.9 TextEventDispatcher::CommitComposition() should return error if caller tries to commit composition non-existing composition with the last data or empty string > >+ // When there is no composition, caller shouldn't try to commit composition >+ // with non-existing composition string nor commit composition with empty >+ // string. >+ if (NS_WARN_IF(!IsComposing() && >+ (!aCommitString || aCommitString->IsEmpty()))) { if (NS_WARN_IF(!IsComposing() && (!aCommitString || aCommitString->IsEmpty()))) { might be a tad more readable.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #86) > Therefore, this causes a spam (or crash). Oh, in that case no assertion, but looks like I'm still misunderstanding something here. Why would we ever commit/cancel composition without being in composition. Can we change the code to not have that invariant and always rely on TextEventDispatcher?

Comment on attachment 8548790 [details] [diff] [review] part.7 TextEventDispatcher should manage if it has composition I think I should re-review this then.

Comment on attachment 8549360 [details] [diff] [review] part.18 Add tests of nsIInputMethodEditor Additional tests for this could include testing what happens if a new page is loaded during composition.

(In reply to Olli Pettay [:smaug] from comment #88) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #86) > > Therefore, this causes a spam (or crash). > Oh, in that case no assertion, but looks like I'm still misunderstanding > something here. > Why would we ever commit/cancel composition without being in composition. > Can we change the code to not have that invariant and always rely on > TextEventDispatcher? In the future, i.e., after all native IME handlers use TextEventDispatcher, we can just rewrite it as: return mTextEventDispatcher ? mTextEventDispatcher->NotifyIME() : NS_OK; However, currently, we need to manage two composition state, native IME's and TextEventDispatcher's. Therefore, we need to do like this: return mTextEventDispatcher && mTextEventDispatcher->IsComposing() ? mTextEventDispatcher->NotifyIME() : // mTextEventDispatcher has composition and it needs to handle the request NotifyIMEInternal(); // Otherwise, native IME handler has composition and native IME needs to handle it. Actually, this has a bug. Even while native IME has composition, TextEventDispatcher can have composition. However, this is not so important issue. It's possible only when native IME has composition and *addon* has composition via TextEventDispatcher. This issue will be fixed when native IME handlers use TextEventDispatcher.

(In reply to Olli Pettay [:smaug] from comment #90) > Comment on attachment 8549360 [details] [diff] [review] > part.18 Add tests of nsIInputMethodEditor > > Additional tests for this could include testing what happens if a new page > is loaded during composition. Hmm, could be. Using iframe is enough?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #92) > > Hmm, could be. Using iframe is enough? Yeah, I think so if the nsIInputMethodEditor has been initialized with iframe.contentWindow.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #91) > However, currently, we need to manage two composition state, native IME's > and TextEventDispatcher's. Ah, right, ofc. A comment explaining the current situation would be good.

Comment on attachment 8548798 [details] [diff] [review] part.11 Create nsIInputMethodEditor and implement it as mozilla::InputMethodEditor (sr=smaug) >+ * An nsIInputMethodEditor instance is associated to a top level widget I think it is 'associated with', not 'to' >+ * Example #2 JS-IME can separate composition string to two or more clauses: >+ * >+ * // First, set composition string again >+ * IME.setPendingCompositionString("some-words-are-inputted"); >+ * // Then, if "are" is selected to convert, there are 3 clauses: >+ * IME.appendClauseToPendingComposition(11, IME.ATTR_CONVERTED_CLAUSE); >+ * IME.appendClauseToPendingComposition(3, IME.ATTR_SELECTED_CLAUSE); >+ * IME.appendClauseToPendingComposition(9, IME.ATTR_CONVERTED_CLAUSE); >+ * // Show caret at the head of selected clause s/head of/beginning of the/ >+ * Example #3 JS-IME can commit composition with specific string with this: >+ * >+ * // This is useful when user selects a commit string from candidate list UI >+ * // which is provided by JS-IME. >+ * IME.commitComposition("selected-words-from-candidate-list"); Could you document whether IME.setPendingCompositionString() or IME.flushPendingComposition(); is needed with IME.commitComposition() >+ * // If JS-IME don't want to show composing string in the focused editor, >+ * // JS-IME can dispatch only compositionstart event with this. >+ * if (!IME.startComposition()) { >+ * // Failed to start composition. >+ * return; >+ * } and to end the composition, call commitComposition() or cancelComposition(), right? please add a comment. >+ * Note that if you tries try >+ /** >+ * cancelComposition() will cancel composition. This is now same as a call This is for now the same as calling commitComposition("")

Comment on attachment 8548807 [details] [diff] [review] part.19 Add nsIInputMethodEditorCallback >+[scriptable, function, uuid(23d5f242-adb5-46f1-8766-90d1bf0383df)] >+interface nsIInputMethodEditorCallback : nsISupports >+{ >+ /** >+ * When Gecko notifies IME of something or requests something to IME, >+ * this is called. >+ * >+ * @param aInputMethodEditor Reference to the nsIInputMethodEditor service >+ * which is the original receiver of the request >+ * or notification. >+ * @param aNotification Stores type of notifications and additional >+ * information. >+ * @return Must be one of RESULT_*. >+ */ >+ unsigned long onNotify(in nsIInputMethodEditor aInputMethodEditor, >+ in nsIInputMethodEditorNotification aNotification); >+ >+ // RESULT_OK means that the notification doesn't cause any trouble. >+ const unsigned long RESULT_OK = 0x00000000; >+ // RESULT_ERROR means that some trouble occur at handling the notification. >+ const unsigned long RESULT_ERROR = 0x00010000; This part feels a bit ugly. Couldn't we just return boolean value. true is success, false error. Or if not that, just use nsresult?

Comment on attachment 8548790 [details] [diff] [review] part.7 TextEventDispatcher should manage if it has composition (at some point I'd like to still see a new all-the-changes-in-one-patch)

(In reply to Olli Pettay [:smaug] from comment #97) > Comment on attachment 8548790 [details] [diff] [review] > part.7 TextEventDispatcher should manage if it has composition > > (at some point I'd like to still see a new all-the-changes-in-one-patch) Sure. (In reply to Olli Pettay [:smaug] from comment #96) > Comment on attachment 8548807 [details] [diff] [review] > part.19 Add nsIInputMethodEditorCallback > > >+[scriptable, function, uuid(23d5f242-adb5-46f1-8766-90d1bf0383df)] > >+interface nsIInputMethodEditorCallback : nsISupports > >+{ > >+ /** > >+ * When Gecko notifies IME of something or requests something to IME, > >+ * this is called. > >+ * > >+ * @param aInputMethodEditor Reference to the nsIInputMethodEditor service > >+ * which is the original receiver of the request > >+ * or notification. > >+ * @param aNotification Stores type of notifications and additional > >+ * information. > >+ * @return Must be one of RESULT_*. > >+ */ > >+ unsigned long onNotify(in nsIInputMethodEditor aInputMethodEditor, > >+ in nsIInputMethodEditorNotification aNotification); > >+ > >+ // RESULT_OK means that the notification doesn't cause any trouble. > >+ const unsigned long RESULT_OK = 0x00000000; > >+ // RESULT_ERROR means that some trouble occur at handling the notification. > >+ const unsigned long RESULT_ERROR = 0x00010000; > This part feels a bit ugly. Couldn't we just return boolean value. > true is success, false error. Even if we use boolean, we might need to redefine another meaning of them for some notifications in the future. E.g., if we'll support NOTIFY_IME_OF_MOUSE_BUTTON_EVENT, true means that "do default", false means that "consumed by IME". I think that using constants making the callback implementation clearer. How do you think? > Or if not that, just use nsresult? nsresult is available in JS??

Renaming: nsIInputMethodEditor -> nsITextInputProcessor mozilla::InputMethodEditor -> mozilla::TextInputProcessor

Replacing "IME" in variable names and function names to "TIP".

Same as above. And CompositionManager._inputMethodEditor -> CompositionManager._textInputProcessor. And this should be reviewed by B2G staff too. Yuan, we're removing composition event dispatchers from nsIDOMWindowUtils. Instead, we're creating nsITextInputProcessor. That should be created by JS-IME (or its framework). And it should be initialized (a call of init() with a DOM window which will have a composition) before starting composition every time. Other usage are similar.

Adding runUnloadTests(). However, mysteriously, I cannot use "load" event for this. But "DOMContentLoaded" works and it's enough for this test. If you know why "load" doesn't work in this case, let me know.

And probably, I found a bug. If JS-IME releases TextInputProcessor, the instance is released and detached from TextEventDispatcher even while there is a composition. I'll add a patch for this bug.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #98) > Even if we use boolean, we might need to redefine another meaning of them > for some notifications in the future. Well, let's then change the interface in the future. But for now boolean should be enough. E.g., if we'll support > NOTIFY_IME_OF_MOUSE_BUTTON_EVENT, true means that "do default", false means > that "consumed by IME". I think that using constants making the callback > implementation clearer. How do you think? I don't think so. But, we have already NS_OK and such which idl methods can "throw". So using NS_* constants would be fine, but I would like us to avoid adding yet another "did this method succeed" mechanism. > > Or if not that, just use nsresult? > > nsresult is available in JS?? I think http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/idl/xpccomponents.idl?rev=6d97109e0c30&mark=719-721#719 could be useful. And Components.results.*

Comment on attachment 8550254 [details] [diff] [review] Bug 917322 part.18 Add tests of nsITextInputProcessor >+ // XXX I'm not sure why we cannot listen load event here. Instead, use DOMContentLoaded. Because you add event listener to bubble phase and load event doesn't bubble? And load event isn't dispatched to this iframe since it is in chrome. See nsGlobalWindow::PostHandleEvent

Though, how does TIP1 (initialize to childWindow) work after the page load?

(In reply to Olli Pettay [:smaug] from comment #110) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #98) > > Even if we use boolean, we might need to redefine another meaning of them > > for some notifications in the future. > Well, let's then change the interface in the future. But for now boolean > should be enough. Okay. (In reply to Olli Pettay [:smaug] from comment #111) > Comment on attachment 8550254 [details] [diff] [review] > Bug 917322 part.18 Add tests of nsITextInputProcessor > > >+ // XXX I'm not sure why we cannot listen load event here. Instead, use DOMContentLoaded. > Because you add event listener to bubble phase and load event doesn't bubble? > And load event isn't dispatched to this iframe since it is in chrome. > See nsGlobalWindow::PostHandleEvent Oh, thanks. (In reply to Olli Pettay [:smaug] from comment #112) > Though, how does TIP1 (initialize to childWindow) work after the page load? Yes because they're in same top level widget. I'll add tests for it. BTW, I have a question. I'm researching an issue mentioned in comment 109. For example: var TIP = Cc.createInstance(Ci.nsITextInputProcesstor); TIP.initForTests(window); // Create composition ... TIP = null; // Even here, mozilla::TextInputProcessor's destructor hasn't been run yet. TIP = Cc.createInstance(Ci.nsITextInputProcesstor); TIP.initForTests(window); // FAILED due to there is still existing the composition! Looks like that at setting null in JS, it doesn't cause calling nsISupports::Release() at least immediately after that. Is this right? As far as I tested, all TIP instances are released after finishing test_nsITextInputHandler.xul. So, cannot we cancel composition immediately after all JS variables release the TIP instances? What should/can we do for this scenario? Nothing?

Adding runUnloadTests2(). Although, I don't like the name but I have no idea.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #113) > (In reply to Olli Pettay [:smaug] from comment #110) > > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #98) > var TIP = Cc.createInstance(Ci.nsITextInputProcesstor); > TIP.initForTests(window); > // Create composition > ... > TIP = null; > // Even here, mozilla::TextInputProcessor's destructor hasn't been run yet. Because the dtor runs when TIP is garbage collected. Or more precisely, JS GC collects the JS wrapper, and then the reference which the wrapper hold is released asynchronously, and if that happens to be the last reference to the C++ object, its destructor is called. In tests SpecialPowers.gc(); should work. > Looks like that at setting null in JS, it doesn't cause calling > nsISupports::Release() at least immediately after that. Is this right? So yes, this is all right. JS is GCed language. Only the C++ side deals with reference counting. > As far as I tested, all TIP instances are released after finishing > test_nsITextInputHandler.xul. So, cannot we cancel composition immediately > after all JS variables release the TIP instances? What should/can we do for > this scenario? Nothing? Explicitly call GC, or add some kind of shutdown(); method to TIP, and that method would do what the destructor currently. (and destructor would call it too, in case someone forgot to call shutdown() explicitly.)

Comment on attachment 8550331 [details] [diff] [review] part.18 Add tests of nsITextInputProcessor >+ // We cannot listen load event here because load event isn't fired on iframe >+ // in chrome. Instead, let's use DOMContentLoaded. I think you could use 'load' if you just used capture phase.

Comment on attachment 8550146 [details] [diff] [review] part.13 B2G should use nsITextInputProcessor in forms.js (r=smaug) Review of attachment 8550146 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.

Okay, using "load" event in capture phase.

Making the result type of nsITextInputProcessor::onNotify() boolean.

Changing the callback function's return statements. (RESULT_OK -> true)

Thanks for the explanation. This may not help some cases if addon releases TextInputProcessor without canceling composition. However, this patch protects from such situation at least after a few time (I hope so!).

Removing a debug line. Sorry for the spam.

Comment on attachment 8551255 [details] [diff] [review] part.19 Add nsITextInputProcessorCallback ># HG changeset patch ># User Masayuki Nakano <masayuki@d-toybox.com> ># Parent 70fc58553fb6051ac961e9d2d02e5e63875aa6fc >Bug 917322 part.19 Add nsITextInputProcessorCallback r=, sr= > >diff --git a/dom/base/TextInputProcessor.cpp b/dom/base/TextInputProcessor.cpp >--- a/dom/base/TextInputProcessor.cpp >+++ b/dom/base/TextInputProcessor.cpp >@@ -10,16 +10,53 @@ > #include "nsIWidget.h" > #include "nsPIDOMWindow.h" > #include "nsPresContext.h" > > using namespace mozilla::widget; > > namespace mozilla { > >+/****************************************************************************** >+ * TextInputProcessorNotification >+ ******************************************************************************/ >+ >+class TextInputProcessorNotification MOZ_FINAL : >+ public nsITextInputProcessorNotification >+{ >+public: >+ explicit TextInputProcessorNotification(const char* aType) >+ : mType(aType) >+ { >+ } >+ >+ NS_DECL_ISUPPORTS >+ >+ NS_IMETHOD GetType(nsACString& aType) MOZ_OVERRIDE MOZ_FINAL >+ { >+ aType = mType; >+ return NS_OK; >+ } >+ >+protected: >+ ~TextInputProcessorNotification() { } >+ >+private: >+ nsAutoCString mType; >+ >+ TextInputProcessorNotification() { } >+}; >+ >+NS_IMPL_ISUPPORTS(TextInputProcessorNotification, >+ nsITextInputProcessorNotification) >+ >+/****************************************************************************** >+ * TextInputProcessor >+ ******************************************************************************/ >+ > NS_IMPL_ISUPPORTS(TextInputProcessor, > nsITextInputProcessor, > TextEventDispatcherListener, > nsISupportsWeakReference) > > TextInputProcessor::TextInputProcessor() > : mDispatcher(nullptr) > , mForTests(false) >@@ -27,34 +64,44 @@ TextInputProcessor::TextInputProcessor() > } > > TextInputProcessor::~TextInputProcessor() > { > } > > NS_IMETHODIMP > TextInputProcessor::Init(nsIDOMWindow* aWindow, >+ nsITextInputProcessorCallback* aCallback, > bool* aSucceeded) > { > MOZ_RELEASE_ASSERT(aSucceeded, "aSucceeded must not be nullptr"); > MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()); >- return InitInternal(aWindow, false, *aSucceeded); >+ if (NS_WARN_IF(!aCallback)) { >+ *aSucceeded = false; >+ return NS_ERROR_INVALID_ARG; >+ } >+ return InitInternal(aWindow, aCallback, false, *aSucceeded); > } > > NS_IMETHODIMP > TextInputProcessor::InitForTests(nsIDOMWindow* aWindow, >+ nsITextInputProcessorCallback* aCallback, >+ uint8_t aOptionalArgc, > bool* aSucceeded) > { > MOZ_RELEASE_ASSERT(aSucceeded, "aSucceeded must not be nullptr"); > MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome()); >- return InitInternal(aWindow, true, *aSucceeded); >+ nsITextInputProcessorCallback* callback = >+ aOptionalArgc >= 1 ? aCallback : nullptr; >+ return InitInternal(aWindow, callback, true, *aSucceeded); > } > > nsresult > TextInputProcessor::InitInternal(nsIDOMWindow* aWindow, >+ nsITextInputProcessorCallback* aCallback, > bool aForTests, > bool& aSucceeded) > { > aSucceeded = false; > if (NS_WARN_IF(!aWindow)) { > return NS_ERROR_INVALID_ARG; > } > nsCOMPtr<nsPIDOMWindow> pWindow(do_QueryInterface(aWindow)); >@@ -79,17 +126,18 @@ TextInputProcessor::InitInternal(nsIDOMW > } > > nsRefPtr<TextEventDispatcher> dispatcher = widget->GetTextEventDispatcher(); > MOZ_RELEASE_ASSERT(dispatcher, "TextEventDispatcher must not be null"); > > // If the instance was initialized and is being initialized for same > // dispatcher and same purpose, we don't need to initialize the dispatcher > // again. >- if (mDispatcher && dispatcher == mDispatcher && aForTests == mForTests) { >+ if (mDispatcher && dispatcher == mDispatcher && aCallback == mCallback && >+ aForTests == mForTests) { > aSucceeded = true; > return NS_OK; > } > > // If this instance is composing, don't allow to initialize again. > if (mDispatcher && mDispatcher->IsComposing()) { > return NS_ERROR_ALREADY_INITIALIZED; > } >@@ -111,26 +159,38 @@ TextInputProcessor::InitInternal(nsIDOMW > rv = dispatcher->Init(this); > } > > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > > mDispatcher = dispatcher; >+ mCallback = aCallback; > mForTests = aForTests; > aSucceeded = true; > return NS_OK; > } > > void > TextInputProcessor::UnlinkFromTextEventDispatcher() > { > mDispatcher = nullptr; > mForTests = false; >+ if (mCallback) { >+ nsCOMPtr<nsITextInputProcessorCallback> callback(mCallback); >+ mCallback = nullptr; >+ >+ nsRefPtr<TextInputProcessorNotification> notification = >+ new TextInputProcessorNotification("notify-detached"); >+ bool result = false; >+ callback->OnNotify(this, notification, &result); >+ NS_ASSERTION(result, >+ "nsITextInputProcesserCallback::OnNotify() should return true"); >+ } > } > > nsresult > TextInputProcessor::IsValidStateForComposition() > { > if (NS_WARN_IF(!mDispatcher)) { > return NS_ERROR_NOT_INITIALIZED; > } >@@ -289,16 +349,53 @@ TextInputProcessor::NotifyIME(TextEventD > const IMENotification& aNotification) > { > // If This is called while this is being initialized, ignore the call. > if (!mDispatcher) { > return NS_ERROR_NOT_AVAILABLE; > } > MOZ_ASSERT(aTextEventDispatcher == mDispatcher, > "Wrong TextEventDispatcher notifies this"); >+ NS_ASSERTION(mForTests || mCallback, >+ "mCallback can be null only when IME is initialized for tests"); >+ if (mCallback) { >+ nsRefPtr<TextInputProcessorNotification> notification; >+ switch (aNotification.mMessage) { >+ case REQUEST_TO_COMMIT_COMPOSITION: { >+ NS_ASSERTION(aTextEventDispatcher->IsComposing(), >+ "Why is this requested without composition?"); >+ notification = new TextInputProcessorNotification("request-to-commit"); >+ break; >+ } >+ case REQUEST_TO_CANCEL_COMPOSITION: { >+ NS_ASSERTION(aTextEventDispatcher->IsComposing(), >+ "Why is this requested without composition?"); >+ notification = new TextInputProcessorNotification("request-to-cancel"); >+ break; >+ } >+ case NOTIFY_IME_OF_FOCUS: >+ notification = new TextInputProcessorNotification("notify-focus"); >+ break; >+ case NOTIFY_IME_OF_BLUR: >+ notification = new TextInputProcessorNotification("notify-blur"); >+ break; >+ default: >+ return NS_ERROR_NOT_IMPLEMENTED; >+ } >+ MOZ_RELEASE_ASSERT(notification); >+ bool result = false; >+ nsresult rv = mCallback->OnNotify(this, notification, &result); >+ if (NS_WARN_IF(NS_FAILED(rv))) { >+ return rv; >+ } >+ NS_ASSERTION(result, >+ "nsITextInputProcesserCallback::OnNotify() should return true"); >+ return result ? NS_OK : NS_ERROR_FAILURE; >+ } >+ > switch (aNotification.mMessage) { > case REQUEST_TO_COMMIT_COMPOSITION: { > NS_ASSERTION(aTextEventDispatcher->IsComposing(), > "Why is this requested without composition?"); > CommitCompositionInternal(); > return NS_OK; > } > case REQUEST_TO_CANCEL_COMPOSITION: { >diff --git a/dom/base/TextInputProcessor.h b/dom/base/TextInputProcessor.h >--- a/dom/base/TextInputProcessor.h >+++ b/dom/base/TextInputProcessor.h >@@ -3,16 +3,17 @@ > * 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 mozilla_dom_textinputprocessor_h_ > #define mozilla_dom_textinputprocessor_h_ > > #include "mozilla/TextEventDispatcherListener.h" > #include "nsITextInputProcessor.h" >+#include "nsITextInputProcessorCallback.h" > > namespace mozilla { > > namespace widget{ > class TextEventDispatcher; > } // namespace widget > > class TextInputProcessor MOZ_FINAL : public nsITextInputProcessor >@@ -32,23 +33,25 @@ public: > const IMENotification& aNotification) MOZ_OVERRIDE; > NS_IMETHOD_(void) > OnRemovedFrom(TextEventDispatcher* aTextEventDispatcher) MOZ_OVERRIDE; > > private: > ~TextInputProcessor(); > > nsresult InitInternal(nsIDOMWindow* aWindow, >+ nsITextInputProcessorCallback* aCallback, > bool aForTests, > bool& aSucceeded); > nsresult CommitCompositionInternal(const nsAString* aCommitString = nullptr, > bool* aSucceeded = nullptr); > nsresult CancelCompositionInternal(); > nsresult IsValidStateForComposition(); > void UnlinkFromTextEventDispatcher(); > > TextEventDispatcher* mDispatcher; // [Weak] >+ nsCOMPtr<nsITextInputProcessorCallback> mCallback; > bool mForTests; > }; > > } // namespace mozilla > > #endif // #ifndef mozilla_dom_textinputprocessor_h_ >diff --git a/dom/base/test/chrome/window_nsITextInputProcessor.xul b/dom/base/test/chrome/window_nsITextInputProcessor.xul >--- a/dom/base/test/chrome/window_nsITextInputProcessor.xul >+++ b/dom/base/test/chrome/window_nsITextInputProcessor.xul >@@ -1,16 +1,18 @@ > <?xml version="1.0"?> > <?xml-stylesheet href="chrome://global/skin" type="text/css"?> > <?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" > type="text/css"?> > <window title="Testing nsITextInputProcessor behavior" > xmlns:html="http://www.w3.org/1999/xhtml" > xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > onunload="onunload();"> >+<script type="application/javascript" >+ src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script> > <body xmlns="http://www.w3.org/1999/xhtml"> > <p id="display"> > <input id="input" type="text"/><br/> > <iframe id="iframe" width="300" height="150" > src="data:text/html,&lt;textarea id='textarea' cols='20' rows='4'&gt;&lt;/textarea&gt;"></iframe><br/> > </p> > <div id="content" style="display: none"> > >@@ -72,27 +74,40 @@ function createTIP() > } > > function runInitMethodTests() > { > var description = "runInitMethodTest: "; > input.value = ""; > input.focus(); > >+ var simpleCallback = function (aTIP, aNotification) >+ { >+ switch (aNotification.type) { >+ case "request-to-commit": >+ aTIP.commitComposition(); >+ break; >+ case "request-to-cancel": >+ aTIP.cancelComposition(); >+ break; >+ } >+ return true; >+ }; >+ > var TIP1 = createTIP(); > var TIP2 = createTIP(); > isnot(TIP1, TIP2, > description + "TIP instances should be different"); > > // init() and initForTests() can take ownership if there is no composition. >- ok(TIP1.init(window), >+ ok(TIP1.init(window, simpleCallback), > description + "TIP1.init(window) should succeed because there is no composition"); > ok(TIP1.initForTests(window), > description + "TIP1.initForTests(window) should succeed because there is no composition"); >- ok(TIP2.init(window), >+ ok(TIP2.init(window, simpleCallback), > description + "TIP2.init(window) should succeed because there is no composition"); > ok(TIP2.initForTests(window), > description + "TIP2.initForTests(window) should succeed because there is no composition"); > > // Start composition with TIP1, then, other TIPs cannot take ownership during a composition. > ok(TIP1.initForTests(window), > description + "TIP1.initForTests() should succeed because there is no composition"); > var composingStr = "foo"; >@@ -100,17 +115,17 @@ function runInitMethodTests() > TIP1.appendClauseToPendingComposition(composingStr.length, TIP1.ATTR_RAW_CLAUSE); > ok(TIP1.flushPendingComposition(), > description + "TIP1.flushPendingComposition() should return true becuase it should be valid composition"); > is(input.value, composingStr, > description + "The input element should have composing string"); > > // Composing nsITextInputProcessor instance shouldn't allow initialize it again. > try { >- TIP1.init(window); >+ TIP1.init(window, simpleCallback); > ok(false, > "TIP1.init(window) should cause throwing an exception because it's composing with different purpose"); > } catch (e) { > ok(e.message.contains("NS_ERROR_ALREADY_INITIALIZED"), > description + "TIP1.init(window) should cause throwing an exception including NS_ERROR_ALREADY_INITIALIZED because it's composing for tests"); > } > try { > TIP1.initForTests(otherWindow); >@@ -119,36 +134,36 @@ function runInitMethodTests() > } catch (e) { > ok(e.message.contains("NS_ERROR_ALREADY_INITIALIZED"), > description + "TIP1.init(otherWindow) should cause throwing an exception including NS_ERROR_ALREADY_INITIALIZED because it's composing on this window"); > } > ok(TIP1.initForTests(window), > description + "TIP1.initForTests(window) should succeed because TextEventDispatcher was initialized with same purpose"); > ok(TIP1.initForTests(childWindow), > description + "TIP1.initForTests(childWindow) should succeed because TextEventDispatcher was initialized with same purpose and is shared by window and childWindow"); >- ok(!TIP2.init(window), >+ ok(!TIP2.init(window, simpleCallback), > description + "TIP2.init(window) should not succeed because there is composition synthesized by TIP1"); > ok(!TIP2.initForTests(window), > description + "TIP2.initForTests(window) should not succeed because there is composition synthesized by TIP1"); >- ok(!TIP2.init(childWindow), >+ ok(!TIP2.init(childWindow, simpleCallback), > description + "TIP2.init(childWindow) should not succeed because there is composition synthesized by TIP1"); > ok(!TIP2.initForTests(childWindow), > description + "TIP2.initForTests(childWindow) should not succeed because there is composition synthesized by TIP1"); >- ok(TIP2.init(otherWindow), >+ ok(TIP2.init(otherWindow, simpleCallback), > description + "TIP2.init(otherWindow) should succeed because there is composition synthesized by TIP1 but it's in other window"); > ok(TIP2.initForTests(otherWindow), > description + "TIP2.initForTests(otherWindow) should succeed because there is composition synthesized by TIP1 but it's in other window"); > > // Let's confirm that the composing string is NOT committed by above tests. > ok(TIP1.commitComposition(), > description + "TIP1.commitString() should succeed because there should be composing string"); > is(input.value, composingStr, > description + "TIP1.commitString() without specifying commit string should be committed with the last composing string"); > >- ok(TIP1.init(window), >+ ok(TIP1.init(window, simpleCallback), > description + "TIP1.init() should succeed because there is no composition #2"); > ok(TIP1.initForTests(window), > description + "TIP1.initForTests() should succeed because there is no composition #2"); > ok(TIP2.initForTests(window), > description + "TIP2.initForTests() should succeed because the composition was already committed #2"); > > // Let's check if startComposition() throws an exception after ownership is strolen. > input.value = ""; >@@ -198,16 +213,46 @@ function runInitMethodTests() > description + "TIP1.commitComposition(\"bar\") should cause throwing an exception because TIP2 took the ownership"); > } catch (e) { > ok(e.message.contains("NS_ERROR_NOT_INITIALIZED"), > description + "TIP1.commitComposition(\"bar\") should cause throwing an exception including NS_ERROR_NOT_INITIALIZED"); > } finally { > is(input.value, "", > description + "The input element should not have commit string"); > } >+ >+ // aCallback of nsITextInputProcessor.init() must not be omitted. >+ try { >+ TIP1.init(window); >+ ok(false, >+ description + "TIP1.init(window) should be failed since aCallback is omitted"); >+ } catch (e) { >+ ok(e.message.contains("Not enough arguments"), >+ description + "TIP1.init(window) should cause throwing an exception including \"Not enough arguments\" since aCallback is omitted"); >+ } >+ >+ // aCallback of nsITextInputProcessor.init() must not be undefined. >+ try { >+ TIP1.init(window, undefined); >+ ok(false, >+ description + "TIP1.init(window, undefined) should be failed since aCallback is undefined"); >+ } catch (e) { >+ ok(e.message.contains("NS_ERROR_ILLEGAL_VALUE"), >+ description + "TIP1.init(window, undefined) should cause throwing an exception including NS_ERROR_ILLEGAL_VALUE since aCallback is undefined"); >+ } >+ >+ // aCallback of nsITextInputProcessor.init() must not be null. >+ try { >+ TIP1.init(window, null); >+ ok(false, >+ description + "TIP1.init(window, null) should be failed since aCallback is null"); >+ } catch (e) { >+ ok(e.message.contains("NS_ERROR_ILLEGAL_VALUE"), >+ description + "TIP1.init(window, null) should cause throwing an exception including NS_ERROR_ILLEGAL_VALUE since aCallback is null"); >+ } > } > > function runCompositionTests() > { > var description = "runCompositionTests(): "; > > var TIP = createTIP(); > ok(TIP.initForTests(window), >@@ -848,24 +893,111 @@ function runUnloadTests2(aNextTest) > TIP.flushPendingComposition(); > is(textareaInFrame.value, "foo", > description + "the textarea in the iframe should have composition string"); > > // Load different web page on the frame. > iframe.src = "data:text/html,<body>dummy page</body>"; > } > >+function runCallbackTests(aForTests) >+{ >+ var description = "runCallbackTests(aForTests=" + aForTests + "): "; >+ >+ input.value = ""; >+ input.focus(); >+ input.blur(); >+ >+ var TIP = createTIP(); >+ var notifications = []; >+ function callback(aTIP, aNotification) >+ { >+ switch (aNotification.type) { >+ case "request-to-commit": >+ aTIP.commitComposition(); >+ break; >+ case "request-to-cancel": >+ aTIP.cancelComposition(); >+ break; >+ } >+ if (aTIP == TIP) { >+ notifications.push(aNotification); >+ } >+ return true; >+ } >+ >+ function dumpUnexpectedNotifications(aExpectedCount) >+ { >+ if (notifications.length <= aExpectedCount) { >+ return; >+ } >+ for (var i = aExpectedCount; i < notifications.length; i++) { >+ ok(false, >+ description + "Unexpected notification: " + notifications[i].type); >+ } >+ } >+ >+ if (aForTests) { >+ TIP.initForTests(window, callback); >+ } else { >+ TIP.init(window, callback); >+ } >+ >+ notifications = []; >+ input.focus(); >+ is(notifications.length, 1, >+ description + "input.focus() should cause a notification"); >+ is(notifications[0].type, "notify-focus", >+ description + "input.focus() should cause \"notify-focus\""); >+ dumpUnexpectedNotifications(1); >+ >+ notifications = []; >+ input.blur(); >+ is(notifications.length, 1, >+ description + "input.blur() should cause a notification"); >+ is(notifications[0].type, "notify-blur", >+ description + "input.blur() should cause \"notify-focus\""); >+ dumpUnexpectedNotifications(1); >+ >+ input.focus(); >+ TIP.setPendingCompositionString("foo"); >+ TIP.appendClauseToPendingComposition(3, TIP.ATTR_RAW_CLAUSE); >+ TIP.flushPendingComposition(); >+ notifications = []; >+ synthesizeMouseAtCenter(input, {}); >+ is(notifications.length, 1, >+ description + "synthesizeMouseAtCenter(input, {}) during composition should cause a notification"); >+ is(notifications[0].type, "request-to-commit", >+ description + "synthesizeMouseAtCenter(input, {}) during composition should cause \"request-to-commit\""); >+ dumpUnexpectedNotifications(1); >+ >+ notifications = []; >+ var TIP2 = createTIP(); >+ if (aForTests) { >+ TIP2.initForTests(window, callback); >+ } else { >+ TIP2.init(window, callback); >+ } >+ is(notifications.length, 1, >+ description + "Initializing another TIP should cause a notification"); >+ is(notifications[0].type, "notify-detached", >+ description + "Initializing another TIP should cause \"notify-detached\""); >+ dumpUnexpectedNotifications(1); >+} >+ > function runTests() > { > textareaInFrame = iframe.contentDocument.getElementById("textarea"); > > runInitMethodTests(); > runCompositionTests(); > runErrorTests(); > runCommitCompositionTests(); >+ runCallbackTests(false); >+ runCallbackTests(true); > runUnloadTests1(function () { > runUnloadTests2(function () { > finish(); > }); > }); > } > > ]]> >diff --git a/dom/inputmethod/forms.js b/dom/inputmethod/forms.js >--- a/dom/inputmethod/forms.js >+++ b/dom/inputmethod/forms.js >@@ -1214,24 +1214,41 @@ let CompositionManager = { > 'selected-raw-text': > Ci.nsITextInputProcessor.ATTR_SELECTED_RAW_CLAUSE, > 'converted-text': > Ci.nsITextInputProcessor.ATTR_CONVERTED_CLAUSE, > 'selected-converted-text': > Ci.nsITextInputProcessor.ATTR_SELECTED_CLAUSE > }, > >+ _callback: function cm_callback(aTIP, aNotification) >+ { >+ try { >+ switch (aNotification.type) { >+ case "request-to-commit": >+ aTIP.commitComposition(); >+ break; >+ case "request-to-cancel": >+ aTIP.cancelComposition(); >+ break; >+ } >+ } catch (e) { >+ return false; >+ } >+ return true; >+ }, >+ > _prepareTextInputProcessor: function cm_prepareTextInputProcessor(aWindow) > { > if (!this._textInputProcessor) { > this._textInputProcessor = > Cc["@mozilla.org/text-input-processor;1"]. > createInstance(Ci.nsITextInputProcessor); > } >- return this._textInputProcessor.init(aWindow); >+ return this._textInputProcessor.init(aWindow, this._callback); > }, > > setComposition: function cm_setComposition(element, text, cursor, clauses) { > // Check parameters. > if (!element) { > return; > } > let len = text.length; >diff --git a/dom/interfaces/base/moz.build b/dom/interfaces/base/moz.build >--- a/dom/interfaces/base/moz.build >+++ b/dom/interfaces/base/moz.build >@@ -31,12 +31,13 @@ XPIDL_SOURCES += [ > 'nsIIdleObserver.idl', > 'nsIQueryContentEventResult.idl', > 'nsIRemoteBrowser.idl', > 'nsIServiceWorkerManager.idl', > 'nsIStructuredCloneContainer.idl', > 'nsITabChild.idl', > 'nsITabParent.idl', > 'nsITextInputProcessor.idl', >+ 'nsITextInputProcessorCallback.idl', > ] > > XPIDL_MODULE = 'dom_base' > >diff --git a/dom/interfaces/base/nsITextInputProcessor.idl b/dom/interfaces/base/nsITextInputProcessor.idl >--- a/dom/interfaces/base/nsITextInputProcessor.idl >+++ b/dom/interfaces/base/nsITextInputProcessor.idl >@@ -1,30 +1,34 @@ > /* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > /* 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 "nsISupports.idl" > > interface nsIDOMWindow; >+interface nsITextInputProcessorCallback; > > /** > * An nsITextInputProcessor instance is associated with a top level widget which > * handles native IME. It's associated by calling init() or initForTests(). > * While an instance has composition, nobody can steal the rights to make > * composition on the top level widget. In other words, if another instance is > * composing on a top level widget, either init() or initForTests() returns > * false (i.e., not throws an exception). > * >+ * NOTE: See nsITextInputProcessorCallback.idl for examples of |callback| in >+ * following examples, >+ * > * Example #1 JS-IME can start composition like this: > * > * var TIP = Components.classes["@mozilla.org/text-input-processor;1"]. > * createInstance(Components.interfaces.nsITextInputProcessor); >- * if (!TIP.init(window)) { >+ * if (!TIP.init(window, callback)) { > * return; // You failed to get the rights to make composition > * } > * // Set new composition string first > * TIP.setPendingCompositionString("some-words-are-inputted"); > * // Set clause information. > * TIP.appendClauseToPendingComposition(23, TIP.ATTR_RAW_CLAUSE); > * // Set caret position, this is optional. > * TIP.setCaretInPendingComposition(23); >@@ -75,58 +79,66 @@ interface nsIDOMWindow; > * TIP.appendClauseToPendingComposition(27, TIP.ATTR_RAW_CLAUSE); > * TIP.flushPendingComposition(); > * // This is useful when user doesn't want to commit the composition. > * // FYI: This is same as TIP.commitComposition("") for now. > * TIP.cancelComposition(); > * > * Example #6 JS-IME can insert text only with commitComposition(): > * >- * if (!TIP.init(window)) { >+ * if (!TIP.init(window, callback)) { > * return; // You failed to get the rights to make composition > * } > * TIP.commitComposition("Some words"); > * > * Example #7 JS-IME can start composition explicitly: > * >- * if (!TIP.init(window)) { >+ * if (!TIP.init(window, callback)) { > * return; // You failed to get the rights to make composition > * } > * // If JS-IME don't want to show composing string in the focused editor, > * // JS-IME can dispatch only compositionstart event with this. > * if (!TIP.startComposition()) { > * // Failed to start composition. > * return; > * } > * // And when user selects a result from UI of JS-IME, commit with it. > * TIP.commitComposition("selected-words"); > */ > >-[scriptable, builtinclass, uuid(c9daacc8-17a1-4979-85fa-918c9c0ccb30)] >+[scriptable, builtinclass, uuid(8c20753c-8339-4e9c-86c5-ae30f1b456c3)] > interface nsITextInputProcessor : nsISupports > { > /** > * When you create an instance, you must call init() first except when you > * created the instance for automated tests. > * > * @param aWindow A DOM window. The instance will look for a top > * level widget from this. >+ * @param aCallback Callback interface which handles requests to >+ * IME and notifications to IME. This must not be >+ * null. > * @return If somebody uses internal text input service for a > * composition, this returns false. Otherwise, returns > * true. I.e., only your TIP can create composition > * when this returns true. If this returns false, > * your TIP should wait next chance. > */ >- boolean init(in nsIDOMWindow aWindow); >+ boolean init(in nsIDOMWindow aWindow, >+ in nsITextInputProcessorCallback aCallback); > > /** > * When you create an instance for automated test, you must call > * initForTest(), first. See init() for more detail of this. >+ * Note that aCallback can be null. If it's null, nsITextInputProcessor >+ * implementation will handle them automatically. > */ >- boolean initForTests(in nsIDOMWindow aWindow); >+ [optional_argc] boolean >+ initForTests(in nsIDOMWindow aWindow, >+ [optional] in nsITextInputProcessorCallback aCallback); > > /** > * startComposition() dispatches compositionstart event explicitly. > * IME does NOT need to call this typically since compositionstart event > * is automatically dispatched by sendPendingComposition() if > * compositionstart event hasn't been dispatched yet. If this is called > * when compositionstart has already been dispatched, this throws an > * exception. >diff --git a/dom/interfaces/base/nsITextInputProcessorCallback.idl b/dom/interfaces/base/nsITextInputProcessorCallback.idl >new file mode 100644 >--- /dev/null >+++ b/dom/interfaces/base/nsITextInputProcessorCallback.idl >@@ -0,0 +1,102 @@ >+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* 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 "nsISupports.idl" >+ >+interface nsITextInputProcessor; >+ >+/** >+ * nsITextInputProcessorNotification stores the type of notification to IME and >+ * its detail. See each explanation of attribute for the detail. >+ */ >+ >+[scriptable, builtinclass, uuid(c0ce1add-82bb-45ab-b99a-42cfba7fd5d7)] >+interface nsITextInputProcessorNotification : nsISupports >+{ >+ /** >+ * type attribute represents what's notified or requested. Value must be >+ * one of following values: >+ * >+ * "request-to-commit" (required to be handled) >+ * This is requested when Gecko believes that active composition should be >+ * committed. nsITextInputProcessorCallback::onNotify() has to handle this >+ * notification. >+ * >+ * "request-to-cancel" (required to be handled) >+ * This is requested when Gecko believes that active composition should be >+ * canceled. I.e., composition should be committed with empty string. >+ * nsITextInputProcessorCallback::onNotify() has to handle this >+ * notification. >+ * >+ * "notify-detached" (optional) >+ * This is notified when the callback is detached from >+ * nsITextInputProcessor. >+ * >+ * "notify-focus" (optional) >+ * This is notified when an editable editor gets focus and Gecko starts >+ * to observe changes in the content. E.g., selection changes. >+ * IME shouldn't change DOM tree, focus nor something when this is notified. >+ * >+ * "notify-blur" (optional) >+ * This is notified when an editable editor loses focus and Gecko stops >+ * observing the changes in the content. >+ */ >+ readonly attribute ACString type; >+}; >+ >+/** >+ * nsITextInputProcessorCallback is a callback interface for JS to implement >+ * IME. IME implemented by JS can implement onNotify() function and must send >+ * it to nsITextInputProcessor at initializing. Then, onNotify() will be >+ * called with nsITextInputProcessorNotification instance. >+ * The reason why onNotify() uses string simply is that if we will support >+ * other notifications such as text changes and selection changes, we need to >+ * notify IME of some other information. Then, only changing >+ * nsITextInputProcessorNotification interface is better for compatibility. >+ */ >+ >+[scriptable, function, uuid(23d5f242-adb5-46f1-8766-90d1bf0383df)] >+interface nsITextInputProcessorCallback : nsISupports >+{ >+ /** >+ * When Gecko notifies IME of something or requests something to IME, >+ * this is called. >+ * >+ * @param aTextInputProcessor Reference to the nsITextInputProcessor service >+ * which is the original receiver of the request >+ * or notification. >+ * @param aNotification Stores type of notifications and additional >+ * information. >+ * @return Return true if it succeeded or does nothing. >+ * Otherwise, return false. >+ * >+ * Example #1 The simplest implementation of nsITextInputProcessorCallback is: >+ * >+ * function simpleCallback(aTIP, aNotification) >+ * { >+ * try { >+ * switch (aNotification.type) { >+ * case "request-to-commit": >+ * aTIP.commitComposition(); >+ * break; >+ * case "request-to-cancel": >+ * aTIP.cancelComposition(); >+ * break; >+ * } >+ * } catch (e) { >+ * return false; >+ * } >+ * return true; >+ * } >+ * >+ * var TIP = Components.classes["@mozilla.org/text-input-processor;1"]. >+ * createInstance(Components.interfaces.nsITextInputProcessor); >+ * if (!TIP.init(window, simpleCallback)) { >+ * return; >+ * } >+ */ >+ boolean onNotify(in nsITextInputProcessor aTextInputProcessor, >+ in nsITextInputProcessorNotification aNotification); >+}; >diff --git a/widget/nsBaseWidget.cpp b/widget/nsBaseWidget.cpp >--- a/widget/nsBaseWidget.cpp >+++ b/widget/nsBaseWidget.cpp >@@ -1594,18 +1594,29 @@ nsBaseWidget::NotifyIME(const IMENotific > case REQUEST_TO_CANCEL_COMPOSITION: > // Currently, if native IME handler doesn't use TextEventDispatcher, > // the request may be notified to mTextEventDispatcher or native IME > // directly. Therefore, if mTextEventDispatcher has a composition, > // the request should be handled by the mTextEventDispatcher. > if (mTextEventDispatcher && mTextEventDispatcher->IsComposing()) { > return mTextEventDispatcher->NotifyIME(aIMENotification); > } >- // Otherwise, call NotifyIMEInternal() for native IME handlers. >+ // Otherwise, it should be handled by native IME. >+ return NotifyIMEInternal(aIMENotification); >+ case NOTIFY_IME_OF_FOCUS: >+ case NOTIFY_IME_OF_BLUR: >+ // If the notification is a notification which is supported by >+ // nsITextInputProcessorCallback, we should notify the >+ // TextEventDispatcher, first. After that, notify native IME too. >+ if (mTextEventDispatcher) { >+ mTextEventDispatcher->NotifyIME(aIMENotification); >+ } >+ return NotifyIMEInternal(aIMENotification); > default: >+ // Otherwise, notify only native IME for now. > return NotifyIMEInternal(aIMENotification); > } > } > > NS_IMETHODIMP_(nsIWidget::TextEventDispatcher*) > nsBaseWidget::GetTextEventDispatcher() > { > if (!mTextEventDispatcher) {

Comment on attachment 8551255 [details] [diff] [review] part.19 Add nsITextInputProcessorCallback > TextInputProcessor::UnlinkFromTextEventDispatcher() > { > mDispatcher = nullptr; > mForTests = false; >+ if (mCallback) { >+ nsCOMPtr<nsITextInputProcessorCallback> callback(mCallback); >+ mCallback = nullptr; >+ >+ nsRefPtr<TextInputProcessorNotification> notification = >+ new TextInputProcessorNotification("notify-detached"); >+ bool result = false; >+ callback->OnNotify(this, notification, &result); >+ NS_ASSERTION(result, >+ "nsITextInputProcesserCallback::OnNotify() should return true"); Why this assertion. Per the API documentation the callback may return false. So, drop the NS_ASSERTION, or change it to NS_WARN_IF_FALSE >@@ -289,16 +349,53 @@ TextInputProcessor::NotifyIME(TextEventD > const IMENotification& aNotification) ... >+ NS_ASSERTION(result, >+ "nsITextInputProcesserCallback::OnNotify() should return true"); ditto (hopefully I did proper interdiff)

(In reply to Olli Pettay [:smaug] from comment #124) > Comment on attachment 8551255 [details] [diff] [review] > part.19 Add nsITextInputProcessorCallback > > > TextInputProcessor::UnlinkFromTextEventDispatcher() > > { > > mDispatcher = nullptr; > > mForTests = false; > >+ if (mCallback) { > >+ nsCOMPtr<nsITextInputProcessorCallback> callback(mCallback); > >+ mCallback = nullptr; > >+ > >+ nsRefPtr<TextInputProcessorNotification> notification = > >+ new TextInputProcessorNotification("notify-detached"); > >+ bool result = false; > >+ callback->OnNotify(this, notification, &result); > >+ NS_ASSERTION(result, > >+ "nsITextInputProcesserCallback::OnNotify() should return true"); > Why this assertion. Per the API documentation the callback may return false. > So, drop the NS_ASSERTION, or change it to NS_WARN_IF_FALSE Oh, right.

This is really helpful change for forms.js. This patch notifies forms.js of requests to IME to commit/cancel composition and focusing and losing focus of IME. This patch does almost NOT change the behavior of forms.js. However, in other bugs, we can fix a lot of bugs which are caused by not listening the notification from Gecko. (I guess that forms.js should notify JS-IME (keyboard) of the requests. Then, JS-IME or keyboard can forget active composition.

Comment on attachment 8551335 [details] [diff] [review] All changes of following patches (v3.0) Please re-trigger the tests on try for few times before landing.

Bug 1115616 added a test unfortunately. We need to make it new API aware.

FYI: "compositionupdate" has already done nothing. So, it shouldn't be added in the test.

Could you review the patch? If you wouldn't review it in a couple of days, I'd land them without your review since the API change is important and blocks next keyboard API change bug. Even if you didn't like the change in forms.js, I'd fix it in another bug since we're in very early stage of 38 cycle.

Comment on attachment 8551339 [details] [diff] [review] part.19 Add nsITextInputProcessorCallback (r=smaug, sr=smaug) Review of attachment 8551339 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for late reply. I filtered the bugzilla mail mistakenly and just noticed this. The change to forms.js is reasonable and we may need futher work to forward these notification to gaia keyboard apps. So I will set forms.js as r+ed. I'm not the right person to review other C++ and idl codes. This is my first time to touch these code. It may take a few days for me to understand how it works. If you don't need me to review these code, you are happy to go on:-) ::: dom/base/TextInputProcessor.cpp @@ +19,5 @@ > + * TextInputProcessorNotification > + ******************************************************************************/ > + > +class TextInputProcessorNotification MOZ_FINAL : > + public nsITextInputProcessorNotification nit: indentation is strange.

(In reply to Yuan Xulei [:yxl] from comment #132) > Sorry for late reply. I filtered the bugzilla mail mistakenly and just > noticed this. No problem, me too, sometimes ;-) > The change to forms.js is reasonable and we may need futher work to forward > these notification to gaia keyboard apps. > So I will set forms.js as r+ed. Thanks! > I'm not the right person to review other C++ and idl codes. This is my first > time to touch these code. It may take a few days for me to understand how it > works. If you don't need me to review these code, you are happy to go on:-) Ah, I requested only about the forms.js part. The other part is already reviewed by smaug. I think that handling the commit/cancel requests from Gecko fixes a lot of existing bugs of Firefox OS. If you or somebody will check them, I'll be happy!

Fujisawa-san: You added an IME tests into browser_aboutHome.js. http://hg.mozilla.org/mozilla-central/rev/b683c06bf809 At this time, you don't specify content's window to the second argument of EventUtils.synthesizeComposition() and EventUtils.synthesizeCompositionChange(). With the patches in this bug, EventUtils caches TextInputProcessor instance to the specified window. Therefore, your code is detected as memory leak. For fixing it, I added gBrowser.contentWindow to them. However, it causes this error: > 191 INFO TEST-PASS | browser/base/content/test/general/browser_aboutHome.js | Search suggestion table unhidden > 192 INFO TEST-PASS | browser/base/content/test/general/browser_aboutHome.js | Search suggestion table hidden > 193 INFO Cleanup > 194 INFO Clicking suggestion list while composing > 195 INFO Waiting for snippets map > 196 INFO Wait tab event: AboutHomeLoadSnippetsCompleted > 197 INFO Console message: [JavaScript Warning: "An IndexedDB transaction that was not yet complete has been aborted due to page navigation." {file: "chrome://browser/content/abouthome/aboutHome.js" line: 284}] > 198 INFO Console message: [JavaScript Warning: "An IndexedDB transaction that was not yet complete has been aborted due to page navigation." {file: "chrome://browser/content/abouthome/aboutHome.js" line: 278}] > 199 INFO Console message: [JavaScript Warning: "An IndexedDB transaction that was not yet complete has been aborted due to page navigation." {file: "chrome://browser/content/abouthome/aboutHome.js" line: 278}] > 200 INFO Got snippets map: { last-update: 1422344740506, cached-version: 4 } > 201 INFO Tab event received: AboutHomeLoadSnippetsCompleted > 202 INFO Running test > ************************* > A coding exception was thrown in a Promise rejection callback. > See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise > Full message: TypeError: "stack" is read-only > Full stack: TaskImpl_handleException@resource://gre/modules/Task.jsm:422:9 > TaskImpl_run@resource://gre/modules/Task.jsm:342:11 > Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:873:21 > this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7 > this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37 > ************************* > 203 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_aboutHome.js | A promise chain failed to handle a rejection: - at resource://gre/modules/Task.jsm:422 - TypeError: "stack" is read-only > Stack trace: > TaskImpl_handleException@resource://gre/modules/Task.jsm:422:9 > TaskImpl_run@resource://gre/modules/Task.jsm:342:11 > Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:873:21 > this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7 > this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37 Do you have any idea about this? If I specified gBrowser.contentWindow without my patches, the error doesn't occur, so, I'm really in trouble for landing this... If you don't have any idea about this issue, I'll land with this hacky patch. FYI: You don't need to synthesize "compositionupdate". Even with new API, it doesn't cause error for backward compatibility. But it's already obsolete. compositionchange will dispatch compositionupdate only when it's really necessary.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #134) > > 203 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_aboutHome.js | A promise chain failed to handle a rejection: - at resource://gre/modules/Task.jsm:422 - TypeError: "stack" is read-only > > Stack trace: > > TaskImpl_handleException@resource://gre/modules/Task.jsm:422:9 > > TaskImpl_run@resource://gre/modules/Task.jsm:342:11 > > Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:873:21 > > this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7 > > this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37 > > Do you have any idea about this? If I specified gBrowser.contentWindow > without my patches, the error doesn't occur, so, I'm really in trouble for > landing this... That's thrown from exception handling code in Task.jsm, and the original exception was thrown here: https://hg.mozilla.org/try/file/7b74c24d60c2/testing/mochitest/tests/SimpleTest/EventUtils.js#l883 > aWindow._EU_TIP = > _EU_Cc["@mozilla.org/text-input-processor;1"]. > createInstance(_EU_Ci.nsITextInputProcessor); The exception is "Permission denied for <moz-safe-about:home> to create wrapper for object of class UnnamedClass". How about using WeakMap instead of storing it into window's property? This seems to work for me (not yet tested on try server though). > FYI: You don't need to synthesize "compositionupdate". Even with new API, it > doesn't cause error for backward compatibility. But it's already obsolete. > compositionchange will dispatch compositionupdate only when it's really > necessary. Thank you for letting me know that :)

Comment on attachment 8555077 [details] [diff] [review] part.22 Fix new orange which was caused by bug 1115616 (r=smaug) Review of attachment 8555077 [details] [diff] [review]: ----------------------------------------------------------------- If the patch in comment #135 is acceptable, this hack seems not to be required, and passing `gBrowser.contentWindow` will work (tested with https://hg.mozilla.org/try/rev/7b74c24d60c2 ). If not, and there is no other way, I agree with this patch (with minor change in the comment about the exception, in browser_aboutHome.js). Anyway, thank you for fixing the test :D

Thank you, Fujisawa-san! Your patch works with this. I posted to trysever: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2942e8d590cc Could you request the review to smaug? Or can I request the request instead of you after confirming the result? (In reply to Tooru Fujisawa [:arai] from comment #135) > > FYI: You don't need to synthesize "compositionupdate". Even with new API, it > > doesn't cause error for backward compatibility. But it's already obsolete. > > compositionchange will dispatch compositionupdate only when it's really > > necessary. > > Thank you for letting me know that :) I usually update IME handling document, please check this sometimes. https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide # I should've documented about testing API here too. I'll do that after landing the patches.

Oops, this is better (specifying the content window). https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5e9d221989f2

(updated commit message only) To support content window as a event target, changed to store cached nsITextInputProcessor instance into WeakMap.

I guess that using WeakMap causes canceling composition if somebody uses SpecialPower.GC(). However, I guess this doesn't cause actual problem for EventUtils because it's a utilities only for automated tests.

Comment on attachment 8555158 [details] [diff] [review] part.23 Store nsITextInputProcessor instance into WeakMap instead of window Oh, wait. This patch causes a lot of oranges: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5e9d221989f2 Perhaps, should use "var" rather than "let" for mochitest-plain?

Perhaps, I'm right. A mochitest-plain test passes on my machine with replacing "let" to "var". Let's see the result on tryserver: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84e643f29560

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #141) > Comment on attachment 8555158 [details] [diff] [review] > part.23 Store nsITextInputProcessor instance into WeakMap instead of window > > Oh, wait. This patch causes a lot of oranges: > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5e9d221989f2 > > Perhaps, should use "var" rather than "let" for mochitest-plain? Thanks, I totally forgot about that.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #134) > > 200 INFO Got snippets map: { last-update: 1422344740506, cached-version: 4 } > > 201 INFO Tab event received: AboutHomeLoadSnippetsCompleted > > 202 INFO Running test > > ************************* > > A coding exception was thrown in a Promise rejection callback. > > See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise > > Full message: TypeError: "stack" is read-only > > Full stack: TaskImpl_handleException@resource://gre/modules/Task.jsm:422:9 > > TaskImpl_run@resource://gre/modules/Task.jsm:342:11 > > Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:873:21 > > this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7 > > this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37 > > ************************* > > 203 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_aboutHome.js | A promise chain failed to handle a rejection: - at resource://gre/modules/Task.jsm:422 - TypeError: "stack" is read-only > > Stack trace: > > TaskImpl_handleException@resource://gre/modules/Task.jsm:422:9 > > TaskImpl_run@resource://gre/modules/Task.jsm:342:11 > > Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:873:21 > > this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7 > > this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37 Paolo or Yoric, do you know what's going on with the Promise/Task exception handling code here? Can we avoid its own exceptions replacing the exceptions it's trying to give us a better stack for?

(In reply to :Gijs Kruitbosch from comment #144) > Paolo or Yoric, do you know what's going on with the Promise/Task exception > handling code here? Can we avoid its own exceptions replacing the exceptions > it's trying to give us a better stack for? We definitely fail when we cannot rewrite the "stack" property: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Task.jsm#422 It's probably worth filing a bug to put the whole block inside try-catch, although when bug 1083359 is done we'll be able to avoid rewriting completely.

Okay, looks fine this patch on tryserver. Let's take this. Thank you, Fujisawa-san! (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #140) > I guess that using WeakMap causes canceling composition if somebody uses > SpecialPower.GC(). However, I guess this doesn't cause actual problem for > EventUtils because it's a utilities only for automated tests. Ah, I guess I misunderstood. WeakMap's key is weak reference and the value is strong reference, right? Then, it shouldn't have any problems.

Thank you for testing! (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #146) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #140) > > I guess that using WeakMap causes canceling composition if somebody uses > > SpecialPower.GC(). However, I guess this doesn't cause actual problem for > > EventUtils because it's a utilities only for automated tests. > > Ah, I guess I misunderstood. WeakMap's key is weak reference and the value > is strong reference, right? Then, it shouldn't have any problems. I think so. http://people.mozilla.org/~jorendorff/es6-draft.html#sec-weakmap-objects > If an object that is being used as the key of a WeakMap key/value pair is > only reachable by following a chain of references that start within that > WeakMap, then that key/value pair is inaccessible and is automatically > removed from the WeakMap.

Comment on attachment 8555252 [details] [diff] [review] part.23 Store nsITextInputProcessor instance into WeakMap instead of window It is not clear to me why this is needed.

Don't we just want disconnect() or some such in nsITextInputProcessor and that would clear all the member variables in the implementation and mark it as uninitialized. (that is what I suggested in comment 115)

(In reply to Olli Pettay [:smaug] from comment #148) > Comment on attachment 8555252 [details] [diff] [review] > part.23 Store nsITextInputProcessor instance into WeakMap instead of window > > It is not clear to me why this is needed. I'm not sure the actual reason of that. When I tested on tryserver before landing the patches, browser_aboutHome.js becomes orange. The part of the test was added for bug 1115616 too. I tried to fix the orange, however, strangely, specifying content window to the EventUtils.synthesizeComposition*() causes failure or memory leak detector. I know that it must be a bug of the test or anothor module which is included by the test. However, I'm in hurry to land this bug for fixing next keyboard event API refactoring since it's necessary for MoCo's demonstration (I'm not sure if I can say it clearly here). So, this patch allows us to land the patches because of avoiding the orange, therefore, I want this patch.

(In reply to Olli Pettay [:smaug] from comment #149) > Don't we just want disconnect() or some such in nsITextInputProcessor and > that would > clear all the member variables in the implementation and mark it as > uninitialized. > (that is what I suggested in comment 115) Hmm, I don't think so. In some cases, yes, it could be useful for some addon developers. However, if addon allows to be stolen the rights of creating composition by another one, it should commit or cancel its composition. It's enough. So, I feel that like .disconnect() is redundant method for developers. Although, it might be useful for such internal reason. Another options is that browser_aboutHome.js should create and magate TextInputProcessor itself. However, I don't like such unusable utility class (i.e., current EventUtils). So, I think that taking the Fujisawa-san's patch (part.23) is the best.

Comment on attachment 8555252 [details] [diff] [review] part.23 Store nsITextInputProcessor instance into WeakMap instead of window Fine, though we should have better API for this. This feels very much like a hack. (but luckily in testing framework, and not in implementation)

Thanks! I'll try to land them tomorrow morning. (AM 2:40 in JST now...)

https://hg.mozilla.org/integration/mozilla-inbound/rev/87c8054d9040 https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c660bbe955 https://hg.mozilla.org/integration/mozilla-inbound/rev/a3c066110813 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b196aeab6de https://hg.mozilla.org/integration/mozilla-inbound/rev/c7fac3ab77af https://hg.mozilla.org/integration/mozilla-inbound/rev/80f142519733 https://hg.mozilla.org/integration/mozilla-inbound/rev/994fee1e166c https://hg.mozilla.org/integration/mozilla-inbound/rev/a80685d0ff6f https://hg.mozilla.org/integration/mozilla-inbound/rev/0458d8e6291e https://hg.mozilla.org/integration/mozilla-inbound/rev/f341e1c47a1a https://hg.mozilla.org/integration/mozilla-inbound/rev/49cd2b26b85d https://hg.mozilla.org/integration/mozilla-inbound/rev/981bc929a82b https://hg.mozilla.org/integration/mozilla-inbound/rev/42fdbe500627 https://hg.mozilla.org/integration/mozilla-inbound/rev/72718ad7b573 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea72759272e https://hg.mozilla.org/integration/mozilla-inbound/rev/19e2530a986d https://hg.mozilla.org/integration/mozilla-inbound/rev/d286a2acd55b https://hg.mozilla.org/integration/mozilla-inbound/rev/b97c59579393 https://hg.mozilla.org/integration/mozilla-inbound/rev/62716b199145 https://hg.mozilla.org/integration/mozilla-inbound/rev/6418c75f250b https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6ed2370717 https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d2f8341db4 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d063686d243 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d9e240b48fd2

https://hg.mozilla.org/mozilla-central/rev/87c8054d9040 https://hg.mozilla.org/mozilla-central/rev/d7c660bbe955 https://hg.mozilla.org/mozilla-central/rev/a3c066110813 https://hg.mozilla.org/mozilla-central/rev/7b196aeab6de https://hg.mozilla.org/mozilla-central/rev/c7fac3ab77af https://hg.mozilla.org/mozilla-central/rev/80f142519733 https://hg.mozilla.org/mozilla-central/rev/994fee1e166c https://hg.mozilla.org/mozilla-central/rev/a80685d0ff6f https://hg.mozilla.org/mozilla-central/rev/0458d8e6291e https://hg.mozilla.org/mozilla-central/rev/f341e1c47a1a https://hg.mozilla.org/mozilla-central/rev/49cd2b26b85d https://hg.mozilla.org/mozilla-central/rev/981bc929a82b https://hg.mozilla.org/mozilla-central/rev/42fdbe500627 https://hg.mozilla.org/mozilla-central/rev/72718ad7b573 https://hg.mozilla.org/mozilla-central/rev/3ea72759272e https://hg.mozilla.org/mozilla-central/rev/19e2530a986d https://hg.mozilla.org/mozilla-central/rev/d286a2acd55b https://hg.mozilla.org/mozilla-central/rev/b97c59579393 https://hg.mozilla.org/mozilla-central/rev/62716b199145 https://hg.mozilla.org/mozilla-central/rev/6418c75f250b https://hg.mozilla.org/mozilla-central/rev/9b6ed2370717 https://hg.mozilla.org/mozilla-central/rev/b5d2f8341db4 https://hg.mozilla.org/mozilla-central/rev/8d063686d243