88831 - Support new IME API "Text Services Framework" from Office XP and Windows XP

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement, P2)

Description

[Makoto Kato [:m_kato]]

Text Services Framework (TSF) is new IME API.
TSF provides a simple and scalable framework for the delivery of advanced text 
input and natural language technologies. TSF can be enabled in applications, or 
as TSF Text Services. TSF Text Services provide multilanguage support and 
deliver text services such as handwriting recognition and speech recognition.

I want to support TSF to Mozilla.

Comment 1

[nhottanscp]

Kato san, please work with Roy for this.

Comment 2

[Roy Yokoyama]

Accepting.
>I want to support TSF to Mozilla.
I definitely agree.  Kato-san, do you have ideas how we should proceed?

Comment 3

[Frank Tang]

mark as m1.0 P2

Comment 4

[Frank Tang]

add to the intl wish list 104930

Comment 5

[Roy Yokoyama]

*** Bug 106721 has been marked as a duplicate of this bug. ***

Comment 6

[Rui Xu]

Bug 106721 need to be verified again after support TSF and fix this bug.

Comment 7

[Roy Yokoyama]

Here are some info for users:

Before you install the Global IME for Office XP, do the following:
   1. Install Office XP (any language version).
   2. If you have not installed a Global IME before, install the Speech  
      or the Handwriting feature in the Office XP installation 
      (Office Shared Features, Alternate User Input).
   3. If you are not running Windows 2000 AND you are not running a 
      Korean language version of the operating system, install the 
      Office XP Tool: Korean Language Pack.
http://office.microsoft.com/downloads/2002/imekor.aspx

Comment 8

[Roy Yokoyama]

Here is more info for developers:

General information about TSF:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/tsf/tsf/text_services_framework.asp

To start developing TSF aware application, you need
1)install MS Platform SDK (Nov.01 or newer)
  http://www.microsoft.com/msdownload/platformsdk/sdkupdate/
  (strongly suggest to appy the VC++ sp5 or greater before you install
   it.  You can obtain the Service Pack 5 from 
   http://msdn.microsoft.com/vstudio/sp/vs6sp5/default.asp )

TSF runtime requirment: you can use one of following methods.
1) TSF is pre-installed in Windows XP. 
2) For other OSs (Windows 98, Windows ME, NT 4.0, and Windows 2000)
   you can install the redistributable from
   www.microsoft.com/msdownload/platformsdk/sdkupdate/psdkredist.htm3) Install
TSF application which include TSF redistributable.
   MS Office XP is a good example of that.

Comment 9

[Roy Yokoyama]

TSF runtime requirment: you can use one of following methods.
1) TSF is pre-installed in Windows XP. 
2) For other OSs (Windows 98, Windows ME, NT 4.0, and Windows 2000)
   you can install the redistributable from
   www.microsoft.com/msdownload/platformsdk/sdkupdate/psdkredist.htm
3) Install TSF application which include TSF redistributable.
   MS Office XP is a good example of that.

Comment 10

[Roy Yokoyama]

Attachment: First cut

Where are we with this patch:
- mozilla is TSF aware app
- can type non-ASCII char
- receives Unicode chars thru nsTextStore::SetText()
- displays candiate windows at right position

What else do we need to do:
- mozilla receives nsTextStore:OnEndComposition()
  with first inputed character (thus commiting the input)
- behaviour of TSF-IME is not very user friendly
- very first candiate window does not position well.

Concerns:- owning reference of nsWindow 
  (NS_ADDREF and NS_RELEASE inside nsTextStore)
- wanted to make nsWindow:nsIWeakReference; but 
  had trouble with accessing nsWindow methods

Comment 11

[Roy Yokoyama]

marking post 1.0

Comment 12

[Roy Yokoyama]

*** Bug 161035 has been marked as a duplicate of this bug. ***

Comment 13

[Marek Z. Jeziorek]

batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html

Comment 14

[Roy Yokoyama]

assign to shanjina

Comment 15

[Shanjian Li]

Attachment: 2nd cut, it should function as well as traditional ime

Comment 16

[Shanjian Li]

Attachment: patch for review scrutiny

Comment 17

[Roy Yokoyama]

Comment on attachment 107381 [details] [diff] [review]
patch for review scrutiny

1) I see some places where you use Wide API
+ g_tsfMsgPump->PeekMessageW(..) 
but A API in others places
+ g_tsfMsgPump->PeekMessage(..)
Any reason?

2) Would we be able to run in Win9x platform
if we use g_tsfMsgPump->PeekMessageW() ?

3) Add space before and after ':'?
+nsTextStores::nsTextStores():m_TextDoc()
and remove spaces between '(' and DWORD' 
+ nsTextStores::InsertTextAtSelection(	  DWORD dwFlags,
and tab between 'isShift' and '='
+  event.isShift	= mIsShiftDown;

4) do you need ?
+    mIMECursorPosition = 0;

5) do you need #ifdef wrapp in nsWindow.h
+class nsTextDoc;
+

6) we discussed about the CLSID hack before.
   What was your conclusion?

Comment 18

[Shanjian Li]

Attachment: patch addressed roy's suggestion.

Comment 19

[Shanjian Li]

>1) I see some places where you use Wide API
>+ g_tsfMsgPump->PeekMessageW(..) 
>but A API in others places
>+ g_tsfMsgPump->PeekMessage(..)
>Any reason?
>2) Would we be able to run in Win9x platform
>if we use g_tsfMsgPump->PeekMessageW() ?

That's a problem. In fact, I didn't even consider about win98. I checked MS
document and win98 is included. So I modified as such.


> 4) do you need ?
> +    mIMECursorPosition = 0;
Yes, but probably not now. Mouse cursor operation is not there yet. In TSF, 
mIMECursorPosition does not have a chance to initialize. I used this variable
in my testing build, it is removed in final patch. But I will need it in future.

>6) we discussed about the CLSID hack before.
>   What was your conclusion?
CLSID is defined as static variable in some module. Whenever they update their
library and change the value, we need to do the same. For now, I just figure out
the value and put it there.

Comment 20

[Shanjian Li]

reassign. 

Comment 21

[Frank Tang]

we won't be able to finish this due to resource reduction
Change it to future and change topembed+ to topembed

Comment 22

[Frank Tang]

the efforts to complete this feature is about 10-15 man days of developement
time and debugging time after QA.

Comment 23

[Frank Tang]

assign future remove nsbeta1+ and adt2

Comment 24

[Dave Barrowman]

EDT says topembed- for now, until some specific embedding customer needs this.

Comment 25

[Jungshik Shin]

Masayuki, can you take a look?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry, I don't know "Text Services Framework".

Comment 28

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: patch adding necessary events and notifications

I added some new query events to the NS_QUERY_* group, and also new selection manipulation events, NS_SELECTION_*. The TSF implementation uses these events to communicate with the textboxes, etc.

I wasn't sure where to put the NS_SELECTION_* event handler. Putting them in nsQueryContentEventHandler.cpp, along with the other query events I added, would seem out of place because the selection events aren't "query"-type events. I was thinking about renaming nsQueryContentEventHandler.cpp to a more generic nsIMEEventHandlers.cpp or something like that. But I decided to just put the selection handler in nsIMEStateManager.cpp in the mean time.

The other part was I created a new interface in widget called nsITextStateObserver. The TSF implementation receives notification of changes in selection, text content, and focus through this interface which nsWindow implements. The code that calls on this interface is in nsIMEStateManager.cpp. 

An alternative to having yet another interface to worry about is to merge nsITextStateObserver into nsIKBStateControl, since both are IME-related and nsWindow implements both. In that case nsIKBStateControl should probably be renamed and for other platforms that implement nsIKBStateControl, I will be writing stubs for them.

Comment 29

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: working patch, works but not scrutinized or tested extensively

