Closed Bug 528396 Opened 10 years ago Closed 10 years ago

Create XP level IME transaction tests

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: inputmethod)

Attachments

(3 files, 8 obsolete files)

12.31 KB, patch
roc
: review+
Details | Diff | Splinter Review
95.25 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.02 KB, patch
roc
: review+
Details | Diff | Splinter Review
We should create simple automated tests for composition transaction.

I'll create nsIDOMWindowUtils::sendIMEEvent method, and using it, we can test the XP code's IME processing.
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #414945 - Attachment is obsolete: true
Attached patch Patch v1.2 (obsolete) — Splinter Review
This patch tests following events:
* NS_TEXT_TEXT
* NS_COMPOSITION_START
* NS_COMPOSTION_END
* NS_QUERY_TEXT_CONTENT
* NS_QUERY_SELECTED_TEXT
* NS_QUERY_CARET_RECT
* NS_QUERY_TEXT_RECT
* NS_QUERY_EDITOR_RECT
* NS_QUERY_CHARACTER_AT_POINT

First of all, the test file tests a typical composition case of Japanese language. Next, it tests simple composition on 3 cases, on the root document's textarea case, on the iframe's textarea case and on the XUL panel's textbox case. Finally, it tests NS_QUERY_CHARACTER_AT_POINT events on the cases.

When I create this testcases, I found bug 531591. This patch includes the fix of bug 531591 the changes are needed for this tests. That is a bug of nsContentEventHandler::OnQueryCharacterAtPoint(). It gets the root frame which is ancestor of the focused editor by the nsIPresShell:GetRootFrame(). However, it returns the document's root frame even if the widget of the event and the refPoint means a XUL panel's content. So, it needs to check whether the point is on the XUL panel or not.
Attachment #414947 - Attachment is obsolete: true
Attachment #414959 - Flags: review?(roc)
Can you change the Japanese characters in widget/tests/test_composition_text_querycontent.xul into Unicode escapes? I'm worried about people's editors mangling them.

