Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 15 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 3 • 15 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 4 • 15 years ago

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?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 15 years ago

(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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 15 years ago

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").

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 7 • 15 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 8 • 15 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 15 years ago

(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?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 10 • 15 years ago

(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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 15 years ago

OK. What if you extend the approach used by TestTSF.cpp?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 12 • 15 years ago

(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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 13 • 15 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 14 • 15 years ago

roc?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 15 • 15 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 16 • 15 years ago

(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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 15 years ago

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?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 18 • 15 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 19 • 15 years ago

I'll post to dev.platform about this.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 20 • 15 years ago

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.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 22 • 15 years ago

oops, sorry, I posted another bug's patch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 23 • 15 years ago

+  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?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 26 • 15 years ago

jst: would you sr the patch?

	Johnny Stenback (:jst)
	Comment 27 • 15 years ago

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...

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 28 • 15 years ago

Thanks. If that is in your queue, there is no problem. Sorry for the spam.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 29 • 15 years ago

Sigh... The query caret rect was already broken. Probably the cause is bug 544852.
http://hg.mozilla.org/mozilla-central/rev/7ee38ee49927

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 30 • 15 years ago

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.

	Mats Palmgren (inactive)
	Comment 31 • 15 years ago

Sorry for the regression in OnQueryCaretRect...  and r=mats on your fix,
because it's exactly the same as I just came up with ;-)

	Mats Palmgren (inactive)
	Comment 32 • 15 years ago

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).

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 33 • 15 years ago

thank you.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 34 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/18d4ef55aceb
http://hg.mozilla.org/mozilla-central/rev/288dc1dda479

	Daniel Holbert [:dholbert]
	Comment 35 • 15 years ago

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.

	(no longer active)
	Comment 37 • 14 years ago

Attachment 438326 [details] [diff] landed as http://hg.mozilla.org/mozilla-central/rev/6e297cf36c10.