Current progress:
Not supported
* Display attributes (i.e. no underlines/text decorations when using IME)
Supported (as far as I've tested)
* Mozilla is TSF-aware
* Tablet Input Panel works
* Windows Speech Recognition mostly works
* Windows-included Chinese and Japanese IMEs work (excluding display attirbutes issue above)

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This looks awesome.

+    IUnknown* comparison1 = NULL;
+    punk->QueryInterface(IID_IUnknown,
+                         reinterpret_cast<void**>(&comparison1));
+    NS_ASSERTION(comparison1, "QI IUnknown failed\n");
+    comparison1->Release();

Can't you use nsRefPtr here like you do below?

+        static_cast<PRInt32>(startNode->GetChildCount()) > startOffset) {

Just use constructor casts --- PRInt32(...).

Is it worth getting rid of nsIKBControlState and nsITextStateObserver and just adding their methods directly to nsIWidget? Seems like that would simplify things.

Renaming nsQueryContentEventHandler to nsIMEStateHandlers sounds good to me.

Masayuki-san, your comments would be very useful.

What can we do to automatically test this? Is there a TSF consumer we can control via script?

My other big concern is that this is a lot of new code, which is not necessarily bad, but it would be good to shrink it as much as possible. I haven't got any great ideas for that yet, though.

+    nsCOMPtr<nsIDOMRange> domRange = do_QueryInterface(range);
+    nsCOMPtr<nsIDOMNode> domNode = do_QueryInterface(aContent);
+    if (domRange && domNode) {

Why do we need to use nsIDOM* here? We should get smaller and faster code by using nsIContent/nsIRange as much as possible.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the delay. I'm still working on non-development works :-(

(In reply to comment #29)
> Current progress:
> Not supported
> * Display attributes (i.e. no underlines/text decorations when using IME)
> Supported (as far as I've tested)

See bug 307396 and bug 113161. nsTextFrame gets the system settings via nsILookAndFeel. However, if the patch becomes big, you should separate the bug, maybe.

I have an worry, after this patch landed, the IMM related code is obsoleted, right? If so, we need Win2k environment for debugging IMM. It's too bad thing. TSF should be able to be disabled by prefs. Then, developers can debug the legacy IMM code on WinXP and WinVista. And also such pref helps the testing.

(In reply to comment #30)
> Is it worth getting rid of nsIKBControlState and nsITextStateObserver and just
> adding their methods directly to nsIWidget? Seems like that would simplify
> things.

Yeah, it sounds good for me. Even if we need new method only for one platform, we only need to change the nsBaseWidget for other platform*s*.

Shouldn't it be fixed before this bug?

> Renaming nsQueryContentEventHandler to nsIMEStateHandlers sounds good to me.

I don't think so, because we can use the class for non-IME-related implementations too. How about that only remove the "Query"?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #31)
> Shouldn't it be fixed before this bug?

Yeah, that would be nice. Jim, can you handle that?

> > Renaming nsQueryContentEventHandler to nsIMEStateHandlers sounds good to me.
> 
> I don't think so, because we can use the class for non-IME-related
> implementations too. How about that only remove the "Query"?

How about nsTextStateHandlers?

Comment 33

[(inactive) Jim Chen [:jchen] [:darchons]]

Thank you Rob and Masayuki-san for the comments!


(In reply to comment #30)
> Can't you use nsRefPtr here like you do below?
>
> Just use constructor casts --- PRInt32(...).
> 
> Is it worth getting rid of nsIKBControlState and nsITextStateObserver and just
> adding their methods directly to nsIWidget? Seems like that would simplify
> things.

OK, I will change these.

For the nsIWidget change I just file a new bug right?


> What can we do to automatically test this? Is there a TSF consumer we can
> control via script?

The best way right now seems to be to write a "dummy" IME to interact with our TSF implementation.


> My other big concern is that this is a lot of new code, which is not
> necessarily bad, but it would be good to shrink it as much as possible. I
> haven't got any great ideas for that yet, though.

I will see what I can do. I know it's not going to make the code smaller but if I split this bug into smaller ones would that help with organizing/tracking?


> +    nsCOMPtr<nsIDOMRange> domRange = do_QueryInterface(range);
> +    nsCOMPtr<nsIDOMNode> domNode = do_QueryInterface(aContent);
> +    if (domRange && domNode) {
> 
> Why do we need to use nsIDOM* here? We should get smaller and faster code by
> using nsIContent/nsIRange as much as possible.

I'm using nsIDOMRange::SetStart for that part.


(In reply to comment #31)
> See bug 307396 and bug 113161. nsTextFrame gets the system settings via
> nsILookAndFeel. However, if the patch becomes big, you should separate the bug,
> maybe.

TSF allows the IME to tell the application what text decorations to use, instead of having the application guess what to use. Do we want to support this? But this would need some major change though, because when we draw text we would need to query the IME for text decorations.


> I have an worry, after this patch landed, the IMM related code is obsoleted,
> right? If so, we need Win2k environment for debugging IMM. It's too bad thing.
> TSF should be able to be disabled by prefs. Then, developers can debug the
> legacy IMM code on WinXP and WinVista. And also such pref helps the testing.

OK, I can add this. Should I use #ifdef or prefs?

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #33)
> For the nsIWidget change I just file a new bug right?

Sure

> > What can we do to automatically test this? Is there a TSF consumer we can
> > control via script?
> 
> The best way right now seems to be to write a "dummy" IME to interact with our
> TSF implementation.

OK. You're already doing great work and I don't want to overburden you, but could you do that? It would be super, super great if we had automated testing of IME stuff.

> I will see what I can do. I know it's not going to make the code smaller but
> if I split this bug into smaller ones would that help with
> organizing/tracking?

Maybe a little but it's not worth it. Just do your best to shrink the code.

> > +    nsCOMPtr<nsIDOMRange> domRange = do_QueryInterface(range);
> > +    nsCOMPtr<nsIDOMNode> domNode = do_QueryInterface(aContent);
> > +    if (domRange && domNode) {
> > 
> > Why do we need to use nsIDOM* here? We should get smaller and faster code by
> > using nsIContent/nsIRange as much as possible.
> 
> I'm using nsIDOMRange::SetStart for that part.

Hmm, can you add a setter to nsIRange to avoid that?

> TSF allows the IME to tell the application what text decorations to use,
> instead of having the application guess what to use. Do we want to support
> this? But this would need some major change though, because when we draw text
> we would need to query the IME for text decorations.

Seems to me we could do that as a followup project. If we can keep that out of this bug, we should.

> > I have an worry, after this patch landed, the IMM related code is obsoleted,
> > right? If so, we need Win2k environment for debugging IMM. It's too bad
> > thing. TSF should be able to be disabled by prefs. Then, developers can
> > debug the legacy IMM code on WinXP and WinVista. And also such pref helps
> > the testing.
> 
> OK, I can add this. Should I use #ifdef or prefs?

Prefs are easy if you don't need to cache the pref (which you shouldn't, I think) so might as well do prefs.

Comment 35

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to comment #34)
> OK. You're already doing great work and I don't want to overburden you, but
> could you do that? It would be super, super great if we had automated testing
> of IME stuff.

Absolutely. My main plan for the second half of GSoC is basically testing, bug fix, and writing tests. Should be plenty of time too, and I'll be happy to continue work on it if it somehow takes me over the end of GSoC.

> Hmm, can you add a setter to nsIRange to avoid that?

Alright

> Seems to me we could do that as a followup project. If we can keep that out of
> this bug, we should.

OK, that sounds good.

Comment 36

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 2nd patch for /content

I renamed nsQueryContentEventHandler to nsContentEventHandler per Masayuki-san's suggestion.

Also added SetRange method to nsIRange.

Comment 37

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 2nd patch for /widget

I added support for compositions and text decorations so the TSF implementation now matches the legacy IME implementation in terms of functionality.

I wasn't sure about what name to use for the pref for turning TSF on/off. I settled on 'config.windows.use_tsf', and I guess it could be something else. Also this is good because the legacy IME code is not entirely obsolete because a lot of the non-Microsoft IME's still haven't switched to TSF yet.

Comment 38

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: automatic unit test

It is an automatic C++ test and I put it under widget/tests. Should it be under widget/src/windows?

And I'm going to use the next couple of weeks to see if there's any more clean-up or commenting to do, but I think everything looks good so far.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This looks amazing. Are you ready to request review?

Is there a way to support both TSF and non-TSF IMEs at the same time? What do other apps do?

The tests you have here look great, but I was expecting to see us using the system TSF implementation with a test IME or something like that. Why did you do it this way? (Not complaining, just wondering :-) )

Comment 40

[(inactive) Jim Chen [:jchen] [:darchons]]

I think I will wait a couple of weeks to see if anyone else has anything to say :)  And then request for review. But I do hope it can make it into FF3.1 if possible. So if I need to request review now to have a chance at FF3.1 I can do that too.

Also would you mind uploading the content and widget patches to the try server so other people can have a try?

TSF and non-TSF IMEs should both work. All the legacy code are kept so for non-TSF IMEs they will work 100% the way they work now. There should not be any regression. For TSF-aware IMEs like Microsoft IME's, handwriting recognition, speech recognition, and a handful of newer third party IME's they will work with the new TSF code.

For the tests, I was going for what you described since that would be ideal but I decided to use a custom implementation because this way we don't have to depend on the system. The system running the tests might not have TSF enabled or even installed and it might be too much of a hassle having the test program to configure things, or even worse having to configure things manually just to make the tests run. (Actually I just realized that my test right now fails on purpose if TSF is not installed/enabled. Oops :) I'll upload the new version later)

Thanks!

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think you should go for review now. You're not likely to get much feedback with the patches just sitting here, and if you want to be in 3.1 then we need to get it landed ASAP.

Since this doesn't impact existing code paths too much, I think we should skip tryserver stuff and just try to get it landed.

> TSF and non-TSF IMEs should both work.

Then why do we need a pref to disable TSF support?

Thanks for explaining about the tests!

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #41)
> > TSF and non-TSF IMEs should both work.
> 
> Then why do we need a pref to disable TSF support?

I hope that the pref should be for debug.

Comment 43

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: automatic test (1.1)

(In reply to comment #41)
> I think you should go for review now. You're not likely to get much feedback
> with the patches just sitting here, and if you want to be in 3.1 then we need
> to get it landed ASAP.
> 
> Since this doesn't impact existing code paths too much, I think we should skip
> tryserver stuff and just try to get it landed.

OK thanks. I'm ready for review. Who do you think should review/super-review the different parts?

(In reply to comment #42)
> (In reply to comment #41)
> > > TSF and non-TSF IMEs should both work.
> > 
> > Then why do we need a pref to disable TSF support?
> 
> I hope that the pref should be for debug.

Yes I think this will be useful for that.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Request r from Masayuki, sr from me. I'd mark it myself but Bugzilla works better if you do it because then you get the notification emails when someone responds.

Comment 45

[(inactive) Jim Chen [:jchen] [:darchons]]

Requesting r+sr. Thank you Rob and Masayuki-san!

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I tried to apply the patches to trunk, but it failed. Would you update the patches for the latest trunk?

Comment 47

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: patch v2.1

I'm adding one big patch but if you want three separate ones let me know. Thanks.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #47)
> I'm adding one big patch but if you want three separate ones let me know.
> Thanks.

No problem, the merged patch is better for me.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

>  nsINode::GetTextEditorRootContent(nsIEditor** aEditor)

> +  nsIContent* rv = nsnull;

You should not use "rv" for non nsresult variables. We are using the name only for nsresult.

> +static PRBool AdjustRangeForSelection(nsIContent* aRoot, nsINode** aNode, PRInt32* aOffset)
> +{
> +  PRBool rv = PR_FALSE;

Here is too.

> +static PRBool IsContentBR(nsIContent* aContent)

Why are you need this? I think that we should not ignore the anonymous brs at generating the text from contents.

> +nsContentEventHandler::OnQueryTextRect(nsQueryContentEvent* aEvent)

> +  nsINode* node = iter->GetCurrentNode();
> +  if (!node) {
> +    node = range->GetStartParent();
> +    childCount = PRInt32(node->GetChildCount());
> +    if (childCount && childCount >= offset) {
> +      node = node->GetChildAt(PR_MIN(offset, childCount - 1));
> +      offset = node->IsNodeOfType(nsINode::eTEXT) ?
> +          static_cast<nsIContent*>(node)->TextLength() : 1;
> +    }
> +  }
> +  NS_ENSURE_TRUE(node && node->IsNodeOfType(nsINode::eCONTENT),
> +                 NS_ERROR_UNEXPECTED);

If you insert NS_ENSURE_TRUE(node, ...) before the if block, you can remove the if statement and reduce the indent.

> @@ -871,16 +872,19 @@ nsEventStateManager::PreHandleEvent(nsPr

>        if (mDocument) {
> +
> +        nsIMEStateManager::OnTextStateBlur(mPresContext, mCurrentFocus);
> +

You are calling nsIMEStateManager::OnTextStateBlur and nsIMEStateManager::OnTextStateFocus from many points in ESM. It looks like that you can call them from nsIMEStateManager::OnChangeFocus. Isn't it enough?

> +nsTextStateManager::CharacterDataWillChange(nsIDocument* aDocument,
> +                                             nsIContent* aContent,
> +                                             CharacterDataChangeInfo*)

wrong intent at 2nd and 3rd lines. Other some methods also have this issue.

> +nsTextStateManager::CharacterDataChanged(nsIDocument* aDocument,
> +                                          nsIContent* aContent,
> +                                          CharacterDataChangeInfo* aInfo)
> +{
> +  if (aContent->IsNodeOfType(nsINode::eTEXT)) {
> +    nsCOMPtr<nsIRange> range = new nsRange();
> +    if (!range) return;
> +    range->SetRange(mRangeRoot, aContent, aInfo->mChangeStart,
> +                    aContent, aInfo->mChangeStart);
> +    PRUint32 offset = 0;
> +    // get offsets of change and fire notification
> +    if (NS_SUCCEEDED(nsContentEventHandler::GetFlatTextOffsetOfRange(
> +                         mRootContent, range, &offset))) {
> +      PRUint32 oldEnd = offset + aInfo->mChangeEnd - aInfo->mChangeStart;
> +      PRUint32 newEnd = offset + aInfo->mReplaceLength;
> +      mWidget->OnIMETextChange(offset, oldEnd, newEnd);
> +    }
> +  }
> +}

See "Return from errors immediately" of http://developer.mozilla.org/en/docs/Mozilla_Coding_Style_Guide#Check_for_errors_early_and_often
You can reduce the indent.

Following methods are same.
+nsIMEStateManager::OnTextStateBlur
+nsIMEStateManager::OnTextStateFocus
+nsIMEStateManager::GetFocusSelectionAndRoot

> +static nsINode* GetRootEditableNode(nsPresContext* aPresContext,
> +                                    nsIContent* aContent)
> +  if (aPresContext) {
> +    nsIDocument* document = aPresContext->Document();
> +    if (document && document->IsEditable())
> +      return document;
> +  }

The logic of this method is wrong. See bug 415026 and bug 416551. Even if the document is editable, it can be non root editable node. You should find editable root node if aContent is not null. And if aContent is null, you should check whether the document is editable.

I'll review widget/src.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The patch almost works fine for me.

However, I find two issues.

1. Japanese IME status bar looks like the current IME state is enabled when IME disabled editor gets focus. But the IME is actually disabled, so, the status bar status and the actual status don't match. You can test on following testcase. I confirmed on Vista with its default MS-IME.
https://bugzilla.mozilla.org/attachment.cgi?id=264849

2. ATOK (Japanese IME of a third party vendor) 's navi bar that is floating window at caret position should disappear when the caret position is changed. However, even if I click any points in same input/textarea, it doesn't disappear. If I use allow keys, it disappears. I'm not sure whether this is your patch's bug.

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Er, please ignore the 2 of comment 50. Now, I cannot reproduce it after reboot the patched build...

Comment 52

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to comment #49)
> > +static PRBool IsContentBR(nsIContent* aContent)
> 
> Why are you need this? I think that we should not ignore the anonymous brs at
> generating the text from contents.

I don't think the bogus brs are part of the text content. For example for a single-line input element, there is a bogus br at the end of the text. If we don't ignore it, the generated text will have a newline at the end.

> > +nsContentEventHandler::OnQueryTextRect(nsQueryContentEvent* aEvent)
> 
> > +  nsINode* node = iter->GetCurrentNode();
> > +  if (!node) {
> > +    node = range->GetStartParent();
> > +    childCount = PRInt32(node->GetChildCount());
> > +    if (childCount && childCount >= offset) {
> > +      node = node->GetChildAt(PR_MIN(offset, childCount - 1));
> > +      offset = node->IsNodeOfType(nsINode::eTEXT) ?
> > +          static_cast<nsIContent*>(node)->TextLength() : 1;
> > +    }
> > +  }
> > +  NS_ENSURE_TRUE(node && node->IsNodeOfType(nsINode::eCONTENT),
> > +                 NS_ERROR_UNEXPECTED);
> 
> If you insert NS_ENSURE_TRUE(node, ...) before the if block, you can remove the
> if statement and reduce the indent.

I don't think I can remove the if, because the if block is there so that when node is null, we can try to change node to something else and not fail.

> > @@ -871,16 +872,19 @@ nsEventStateManager::PreHandleEvent(nsPr
> 
> >        if (mDocument) {
> > +
> > +        nsIMEStateManager::OnTextStateBlur(mPresContext, mCurrentFocus);
> > +
> 
> You are calling nsIMEStateManager::OnTextStateBlur and
> nsIMEStateManager::OnTextStateFocus from many points in ESM. It looks like that
> you can call them from nsIMEStateManager::OnChangeFocus. Isn't it enough?

I want to call OnTextStateBlur before ESM fires blur events, and call OnTextStateFocus after ESM fires focus events. So I think they should be called from inside ESM.

(In reply to comment #50)
> 1. Japanese IME status bar looks like the current IME state is enabled when IME
> disabled editor gets focus. But the IME is actually disabled, so, the status
> bar status and the actual status don't match. You can test on following
> testcase. I confirmed on Vista with its default MS-IME.
> https://bugzilla.mozilla.org/attachment.cgi?id=264849

Hmm, I can reproduce with the Japanese MS-IME, but the Chinese MS-IME works OK. Maybe a bug in the Japanese IME? I will look into it more.

ATOK doesn't use TSF. So should not affect ATOK at all.

Thanks for all the helpful comments!

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #52)
> > > +nsContentEventHandler::OnQueryTextRect(nsQueryContentEvent* aEvent)
> > 
> > > +  nsINode* node = iter->GetCurrentNode();
> > > +  if (!node) {
> > > +    node = range->GetStartParent();
> > > +    childCount = PRInt32(node->GetChildCount());
> > > +    if (childCount && childCount >= offset) {
> > > +      node = node->GetChildAt(PR_MIN(offset, childCount - 1));
> > > +      offset = node->IsNodeOfType(nsINode::eTEXT) ?
> > > +          static_cast<nsIContent*>(node)->TextLength() : 1;
> > > +    }
> > > +  }
> > > +  NS_ENSURE_TRUE(node && node->IsNodeOfType(nsINode::eCONTENT),
> > > +                 NS_ERROR_UNEXPECTED);
> > 
> > If you insert NS_ENSURE_TRUE(node, ...) before the if block, you can remove the
> > if statement and reduce the indent.
> 
> I don't think I can remove the if, because the if block is there so that when
> node is null, we can try to change node to something else and not fail.

Ah, ok. You're right.

> (In reply to comment #50)
> > 1. Japanese IME status bar looks like the current IME state is enabled when IME
> > disabled editor gets focus. But the IME is actually disabled, so, the status
> > bar status and the actual status don't match. You can test on following
> > testcase. I confirmed on Vista with its default MS-IME.
> > https://bugzilla.mozilla.org/attachment.cgi?id=264849
> 
> Hmm, I can reproduce with the Japanese MS-IME, but the Chinese MS-IME works OK.
> Maybe a bug in the Japanese IME? I will look into it more.

Yeah, I also thought it first, however, when I move the focus to non-editable elements, the status is updated to correct status (disabled). If the cause is only the bug of Japanese IME, this is strange behavior...
 
> ATOK doesn't use TSF. So should not affect ATOK at all.

Yeah, and now I cannot reproduce the issue. It's strange... :-(

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> +nsTextStore::Destroy(ITfThreadMgr* aThreadMgr)

> +  mSink = NULL;
> +#ifdef DEBUG_IME_TSF
> +    printf("TSF: Destroyed, window=%08x\n", mWindow);
> +#endif
> +  mWindow = NULL;

wrong indent in #ifdef DEBUG_IME_TSF

> +nsTextStore::Blur(ITfThreadMgr* aThreadMgr)
> +{
> +  aThreadMgr->SetFocus(NULL);
> +  mFocused = PR_FALSE;
> +#ifdef DEBUG_IME_TSF
> +    printf("TSF: Blurred\n");
> +#endif

same.

And MSDN said that "[in] Pointer to a ITfDocumentMgr interface that receives the input focus. *This parameter cannot be NULL.*"
http://msdn.microsoft.com/en-us/library/ms628989(VS.85).aspx
Your code is correct??

> +nsTextStore::AdviseSink(REFIID riid,

> +#ifdef DEBUG_IME_TSF
> +    printf("TSF: Sink installed, punk=%08x\n", punk);
> +#endif

indent.

> +nsTextStore::UnadviseSink(IUnknown *punk)
> +{
> +  NS_ENSURE_TRUE(punk, E_INVALIDARG);
> +  if (punk && mSink) {

You don't need to check punk in if statement.

> +nsTextStore::QueryInsert(LONG acpTestStart,

> +  // We don't test to see if these positions are
> +  // after the end of document for performance reasons

Is this method called too many times? If not so, we don't need to worry the performance issue.

And it seems that we should check this strictly if the focused editor is HTML editor. However, it should be future, not now.

> +#ifdef DEBUG_IME_TSF
> +    printf("SUCCEEDED\n");
> +#endif

indent.

> +nsTextStore::GetSelection(ULONG ulIndex,

> +      pSelection->style.ase = event.mReply.mString.Length() ?
> +          (event.mReply.mReversed ? TS_AE_START : TS_AE_END) : TS_AE_NONE;

Is this correct when there is not selections? If there are no selections, the result position is a caret position. Then, it seems that we should not return TS_AE_NONE.

> +nsTextStore::SetSelection(ULONG ulCount,
> +                          const TS_SELECTION_ACP *pSelection)
> +{

> +  NS_ENSURE_TRUE(TS_LF_READWRITE == (mLock & TS_LF_READWRITE), TS_E_NOLOCK);
> +  NS_ENSURE_TRUE(ulCount && pSelection, E_INVALIDARG);

If ulCount is larger than 1, should not we return E_FAIL?

> +nsTextStore::RequestAttrsTransitioningAtPosition(LONG acpPos,
> +                                                ULONG cFilterAttrs,
> +                                                const TS_ATTRID *paFilterAttrs,
> +                                                DWORD dwFlags)

wrong indent at second and later lines.

> +#define TEXTSTORE_DEFAULT_VIEW    (1)
> +
> +STDMETHODIMP
> +nsTextStore::GetActiveView(TsViewCookie *pvcView)
> +{
> +  NS_ENSURE_TRUE(pvcView, E_INVALIDARG);
> +  *pvcView = TEXTSTORE_DEFAULT_VIEW;
> +  return S_OK;
> +}

Is this correct? Should not we return unique IDs for each editors? If we should do so, you can use |mReply.mContentsRoot| of query text content event. See |conversationIdentifier| of nsChildView.mm.
http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsChildView.mm#4935

> +nsTextStore::GetACPFromPoint(TsViewCookie vcView,
> +                             const POINT *pt,
> +                             DWORD dwFlags,
> +                             LONG *pacp)
> +{
> +  NS_ENSURE_TRUE(TS_LF_READ == (mLock & TS_LF_READ), TS_E_NOLOCK);
> +  NS_ENSURE_TRUE(TEXTSTORE_DEFAULT_VIEW == vcView, E_INVALIDARG);
> +  // not supported for now
> +  return E_NOTIMPL;
> +}

Should not we return E_NOTIMPL always?

And do you know the importance for this method? If this is too important, we should plan to implement this after this bug. But I'm not sure this is important.

I'll review after nsTextStore::GetTextExt.

The patch is too big! Thank you for your great work!

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> +nsTextStore::GetTextExt(TsViewCookie vcView,

> +  // use NS_QUERY_TEXT_RECT to get rect in system, screen coordinates

I think that NS_QUERY_TEXT_RECT should return the rect in window coordinates. Because char_rect and caret_rect events already do so. And the screen coordinates may not be useful on other platforms. These query events should use same coordinates I think. You can convert the coordinates from in window to in screen easily. See bug 449955.

> +nsTextStore::GetScreenExt(TsViewCookie vcView,

> +  // use NS_QUERY_FRAME_RECT to get rect in system, screen coordinates

I think this event also should return window coordinates.

And it looks like that |nsIFrame::GetScreenRectInAppUnits()| uses it's frame rect. Shouldn't you clip the rect by its owner window rect? If so, you note that the owner window may not be the current (mWidget's) window. E.g., when <panel> element (e.g., bookmark dialog of Fx3 that is appeared when you click the "star" button in the location bar) has focus, mWidget is in its parent window, but the element might overflow from the owner window rect.

> +nsTextStore::OnStartComposition(ITfCompositionView* pComposition,
> +                                ITfRange* aRange,
> +                                PRBool aPreserveSelection)

Please rename this method. I was confused by this overload.

> +nsTextStore::OnUpdateComposition(ITfCompositionView* pComposition,

You assume that this is always called after OnStartComposition. But with old IMM32, we know some IMEs don't send WM_IME_STARTCOMPOSITION message. I worry that we might see such case... (And also in |nsTextStore::OnEndComposition|, please.)

When OnUpdateComposition is called but mCompositionView is null, please call NS_WARNING or something.

> +        event.rangeArray = reinterpret_cast<nsTextRange*>(nsMemory::Realloc(
> +            event.rangeArray, sizeof(nsTextRange) * event.rangeCount));

This looks like ugly... Probably, we should change it to nsTArray in another bug.

> nsTextStore::EndComposition(PRBool aDiscard)

Would you rename this? ResetComposition or CommitComposition or something.

I finished to check nsTextStore.h.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I found a new bug.

Input some characters with Japanese MS-IME on Vista, and commit a composing by left click of mouse in the focused editor. And down the left button of the mouse, I can see the last composing selection at the committed string. If after the editor lost focus, I cannot see this bug.

I cannot reproduce this bug with Chinese and Korean IMEs :-(

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> diff --git a/widget/src/windows/nsWindow.cpp b/widget/src/windows/nsWindow.cpp

> +#ifndef WINCE
> +#include "nsTextStore.h"
> +#endif //WINCE
> +

You should define NS_ENABLE_TSF in nsWindow.h, use it for these #ifdefs? It is too clear the reason of #ifdef.

> +nsWindow::OnIMEFocusChange(PRBool aFocus)

use "early return" style and reduce the indent.

> +NS_IMETHODIMP
> +nsWindow::OnIMEFocusChange(PRBool aFocus)
> +{
> +  if (sTsfThreadMgr && sTsfTextStore) {
> +    if (aFocus) {
> +      NS_ASSERTION(!sTsfTextStore->IsCreated(), "Text store still focused");
> +      if (sTsfTextStore->Create(sTsfThreadMgr, sTsfClientId, this)) {
> +        sTsfTextStore->SetIMEEnabled(sTsfThreadMgr, sTsfClientId, mIMEEnabled);
> +        sTsfTextStore->Focus(sTsfThreadMgr);
> +      }
> +    } else {
> +      NS_ASSERTION(sTsfTextStore->IsCreated(), "No text store has focus");
> +      sTsfTextStore->Destroy(sTsfThreadMgr);
> +    }
> +    return NS_OK;
> +  }
> +  // no change notifications if TSF is disabled
> +  return NS_ERROR_NOT_AVAILABLE;
> +}

This is called *before* the editor lost focus, right? Then, nsEditor calls nsIWidget::ResetInputState() after sTsfThreadMgr is destroyed. Does this work correctly??

Comment 58

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to comment #50)
> 1. Japanese IME status bar looks like the current IME state is enabled when IME
> disabled editor gets focus. But the IME is actually disabled, so, the status
> bar status and the actual status don't match. You can test on following
> testcase. I confirmed on Vista with its default MS-IME.
> https://bugzilla.mozilla.org/attachment.cgi?id=264849

Found the bug and fixed! I had to set IME state before the focus code.

(In reply to comment #54)
> And MSDN said that "[in] Pointer to a ITfDocumentMgr interface that receives
> the input focus. *This parameter cannot be NULL.*"
> http://msdn.microsoft.com/en-us/library/ms628989(VS.85).aspx
> Your code is correct??

Yes, it's OK to pass in NULL to blur the focused document and it works.

> > +nsTextStore::QueryInsert(LONG acpTestStart,
> 
> Is this method called too many times? If not so, we don't need to worry the
> performance issue.

I just tested it and actually it seems no IMEs are calling it. But it's very unlikely that the positions will be out of bounds, and even if they are, it should not cause any problems.

> Is this correct when there is not selections? If there are no selections, the
> result position is a caret position. Then, it seems that we should not return
> TS_AE_NONE.

I think both TS_AE_NONE and TS_AE_END are OK for caret position. I changed it to TS_AE_END to simplify.

> > +nsTextStore::SetSelection(ULONG ulCount,
> > +                          const TS_SELECTION_ACP *pSelection)
> > +{
> 
> > +  NS_ENSURE_TRUE(TS_LF_READWRITE == (mLock & TS_LF_READWRITE), TS_E_NOLOCK);
> > +  NS_ENSURE_TRUE(ulCount && pSelection, E_INVALIDARG);
> 
> If ulCount is larger than 1, should not we return E_FAIL?

You are right.

> Is this correct? Should not we return unique IDs for each editors? If we should
> do so, you can use |mReply.mContentsRoot| of query text content event. See
> |conversationIdentifier| of nsChildView.mm.
> http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsChildView.mm#4935

They don't have to be unique unless we support many views for a single editor.

> > +nsTextStore::GetACPFromPoint(TsViewCookie vcView,
> > +                             const POINT *pt,
> > +                             DWORD dwFlags,
> > +                             LONG *pacp)
> > +{
> > +  NS_ENSURE_TRUE(TS_LF_READ == (mLock & TS_LF_READ), TS_E_NOLOCK);
> > +  NS_ENSURE_TRUE(TEXTSTORE_DEFAULT_VIEW == vcView, E_INVALIDARG);
> > +  // not supported for now
> > +  return E_NOTIMPL;
> > +}
> 
> Should not we return E_NOTIMPL always?
> 
> And do you know the importance for this method? If this is too important, we
> should plan to implement this after this bug. But I'm not sure this is
> important.

The IMEs I'm testing don't call it, so probably not important. And I think we can leave the error checking code at the beginning.

(In reply to comment #55)
> > +nsTextStore::GetTextExt(TsViewCookie vcView,
> 
> > +  // use NS_QUERY_TEXT_RECT to get rect in system, screen coordinates
> 
> I think that NS_QUERY_TEXT_RECT should return the rect in window coordinates.
> 
> > +nsTextStore::GetScreenExt(TsViewCookie vcView,
> 
> > +  // use NS_QUERY_FRAME_RECT to get rect in system, screen coordinates
> 
> I think this event also should return window coordinates.
>
> And it looks like that |nsIFrame::GetScreenRectInAppUnits()| uses it's frame
> rect. Shouldn't you clip the rect by its owner window rect?

Changed to clipped window coordinates. Thanks.

> > +nsTextStore::OnStartComposition(ITfCompositionView* pComposition,
> > +                                ITfRange* aRange,
> > +                                PRBool aPreserveSelection)
> 
> Please rename this method. I was confused by this overload.

Renamed to OnStartCompositionInternal

> When OnUpdateComposition is called but mCompositionView is null, please call
> NS_WARNING or something.

Added.

> > nsTextStore::EndComposition(PRBool aDiscard)
> 
> Would you rename this? ResetComposition or CommitComposition or something.

Renamed to CommitComposition

(In reply to comment #56)
> I found a new bug.
> 
> Input some characters with Japanese MS-IME on Vista, and commit a composing by
> left click of mouse in the focused editor. And down the left button of the
> mouse, I can see the last composing selection at the committed string. If after
> the editor lost focus, I cannot see this bug.
> 
> I cannot reproduce this bug with Chinese and Korean IMEs :-(

It seems that the Japanese IME doesn't like it when we force commit a composition through code, so the next time when we click in the editor the IME will reconvert the composition, and we will commit it again, and so on. I will try to find a fix.

(In reply to comment #57)
> You should define NS_ENABLE_TSF in nsWindow.h, use it for these #ifdefs? It is
> too clear the reason of #ifdef.

Added.

> This is called *before* the editor lost focus, right? Then, nsEditor calls
> nsIWidget::ResetInputState() after sTsfThreadMgr is destroyed. Does this work
> correctly??

It's fine. The IMEs will commit any compositions before losing focus and ResetInputState will not do anything.

Comment 59

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: patch v2.2 addressing fixes

This contains all the fixes so far. I will continue to work on this bug, but since the GSoC deadline is today, the final evaluation will be based on this patch.

I think the goals of the project are largely achieved including the implementation and tests for TSF. As of now, there is still the Japanese MS-IME specific bug, and it might potentially be better to have a separate bug for it.

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm very very happy with your work. The FF 3.1 beta 1 freeze was moved out a few weeks so we should be able to land this in time for beta 1 and FF 3.1. This is fantastic!

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is there any reason to not enable TSF at build time --- is #ifdef NS_ENABLE_TSF really necessary?

It looks like TSF is always enabled and running at run time, if the TSF framework is present, even if no IME or other tool is in use --- is that correct? Is there anything we should worry about in terms of performance impact? It looks like we have to do extra work when focus changes, is there anything else to worry about?

Can you use nsCxPusher from nsContentUtils.h in nsINode::GetTextEditorRootContent?

+  nsCOMPtr<nsIAtom> typeAttr(do_GetAtom("type"));
+  nsCOMPtr<nsIAtom> bogusNodeAttr(do_GetAtom("_moz_editor_bogus_node"));

Put these in nsGkAtomList

+                                NS_LITERAL_STRING("_moz"),
+                                NS_LITERAL_STRING("TRUE"),

These should be atoms too. Although probably we should use a lower-case value of "true" so we can use the existing nsGkAtom.

+// Query for the bounding rect of the current focused frame
+#define NS_QUERY_FRAME_RECT             (NS_QUERY_CONTENT_EVENT_START + 5)
 
What's a "frame" here?

Why do we need QUERY_TEXT_RECT and QUERY_CHARACTER_RECT? Couldn't the latter just use QUERY_TEXT_RECT with length of 1?

+    PRBool mReversed; // true if selection is reversed (end < start)

PRPackedBool and move it to the end

+  PRBool mSucceeded;
+  PRUint32 mOffset;
+  PRUint32 mLength;
+  PRBool mReversed;

PRPackedBool and move them to the end

+nsRefPtr<ITfDisplayAttributeMgr> nsTextStore::sDAMgr;
+nsRefPtr<ITfCategoryMgr>         nsTextStore::sCatMgr;

Using global nsRefPtrs is bad. Use bare pointers and manually addref/release. Or why not just make them fields of nsTextStore?

+#ifdef DEBUG_IME_TSF
+      printf("TSF: Created, window=%08x\n", aWindow);
+#endif

If it's not too much trouble, it would be better to use PR_LOG.

+    // Some text services (Tablet Input Panel) will post "blur" messages
+    // and try to insert text when the message is retrieved later. But
+    // by that time the text store is already destroyed, so try to get
+    // these messages early
+    MSG msg;
+    HWND hwnd = mWindow->GetWindowHandle();
+    // Set a hard limit of how many messages to retrieve
+    // so we don't end up with an infinite loop
+    PRInt32 counter = 10;
+    while (--counter >= 0 &&
+           ::PeekMessageW(&msg, hwnd, 0xC000, 0xFFFF, PM_REMOVE)) {
+      ::DispatchMessageW(&msg);
+    }

This is very bad, we can't run the Windows message loop in nsTextStore::Destroy(), which is called during the nsWindow destructor. You're only processing a limited message range but I don't know what messages could be in that range and what they could do. Help me understand the problem here. This can only happen on shutdown, right? So is the problem that the Tablet Input Panel is causing a shutdown crash?

+  return !!mDocumentMgr;

I prefer the more obvious mDocumentMgr != NULL

+  if (TS_DEFAULT_SELECTION == ulIndex || 0 == ulIndex) {

We generally prefer to write this as an early exit rather than indent the bulk of the function.

+#ifdef NS_ENABLE_TSF
+  if (!sTsfThreadMgr) {
+    PRBool enableTsf = PR_TRUE;
+    nsCOMPtr<nsIPrefService> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
+    if (prefs) {
+      nsCOMPtr<nsIPrefBranch> prefBranch;
+      prefs->GetBranch(nsnull, getter_AddRefs(prefBranch));
+      if (prefBranch && NS_FAILED(prefBranch->GetBoolPref(
+            "config.windows.use_tsf", &enableTsf)))
+        enableTsf = PR_TRUE;
+    }
+    if (enableTsf) {

So it seems that when the preference is not set, we'll hit the prefs ever single time we call the nsWindow constructor. Would the same thing happen when TSF is not installed, e.g. on Windows 2000? I think it would be best to avoid that.

I think it would be best to put as much of the TSF code as possible in nsTextStore away from nsWindow. So I wonder whether we can actually make sTsfThreadMgr a field of nsTextStore and have a static method in nsTextStore that gets the threadmgr and if that's successful, creates and returns the nsTextStore. Then nsWindow wouldn't have to pass sTsfThreadMgr to nsTextStore calls.

So the test app actually builds with --enable-tests and --enable-libxul?

+  STDMETHODIMP SetFocus(ITfDocumentMgr *pdimFocus)
+  {
+    mFocused = !!pdimFocus;

Maybe NS_ASSERTION(!pdimFocus || pdimFocus == this)?

All the places you return E_NOTIMPL, it might also be helpful to put an NS_NOTREACHED.

+    // Adjust range if change is within range
+    if (LONG(mRangeStart) <= pChange->acpStart &&
+        LONG(mRangeStart + mRangeLength) >= pChange->acpOldEnd)
+      mRangeLength += PRUint32(pChange->acpNewEnd - pChange->acpOldEnd);

What if the change overlaps the range? If our code just doesn't do that, better NS_ASSERTION here to ensure that that stays true.

+    printf("TSF not initialized properly (TSF is not enabled/installed?)");

Just exit if this happens.

Seems like TestApp::TestSelection could be simplified by introducing a helper function that sets the selection and then tests that we've set it to what we wanted. In fact, just wrapping mImpl->mStore->SetSelection(1, ...) would be helpful.

TestApp::TestText has too many magic constants 0x100, 0x10, 18 etc. Use #defines or constants, with the names in UPPER_CASE, and increment variables if you're stepping through a buffer. Or find another way to make it obvious what's going on.

+        textRect1.bottom <= screenRect.bottom + 1 &&

What's going on here? The text rect can extend exactly one pixel past the screen rect? That seems strange.

TestApp::TestComposition also has a lot of magic numbers flying around :-)

+  if (!TestNotificationTextChange(widget, 0x41, characterA, 1, 2, 2)) {

Use 'A' instead of 0x41

+  if (!TestNotificationTextChange(widget, 0x08, character, 2, 3, 2)) {

use '\b'?

Where are fail() and succeeded() defined?

It would be great if there was a test that can check whether we're handling clustering properly. E.g., verify that the caret rect for a character+accent is equal to the bounds rect for just the accent *and* the bounds rect for just the character. (Assuming that *is* correct --- it's what we currently do, anyway.)

I still need to finish nsTextStore and the content/layout/view changes.

Comment 62

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to comment #60)
> I'm very very happy with your work. The FF 3.1 beta 1 freeze was moved out a
> few weeks so we should be able to land this in time for beta 1 and FF 3.1. This
> is fantastic!

Thanks and glad to hear that!

(In reply to comment #61)
> Is there any reason to not enable TSF at build time --- is #ifdef NS_ENABLE_TSF
> really necessary?

Right now NS_ENABLE_TSF is defined in nsWindows.h, inside a #ifndef WINCE block. Since WinCE doesn't have TSF, if we don't use NS_ENABLE_TSF, we would have to use a bunch of #ifndef WINCE. So NS_ENABLE_TSF might be more clear in this case.

> Can you use nsCxPusher from nsContentUtils.h in
> nsINode::GetTextEditorRootContent?

Tried that but didn't seemed to work because nsCxPusher doesn't like null argument to Push. There are few other places that also had to use this hack: http://mxr.mozilla.org/mozilla-central/search?string=Push(nsnull)

> +// Query for the bounding rect of the current focused frame
> +#define NS_QUERY_FRAME_RECT             (NS_QUERY_CONTENT_EVENT_START + 5)
> 
> What's a "frame" here?

I guess it could use a better name. The frame is the nsIFrame for the root content of the focused editor. For a text input or a textarea element, the root content of the editor would be an anonymous div. And this event's handler returns the bounds for that.

> Why do we need QUERY_TEXT_RECT and QUERY_CHARACTER_RECT? Couldn't the latter
> just use QUERY_TEXT_RECT with length of 1?

That's true. Maybe for another bug? Since getting rid of QUERY_CHARACTER_RECT involves changing the existing IME code.

> +nsRefPtr<ITfDisplayAttributeMgr> nsTextStore::sDAMgr;
> +nsRefPtr<ITfCategoryMgr>         nsTextStore::sCatMgr;
> 
> Or why not just make them fields of nsTextStore?

That would work.

> +    // Some text services (Tablet Input Panel) will post "blur" messages
> +    // and try to insert text when the message is retrieved later. But
> +    // by that time the text store is already destroyed, so try to get
> +    // these messages early
> +    MSG msg;
> +    HWND hwnd = mWindow->GetWindowHandle();
> +    // Set a hard limit of how many messages to retrieve
> +    // so we don't end up with an infinite loop
> +    PRInt32 counter = 10;
> +    while (--counter >= 0 &&
> +           ::PeekMessageW(&msg, hwnd, 0xC000, 0xFFFF, PM_REMOVE)) {
> +      ::DispatchMessageW(&msg);
> +    }
> 
> This is very bad, we can't run the Windows message loop in
> nsTextStore::Destroy(), which is called during the nsWindow destructor. You're
> only processing a limited message range but I don't know what messages could be
> in that range and what they could do. Help me understand the problem here. This
> can only happen on shutdown, right? So is the problem that the Tablet Input
> Panel is causing a shutdown crash?

nsTextStore::Destroy is called during blurring mostly (OnIMEFocusChange). Actually we don't have to call nsTextStore::Destroy in the nsWindow destructor.

What happens is whenever an editor is blurred, nsTextStore::Destroy is called. nsTextStore::Destroy calls the TSF blur code first and then destroys the text store. During the TSF blur code, most IMEs would try to commit any pending changes immediately. But the Tablet Input Panel is weird in that it simply posts a custom message to the host window, and later during ::GetMessage, its message hook would pick up the custom message and then commit any pending changes. The problem is since we destroy the text store right after the TSF blur code. By the time the custom message gets picked up, the text store is already destroyed and the pending changes are lost. So we try to get the custom message through after TSF blur code but before text store is destroyed. The Tablet Input Panel registers the custom message using ::RegisterWindowMessage, and so the message value is always between 0xC000 and 0xFFFF. That's why I have that message loop there after the Blur method call and before the destruction code, so the Tablet Input Panel hook can pick up its own message.

This has nothing to do with shutdown if that's what you are concerned about, and I'm taking out the Destroy call in nsWindow destructor, because in all cases the text store should already be blurred and destroyed by that time.

> So it seems that when the preference is not set, we'll hit the prefs ever
> single time we call the nsWindow constructor. Would the same thing happen when
> TSF is not installed, e.g. on Windows 2000? I think it would be best to avoid
> that.

I'm changing it to only try to initialize the first-time the nsWindow constructor is called.

> I think it would be best to put as much of the TSF code as possible in
> nsTextStore away from nsWindow. So I wonder whether we can actually make
> sTsfThreadMgr a field of nsTextStore and have a static method in nsTextStore
> that gets the threadmgr and if that's successful, creates and returns the
> nsTextStore. Then nsWindow wouldn't have to pass sTsfThreadMgr to nsTextStore
> calls.

OK, I'm migrating the code to nsTextStore.

> So the test app actually builds with --enable-tests and --enable-libxul?

Yes, but you might want to review widget/tests/Makefile.in to see if I did it right.

> +    // Adjust range if change is within range
> +    if (LONG(mRangeStart) <= pChange->acpStart &&
> +        LONG(mRangeStart + mRangeLength) >= pChange->acpOldEnd)
> +      mRangeLength += PRUint32(pChange->acpNewEnd - pChange->acpOldEnd);
> 
> What if the change overlaps the range? If our code just doesn't do that, better
> NS_ASSERTION here to ensure that that stays true.

We don't need to adjust range for other situations.

> +    printf("TSF not initialized properly (TSF is not enabled/installed?)");
> 
> Just exit if this happens.

I think we should still finish the test even if TSF is not installed, since the test doesn't depend on TSF being present on the system.

> +        textRect1.bottom <= screenRect.bottom + 1 &&
> 
> What's going on here? The text rect can extend exactly one pixel past the
> screen rect? That seems strange.

That's some leftover from a quirk of the old code :-)

> Where are fail() and succeeded() defined?

It's in the C++ unit test header, TestHarness.h

> It would be great if there was a test that can check whether we're handling
> clustering properly. E.g., verify that the caret rect for a character+accent is
> equal to the bounds rect for just the accent *and* the bounds rect for just the
> character. (Assuming that *is* correct --- it's what we currently do, anyway.)

Accents as in accents used in French etc.? Something like this? http://en.wikipedia.org/wiki/Combining_character

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #62)
> Right now NS_ENABLE_TSF is defined in nsWindows.h, inside a #ifndef WINCE
> block. Since WinCE doesn't have TSF, if we don't use NS_ENABLE_TSF, we would
> have to use a bunch of #ifndef WINCE. So NS_ENABLE_TSF might be more clear in
> this case.

Sure, sounds good.

> > Can you use nsCxPusher from nsContentUtils.h in
> > nsINode::GetTextEditorRootContent?
> 
> Tried that but didn't seemed to work because nsCxPusher doesn't like null
> argument to Push. There are few other places that also had to use this hack:
> http://mxr.mozilla.org/mozilla-central/search?string=Push(nsnull)

OK, I guess we need to refactor that at some point. But not necessary here.

> > +// Query for the bounding rect of the current focused frame
> > +#define NS_QUERY_FRAME_RECT             (NS_QUERY_CONTENT_EVENT_START + 5)
> > 
> > What's a "frame" here?
> 
> I guess it could use a better name. The frame is the nsIFrame for the root
> content of the focused editor. For a text input or a textarea element, the
> root content of the editor would be an anonymous div. And this event's handler
> returns the bounds for that.

OK. How about QUERY_EDITOR_RECT then?

I wonder what happens if you have something like <span contenteditable> breaking over a line so that the root content has multiple frames. I wouldn't be surprised if the editor code gets very confused...

> > Why do we need QUERY_TEXT_RECT and QUERY_CHARACTER_RECT? Couldn't the latter
> > just use QUERY_TEXT_RECT with length of 1?
> 
> That's true. Maybe for another bug? Since getting rid of QUERY_CHARACTER_RECT
> involves changing the existing IME code.

But not by much I imagine. Probably better to do it here so we don't go through an intermediate state of extra complexity.

> What happens is whenever an editor is blurred, nsTextStore::Destroy is called.
> nsTextStore::Destroy calls the TSF blur code first and then destroys the text
> store. During the TSF blur code, most IMEs would try to commit any pending
> changes immediately. But the Tablet Input Panel is weird in that it simply
> posts a custom message to the host window, and later during ::GetMessage, its
> message hook would pick up the custom message and then commit any pending
> changes. The problem is since we destroy the text store right after the TSF
> blur code. By the time the custom message gets picked up, the text store is
> already destroyed and the pending changes are lost. So we try to get the custom
> message through after TSF blur code but before text store is destroyed. The
> Tablet Input Panel registers the custom message using ::RegisterWindowMessage,
> and so the message value is always between 0xC000 and 0xFFFF. That's why I have
> that message loop there after the Blur method call and before the destruction
> code, so the Tablet Input Panel hook can pick up its own message.
> 
> This has nothing to do with shutdown if that's what you are concerned about,
> and I'm taking out the Destroy call in nsWindow destructor, because in all
> cases the text store should already be blurred and destroyed by that time.

I'm still nervous about processing arbitrary messages here. It could confuse our focus code. Is it possible to identify precisely the message the Tablet Input Panel is using?

> > So the test app actually builds with --enable-tests and --enable-libxul?
> 
> Yes, but you might want to review widget/tests/Makefile.in to see if I did it
> right.

It looks OK to me, but I'm not a build system wizard. We can get additional review for the buildsystem changes.

> > +    // Adjust range if change is within range
> > +    if (LONG(mRangeStart) <= pChange->acpStart &&
> > +        LONG(mRangeStart + mRangeLength) >= pChange->acpOldEnd)
> > +      mRangeLength += PRUint32(pChange->acpNewEnd - pChange->acpOldEnd);
> > 
> > What if the change overlaps the range? If our code just doesn't do that, better
> > NS_ASSERTION here to ensure that that stays true.
> 
> We don't need to adjust range for other situations.

Why not? Why don't we need to adjust the range if the change overlaps the start of the range? E.g. if our range is at offset 5, length 10, and someone replaces 2 characters at offset 4 with 4 new characters, shouldn't something happen to our range? I'm not sure what though.

> > +    printf("TSF not initialized properly (TSF is not enabled/installed?)");
> > 
> > Just exit if this happens.
> 
> I think we should still finish the test even if TSF is not installed, since the
> test doesn't depend on TSF being present on the system.

OK.

> > It would be great if there was a test that can check whether we're handling
> > clustering properly. E.g., verify that the caret rect for a character+accent is
> > equal to the bounds rect for just the accent *and* the bounds rect for just the
> > character. (Assuming that *is* correct --- it's what we currently do, anyway.)
> 
> Accents as in accents used in French etc.? Something like this?
> http://en.wikipedia.org/wiki/Combining_character

Yes.

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks like TSF supports multiple selections, but we're only exposing one selection, even though internally we do support multiple selections. Why's that?

+          (compNewStart > mCompositionStart ?
+              compNewStart - mCompositionStart : 0);

PR_MIN(compNewStart - mCompositionStart, 0)?

How correct is it for GetFormattedText to return E_NOTIMPL? What would use this?

+nsTextStore::QueryInsertEmbedded(const GUID *pguidService,
+                                 const FORMATETC *pFormatEtc,
+                                 BOOL *pfInsertable)
+{
+  // embedded objects are not supported
+  return E_NOTIMPL;

Wouldn't it make more sense to return false in pfInsertable to indicate that the object is not supported? I notice that E_NOTIMPL is not listed as a return value for QueryInsertEmbedded (although it IS listed for GetEmbedded).

+  // XXX we are relying on the behavior that if requested length >
+  // actual length, NS_QUERY_TEXT_CONTENT returns the entire text

Take out the XXX and just make sure this is documented somewhere in nsGUIEvent.h.

+nsTextStore::GetACPFromPoint(TsViewCookie vcView,
+                             const POINT *pt,
+                             DWORD dwFlags,
+                             LONG *pacp)
+{
+  NS_ENSURE_TRUE(TS_LF_READ == (mLock & TS_LF_READ), TS_E_NOLOCK);
+  NS_ENSURE_TRUE(TEXTSTORE_DEFAULT_VIEW == vcView, E_INVALIDARG);
+  // not supported for now
+  return E_NOTIMPL;

Is this really OK? It doesn't seem like it would be very hard to implement. At worst, you could write a loop that calls QUERY_TEXT_RECT once per character.

+  // rect might be zero-width and/or zero-height, fix here
+  event.mReply.mRect.SizeBy(1, 1);

Why do we need to do this?

+  nsWindow* refWindow = static_cast<nsWindow*>(
+      event.mReply.mFocusedWidget ? event.mReply.mFocusedWidget : mWindow);
+  refWindow = refWindow->GetTopLevelWindow();
+  NS_ENSURE_TRUE(refWindow, E_FAIL);
+
+  nsresult rv = refWindow->WidgetToScreen(event.mReply.mRect,
+                                          event.mReply.mRect);

mRect was relative to mReply.mFocusedWidget, but we're now treating as relative to refWindow even if refWindow changed due to calling GetTopLevelWindow?

It's not clear to me what pfClipped is supposed to mean here. What if the text is clipped by clipping in the application itself, e.g. due to CSS or something?

+  if (!boundRect.IsEmpty()) {
+    ::SetRect(prc, boundRect.x, boundRect.y,
+              boundRect.XMost(), boundRect.YMost());
+  } else {
+    ::SetRectEmpty(prc);
+  }

Don't we do this in other places? Surely we can reuse code to do the conversion.

+STDMETHODIMP
+nsTextStore::GetWnd(TsViewCookie vcView,
+                    HWND *phwnd)
+{
+  NS_ENSURE_TRUE(TEXTSTORE_DEFAULT_VIEW == vcView && phwnd, E_INVALIDARG);
+  *phwnd = mWindow->GetWindowHandle();
+  return S_OK;
+}

Just out of curiosity, do things break if we return NULL here? I really hate exposing our HWNDs since I want to make most of those HWNDs go away...

+        if (sel.acpEnd == sel.acpStart)
+          mCompositionString.Insert(pchText, realStart, cch);
+        else
+          mCompositionString.Replace(
+              realStart, sel.acpEnd - sel.acpStart, pchText, cch);

Why can't you just call Replace? I thought it should do the right thing if length is 0.

+      } else if (sel.acpEnd != sel.acpStart)
+        mCompositionString.Cut(realStart, sel.acpEnd - sel.acpStart);

Likewise you shouldn't need the if check here.

+      mCompositionSelection.acpEnd = (mCompositionSelection.acpStart += cch);

Put the += in its own statement.

+      // OnUpdateComposition is not called here because it will
+      // result in fun visual artifacts

Do you know why?

+        // XXX See OnEndComposition comment on inserting empty strings
+        event.theText = NS_LITERAL_STRING(" ");
+        mWindow->DispatchWindowEvent(&event);
+      }
+      event.theText.Assign(pchText, cch);
+      event.theText.ReplaceSubstring(NS_LITERAL_STRING("\r\n"),
+                                     NS_LITERAL_STRING("\n"));
+      mWindow->DispatchWindowEvent(&event);

Instead of this fake " " event, how about we just have a flag in the event to indicate whether to enable the "ignore empty string" hack? Better still, how about moving that hack out of content to the old-IME widget code?

+    if (index == event.rangeCount) {
+      event.rangeCount += 4;
+      event.rangeArray = reinterpret_cast<nsTextRange*>(nsMemory::Realloc(
+          event.rangeArray, sizeof(nsTextRange) * event.rangeCount));

Instead of doing this, it's probably better to use an "nsTArray<nsTextRange> ra ngeArray;", just call AppendElement as needed, and when it's all set up, set event.rangeArray = rangeArray.Elements() and event.rangeCount = rangeArray.Length().

In fact, it looks to me like thanks to the index += 2 below, you can actually overshoot the array end and crash...

+    if ((textRange - 1)->mStartOffset == PRUint32(start)) {
+      // Reuse range if last range is the same as this one
+      --textRange;
+      --index;

Is this necessary for correctness or just an optimization? If it's just an optimization I'd rather you didn't do it.

+    // Who came up with this convoluted way that we have to follow?

Wow, this is hilarious!

+  DWORD oldLock = mLock;
+  mLock = TS_LF_READWRITE;
+  SetSelection(1, &mCompositionSelection);
+  mLock = oldLock;

I'd slightly prefer a SetSelectionInternal method that doesn't take a selection count and doesn't check the lock (and is called by SetSelection).

+    mTextChange.acpStart = PR_MIN(mTextChange.acpStart, LONG(aStart));
+    mTextChange.acpOldEnd = PR_MAX(mTextChange.acpOldEnd, LONG(aOldEnd));
+    mTextChange.acpNewEnd = PR_MAX(mTextChange.acpNewEnd, LONG(aNewEnd));

So mTextChange just gets bigger and bigger ... this seems strange, why is that? It's especially strange for acpOldEnd to not actually be the old end.

You might want to share code between nsTextStore::SetIMEOpenState and nsTextStore::GetIMEOpenState to get/set the value. Possibly with nsTextStore::SetIMEEnabled as well.

It seems like it would be possible to leverage our IDataObject support for clipboard and drag-drop to actually support embedded objects and GetFormattedText. But it would certainly be a significant amount of work and I'm glad you haven't done it here. I'd like to know what benefit it would bring us if someone did it, though.

+  nsRefPtr<ITfDocumentMgr>     mDocumentMgr;
+  DWORD                        mEditCookie;
+  nsRefPtr<ITfContext>         mContext;
+  nsRefPtr<ITextStoreACPSink>  mSink;
+  DWORD                        mSinkMask;
+  nsWindow*                    mWindow;
+  DWORD                        mLock;
+  DWORD                        mLockQueued;
+  PRPackedBool                 mFocused;
+  TS_TEXTCHANGE                mTextChange;
+  nsRefPtr<ITfCompositionView> mCompositionView;
+  nsString                     mCompositionString;
+  TS_SELECTION_ACP             mCompositionSelection;
+  LONG                         mCompositionStart;
+  LONG                         mCompositionLength;

These could use some documentation, especially the mComposition stuff. I'd like some documentation about how the composition range interacts with the selection. It seems like your strategy is to keep the composition text in mCompositionString and not put it in the document, whereas TSF kind of expects it to be in the document. Is there a good reason to not put it in our document?

Phew! I'm more or less done with nsTextStore. Hopefully I'll finish the content changes tomorrow.

Comment 65

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to comment #63)
> I wonder what happens if you have something like <span contenteditable>
> breaking over a line so that the root content has multiple frames. I wouldn't
> be surprised if the editor code gets very confused...

Hmm, that might happen. I'm changing the QUERY_EDITOR_RECT handler though to account for multiple frames.

> > That's true. Maybe for another bug? Since getting rid of QUERY_CHARACTER_RECT
> > involves changing the existing IME code.
> 
> But not by much I imagine. Probably better to do it here so we don't go through
> an intermediate state of extra complexity.

OK, I'm getting rid of QUERY_CHARACTER_RECT

> I'm still nervous about processing arbitrary messages here. It could confuse
> our focus code. Is it possible to identify precisely the message the Tablet
> Input Panel is using?

Yes. I can change it to only process the TIP message.

> Why not? Why don't we need to adjust the range if the change overlaps the start
> of the range? E.g. if our range is at offset 5, length 10, and someone replaces
> 2 characters at offset 4 with 4 new characters, shouldn't something happen to
> our range? I'm not sure what though.

Hmm, I think you are right. I'm not sure what the correct way is though and the docs don't help much either. Since this feature isn't used by our code, I think I'll just leave the whole thing out instead of having an inaccurate implementation.

(In reply to comment #64)
> Looks like TSF supports multiple selections, but we're only exposing one
> selection, even though internally we do support multiple selections. Why's
> that?

I thought that'll just make things more complicated and I don't think any IME uses multiple selections. And it seemed the editor code didn't support multiple selections that well. E.g. strange things happen when you select two text ranges in the URL bar with Ctrl and try to delete them. I don't know if there's already a bug on that.

> How correct is it for GetFormattedText to return E_NOTIMPL? What would use
> this?

Nothing uses it AFAIK so I didn't try to implement it.

> +nsTextStore::QueryInsertEmbedded(const GUID *pguidService,
> +                                 const FORMATETC *pFormatEtc,
> +                                 BOOL *pfInsertable)
> +{
> +  // embedded objects are not supported
> +  return E_NOTIMPL;
> 
> Wouldn't it make more sense to return false in pfInsertable to indicate that
> the object is not supported? I notice that E_NOTIMPL is not listed as a return
> value for QueryInsertEmbedded (although it IS listed for GetEmbedded).

OK, makes sense.

> +nsTextStore::GetACPFromPoint(TsViewCookie vcView,
> +                             const POINT *pt,
> +                             DWORD dwFlags,
> +                             LONG *pacp)
> +{
> +  NS_ENSURE_TRUE(TS_LF_READ == (mLock & TS_LF_READ), TS_E_NOLOCK);
> +  NS_ENSURE_TRUE(TEXTSTORE_DEFAULT_VIEW == vcView, E_INVALIDARG);
> +  // not supported for now
> +  return E_NOTIMPL;
> 
> Is this really OK? It doesn't seem like it would be very hard to implement. At
> worst, you could write a loop that calls QUERY_TEXT_RECT once per character.

It's fine right now, because I haven't found any IME that uses it.

> +  // rect might be zero-width and/or zero-height, fix here
> +  event.mReply.mRect.SizeBy(1, 1);
> 
> Why do we need to do this?

When start == end, the width of the returned rect will be zero, and sometimes the height will be zero in an HTML document. A lot of IMEs don't like empty rects, so we have to make the rect nonempty.

> +  nsWindow* refWindow = static_cast<nsWindow*>(
> +      event.mReply.mFocusedWidget ? event.mReply.mFocusedWidget : mWindow);
> +  refWindow = refWindow->GetTopLevelWindow();
> +  NS_ENSURE_TRUE(refWindow, E_FAIL);
> +
> +  nsresult rv = refWindow->WidgetToScreen(event.mReply.mRect,
> +                                          event.mReply.mRect);
> 
> mRect was relative to mReply.mFocusedWidget, but we're now treating as relative
> to refWindow even if refWindow changed due to calling GetTopLevelWindow?

I made mRect relative to the top level window, to make it consistent with other query events.

> It's not clear to me what pfClipped is supposed to mean here. What if the text
> is clipped by clipping in the application itself, e.g. due to CSS or something?

Well the returned text rect is supposed to be within the rect returned by GetScreenExt. pfClipped is set to true if the text rect had to be clipped to stay within the GetScreenExt rect.

> +STDMETHODIMP
> +nsTextStore::GetWnd(TsViewCookie vcView,
> +                    HWND *phwnd)
> 
> Just out of curiosity, do things break if we return NULL here? I really hate
> exposing our HWNDs since I want to make most of those HWNDs go away...

Yeah, unfortunately it's pretty much required.

> +      // OnUpdateComposition is not called here because it will
> +      // result in fun visual artifacts
> 
> Do you know why?

Probably something with editor and layout but I'm not familiar enough with the code to find out exactly.

> +        // XXX See OnEndComposition comment on inserting empty strings
> +        event.theText = NS_LITERAL_STRING(" ");
> +        mWindow->DispatchWindowEvent(&event);
> +      }
> +      event.theText.Assign(pchText, cch);
> +      event.theText.ReplaceSubstring(NS_LITERAL_STRING("\r\n"),
> +                                     NS_LITERAL_STRING("\n"));
> +      mWindow->DispatchWindowEvent(&event);
> 
> Instead of this fake " " event, how about we just have a flag in the event to
> indicate whether to enable the "ignore empty string" hack? Better still, how
> about moving that hack out of content to the old-IME widget code?

The event handler is in editor and I'm afraid I'm not familiar with the code enough to make a proper change.

> +    if (index == event.rangeCount) {
> +      event.rangeCount += 4;
> +      event.rangeArray = reinterpret_cast<nsTextRange*>(nsMemory::Realloc(
> +          event.rangeArray, sizeof(nsTextRange) * event.rangeCount));
> 
> Instead of doing this, it's probably better to use an "nsTArray<nsTextRange> ra
> ngeArray;", just call AppendElement as needed, and when it's all set up, set
> event.rangeArray = rangeArray.Elements() and event.rangeCount =
> rangeArray.Length().

OK that sounds good.

> +    if ((textRange - 1)->mStartOffset == PRUint32(start)) {
> +      // Reuse range if last range is the same as this one
> +      --textRange;
> +      --index;
> 
> Is this necessary for correctness or just an optimization? If it's just an
> optimization I'd rather you didn't do it.

Yes, for correctness. I'll add a comment.

> +    mTextChange.acpStart = PR_MIN(mTextChange.acpStart, LONG(aStart));
> +    mTextChange.acpOldEnd = PR_MAX(mTextChange.acpOldEnd, LONG(aOldEnd));
> +    mTextChange.acpNewEnd = PR_MAX(mTextChange.acpNewEnd, LONG(aNewEnd));
> 
> So mTextChange just gets bigger and bigger ... this seems strange, why is that?
> It's especially strange for acpOldEnd to not actually be the old end.

Since we won't call the actual TSF method until when we get WM_USER_TSF_TEXTCHANGE later, we need to save the cumulative offsets. When we do get the message later in OnTextChangeMsg, mTextChange is reset and TSF called.

> It seems like it would be possible to leverage our IDataObject support for
> clipboard and drag-drop to actually support embedded objects and
> GetFormattedText. But it would certainly be a significant amount of work and
> I'm glad you haven't done it here. I'd like to know what benefit it would bring
> us if someone did it, though.

I think very little benefit if any. There is no TSF text service I'm aware of that uses embedded objects and GetFormattedText. So I think the effort, man-hour, and code size way outweighs the benefit.

> These could use some documentation, especially the mComposition stuff. I'd like
> some documentation about how the composition range interacts with the
> selection. It seems like your strategy is to keep the composition text in
> mCompositionString and not put it in the document, whereas TSF kind of expects
> it to be in the document. Is there a good reason to not put it in our document?

mCompositionString is like a buffer to keep the most current copy of the composition string. We only flush the composition string to the editor code in OnUpdateComposition by sending NS_TEXT_TEXT. This way we update the editor composition text in batches. Since a composition update batch can contain multiple SetSelection/InsertTextAtSelection/other calls, by using a buffer we avoid possible inconsistencies and artifacts.

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should get a security review of this feature. I've filled out the template here:
https://wiki.mozilla.org/Firefox3.1/TSF_Security_Review
At some point in the future we'll talk to some security people to go through the review. I'll let you know when that gets scheduled. If you can call into the review on the phone, great, otherwise I'll do my best to handle it.

I still need to review the content part of the patch.

Comment 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (childCount && childCount >= offset) {
+    node = node->GetChildAt(PR_MIN(offset, childCount - 1));
+    offset = node->IsNodeOfType(nsINode::eTEXT) ?
+        static_cast<nsIContent*>(node)->TextLength() : 1;
+  }

Can this start/end adjustment code be shared?

+  NS_ENSURE_TRUE(node && node->IsNodeOfType(nsINode::eCONTENT),
+                 NS_ERROR_UNEXPECTED);
+  nsIContent* content = static_cast<nsIContent*>(node);
+  nsIFrame* firstFrame = mPresShell->GetPrimaryFrameFor(content);
+  NS_ENSURE_TRUE(firstFrame, NS_ERROR_FAILURE);
+  rv = firstFrame->GetChildFrameContainingOffset(offset, PR_TRUE,
+                                                 &childOffset, &firstFrame);

This should be shareable too.

+  // get the starting frame rect
+  nsRect rect(nsPoint(0, 0), firstFrame->GetRect().Size());
+  rv = ConvertToRootViewRelativeOffset(firstFrame, rect);
+  NS_ENSURE_SUCCESS(rv, rv);
+  nsRect frameRect = rect;
+  nsPoint ptOffset;
+  firstFrame->GetPointFromOffset(offset, &ptOffset);
+  rect.x = ptOffset.x + (frameRect.x - firstFrame->GetRect().x);
+  rect.width -= rect.x - frameRect.x;

This is really confusing. I don't know why we're subtracting firstFrame->GetRect().x here. Can't we just do rect.x += ptOffset.x, rect.width -= ptOffset.x?

Hmm, I seem to recall code very much like this (i.e., given a DOM range, compute a bounding box) in our drag-drop code. You might want to take a look there.

Some documentation on what the new functions do would be really great. Some documentation around nsTextStateManager would be good too.

+  nsIPresShell* presShell = aPresContext->GetPresShell();
+  NS_ENSURE_TRUE(presShell, NS_ERROR_UNEXPECTED);

Just call ->PresShell(), this can't return null.

+  if (aNode->IsNodeOfType(nsINode::eCONTENT)) {
+    nsIFrame* frame = presShell->GetPrimaryFrameFor(
+                                     static_cast<nsIContent*>(aNode));
+    NS_ENSURE_TRUE(frame, NS_ERROR_UNEXPECTED);
+
+    frame->GetSelectionController(aPresContext,
+                                  getter_AddRefs(selCon));
+  } else {
+    selCon = do_QueryInterface(presShell);
+  }

Why not always take the GetPrimaryFrameFor path? In fact, how can we get here with aNode not being eCONTENT? Shouldn't we just make it nsIContent*?

+nsresult
+nsTextStateManager::Destroy()

This might as well just return void. In fact, why not just make it the destructor?

+  if (!aContent->IsNodeOfType(nsINode::eTEXT)) return;

Probably should make this an assertion, I don't think CharacterDataChanged should be called on any other kind of node.

+  if (!aChild->IsNodeOfType(nsINode::eTEXT) &&
+      !(aChild->IsNodeOfType(nsINode::eHTML) &&
+          aChild->Tag() == nsGkAtoms::br) &&
+      !aChild->GetChildCount())
+    return;

Why bother doing this? Wouldn't it be easier to check in NotifyContentAdded whether newOffset == offset? Similarly in ContentRemoved.

I think you can simplify nsTextStateManager a bit by making it inherit from nsStubMutationObserver.

There are already functions like GetRootEditableNode, did you check you can't use any of them? I'd search for uses of IsEditable...

That's all I have for now! Overall I'm still very impressed :-).

We probably should get a content/editor person (preferably peterv) to also review the content parts.

Comment 68

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to comment #66)
> We should get a security review of this feature. I've filled out the template
> here:
> https://wiki.mozilla.org/Firefox3.1/TSF_Security_Review
> At some point in the future we'll talk to some security people to go through
> the review. I'll let you know when that gets scheduled. If you can call into
> the review on the phone, great, otherwise I'll do my best to handle it.

I can do it if you let me know who to call etc. Thanks.

(In reply to comment #67)
> 
> Can this start/end adjustment code be shared?
>
> This should be shareable too.

OK

> This is really confusing. I don't know why we're subtracting
> firstFrame->GetRect().x here. Can't we just do rect.x += ptOffset.x, rect.width
> -= ptOffset.x?

Yeah you are right. Fixed.

> 
> +  if (aNode->IsNodeOfType(nsINode::eCONTENT)) {
> +    nsIFrame* frame = presShell->GetPrimaryFrameFor(
> +                                     static_cast<nsIContent*>(aNode));
> +    NS_ENSURE_TRUE(frame, NS_ERROR_UNEXPECTED);
> +
> +    frame->GetSelectionController(aPresContext,
> +                                  getter_AddRefs(selCon));
> +  } else {
> +    selCon = do_QueryInterface(presShell);
> +  }
> 
> Why not always take the GetPrimaryFrameFor path? In fact, how can we get here
> with aNode not being eCONTENT? Shouldn't we just make it nsIContent*?

aNode might be a document for design-mode documents.

> +nsresult
> +nsTextStateManager::Destroy()
> 
> This might as well just return void. In fact, why not just make it the
> destructor?

Destructor sounds good.

> +  if (!aContent->IsNodeOfType(nsINode::eTEXT)) return;
> 
> Probably should make this an assertion, I don't think CharacterDataChanged
> should be called on any other kind of node.

OK

> +  if (!aChild->IsNodeOfType(nsINode::eTEXT) &&
> +      !(aChild->IsNodeOfType(nsINode::eHTML) &&
> +          aChild->Tag() == nsGkAtoms::br) &&
> +      !aChild->GetChildCount())
> +    return;
> 
> Why bother doing this? Wouldn't it be easier to check in NotifyContentAdded
> whether newOffset == offset? Similarly in ContentRemoved.

OK

> I think you can simplify nsTextStateManager a bit by making it inherit from
> nsStubMutationObserver.

OK

> There are already functions like GetRootEditableNode, did you check you can't
> use any of them? I'd search for uses of IsEditable...

I couldn't find any to use.

> We probably should get a content/editor person (preferably peterv) to also
> review the content parts.

Should I contact him and/or add him as a reviewer?

Thank you for all your reviewing and comments!

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #68)
> Should I contact him and/or add him as a reviewer?

Post an updated patch and add him as a reviewer. Thanks!

Comment 70

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: patch v2.3, addressing roc's comments

peterv:
Could you please have a look at the content parts of the patch? Thank you!

Comment 71

[Marco Zehe (:MarcoZ)]

Implementing tSF will also help Vista speech recognition dictate text into web page and XUL textfields, see bug 395484.

Comment 72

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Peter:

What's the status of this review?

Comment 73

[Peter Van der Beken [:peterv]]

Comment on attachment 336214 [details] [diff] [review]
patch v2.3, addressing roc's comments

>diff --git a/content/base/public/nsIRange.h b/content/base/public/nsIRange.h

>+  virtual void SetRange(nsINode* aRoot,
>+                        nsINode* aStartParent, PRInt32 aStartOffset,
>+                        nsINode* aEndParent, PRInt32 aEndOffset) = 0;

I think it would make more sense to add a NS_NewRange function that takes these arguments and not add a new method to nsIRange.
It looks like this method will not do any checking on the arguments, which seems wrong for an API. In fact, it looks from your code that you might actually create invalid ranges using this function, where start is after end.

>diff --git a/content/base/src/nsGenericElement.cpp b/content/base/src/nsGenericElement.cpp
>--- a/content/base/src/nsGenericElement.cpp
>+++ b/content/base/src/nsGenericElement.cpp

>+
>+  // Push null JSContext to avoid security check in GetEditor
>+  nsCOMPtr<nsIJSContextStack> stack(
>+    do_GetService("@mozilla.org/js/xpc/ContextStack;1"));
>+  PRBool pushed = stack && NS_SUCCEEDED(stack->Push(nsnull));
>+
>+  nsIContent* content = nsnull;
>   for (nsINode* node = this; node; node = node->GetNodeParent()) {
>     nsCOMPtr<nsIDOMNSEditableElement> editableElement(do_QueryInterface(node));
>     if (!editableElement)
>       continue;

Make GetEditorInternal a method of nsITextControlElement, QI to that here and call GetEditorInternal. While doing that, change GetEditorInternal to |already_AddRefed<nsIEditor> GetEditor()|. Remove the code to push a context.

>diff --git a/content/events/src/nsQueryContentEventHandler.cpp b/content/events/src/nsContentEventHandler.cpp

>@@ -200,52 +223,59 @@ nsQueryContentEventHandler::GenerateFlat

>+  if (startNode == endNode && startNode->IsNodeOfType(nsINode::eHTML) &&
>+      static_cast<nsIContent*>(startNode)->Tag() == nsGkAtoms::br &&
>+      0 == aRange->StartOffset() && 0 < aRange->EndOffset()) {

Don't really understand what you're trying to do here, why does it matter that the range selects the children of the BR node (but not the BR node itself)?

>@@ -362,50 +392,57 @@ nsQueryContentEventHandler::SetRangeFrom

>+nsContentEventHandler::OnQuerySelectedText(nsQueryContentEvent* aEvent)

>+  PRInt16 compare = nsContentUtils::ComparePoints(
>+                        mFirstSelectedRange->GetStartParent(),
>+                        mFirstSelectedRange->StartOffset(),
>+                        mFirstSelectedRange->GetEndParent(),
>+                        mFirstSelectedRange->EndOffset());
>+  aEvent->mReply.mReversed = compare > 0;

A range's start point is always before its end point, so compare will always be <= 0 and mReversed will always be false?

>+  if (compare) {
>+    nsCOMPtr<nsIRange> range = mFirstSelectedRange;
>+    if (compare > 0) { // Reverse range
>+      range = new nsRange();
>+      NS_ENSURE_TRUE(range, NS_ERROR_OUT_OF_MEMORY);
>+      range->SetRange(mFirstSelectedRange->GetRoot(),
>+                      mFirstSelectedRange->GetEndParent(),
>+                      mFirstSelectedRange->EndOffset(),
>+                      mFirstSelectedRange->GetStartParent(),
>+                      mFirstSelectedRange->StartOffset());
>+    }

compare <= 0 (see above) so there's no need for this.

>+nsContentEventHandler::QueryRectFor(nsQueryContentEvent* aEvent,
>+                                    nsIRange* aRange,
>+                                    nsCaret* aCaret)

>+  if (aEvent->message == NS_QUERY_CARET_RECT) {

Can aEvent->message be different from NS_QUERY_CARET_RECT here? If not I would just move the code for QueryRectFor into OnQueryCaretRect.

>+static nsINode* AdjustTextRectNode(nsINode* aNode,
>+                                   PRInt32& aOffset)
>+{
>+  PRInt32 childCount = PRInt32(aNode->GetChildCount());
>+  nsINode* node = aNode;
>+  if (childCount && childCount >= aOffset) {
>+    node = aNode->GetChildAt(PR_MIN(aOffset, childCount - 1));
>+    aOffset = node->IsNodeOfType(nsINode::eTEXT) ?
>+        static_cast<nsIContent*>(aNode)->TextLength() : 1;

Shouldn't this call TextLength on node? You'll always return 0 if you call it on aNode.

>+static nsresult GetFrameForTextRect(nsIPresShell* aPresShell,

>+  nsresult rv = frame->GetChildFrameContainingOffset(
>+      aOffset, aHint, &childOffset, aReturnFrame);
>+  NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && *aReturnFrame, NS_ERROR_FAILURE);
>+  return NS_OK;

Why can't this just be |return rv;|? Does GetChildFrameContainingOffset return nsnull with a success rv?

>+nsContentEventHandler::OnQueryTextRect(nsQueryContentEvent* aEvent)

>   nsCOMPtr<nsIRange> range = new nsRange();

Make this nsRefPtr<nsRange> so you don't have to cast/QI below.

>+  rect.x += ptOffset.x - 1;
>+  rect.width -= ptOffset.x - 1;

>+  // minus 1 to avoid creating an empty rect
>+  frameRect.width -= lastFrame->GetRect().width - ptOffset.x - 1;

No need to adjust frameRect like you did for rect above?

>+nsContentEventHandler::GetFlatTextOffsetOfRange(nsIContent* aRootContent,

>+  nsCOMPtr<nsIDOMNode> rootDOMNode(do_QueryInterface(aRootContent));

Make GetFlatTextOffsetOfRange a static function (same for GenerateFlatTextContent btw)?

>+static PRBool AdjustRangeForSelection(nsIContent* aRoot, nsINode** aNode, PRInt32* aOffset)

Long line.

>+  if (static_cast<nsINode*>(aRoot) != node &&

Do you need the static_cast?

>+  *aOffset = 0 < offset ? offset : 0;

  *aOffset = PR_MAX(offset, 0);

>+nsContentEventHandler::OnSelectionEvent(nsSelectionEvent* aEvent)

>+  nsCOMPtr<nsIRange> range = new nsRange();

nsRefPtr<nsRange>

>+  if (adjustRange) {
>+    range->SetRange(range->GetRoot(),
>+                    startNode, startOffset, endNode, endOffset);

Use SetStart and SetEnd

>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp

>@@ -884,16 +885,19 @@ nsEventStateManager::PreHandleEvent(nsPr

>       if (mDocument) {
>+

Drop this newline.

>+        nsIMEStateManager::OnTextStateBlur(mPresContext, mCurrentFocus);

>@@ -5096,16 +5120,19 @@ nsEventStateManager::SendFocusBlur(nsPre

>+    nsIMEStateManager::OnTextStateFocus(mPresContext, mCurrentFocus);
>+

Same here.

>   } else if (!aContent) {

>diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp

>+  PRBool                         mDestroying;

What's the use of mDestroying?

>+nsTextStateManager::~nsTextStateManager()
>+{
>+}

No need for empty destructors.

>+nsTextStateManager::Create(nsIWidget* aWidget,

Why isn't this called Init?

>+nsTextStateManager::CharacterDataChanged(nsIDocument* aDocument,

>+  nsCOMPtr<nsIRange> range = new nsRange();
>+  if (!range) return;
>+  range->SetRange(mRangeRoot, aContent, aInfo->mChangeStart,
>+                  aContent, aInfo->mChangeStart);

Use SetStart

>+nsTextStateManager::NotifyContentAdded(nsINode* aContainer,
>+                                       PRInt32 aStartIndex,
>+                                       PRInt32 aEndIndex)

>+  range->SetRange(mRangeRoot, aContainer, aStartIndex,
>+                  aContainer, aStartIndex);

Use SetStart

>+  range->SetRange(mRangeRoot, aContainer, aEndIndex, aContainer, aEndIndex);

Use SetStart

>+nsTextStateManager::ContentInserted(nsIDocument* aDocument,

>+  nsINode* containerNode = (!aContainer ?
>+      static_cast<nsINode*>(aDocument) : static_cast<nsINode*>(aContainer));

Use NODE_FROM

>+nsTextStateManager::ContentRemoved(nsIDocument* aDocument,

>+  nsINode* startNode = !aContainer ?
>+      static_cast<nsINode*>(aDocument) : static_cast<nsINode*>(aContainer);

Use NODE_FROM

>+  nsCOMPtr<nsIRange> range = new nsRange();
>+  if (!range) return;
>+  range->SetRange(mRangeRoot, startNode, aIndexInContainer,
>+                  startNode, aIndexInContainer);

Use SetStart

Seems like we could avoid a lot of the ranges you create if there was a function like GetFlatTextOffsetOfRange that takes a node and an offset instead of a range. Creating ranges with a node and an offset just so that GetFlatTextOffsetOfRange can get that node and offset is silly.

>+static nsINode* GetRootEditableNode(nsPresContext* aPresContext,

>+  if (aContent) {
>+    if (!aContent->IsEditable())
>+      return nsnull;
>+    nsINode* child = aContent;
>+    nsINode* parent = child->GetParent();
>+    while (parent && parent->IsEditable()) {
>+      child = parent;
>+      parent = child->GetParent();
>+    }
>+    return child;

How about:

  if (aContent) {
    nsINode* root = nsnull;
    nsINode* node = aContent;
    while (node && node->IsEditable()) {
      root = node;
      node = node->GetParent();
    }
    return root;
  }

>+  } else if (aPresContext) {

Drop the else after return.

>+nsIMEStateManager::OnTextStateBlur(nsPresContext* aPresContext,

>+  NS_RELEASE(sTextStateObserver);

Are we sure this is always called before shutdown? It makes me nervous because sTextStateObserver leaks if not.

>+nsIMEStateManager::OnTextStateFocus(nsPresContext* aPresContext,

>+  if (sTextStateObserver) return NS_OK;

>+  NS_ENSURE_TRUE(!sTextStateObserver, NS_OK);

Why do you need to check this twice?

>+nsIMEStateManager::GetFocusSelectionAndRoot(nsISelection** aSel,
>+                                            nsIContent** aRoot)
>+{
>+  NS_ENSURE_ARG_POINTER(aSel && aRoot);

No need for this.

>+  NS_ENSURE_TRUE(sTextStateObserver->mSel &&
>+                 sTextStateObserver->mRootContent, NS_ERROR_UNEXPECTED);

Wouldn't an NS_ASSERTION be enough?

>diff --git a/content/events/src/nsIMEStateManager.h b/content/events/src/nsIMEStateManager.h

>+  // OnTextStateBlur should be called *before* NS_BLUR_CONTENT fires
>+  // aPresContext is the nsPresContext receiving focus (not lost focus)
>+  // aContent is the nsIContent receiving focus (not lost focus)

Please note that aPresContext and aContent may be null.

Comment 74

[(inactive) Jim Chen [:jchen] [:darchons]]

Thank you very much for the comments!

(In reply to comment #73)
> (From update of attachment 336214 [details] [diff] [review])
> I think it would make more sense to add a NS_NewRange function that takes these
> arguments and not add a new method to nsIRange.
> It looks like this method will not do any checking on the arguments, which
> seems wrong for an API. In fact, it looks from your code that you might
> actually create invalid ranges using this function, where start is after end.

OK. Actually SetRange is not needed now, if we just use SetStart/SetEnd.

> Make GetEditorInternal a method of nsITextControlElement, QI to that here and
> call GetEditorInternal. While doing that, change GetEditorInternal to
> |already_AddRefed<nsIEditor> GetEditor()|. Remove the code to push a context.

OK. Is this good? I kept the name GetEditorInternal to avoid confusion with nsIDOMNSEditableElement::GetEditor

diff --git a/content/html/content/public/nsITextControlElement.h b/content/html/content/public/nsITextControlElement.h

+  /**
+   * Return the editor associated with the text control
+   */
+  virtual already_AddRefed<nsIEditor> GetEditorInternal(void) = 0;

diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
 
+  already_AddRefed<nsIEditor> GetEditorInternal(void)
+  {
+    nsIEditor* editor = nsnull;
+    nsGenericHTMLElement::GetEditorInternal(&editor);
+    return editor;
+  }

diff --git a/content/html/content/src/nsHTMLTextAreaElement.cpp b/content/html/content/src/nsHTMLTextAreaElement.cpp
 
+  already_AddRefed<nsIEditor> GetEditorInternal(void)
+  {
+    nsIEditor* editor = nsnull;
+    nsGenericHTMLElement::GetEditorInternal(&editor);
+    return editor;
+  }

> >diff --git a/content/events/src/nsQueryContentEventHandler.cpp b/content/events/src/nsContentEventHandler.cpp
> 
> >@@ -200,52 +223,59 @@ nsQueryContentEventHandler::GenerateFlat
> 
> >+  if (startNode == endNode && startNode->IsNodeOfType(nsINode::eHTML) &&
> >+      static_cast<nsIContent*>(startNode)->Tag() == nsGkAtoms::br &&
> >+      0 == aRange->StartOffset() && 0 < aRange->EndOffset()) {
> 
> Don't really understand what you're trying to do here, why does it matter that
> the range selects the children of the BR node (but not the BR node itself)?

It was kind of a workaround. I'm removing it because I don't think it's needed now.

> >+nsContentEventHandler::OnQuerySelectedText(nsQueryContentEvent* aEvent)
> 
> >+  PRInt16 compare = nsContentUtils::ComparePoints(
> 
> A range's start point is always before its end point, so compare will always be
> <= 0 and mReversed will always be false?

OK I didn't realize that range start is always before end. I'm changing the logic.

> >+  rect.x += ptOffset.x - 1;
> >+  rect.width -= ptOffset.x - 1;
> 
> >+  // minus 1 to avoid creating an empty rect
> >+  frameRect.width -= lastFrame->GetRect().width - ptOffset.x - 1;
> 
> No need to adjust frameRect like you did for rect above?

Right, no need to do that.

> >+nsContentEventHandler::GetFlatTextOffsetOfRange(nsIContent* aRootContent,
> 
> >+  nsCOMPtr<nsIDOMNode> rootDOMNode(do_QueryInterface(aRootContent));
> 
> Make GetFlatTextOffsetOfRange a static function (same for
> GenerateFlatTextContent btw)?

I made GenerateFlatTextContent static, but since GetFlatTextOffsetOfRange is used by nsIMEStateManager, I kept it as a static member of nsContentEvents.

> >diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp
> 
> >+  PRBool                         mDestroying;
> 
> What's the use of mDestroying?

Sometimes OnTextStateBlur can cause OnTextStateBlur to be called again, and mDestroying prevents that.

> >+nsTextStateManager::Create(nsIWidget* aWidget,
> 
> Why isn't this called Init?

Just to match Destroy. I can change it to Init if that's the appropriate name. 

> Seems like we could avoid a lot of the ranges you create if there was a
> function like GetFlatTextOffsetOfRange that takes a node and an offset instead
> of a range. Creating ranges with a node and an offset just so that
> GetFlatTextOffsetOfRange can get that node and offset is silly.

You are right. I'm adding another GetFlatTextOffsetOfRange for this.

> >+nsIMEStateManager::OnTextStateBlur(nsPresContext* aPresContext,
> 
> >+  NS_RELEASE(sTextStateObserver);
> 
> Are we sure this is always called before shutdown? It makes me nervous because
> sTextStateObserver leaks if not.

If the focus/blur code works it will be called, but I'm going to make sure it will always be released.

> >+nsIMEStateManager::OnTextStateFocus(nsPresContext* aPresContext,
> 
> >+  if (sTextStateObserver) return NS_OK;
> 
> >+  NS_ENSURE_TRUE(!sTextStateObserver, NS_OK);
> 
> Why do you need to check this twice?

IIRC the widget->OnIMEFocusChange call can cause the focus to change, and in turn cause OnTextStateFocus to be called again. We check sTextStateObserver the second time to make sure that we simply return if this happens.

Comment 75

[Peter Van der Beken [:peterv]]

(In reply to comment #74)
> OK. Is this good? I kept the name GetEditorInternal to avoid confusion with
> nsIDOMNSEditableElement::GetEditor

On second thought, just check IsNodeOfType(eHTML), continue if it's false, cast to nsGenericHTMLElement if it's true and then call GetEditorInternal.

> > >+nsTextStateManager::Create(nsIWidget* aWidget,
> > 
> > Why isn't this called Init?
> 
> Just to match Destroy. I can change it to Init if that's the appropriate name. 

Yeah, a lot of other code uses Init.

> IIRC the widget->OnIMEFocusChange call can cause the focus to change, and in
> turn cause OnTextStateFocus to be called again. We check sTextStateObserver the
> second time to make sure that we simply return if this happens.

Please add a comment explaining this.

Comment 76

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to comment #75)
> (In reply to comment #74)
> > OK. Is this good? I kept the name GetEditorInternal to avoid confusion with
> > nsIDOMNSEditableElement::GetEditor
> 
> On second thought, just check IsNodeOfType(eHTML), continue if it's false, cast
> to nsGenericHTMLElement if it's true and then call GetEditorInternal.

We are using GetEditorInternal in /content/base/src/nsGenericElement.cpp so we can't include nsGenericHTMLElement.h from /content/html/content/src

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why not? content/base/src/nsContentUtils.cpp does. content/base/src/Makefile.in contains
INCLUDES        += \
...
                -I$(srcdir)/../../html/content/src \

So you should be OK. Admittedly, it's ugly, but better than duplicating code.

Comment 78

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: patch v2.4, addressing peterv's comments

(In reply to comment #77)
> Why not? content/base/src/nsContentUtils.cpp does. content/base/src/Makefile.in
> contains
> INCLUDES        += \
> ...
>                 -I$(srcdir)/../../html/content/src \
> 
> So you should be OK. Admittedly, it's ugly, but better than duplicating code.

Got it. Thanks.

Peter:

nsGenericHTMLElement::GetEditor and nsGenericHTMLElement::GetEditorInternal were protected so I made them public.

It would be great if you can review this new patch.

Comment 79

[Peter Van der Beken [:peterv]]

Comment on attachment 342375 [details] [diff] [review]
patch v2.4, addressing peterv's comments

Sorry for the delays.

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This missed the feature freeze by a lot, so we can't push it into FF3.1. But around mid-November FF3.1 will move onto its own branch and this can land on trunk at that time.

Comment 81

[(inactive) Jim Chen [:jchen] [:darchons]]

That sounds good. Thank you Peter and Rob!

Comment 82

[Tony Chung [:tchung]]

Adding to qa's work item to track.  Can we get an idea when/where this will eventually land, so we can think of testcases around it?  Thanks.

Comment 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This will land when the tree opens for 1.9.2.

Comment 84

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

TSF comprises of many components - it'll be good if there is a summary of what components there are and in which version of Windows.

I would like to help QA test regarding IME but I only have WinXP -- I'm sure there's loads of other parts that I'll be missing.

Is it possible to summarize all the possible test components in TSF (e.g. IME, handwriting) and whichever version of Windows they're in (e.g. XP, Vista) so comprehensive test coverage can be achieved?

Comment 85

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Bug 272847 will add a new IME state. I planned that the bug will be fixed after this bug. However, it will be landed to Gecko1.9.1. So, the patch of this bug may be needed to change.

Comment 86

[Marco Zehe (:MarcoZ)]

(In reply to comment #83)
> This will land when the tree opens for 1.9.2.

Robert, any idea exactly when this will happen? Are you waiting for bug 242847?

Comment 87

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Please wait this landing until bug 272847 will be fixed. The bug is needed for 1.9.1, we need to test it on trunk.

Comment 88

[Masatoshi Kimura [:emk]]

Bug 272847 landed on 191branch, but patch v2.4 no longer applies cleanly.
Chen, would you make an unbitrotted patch?

Comment 89

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: patch v2.5, updated to latest trunk

Kimura-san, here is the updated patch. I don't know how this affects Bug 272847, but it seems TSF doesn't affect it.


Gary, AFAIK TSF is native to every version of Windows that we support. Components that use TSF that I know are the included IME's in Windows (I tested mostly Microsoft Pinyin on Vista), Tablet Input Panel (TIP), and Windows Speech Recognition (in Vista).

Comment 90

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I added new IME status that is nsIWidget::IME_STATUS_PLUGIN. When a windowless plug-in is focused content, it is set.

Then, TSF should be handled by the plug-in itself. So, I think that we should think that our window doesn't have native focus at that time.

Comment 91

[Masatoshi Kimura [:emk]]

I think it's better to file a follow-up bug about the windowless plug-in issue.

Comment 92

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We'd better get this landed on trunk ASAP.

Comment 93

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc: When do you land this to trunk?

Comment 94

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I dunno. Can you land it?

Comment 95

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

ok, I'll try..

Comment 96

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.6

updated for latest trunk. but tinderbox of moz-central is not all green now...

Comment 97

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, the new test failed:

> TestFocus PASSED!
> TestClustering PASSED!
> Testing TSF support in text input element...
> TestSelection (input) PASSED!
> TestText (input) PASSED!
> TestExtents (input) PASSED!
> TestComposition (input) PASSED!
> FAIL TestNotification: text change 1
> Testing TSF support in textarea element...
> TestSelection (textarea) PASSED!
> TestText (textarea) PASSED!
> TestExtents (textarea) PASSED!
> TestComposition (textarea) PASSED!
> FAIL TestNotification: text change 1

Do you know the reason?

Comment 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Masayuki-san, it would be great if you could debug it.

Comment 99

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> +PRBool
> +TestApp::TestNotificationTextChange(nsIWidget* aWidget,
> +                                    PRUint32 aCode,
> +                                    const nsAString& aCharacter,
> +                                    LONG aStart,
> +                                    LONG aOldEnd,
> +                                    LONG aNewEnd)
> +{
> +  MSG msg;
> +  if (::PeekMessageW(&msg, NULL, WM_USER_TSF_TEXTCHANGE,
> +                     WM_USER_TSF_TEXTCHANGE, PM_REMOVE))
> +    ::DispatchMessageW(&msg);
> +  mImpl->mTextChanged = PR_FALSE;
> +  nsresult nsr = aWidget->SynthesizeNativeKeyEvent(0, aCode, 0,
> +                              aCharacter, aCharacter);
> +  if (::PeekMessageW(&msg, NULL, WM_USER_TSF_TEXTCHANGE,
> +                     WM_USER_TSF_TEXTCHANGE, PM_REMOVE))
> +    ::DispatchMessageW(&msg);
> +  return NS_SUCCEEDED(nsr) &&
> +         mImpl->mTextChanged &&
> +         aStart == mImpl->mTextChangeData.acpStart &&
> +         aOldEnd == mImpl->mTextChangeData.acpOldEnd &&
> +         aNewEnd == mImpl->mTextChangeData.acpNewEnd;
> +}

mImpl->mTextChanged, aStart == mImpl->mTextChangeData.acpStart, aOldEnd == mImpl->mTextChangeData.acpOldEnd, aNewEnd == mImpl->mTextChangeData.acpNewEnd are false...

Comment 100

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, I missed to merge the message handler of nsWindow. It should be the cause.

Comment 101

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.7

ok, this passes to the test. I'll land this patch.

Comment 102

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

pushed to trunk.
http://hg.mozilla.org/mozilla-central/rev/fc8d54ab29a6

Comment 103

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

-> FIXED

Comment 104

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

ok, the tinderbox doesn't have new failures by the patch.

Congratulations, Jim! And thank you for your great work!

Comment 105

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Testers:

If you have some major/critical problem by this patch, you can disable the TSF code by that you set "config.windows.use_tsf" to false.

# oh, the pref name has new root "config"... we should rename it in another bug...

Comment 106

[pal-moz]

0207 Nightly,

always IME is ON automatically, when focus on input area, including adressbar.
and typed Japanese words is not displayed soon after typed.

Comment 107

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: screenshot

(In reply to comment #106)
> 0207 Nightly,
> 
> always IME is ON automatically, when focus on input area, including adressbar.
> and typed Japanese words is not displayed soon after typed.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090207 Minefield/3.2a1pre

Confirming with pal-moz - I'm not seeing any characters displayed using IME with this nightly, but it works properly in Fx 3.0.6.

Usually, there's a few steps to type using IME: (I use the example of pinyin to type simplified chinese using Microsoft Pinyin IME 3.0)

1. Type hanyu pinyin. (e.g. wo for me in chinese) Press spacebar.
2. The character should show with an underline underneath - in 3.0.6 it does, but not in this nightly, see screenshot. This is also the stage where one can press the right arrow key to search for other chars with hanyu pinyin "wo". The search for other chars does work properly.
3. Press spacebar to confirm char input. The char works correctly.

Thus, stage 2 doesn't work well. I think it's exactly what pal-moz is seeing. I don't know if this bug should be reopened or whether a regression bug should be filed.

Thoughts?

Comment 108

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #107)
> Confirming with pal-moz - I'm not seeing any characters displayed using IME
> with this nightly, but it works properly in Fx 3.0.6.

Nit: ... not seeing any characters displayed "when in underlined mode -- i.e. the mode to choose characters" using IME with this nightly.

Setting config.windows.use_tsf to false makes this issue go away.

Comment 109

[pal-moz]

(In reply to comment #108)
> (In reply to comment #107)
> > Confirming with pal-moz - I'm not seeing any characters displayed using IME
> > with this nightly, but it works properly in Fx 3.0.6.
> 
> Nit: ... not seeing any characters displayed "when in underlined mode -- i.e.
> the mode to choose characters" using IME with this nightly.
> 
> Setting config.windows.use_tsf to false makes this issue go away.

thanks.


but I think IME should not be ON automatically.

Comment 110

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #104)
> ok, the tinderbox doesn't have new failures by the patch.

That actually wasn't the case.  The windows unit test machine has been orange every cycle since this patch landed, so I backed it out.  (I did it manually so that my backout also had renames in it, since hg backout doesn't do renames correctly.)
The backout is http://hg.mozilla.org/mozilla-central/rev/26c31b847949 .

The test failure on tinderbox showed up as TUnit being orange and running too few tests; the error was:

Running TestWinTSF (bug #88831) tests...
TSF not initialized properly (TSF is not enabled/installed?)FAIL TestFocus: document focus was not set
FAIL no text store (clustering)
Testing TSF support in text input element...
FAIL no text store (input)
Testing TSF support in textarea element...
FAIL no text store (textarea)
Finished running TestWinTSF (bug #88831) tests.
NEXT ERROR make[2]: *** [check] Error 1
Running TestWinTSF (bug #88831) tests...
TSF not initialized properly (TSF is not enabled/installed?)FAIL TestFocus: document focus was not set
FAIL no text store (clustering)
Testing TSF support in text input element...
FAIL no text store (input)
Testing TSF support in textarea element...
FAIL no text store (textarea)
Finished running TestWinTSF (bug #88831) tests.
make[2]: Leaving directory `/e/builds/moz2_slave/mozilla-central-win32-unittest/build/objdir/widget/tests'
make[1]: Leaving directory `/e/builds/moz2_slave/mozilla-central-win32-unittest/build/objdir/widget'
NEXT ERROR make[2]: *** [check] Error 1
make[1]: *** [check] Error 2
make: *** [check] Error 2


I'd note there are two (or maybe three) problems here:
 (1) the test is failing
 (2) the test is getting run twice (is there a problem in the Makefile?)
 (3) Maybe if the test used "TEST-PASS" and "TEST-UNEXPECTED-FAIL" (rather than just FAIL) the failures would get highlighted properly on tinderbox?  I'm not sure.  You could ask somebody more familiar with unit tests.  I think only a few unit tests seem to do that, but they're among the newer ones.

Comment 111

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, I didn't check the log, tinderbox doesn't display the error count of TUnit correctly??

And the log result is strange. Is it Win2k machine? If so, we cannot test of TSF implementation automatically :-(

Thank you for the backing-out. And I'll check the problem on XP. I don't see the regression on Vista.

Comment 112

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The Windows unit test machine is Windows Server 2003 (also known as WINNT 5.2).  (XP is WINNT 5.1; Vista is WINNT 6.0.)

Comment 113

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks like TSF is not installed Windows Server 2003 R2 (Standard Edition). Because I cannot the settings for TSF in language setting dialog.

Comment 114

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Because I cannot the settings for TSF in language setting dialog.
Oops, I meant: I cannot "find" the...

Comment 115

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc:

I'll post a new patch which has following changes:

1. the automated test will not fail on non-TSF supported platform (we cannot run the tests on tinderbox machine, unfortunately :-( )
2. temporary, the TSF support disabled by pref for nightly testers of XP.

I hope that you allow to land the new patch to trunk.

Because the patch has renaming files. It is too troublesome for fixing the bug of the patch. And the changes for fixing bugs of TSF support should be independent patches for review process (of course, the bugs should be separated from this bug too).

And also the patch changes very many points which are in my working area in this several month. I cannot write other patches by this patch.

Comment 116

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #110)
> I'd note there are two (or maybe three) problems here:
>  (1) the test is failing

This is platform issue. We cannot test TSF support on WinServer 2003, probably (If we can install only TSF, we can do it).

>  (2) the test is getting run twice (is there a problem in the Makefile?)

I'm not sure this issue.

>  (3) Maybe if the test used "TEST-PASS" and "TEST-UNEXPECTED-FAIL" (rather than
> just FAIL) the failures would get highlighted properly on tinderbox?  I'm not
> sure.  You could ask somebody more familiar with unit tests.  I think only a
> few unit tests seem to do that, but they're among the newer ones.

in compiled code, we can use only |fail| and |passed|.
http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestHarness.h
the test is using |fail|, of course.

Comment 117

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.8

temporary, disabling TSF support by pref (intl.enable_tsf_support).

I hope this patch should be landed, and TSF will be enabled after the critical bugs will be gone.

Comment 118

[(inactive) Jim Chen [:jchen] [:darchons]]

Hi, I'm working on an updated patch that should make the unit test work.

I originally designed the test so that it can be run even if TSF is not installed, but I just found a bug that disabled this. I'm working on the fix right now.

I don't know why the test is run twice but I'm looking into it, as well as the IME regression.

Comment 119

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 361081 [details] [diff] [review]
Patch v2.8

Welcome back, Jim.

I'm suggesting that if you cannot fix the major/critical IME regression on XP, we should disable TSF support from pref.

Comment 120

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I couldn't build the patch on my XP environment which is not installed Vista SDK. The error is "cannot open msctf.h". So, should we check "--disable-vista-sdk-requirements" at to define NS_ENABLE_TSF? It seems that if it is set, we should not enable TSF support.

Comment 121

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, the VS version is Professional of 2005. And see following document for "--disable-vista-sdk-requirements".
https://developer.mozilla.org/en/Windows_Build_Prerequisites#Microsoft_Windows_SDK

Comment 122

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

some updates here:

1. I cannot build without "--disable-vista-sdk-requirements" on XP + VS2005SP1 + Windows SDK 6.0. Because I cannot fix wincapi.h error (http://developer.mozilla.org/en/docs/wpcapi.h). But I could build TSF supportable build. It seems that we should have new compile option for TSF support enabling.

2. On WinXP + MS-IME, |OnUpdateComposition| is called with pRangeNew == null. Therefore, we don't update IME selection. Looks like we need to create the log of |InsertTextAtSelection|. And we should use it if pRangeNew is null.

3. On WinXP + MS Pinyin, |nsTextStore::SetSelectionInternal|'s |NS_ENSURE_TRUE(pSelection->acpStart >= mCompositionStart &&...| and |nsTextStore::SetText|'s |NS_ENSURE_TRUE(SUCCEEDED(SetSelectionInternal(&selection)), E_FAIL);| are failed. Looks like this is a cause of comment 107.

Comment 123

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: patch v2.9 (fixes test on Win2k3)

Thank you Masayuki-san!

I have an updated patch here that should make the test work on Win2k3, and run only once. TSF is disabled by default also. So I think it's safe to land this if we want to, while we work on the XP IME bugs.

Comment 124

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Jim, looks good for me. and the test can run even if the TSF support code is not loaded by the pref (I confirmed it on my environment).

Roc, do you agree to land the patch? If you agree to land it to trunk, I'll land it, ASAP.

Comment 125

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please do it.

Comment 126

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

pushed to trunk.
http://hg.mozilla.org/mozilla-central/rev/3cb3ab57d0b3

Comment 127

[David Gaskill]

I am the guy that originally reported  Bug 395484

It is stated that, "Bug 395484 depends on Bug 88831".

It is also stated that resolution of 395484 pence upon the implementation of TSF support.

In comment #123 it is stated that, "TSF is disabled by default".

I am not a programmer - just someone who wants to use Vista's speech recognition application to dictate into Firefox and Thunderbird - but it seems to me that the bug is is not RESOLVED/FIXED

Comment 128

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #127)
> I am not a programmer - just someone who wants to use Vista's speech
> recognition application to dictate into Firefox and Thunderbird - but it seems
> to me that the bug is is not RESOLVED/FIXED

Ideally, this bug's status should be same as the actual status of TSF implementation. However, if we do so, we cannot mark the status to FIXED forever. I'll file new bugs for regressions and to enable TSF in default settings. They will block this bug, you can track the actual TSF support status by them.

Comment 129

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

backed-out. the patch might be a cause of timeout of test_keycodes.xul. I want to watch the tinderbox state without this patch.

Comment 130

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

simplest failure log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234280239.1234287493.31852.gz

Comment 131

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

this is not the cause. I relanded the patch.
http://hg.mozilla.org/mozilla-central/rev/4db1cd62b2c3

Comment 132

[David Gaskill]

I don't understand.  Has the TSF issue been resolved?  If not how do I track the support status?

Comment 133

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #132)
> I don't understand.  Has the TSF issue been resolved?  If not how do I track
> the support status?

This bug is for the addition of the TSF support. It has been resolved, though this does not mean it is turned on by default on all applicable OSes.

Bug 478029 tracks the outstanding issues before it is turned on by default on some OSes.

Comment 134

[Ryan VanderMeulen [:RyanVM]]

I'm getting the following compiler error on Vista building with VS8SP1 and I'm pretty it's from this patch.
mozilla-central\widget\src\windows\nsTextStore.h(45) : fatal error C1083: Cannot open include file: 'msctf.h': No such file or directory

Does this require the Vista SDK or something?

Comment 135

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to comment #134)
> I'm getting the following compiler error on Vista building with VS8SP1 and I'm
> pretty it's from this patch.
> mozilla-central\widget\src\windows\nsTextStore.h(45) : fatal error C1083:
> Cannot open include file: 'msctf.h': No such file or directory
> 
> Does this require the Vista SDK or something?

If I recall correctly VS2003 shipped with msctf.h and other TSF headers, but for some reason it was left out of VS2005 and probably some versions of Windows SDK. So yes you would need to install the Vista SDK.

We should probably do something about this.

Comment 136

[Nochum Sossonko [:Natch]]

Per https://developer.mozilla.org/en/Windows_Build_Prerequisites#Microsoft_Windows_SDK Vista SDK is necessary anyways, maybe just include a comment there about this as well?

Comment 137

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I filed bug 478618 for the build error by VS2005 without Vista SDK.

Please file new bugs for the bugs of this bug's patch  (and block this bug by the bugs) instead of enlarging this bug.

Comment 138

[Boris Zbarsky [:bzbarsky]]

This apparently causes crash bug 478245.

Comment 139

[Tony Chung [:tchung]]

hey gary, 
Look like this has landed on trunk, but not yet enabled. Any chance you can QA
this fix with some IME testing?   We should also get more ideas from roc on how
else this can be tested outside of inputting text.

Comment 140

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #139)
> hey gary, 
> Look like this has landed on trunk, but not yet enabled. Any chance you can QA
> this fix with some IME testing?   We should also get more ideas from roc on how
> else this can be tested outside of inputting text.

I did - see https://bugzilla.mozilla.org/show_bug.cgi?id=88831#c107 - further problems should be tracked at the follow up bug.

Comment 141

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Tony, see comment #7, comment #29, and comment #39 for examples of Windows tools that exercise the TSF code.

Comment 143

[Jim Mathies [:jimm]]

This addresses issues with the touch/pen input panel on win7 (bug 286072). Do we have any issues with enabling this by default?

Comment 144

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

See the related bugs, several IMEs don't work fine. There are many critical/major bugs. If we enable TSF support in default settings now, many CJK users cannot use nightly builds.

Comment 145

[Jim Mathies [:jimm]]

(In reply to comment #144)
> See the related bugs, several IMEs don't work fine. There are many
> critical/major bugs. If we enable TSF support in default settings now, many CJK
> users cannot use nightly builds.

Mid-air collision - 

Never mind, I found Bug 478029. ;)

Comment 146

[Jim Mathies [:jimm]]

This request relates to some testing done by MS on Fx for win7. MSFT-7923 / MSFT-10204

Comment 147

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

What? Do you understand the impact of the patch? Our TSF implementation is too immature. We cannot take the patch to branch.

Comment 148

[Jim Mathies [:jimm]]

(In reply to comment #147)
> What? Do you understand the impact of the patch? Our TSF implementation is too
> immature. We cannot take the patch to branch.

Beltzner asked me to nom, he'll be passing through here shortly I'm sure.

Comment 149

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 361222 [details] [diff] [review]
patch v2.9 (fixes test on Win2k3)

a191- as per Masayuki's comments.

Comment 150

[Boris Zbarsky [:bzbarsky]]

It's very likely that this patch (after the change made to it in bug 460059) causes O(N^3) behavior or so when pasting into a textarea.  See bug 496360.

Comment 151

[(not currently active) Ted Mielczarek]

(In reply to comment #136)
> Per
> https://developer.mozilla.org/en/Windows_Build_Prerequisites#Microsoft_Windows_SDK
> Vista SDK is necessary anyways, maybe just include a comment there about this
> as well?

We do require the Vista SDK to compile with all platform features, but we also support disabling those features from configure. We allow specifying --with-windows-version=502 (formerly --disable-vista-sdk-requirements), which should produce a working build with a Server 2003 SDK or just Visual C++ 2005 Pro. Can we skip compiling this code if using an older SDK? We can add a header check to configure, or just use the windows target version. You can see an example of code that's conditionally compiled when targeting the Vista SDK or newer here:
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/decoders/icon/win/nsIconChannel.h#88

Comment 152

[Masatoshi Kimura [:emk]]

I've filed bug 509179.