It would be nice if we could avoid adding more and more stuff to nsIDOMWindowUtils for these events. One possible alternative would be to add methods to the test plugin to fire native messages at the browser window. This would let you test our native message handling as well. Can you try that?
(In reply to comment #4)
> It would be nice if we could avoid adding more and more stuff to
> nsIDOMWindowUtils for these events. One possible alternative would be to add
> methods to the test plugin to fire native messages at the browser window. This
> would let you test our native message handling as well. Can you try that?

I'm not sure what you image. Would you explain the idea?

I'd like to separate the native message processing tests to other bug because I'll need to create such tests on each platform, so, not on XP level. And I'd like to test only whether the query content events, composition events and text event work fine or not.
So, if you don't like to add the methods to nsIDOMWindowUtils, how about this approach?

* Adding "dispatchNativeEvent" method to the nsIDOMWindowUtils.
* It takes "nsINativeEvent" interface.
* "nsINativeEvent" interface can have various native event initializers, e.g., "initAsCompositionEvent", "initAsTextEvent" and "initAsQueryTextContentEvent".
* "dispatchNativeEvent" sets the event result to the "nsINativeEvent" instance, js can access the result via other interfaces ("nsIQueryContentEventResult").
Adding nsINativeEvent still adds a lot of API to code that we ship. I'd prefer to avoid that.

The test plugin (modules/plugin/test/testplugin) exposes scriptable APIs that tests can call. It's pretty easy to add new APIs to nptest.cpp. So the idea would be to add APIs to the test plugin that fire various kinds of IME events. The test plugin has platform-specific components where you can implement those APIs to fire native events at the browser window.

The good thing about the test plugin is that it's available to tests but we don't ship it, so we don't need to worry about adding code footprint or possible bugs to our shipping product. We also don't need to worry about extension authors finding the API and using it.
(In reply to comment #7)
> The test plugin (modules/plugin/test/testplugin) exposes scriptable APIs that
> tests can call. It's pretty easy to add new APIs to nptest.cpp. So the idea
> would be to add APIs to the test plugin that fire various kinds of IME events.
> The test plugin has platform-specific components where you can implement those
> APIs to fire native events at the browser window.

Looks like the test plugin cannot use XPCOM, I don't want to use platform's native events, I just want to test after Gecko dispatches the non-DOM events in this bug. And also the tests need to convert the coordinates between two widgets.

> The good thing about the test plugin is that it's available to tests but we
> don't ship it, so we don't need to worry about adding code footprint or
> possible bugs to our shipping product. We also don't need to worry about
> extension authors finding the API and using it.

I agree. I wonder why we don't have |MOZ_ENABLE_TEST| or something.
(In reply to comment #8)
> (In reply to comment #7)
> > The test plugin (modules/plugin/test/testplugin) exposes scriptable APIs that
> > tests can call. It's pretty easy to add new APIs to nptest.cpp. So the idea
> > would be to add APIs to the test plugin that fire various kinds of IME events.
> > The test plugin has platform-specific components where you can implement those
> > APIs to fire native events at the browser window.
> 
> Looks like the test plugin cannot use XPCOM, I don't want to use platform's
> native events, I just want to test after Gecko dispatches the non-DOM events in
> this bug. And also the tests need to convert the coordinates between two
> widgets.

What's wrong with using the platform's native events?
(In reply to comment #9)
> What's wrong with using the platform's native events?

I just want to test the IME related code of XP part in this bug. The native event handling tests should be separated to another bug because it needs more difficult works (e.g., Win32's IMM32 code need to access to the IME via Win32 APIs, we need to create an API hook system which was commented in bug 460056 comment 2).

And I want to fix bug 519913, but for the automated tests of the bug's patch, I need to fix this bug first.
OK. What if you extend the approach used by TestTSF.cpp?
(In reply to comment #11)
> OK. What if you extend the approach used by TestTSF.cpp?

Sorry, what do you mean? What is "the approach"?

# ideally, I want to remove the testing code from TestTSF.cpp because C++ code isn't useful for the tests on complex documents.
Attached patch Patch v1.3 (obsolete) — Splinter Review
fix two bugs from the previous patch.

1. nsContentEventHandler::OnQueryCharacterAtPoint() now refers the trusted flag of the given event when it creates an dummy event.
2. nsContentEventHandler::OnQueryCharacterAtPoint() gets the root frame of the widget of aEvent directly.

And escaped the all Japanese characters in the test.
Attachment #414959 - Attachment is obsolete: true
Attachment #415380 - Flags: review?(roc)
Attachment #414959 - Flags: review?(roc)
I'm not sure what to do here. I really don't want to add and ship a lot of new API just for these tests.
(In reply to comment #15)
> I'm not sure what to do here. I really don't want to add and ship a lot of new
> API just for these tests.

Looks like we enable tests on release build too, so, we cannot exclude the new APIs from it...

I really need to fix this bug ASAP because the APIs are needed by the new tests in bug 519913.

If I combine sendQuerySelectedTextEvent, sendQueryTextContentEvent, sendQueryCaretRectEvent, sendQueryTextRectEvent, sendQueryEditorRectEvent, sendCharacterAtPointEvent and sendSelectionSetEvent (i.e., the count of the new APIs are 3), can you agree?
Going back to comment #12, what is actually wrong with TestTSF? I realize it's more difficult to write tests there, but is there any other problem?
Basically, no. But it's important that a test can be written easily, I think. We can write complex/many tests with low cost.

And TestTSF cannot test on non-Windows, it might be wrong. Actually, the current patch's tests passed only on Windows first, I needed to fix some bugs in the new APIs for Mac and Linux. So, it's may be good thing that the tests can run on all platforms.
I'll post to dev.platform about this.
OK, we need to make some progress here.

+#define ENSURE_UNIVERSALXPCONNECT_CAPABLE() \
+  PR_BEGIN_MACRO \
+    PRBool hasCap = PR_FALSE; \
+    if (NS_FAILED(nsContentUtils::GetSecurityManager()-> \
+                  IsCapabilityEnabled("UniversalXPConnect", &hasCap)) || \
+        !hasCap) { \
+      return NS_ERROR_DOM_SECURITY_ERR; \
+    } \
+  PR_END_MACRO

Make it a static function, and split out the patch that refactors nsDOMWindowUtils.

Can we combine all the nsDOMWindowUtils methods that return nsIQueryContentEventResult into a single method that takes a type parameter and some optional parameters? Seems like that might simplify the code.
Attached patch Patch v2.0 (obsolete) — Splinter Review
Attachment #415380 - Attachment is obsolete: true
Attachment #429799 - Flags: review?(roc)
Attached patch Patch v2.0 (obsolete) — Splinter Review
oops, sorry, I posted another bug's patch.
Attachment #429799 - Attachment is obsolete: true
Attachment #429800 - Flags: review?(roc)
Attachment #429799 - Flags: review?(roc)
+  PRUint32 len = 0;
+  nsTextRange range;
+  if (aFirstClauseLength > 0) {
+    len += aFirstClauseLength;
+    range.mStartOffset = 0;
+    range.mEndOffset = aFirstClauseLength - 1;
+    range.mRangeType = GetRangeType(aFirstClauseAttr);
+    textRanges.AppendElement(range);
+    if (aSecondClauseLength > 0) {
+      len += aSecondClauseLength;
+      range.mStartOffset = range.mEndOffset + 1;
+      range.mEndOffset = range.mStartOffset + aSecondClauseLength - 1;
+      range.mRangeType = GetRangeType(aSecondClauseAttr);
+      textRanges.AppendElement(range);
+      if (aThirdClauseLength > 0) {
+        len += aThirdClauseLength;
+        range.mStartOffset = range.mEndOffset + 1;
+        range.mEndOffset = range.mStartOffset + aThirdClauseLength - 1;
+        range.mRangeType = GetRangeType(aThirdClauseAttr);
+        textRanges.AppendElement(range);
+      }

How about a helper function AppendClause(aClauseLength, aClauseAttr, nsTArray<nsTextRange>* aRanges), that you just call three times?

Are you going to add a patch that calls IsUniversalXPConnectCapable from other places?

+  switch (aType) {
+    case QUERY_SELECTED_TEXT:
+      message = NS_QUERY_SELECTED_TEXT;
+      break;
+    case QUERY_TEXT_CONTENT:
+      message = NS_QUERY_TEXT_CONTENT;
+      break;
+    case QUERY_CARET_RECT:
+      message = NS_QUERY_CARET_RECT;
+      break;
+    case QUERY_TEXT_RECT:
+      message = NS_QUERY_TEXT_RECT;
+      break;
+    case QUERY_EDITOR_RECT:
+      message = NS_QUERY_EDITOR_RECT;
+      break;

Why not just make these QUERY_ constants equal the NS_QUERY constants? Then you can save some code.

+      if (widget != targetWidget) {
+        pt += widget->WidgetToScreenOffset() -
+                targetWidget->WidgetToScreenOffset();
+      }

This might as well be unconditional, that's simpler.

-  /**
+/**

Don't need this.

+
+  if (!aWindow)
+    aWindow = window;
+
+  var utils =
+    aWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
+            getInterface(Components.interfaces.nsIDOMWindowUtils);
+  if (!utils) {
+    return null;
+  }

Use a helper function to share this code?
Attached patch Patch v2.1 (obsolete) — Splinter Review
Attachment #429800 - Attachment is obsolete: true
Attachment #429959 - Flags: review?(roc)
Attachment #429800 - Flags: review?(roc)
Attachment #429959 - Flags: superreview?(jst)
Attachment #429959 - Flags: review?(roc)
Attachment #429959 - Flags: review+
jst: would you sr the patch?
Yes, I'm planning to do that, but unfortunately I have a couple of other big patches ahead of this one in my queue. I hope to get to this no later than a week from now. Sorry for this taking a while...
Thanks. If that is in your queue, there is no problem. Sorry for the spam.
Attachment #429959 - Flags: superreview?(jst) → superreview+
Sigh... The query caret rect was already broken. Probably the cause is bug 544852.
http://hg.mozilla.org/mozilla-central/rev/7ee38ee49927
Blocks: 544852
Attached patch Patch v2.2 (obsolete) — Splinter Review
Fix the regression of bug 544852.

> @@ -659,19 +679,18 @@ nsContentEventHandler::OnQueryCaretRect(
>      PRUint32 offset;
>      rv = GetFlatTextOffsetOfRange(mRootContent, mFirstSelectedRange, &offset);
>      NS_ENSURE_SUCCESS(rv, rv);
>      if (offset == aEvent->mInput.mOffset) {
>        nsRect rect;
>        nsIFrame* caretFrame = caret->GetGeometry(mSelection, &rect);
>        if (!caretFrame)
>          return NS_ERROR_FAILURE;
> -      nsPoint windowOffset(0, 0);
> -      caretFrame->GetWindowOffset(windowOffset);
> -      rect.MoveBy(windowOffset);
> +      rv = ConvertToRootViewRelativeOffset(caretFrame, rect);
> +      NS_ENSURE_SUCCESS(rv, rv);
>        aEvent->mReply.mRect =
>          rect.ToOutsidePixels(caretFrame->PresContext()->AppUnitsPerDevPixel());
>        aEvent->mSucceeded = PR_TRUE;
>        return NS_OK;
>      }
>    }
>  

And also I found a bug of the latest patch:

> +static void
> +AppendClause(PRInt32 aClauseLength, PRUint32 aClauseAttr,
> +             nsTArray<nsTextRange>* aRanges)
> +{
> +  NS_PRECONDITION(aRanges, "aRange is null");
> +  if (aClauseLength == 0) {
> +    return;
> +  }
> +  nsTextRange range;
> +  range.mStartOffset = aRanges->Length() == 0 ? 0 :
> +    aRanges->ElementAt(aRanges->Length() - 1).mEndOffset + 1;
> +  range.mEndOffset = range.mStartOffset + aClauseLength;

The last line added 1 but that is wrong.
Attachment #429959 - Attachment is obsolete: true
Attachment #433340 - Flags: review?(roc)
Sorry for the regression in OnQueryCaretRect...  and r=mats on your fix,
because it's exactly the same as I just came up with ;-)
FYI, I had to change this line in window_composition_text_querycontent.xul:
  setTimeout(runTest, 0);
to
  window.opener.wrappedJSObject.SimpleTest.waitForFocus(runTest, window)
for the test to pass (on Linux).
Attached patch Patch v2.3Splinter Review
thank you.
Attachment #433340 - Attachment is obsolete: true
Attachment #433365 - Flags: review?(roc)
Attachment #433340 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/18d4ef55aceb
http://hg.mozilla.org/mozilla-central/rev/288dc1dda479
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
This introduced two new compile warnings:

> dom/base/nsDOMWindowUtils.cpp: In member function 'virtual nsresult nsDOMWindowUtils::SendTextEvent(const nsAString_internal&, PRInt32, PRUint32, PRInt32, PRUint32, PRInt32, PRUint32, PRInt32, PRInt32)':
> dom/base/nsDOMWindowUtils.cpp:1011: warning: comparison between signed and unsigned integer expressions
> dom/base/nsDOMWindowUtils.cpp: In member function 'virtual nsresult nsDOMWindowUtils::SendQueryContentEvent(PRUint32, PRUint32, PRUint32, PRInt32, PRInt32, nsIQueryContentEventResult**)':
> dom/base/nsDOMWindowUtils.cpp:1072: warning: unused variable 'rv'

Links to blamed lines of code:
http://hg.mozilla.org/mozilla-central/annotate/e9312d05488f/dom/base/nsDOMWindowUtils.cpp#l1011
http://hg.mozilla.org/mozilla-central/annotate/e9312d05488f/dom/base/nsDOMWindowUtils.cpp#l1072

The first one would be fixed by a PRUint32 cast, on the |len| instance that gets compared vs. Length().

For the second, it looks like we should really be checking |rv| for success vs. failure.
Attachment #438326 - Flags: review?(roc)
You need to log in before you can comment on or make changes to this bug.