Closed Bug 785534 Opened 8 years ago Closed 8 years ago

Load TSF widget module into metrofx

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [metro-mvp][LOE:2][metro-it2][completed-elm])

Attachments

(1 file, 15 obsolete files)

42.79 KB, patch
emk
: review+
Details | Diff | Splinter Review
Investigate this module to see if it is working correctly and consider pref'ing it on by default for metrofx.
Masayuki, we sure could use the help in getting metro fx working properly with imes, keyboard input, mouse, uia, and everything else under the sun. 

We are all currently using the Samsung Series 7 for testing purposes. Here's my Samsung order bug - bug 738613.
nsTextStore is going to need minor work. It currently assumes the nsWindow pointer it has is an nsWindow and calls apis from there to get at window handles. When running under metro the window pointer will be a MetroWidget pointer instead. nsTextStore needs to recognize the difference between the two and make the appropriate calls on each.
Blocks: 783882
Assignee: nobody → jmathies
Depends on: 785425
If there were the most serious issue, TIP (IME for TSF) would stop its work when we return error during composition. Currently, our implementation work like singleton. There is only one nsTextStore instance and it stores only one ITfCompositionView.

If TIP starts composition with another ITfCompositionView instance during composition, we will return error from composition related methods.

We guess that it causes some reports which reported that IME completely stopped to work until rebooting Firefox.

Anyway, I'll give higher priority for TSF related bugs.
Attached patch initial rev (obsolete) — Splinter Review
Masayuki I wasn't sure if the Imm calls we make in nsWindow were needed here, so I only hooked up nsTextStore. If we need Imm as well I can try to get nsIMM32Handler hooked up as well. But I guess metro doesn't support that mode, so I'm thinking it's not needed.
Attached patch initial rev (obsolete) — Splinter Review
with proper ifdef'ing around MetroWidget.
Attachment #656898 - Attachment is obsolete: true
(In reply to Jim Mathies [:jimm] from comment #4)
> Created attachment 656898 [details] [diff] [review]
> initial rev
> 
> Masayuki I wasn't sure if the Imm calls we make in nsWindow were needed
> here, so I only hooked up nsTextStore. If we need Imm as well I can try to
> get nsIMM32Handler hooked up as well. But I guess metro doesn't support that
> mode, so I'm thinking it's not needed.

I guess that IMM is completely unavailable on Metro application. So, maybe, you don't need to do that. And you don't need to add the |#ifdef NS_ENABLE_TSF|s in the MetroWidget because Metro applications shouldn't work without TSF.
Attached patch patch v.1 (obsolete) — Splinter Review
Cleaned up. I still need to push this through try to be sure all the ifdef'ing is right in cases where MOZ_METRO isn't defined.

> +class nsTextEvent;

Note this likely a difference between elm and mc and can be ignored. I'll do a merge before I land this.
Attachment #656902 - Attachment is obsolete: true
Attachment #657256 - Flags: review?(masayuki)
Attached patch patch v.2 (obsolete) — Splinter Review
Just the mc bits and a build bustage fix when MOZ_METRO isn't defined.
Attachment #657256 - Attachment is obsolete: true
Attachment #657256 - Flags: review?(masayuki)
Attachment #657279 - Flags: review?(masayuki)
https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=909181fed108

try builds going with tsf enabled.
Comment on attachment 657279 [details] [diff] [review]
patch v.2

Basically, it's okay. But this patch completely breaks TSF support on desktop firefox. Though, I'm not sure why it is. When I try to input with TIP, looks like the TIPs works via IMM. Because I don't see some bugs on TSF mode and TIP specified style in composition string.

>+// FocusWidget helper
>+
>+FocusWidget::FocusWidget() :
>+  mWindow(nullptr),
>+  mMetroWidget(nullptr)
>+{
>+}
>+
>+void FocusWidget::SetWin32Window(nsWindow* aWindow) { mWindow = aWindow; }
>+#ifdef MOZ_METRO
>+void FocusWidget::SetMetroWindow(MetroWidget* aWindow) { mMetroWidget = aWindow; }
>+#endif
>+bool FocusWidget::IsEmpty() { return (!mWindow && !mMetroWidget); }
>+HWND FocusWidget::GetWindowHandle() { return (HWND)this; }
>+bool FocusWidget::Destroyed() { return (mWindow ? mWindow->Destroyed() : false); }
>+
>+FocusWidget::operator const HWND()
>+{
>+  if (mWindow)
>+    return static_cast<HWND>(mWindow->GetNativeData(NS_NATIVE_WINDOW));
>+#ifdef MOZ_METRO
>+  else if (mMetroWidget)
>+    return static_cast<HWND>(mMetroWidget->GetNativeData(NS_NATIVE_WINDOW));
>+#endif
>+  return NULL;
>+}

Please use {} for the |if| and |else if|.

>+bool FocusWidget::DispatchWindowEvent(nsGUIEvent* event)
>+{
>+  if (mWindow)
>+    return mWindow->DispatchWindowEvent(event);
>+#ifdef MOZ_METRO
>+  if (mMetroWidget)
>+    return mMetroWidget->DispatchWindowEvent(event);
>+#endif
>+  return false;
>+}

Same.

>@@ -1089,35 +1170,37 @@ nsTextStore::GetTextExt(TsViewCookie vcV
>                         BOOL *pfClipped)
> {
>   NS_ENSURE_TRUE(TS_LF_READ == (mLock & TS_LF_READ), TS_E_NOLOCK);
>   NS_ENSURE_TRUE(TEXTSTORE_DEFAULT_VIEW == vcView && prc && pfClipped,
>                  E_INVALIDARG);
>   NS_ENSURE_TRUE(acpStart >= 0 && acpEnd >= acpStart, TS_E_INVALIDPOS);
> 
>   // use NS_QUERY_TEXT_RECT to get rect in system, screen coordinates
>-  nsQueryContentEvent event(true, NS_QUERY_TEXT_RECT, mWindow);
>-  mWindow->InitEvent(event);
>+  nsQueryContentEvent event(true, NS_QUERY_TEXT_RECT, (nsIWidget*)mWidget);
>+  mWidget.InitEvent(event);
>   event.InitForQueryTextRect(acpStart, acpEnd - acpStart);
>-  mWindow->DispatchWindowEvent(&event);
>+  mWidget.DispatchWindowEvent(&event);
>   NS_ENSURE_TRUE(event.mSucceeded, TS_E_INVALIDPOS);
>   // IMEs don't like empty rects, fix here
>   if (event.mReply.mRect.width <= 0)
>     event.mReply.mRect.width = 1;
>   if (event.mReply.mRect.height <= 0)
>     event.mReply.mRect.height = 1;
> 
>+#ifndef MOZ_METRO
>   // convert to unclipped screen rect
>   nsWindow* refWindow = static_cast<nsWindow*>(
>-      event.mReply.mFocusedWidget ? event.mReply.mFocusedWidget : mWindow);
>+      event.mReply.mFocusedWidget ? event.mReply.mFocusedWidget : (nsIWidget*)mWidget);
>   // Result rect is in top level widget coordinates
>   refWindow = refWindow->GetTopLevelWindow(false);
>   NS_ENSURE_TRUE(refWindow, E_FAIL);
> 
>   event.mReply.mRect.MoveBy(refWindow->WidgetToScreenOffset());
>+#endif

The result rect must be in screen coordinates. So, is this code correct on Metro? On Metro, XUL <panel> cannot be popuped up? How does the metro applications work on secondary display?

>@@ -1132,23 +1215,30 @@ nsTextStore::GetTextExt(TsViewCookie vcV
> }
> 
> STDMETHODIMP
> nsTextStore::GetScreenExt(TsViewCookie vcView,
>                           RECT *prc)
> {
>   NS_ENSURE_TRUE(TEXTSTORE_DEFAULT_VIEW == vcView && prc, E_INVALIDARG);
>   // use NS_QUERY_EDITOR_RECT to get rect in system, screen coordinates
>-  nsQueryContentEvent event(true, NS_QUERY_EDITOR_RECT, mWindow);
>-  mWindow->InitEvent(event);
>-  mWindow->DispatchWindowEvent(&event);
>+  nsQueryContentEvent event(true, NS_QUERY_EDITOR_RECT, (nsIWidget*)mWidget);
>+  mWidget.InitEvent(event);
>+  mWidget.DispatchWindowEvent(&event);
>   NS_ENSURE_TRUE(event.mSucceeded, E_FAIL);
> 
>+#ifdef MOZ_METRO
>+  nsIntRect boundRect;
>+  nsresult rv = ((nsIWidget*)mWidget)->GetClientBounds(boundRect);
>+  NS_ENSURE_SUCCESS(rv, E_FAIL);
>+  ::SetRect(prc, boundRect.x, boundRect.y,
>+            boundRect.XMost(), boundRect.YMost());
>+#else
>   nsWindow* refWindow = static_cast<nsWindow*>(
>-      event.mReply.mFocusedWidget ? event.mReply.mFocusedWidget : mWindow);
>+      event.mReply.mFocusedWidget ? event.mReply.mFocusedWidget : (nsIWidget*)mWidget);

I have same question as above.

> nsresult
> nsTextStore::OnTextChangeInternal(uint32_t aStart,
>                                   uint32_t aOldEnd,
>                                   uint32_t aNewEnd)
> {
>   if (!mLock && mSink && 0 != (mSinkMask & TS_AS_TEXT_CHANGE)) {
>     mTextChange.acpStart = NS_MIN(mTextChange.acpStart, LONG(aStart));
>     mTextChange.acpOldEnd = NS_MAX(mTextChange.acpOldEnd, LONG(aOldEnd));
>     mTextChange.acpNewEnd = NS_MAX(mTextChange.acpNewEnd, LONG(aNewEnd));
>-    ::PostMessageW(mWindow->GetWindowHandle(),
>+    ::PostMessageW(mWidget.GetWindowHandle(),
>                    WM_USER_TSF_TEXTCHANGE, 0, 0);
>   }
>   return NS_OK;
> }

I feel this strange. Does Metro widget have window handle and does this code work fine? If so, this is correct.

>diff --git a/widget/windows/nsTextStore.h b/widget/windows/nsTextStore.h
>--- a/widget/windows/nsTextStore.h
>+++ b/widget/windows/nsTextStore.h

>+class FocusWidget
>+{
>+  public:

Unnecessary indent under our coding rules.

>+    FocusWidget();
>+
>+    void SetWin32Window(nsWindow* aWindow);
>+#ifdef MOZ_METRO
>+    void SetMetroWindow(MetroWidget* aWindow);
>+#endif
>+    // operators
>+    operator const HWND();

Why do we need the "const"?

>+    operator nsIWidget*();
>+
>+    // methods
>+    bool IsEmpty();
>+    HWND GetWindowHandle();
>+    void Clear();
>+    bool Destroyed();
>+    void InitEvent(nsGUIEvent& event, nsIntPoint* aPoint = nullptr);
>+    bool DispatchWindowEvent(nsGUIEvent* event);
>+
>+  private:
>+    nsWindow* mWindow;
>+#ifdef MOZ_METRO
>+    MetroWidget* mMetroWidget;
>+#else
>+    void* mMetroWidget;
>+#endif
>+};
>+
>+/*
>  * Text Services Framework text store
>  */
> 
> class nsTextStore MOZ_FINAL : public ITextStoreACP,
>                               public ITfContextOwnerCompositionSink
> {
> public: /*IUnknown*/
>   STDMETHODIMP_(ULONG)  AddRef(void);


If you need to land this patch ASAP, I think it's okay even if the TSF support is completely killed on desktop temporary.

Anyway, I think that we need to add logging code for debug. Current nsTextStore.cpp is hard to understand the actual behavior because we don't know which interface method is called and when it is called (it depends on the TIP implementation). If you agree with this, I'll write a patch for logging nsTextStore.cpp behavior.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> Basically, it's okay. But this patch completely breaks TSF support on
> desktop firefox. Though, I'm not sure why it is. When I try to input with
> TIP, looks like the TIPs works via IMM. Because I don't see some bugs on TSF
> mode and TIP specified style in composition string.

I don't want to break TSF so I will do some testing.

> The result rect must be in screen coordinates. So, is this code correct on
> Metro? On Metro, XUL <panel> cannot be popuped up? How does the metro
> applications work on secondary display?

There are no child windows in metro, so my assumption is this should be ok. I haven't tested on secondary displays so I really can't say.

> I feel this strange. Does Metro widget have window handle and does this code
> work fine? If so, this is correct.

Yes, we have HWND, we hook the ICoreWindow to get it. We don't use it much, but in cases like this we can.

> If you need to land this patch ASAP, I think it's okay even if the TSF
> support is completely killed on desktop temporary.

I'll test TSF first to figure out what broke. Also I'm still trying to figure out bug 783882.

> 
> Anyway, I think that we need to add logging code for debug. Current
> nsTextStore.cpp is hard to understand the actual behavior because we don't
> know which interface method is called and when it is called (it depends on
> the TIP implementation). If you agree with this, I'll write a patch for
> logging nsTextStore.cpp behavior.

I can add debug logging to the code I've added.
(In reply to Jim Mathies [:jimm] from comment #11)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> > Basically, it's okay. But this patch completely breaks TSF support on
> > desktop firefox. Though, I'm not sure why it is. When I try to input with
> > TIP, looks like the TIPs works via IMM. Because I don't see some bugs on TSF
> > mode and TIP specified style in composition string.
> 
> I don't want to break TSF so I will do some testing.


Masayuki, any tips on testing this? I have jp installed on my tablet and am able to switch between the two using win-space. In Metro apps I can bring up the ime composition window when I start to type. When I do the same in metrofx I don't see an ime window and text is entered in english. On the desktop I can bring up the window if I enable tsf in prefs. Do we create ime related windows here, and if so, do they use child widgets?
Hmm, I don't know how Japanese IME works with tablet.

> On the desktop I can bring up the window if I enable tsf in prefs.

Is this on patched build? Or m-c build? I confirmed that debug build with the patch which is built by myself is completely broken the TSF support on desktop. I set NSPR_LOG_MODULES=nsIMM32HandlerWidgets:1 and run the debug build, I can see the IMM module is working. It shouldn't work if TSF is enabled.

> Do we create ime related windows here, and if so, do they use child widgets?

We never create any special window for IME. Such windows are created by IME or TIP itself.
I was just testing on a nightly. I'll get the tsf changes here over on mc and test in the desktop build to figure out what broke.
Attachment #657279 - Flags: review?(masayuki)
Attached patch Updated patch for bug 544773 (obsolete) — Splinter Review
Use this with the patches for bug 544773.

And I removed "IsEmpty()" and changed the "Destroyed()" method. It makes simpler the callers.

This patch has same trouble, but I have no idea...
Attached file log with attachment 659459 (obsolete) —
> 0[b5db98]: TSF: 0x6ba6140 nsTextStore::RequestLock(dwLockFlags=TS_LF_READ, phrSession=0x39ce5c), mLock=not-specified
> 0[b5db98]: TSF: 0x6ba6140   nsTextStore::RequestLock() notifying OnLockGranted()...
> 0[b5db98]: TSF: 0x6ba6140   nsTextStore::RequestLock() succeeded: *phrSession=S_OK
> 0[b5db98]: TSF: 0x6ba6140 nsTextStore::SetInputContextInternal(aState=ENABLED), mContext=22fdf8
> 0[b5db98]: TSF: 0x6ba6140   nsTextStore::SetInputContextInternal(), setting 0x0000 to GUID_COMPARTMENT_KEYBOARD_DISABLED of context 0x22fdf8...
> 0[b5db98]: WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file m:/mc-a/src/gfx/layers/d3d9/Nv3DVUtils.cpp, line 53
> 0[b5db98]: WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file m:/mc-a/src/gfx/layers/d3d9/Nv3DVUtils.cpp, line 53
> 0[b5db98]: TSF: 0x6ba6140 nsTextStore::RequestLock(dwLockFlags=TS_LF_READ | TS_LF_SYNC, phrSession=0x39d3cc), mLock=not-specified
> 0[b5db98]: TSF: 0x6ba6140   nsTextStore::RequestLock() notifying OnLockGranted()...
> 0[b5db98]: TSF: 0x6ba6140 nsTextStore::GetSelection(ulIndex=4294967295, ulCount=1, pSelection=0x39d1d4, pcFetched=0x39d26c)
> 0[b5db98]: TSF: 0x6ba6140   nsTextStore::GetSelectionInternal(), try to get normal selection...
> 0[b5db98]: TSF: 0x6ba6140   nsTextStore::GetSelectionInternal() succeeded: acpStart=8, acpEnd=8, style.ase=TS_AE_END, style.fInterimChar=false
> 0[b5db98]: TSF: 0x6ba6140   nsTextStore::GetSelection() succeeded
> 0[b5db98]: TSF: 0x6ba6140   nsTextStore::RequestLock() succeeded: *phrSession=S_OK

This is expected behavior in the previous log.

> 0[2bdb98]: TSF: 0x67650b0 nsTextStore::RequestLock(dwLockFlags=TS_LF_READ, phrSession=0x25ce18), mLock=not-specified
> 0[2bdb98]: TSF: 0x67650b0   nsTextStore::RequestLock() notifying OnLockGranted()...
> 0[2bdb98]: TSF: 0x67650b0   nsTextStore::RequestLock() succeeded: *phrSession=S_OK
> 0[2bdb98]: TSF: 0x67650b0 nsTextStore::SetInputContextInternal(aState=ENABLED), mContext=0x12af7200
> 0[2bdb98]: TSF: 0x67650b0   nsTextStore::SetInputContextInternal(), setting 0x0000 to GUID_COMPARTMENT_KEYBOARD_DISABLED of context 0x12af7200...
> 0[2bdb98]: WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file m:/mc-a/src/gfx/layers/d3d9/Nv3DVUtils.cpp, line 53
> 0[2bdb98]: WARNING: Nv3DVStreaming CoCreateInstance failed (disabled).: file m:/mc-a/src/gfx/layers/d3d9/Nv3DVUtils.cpp, line 53
> 0[2bdb98]: TSF: 0x67650b0 nsTextStore::OnTextChangeInternal(aStart=8, aOldEnd=8, aNewEnd=9), mLock=not-specified, mSink=0x4d9144, mSinkMask=TS_AS_TEXT_CHANGE | TS_AS_SEL_CHANGE | TS_AS_LAYOUT_CHANGE | TS_AS_ATTR_CHANGE | TS_AS_STATUS_CHANGE, mTextChange={ acpStart=2147483647, acpOldEnd=0, acpNewEnd=0 }
> 0[2bdb98]: TSF: 0x67650b0   nsTextStore::OnSelectionChangeInternal(), mLock=not-specified, mSink=0x4d9144, mSinkMask=TS_AS_TEXT_CHANGE | TS_AS_SEL_CHANGE | TS_AS_LAYOUT_CHANGE | TS_AS_ATTR_CHANGE | TS_AS_STATUS_CHANGE

This is the wrong behavior. I'm not sure why IME didn't try requesting the lock. Then, IMM worked and it made the OnTextChangeInternal().

I cannot see any difference before these lines, sigh.
Sorry Masayuki, I was pulled off tsf work. I will get to this bug this week.
Hi,
I’m Yukawa who is working on a similar issue for Chromium at http://crbug.com/137627

-> comment 12
> When I do the same in metrofx I don't see an ime window and text is entered in english. On the desktop I can bring up the window if I enable tsf in prefs.

We actually saw the same behavior. Have you checked ITfMessagePump and ITfKeystrokeMgr? In our case, we cannot use TIPs on Metro mode without these interfaces.

This is somewhat tricky part - there is another component called Cicero Unaware Application Support (CUAS), which automatically calls these interfaces for each keystroke event. On desktop mode, CUAS is enabled by default thus an application is not necessarily required to call these interfaces to interact with TIPs. On Metro style, however, CUAS is no longer enabled so an application is responsible for forwarding each keystroke event to TSF manager via these interfaces. This might be what you saw.

Fortunately, starting with Win8, you can use ImmDisableLegacyIME API to disable CUAS even on desktop mode. I strongly recommend you to make sure your application can work with TIPs on desktop mode even when you call ImmDisableLegacyIME API.
http://msdn.microsoft.com/en-us/library/windows/desktop/hh706738.aspx

Hope this helps.
Yukawa-san, thank you for the information!

We're not checked them, so, I guess that your idea is correct.
No longer depends on: 785425
Attachment #657279 - Attachment is obsolete: true
Whiteboard: metro-beta
Whiteboard: metro-beta → [metro-mvp]
Whiteboard: [metro-mvp] → [metro-mvp] [LOE:-]
Whiteboard: [metro-mvp] [LOE:-] → [metro-mvp] [LOE:2]
Attached file log w/updated patches (obsolete) —
This is a tsf log with updated patches. Not sure what I should be looking for or testing. Need to do some reading.
Attachment #659459 - Attachment is obsolete: true
Attached patch tsf patch v.1 (obsolete) — Splinter Review
Attached patch metro widget patch v.1 (obsolete) — Splinter Review
Attached patch tsf patch v.2 (obsolete) — Splinter Review
re-organizing code between patches.
Attachment #679108 - Attachment is obsolete: true
Attachment #679109 - Attachment is obsolete: true
(In reply to Jim Mathies [:jimm] from comment #24)
> Created attachment 679111 [details] [diff] [review]
> tsf patch v.2
> 
> re-organizing code between patches.

I've been testing this with the built-in English and Japanese IMEs. Unfortunately I really don't know what I'm doing. :) Masayuki, would you have a set of steps I can follow to confirm TSF module is working? I can generate logs to post here as well.
(In reply to Jim Mathies [:jimm] from comment #25)
> (In reply to Jim Mathies [:jimm] from comment #24)
> > Created attachment 679111 [details] [diff] [review]
> > tsf patch v.2
> > 
> > re-organizing code between patches.
> 
> I've been testing this with the built-in English and Japanese IMEs.
> Unfortunately I really don't know what I'm doing. :) Masayuki, would you
> have a set of steps I can follow to confirm TSF module is working? I can
> generate logs to post here as well.

Hmm, Japanese IME has a lot of functions. It's too difficult non-Japanese language users to understand or check the behavior actually. I think that I should test it. I'll try to build the metro version. Is it okay? Or do you want me to check the behavior on desktop?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #26)
> (In reply to Jim Mathies [:jimm] from comment #25)
> > (In reply to Jim Mathies [:jimm] from comment #24)
> > > Created attachment 679111 [details] [diff] [review]
> > > tsf patch v.2
> > > 
> > > re-organizing code between patches.
> > 
> > I've been testing this with the built-in English and Japanese IMEs.
> > Unfortunately I really don't know what I'm doing. :) Masayuki, would you
> > have a set of steps I can follow to confirm TSF module is working? I can
> > generate logs to post here as well.
> 
> Hmm, Japanese IME has a lot of functions. It's too difficult non-Japanese
> language users to understand or check the behavior actually. I think that I
> should test it. I'll try to build the metro version. Is it okay? Or do you
> want me to check the behavior on desktop?

Yes absolutely. Will you need touch capable hardware or a licensed copy of win8? I can help if you do.
Hmm, I'm still seeing that TSF isn't available on the patched build on desktop. I'm looking for the reason...
Very strange... ITfThreadMgr::SetFocus() succeeded (hr is 0x0), but in next line, ITfThreadMgr::GetFocus() returns different docMgr. So, the nsTextStore::mDocumentMgr doesn't focus actually if the patch applied.
Sorry for the delay. I try finding the cause today.
Attached patch Patch (obsolete) — Splinter Review
Jimm:

I found the cause TSF isn't enabled by the patch. I removed HWND operator from FocusWidget. It could be a bug of compiler or something of the patch.

Anyway, I have some suggestions:

1. Remove nsIWidget* operator of FocusWidget. Instead of that, let's use .get() for nsIWidget pointer. I feel it's more natural since we're using such inline methods at nsCOMPtr, nsRefPtr and so on.

2. Let's hold only one nsIWidget pointer in the FocusWidget. The FocusWidget class is completely capsules the widget handling. Only InitEvent() and DispatchWindowEvent() need to do cast to nsWindow. In other words, it doesn't make sense always holding both pointers. Then, I guess that Create() and OnFocusChange() may not be necessary for both versions.

I'll post the updated patch too. I you agree with that changes, r=me for your patch. And then, we should request additional review to Kimura-san.
Attachment #659460 - Attachment is obsolete: true
Attachment #659461 - Attachment is obsolete: true
Attachment #678762 - Attachment is obsolete: true
Attachment #679111 - Attachment is obsolete: true
This gets rid of some #ifdefs.
Attachment #689154 - Flags: feedback?(jmathies)
I want to rename nsTextStore to widget::TextStore in another bug before it becomes default IME handler on Windows.
Depends on: 818854
Comment on attachment 689154 [details] [diff] [review]
Updated patch with my suggestions

I filed bug 818854 on mOnDestroyCalled.
Attachment #689154 - Flags: feedback?(jmathies) → feedback+
(In reply to Jim Mathies [:jimm] from comment #34)
> Comment on attachment 689154 [details] [diff] [review]
> Updated patch with my suggestions
> 
> I filed bug 818854 on mOnDestroyCalled.

Thanks, and I changed the nsTextStore::OnChangeFocus()'s argument. So, you need to update the MetroWidget too.
Comment on attachment 689154 [details] [diff] [review]
Updated patch with my suggestions

Kimura-san, could you review this? The original patch author is Jimm. And I modified something in this patch. So, both of us are not good for reviewer of this.
Attachment #689154 - Flags: review?(VYV03354)
Comment on attachment 689154 [details] [diff] [review]
Updated patch with my suggestions

FocusWidget looks horrible to me. Why don't you create a common base class from which both nsWindow and MetroWidget inherit?
(In reply to Masatoshi Kimura [:emk] from comment #37)
> Comment on attachment 689154 [details] [diff] [review]
> Updated patch with my suggestions
> 
> FocusWidget looks horrible to me. Why don't you create a common base class
> from which both nsWindow and MetroWidget inherit?

That sounds like a great idea.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #35)
> (In reply to Jim Mathies [:jimm] from comment #34)
> > Comment on attachment 689154 [details] [diff] [review]
> > Updated patch with my suggestions
> > 
> > I filed bug 818854 on mOnDestroyCalled.
> 
> Thanks, and I changed the nsTextStore::OnChangeFocus()'s argument. So, you
> need to update the MetroWidget too.

Thanks, I'll catch that in the subsequent merge of this code from mc.
FYI, bug 818854 is now fixed on elm.
Whiteboard: [metro-mvp] [LOE:2] → [metro-mvp][LOE:2][metro-it2]
jimm:

Do you work on the new super class? Or should I? I want to work on bug 790516 which is also important for TSF.
It's just something like:
  class BaseWinWidget : public nsBaseWidget {
  public:
    HWND GetWindowHandle() const { return static_cast<HWND>(mWidget->GetNativeData(NS_NATIVE_WINDOW)); };
    virtual bool Destroyed() = 0;
    virtual void InitEvent(nsGUIEvent& aEvent, nsIntPoint* aPoint = nullptr) const = 0;
    virtual bool DispatchWindowEvent(nsGUIEvent* aEvent) const = 0;
  };
And nsTextStore.cpp changes would be much fewer than the current patch.
Of course "mWidget->" was unnecessary.
Yep, I can work on this. I'll try to get an update put together by early next week.
Attachment #689154 - Attachment is obsolete: true
Attachment #689154 - Flags: review?(VYV03354)
Attachment #689067 - Attachment is obsolete: true
Attached patch revamp v.1 (obsolete) — Splinter Review
Attached patch revamp v.1 (obsolete) — Splinter Review
- restoring some mWidget null checks after dispatch calls.
Attachment #698146 - Attachment is obsolete: true
Attachment #698153 - Flags: review?(VYV03354)
Comment on attachment 698153 [details] [diff] [review]
revamp v.1

>+++ b/widget/windows/nsTextStore.cpp
>@@ -334,45 +337,44 @@ GetDisplayAttrStr(const TF_DISPLAYATTRIB
>+nsTextStore::Create(nsIWidget* aWidget,
>@@ -380,17 +382,19 @@ nsTextStore::Create(nsWindow* aWindow,
>+  mWidget = static_cast<nsWindowBase*>(aWidget);

Why don't you make the parameter type WindowBase* instead of using the cast?

>@@ -2533,32 +2555,34 @@ nsTextStore::OnEndComposition(ITfComposi
>+nsTextStore::OnFocusChange(bool aGotFocus,
>+                           nsIWidget* aFocusedWidget,

Same here.

>@@ -420,35 +424,35 @@ nsTextStore::Create(nsWindow* aWindow,
>-  if (mWindow) {
>+  if (mWidget->GetWindowHandle()) {

Why did you change the null test? Is mWidget guaranteed to be non-null here?

>@@ -2204,19 +2226,19 @@ nsTextStore::InsertTextAtSelectionIntern
>-          this, mWindow, GetBoolName(mWindow ? mWindow->Destroyed() : true),
>+          this, mWidget, GetBoolName(mWidget->Destroyed()),

Same here.

>@@ -1897,88 +1903,102 @@ nsTextStore::GetScreenExt(TsViewCookie v
>   } else {
>+    nsWindow* refWindow = static_cast<nsWindow*>(

Please add an assertion something like
      MOZ_ASSERT(XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Desktop);
before the cast.

>+++ b/widget/windows/nsTextStore.h
>@@ -187,28 +193,28 @@ protected:
>+  // Holds the pointer to our current win32 or metro widget
>+  nsRefPtr<nsWindowBase>       mWidget;
>-  // Window containing the focused editor
>-  nsWindow*                    mWindow;

Why did you change this to strong ref? Are you sure it will never create a cycle?

>+++ b/widget/windows/nsWindow.h
>@@ -59,34 +60,36 @@ class NativeKey;
>+class nsWindow : public nsWindowBase

Add MOZ_FINAL while you are here.

>+++ b/widget/windows/winrt/MetroWidget.h
>@@ -33,17 +34,17 @@
>+class MetroWidget : public nsWindowBase

Same here.

>+++ b/widget/windows/nsWindow.h
>@@ -59,34 +60,36 @@ class NativeKey;
>+  // nsWindowBase
>+  virtual void InitEvent(nsGUIEvent& aEvent, nsIntPoint* aPoint = nullptr);
>+  virtual bool DispatchWindowEvent(nsGUIEvent* aEvent);

Use MOZ_OVERRIDE to ensure these methods actually override WindowBase methods.

>+++ b/widget/windows/winrt/MetroWidget.h
>@@ -53,16 +54,20 @@ class MetroWidget : public nsBaseWidget
>+  // nsWindowBase
>+  virtual void InitEvent(nsGUIEvent& aEvent, nsIntPoint* aPoint = nullptr);
>+  virtual bool DispatchWindowEvent(nsGUIEvent* aEvent);

Same here.

>+++ b/widget/windows/nsWindowBase.h
>@@ -0,0 +1,39 @@
>+#ifndef nsWindowBase_h__
>+#define nsWindowBase_h__

nsWindowBase_h_. Consecutive underscore is reserved and not usable by the user code.

>+class nsWindowBase : public nsBaseWidget

Please use mozilla::widget::Xxx naming scheme rather than nsXxx for new classes.

>+  operator nsIWidget*() { return static_cast<nsIWidget*>(this); }

Is this cast (and even this operator overload itself) needed?

>+  virtual HWND GetWindowHandle() {

Does this method need to be virtual? How about
   HWND GetWindowHandle() MOZ_FINAL {
?

I would like to see the updated patch (or the explanation about the reason for change).
Attachment #698153 - Flags: review?(VYV03354) → review-
(In reply to Masatoshi Kimura [:emk] from comment #48)
> Comment on attachment 698153 [details] [diff] [review]
> revamp v.1
> 
> >+++ b/widget/windows/nsTextStore.cpp
> >@@ -334,45 +337,44 @@ GetDisplayAttrStr(const TF_DISPLAYATTRIB
> >+nsTextStore::Create(nsIWidget* aWidget,
> >@@ -380,17 +382,19 @@ nsTextStore::Create(nsWindow* aWindow,
> >+  mWidget = static_cast<nsWindowBase*>(aWidget);
> 
> Why don't you make the parameter type WindowBase* instead of using the cast?

sure, I can do that.
 
> >@@ -420,35 +424,35 @@ nsTextStore::Create(nsWindow* aWindow,
> >-  if (mWindow) {
> >+  if (mWidget->GetWindowHandle()) {
> 
> Why did you change the null test? Is mWidget guaranteed to be non-null here?

Mass replace string error, I'll revert it back.

> >+++ b/widget/windows/nsTextStore.h
> >@@ -187,28 +193,28 @@ protected:
> >+  // Holds the pointer to our current win32 or metro widget
> >+  nsRefPtr<nsWindowBase>       mWidget;
> >-  // Window containing the focused editor
> >-  nsWindow*                    mWindow;
> 
> Why did you change this to strong ref? Are you sure it will never create a
> cycle?

Seems safer to keep a refd copy. Generally this is the right way to store a ref pointer. 

> >+class nsWindowBase : public nsBaseWidget
> 
> Please use mozilla::widget::Xxx naming scheme rather than nsXxx for new
> classes.

I did this so that nsWindow and nsWindowBase show up next to each other in file lists. We have a a number of nsWindowXyz classes in widget/windows. I'd really like to keep the naming as is.

> 
> >+  operator nsIWidget*() { return static_cast<nsIWidget*>(this); }
> 
> Is this cast (and even this operator overload itself) needed?

For event constructors which take an nsIWidget*.

> 
> >+  virtual HWND GetWindowHandle() {
> 
> Does this method need to be virtual? How about
>    HWND GetWindowHandle() MOZ_FINAL {
> ?

I suppose not. I made it virtual for consistency in the base class. Don't think it really matters. *shrug*
(In reply to Masatoshi Kimura [:emk] from comment #48)
> >+++ b/widget/windows/nsWindow.h
> >@@ -59,34 +60,36 @@ class NativeKey;
> >+class nsWindow : public nsWindowBase
> 
> Add MOZ_FINAL while you are here.

Note, we have a ChildWindow class which overrides nsWindow.
Attached patch revamp v.2Splinter Review
(In reply to Jim Mathies [:jimm] from comment #49)
> (In reply to Masatoshi Kimura [:emk] from comment #48)
> > 
> > >+  operator nsIWidget*() { return static_cast<nsIWidget*>(this); }
> > 
> > Is this cast (and even this operator overload itself) needed?
> 
> For event constructors which take an nsIWidget*.

I guess I didn't need this afterall, it's building fine without it.

> > >+  virtual HWND GetWindowHandle() {
> > 
> > Does this method need to be virtual? How about
> >    HWND GetWindowHandle() MOZ_FINAL {
> > ?
> 
> I suppose not. I made it virtual for consistency in the base class. Don't
> think it really matters. *shrug*

final requires virtual, so I added MOZ_FINAL, kept virtual.

The outstanding question is the nsRefPtr for the member variable. If you feel it should be a raw point, it's an easy change plus I'll need to add back the nullptr set in the ctor.
Attachment #698153 - Attachment is obsolete: true
Attachment #698276 - Flags: review?(VYV03354)
Comment on attachment 698276 [details] [diff] [review]
revamp v.2

OK, the widget will lose the focus before being destroyed.
Attachment #698276 - Flags: review?(VYV03354) → review+
When will you land the patch on m-c? It will break my patches, so, I'd like you to land it soon.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #53)
> When will you land the patch on m-c? It will break my patches, so, I'd like
> you to land it soon.

I'll land the m-c bits today.
(In reply to Jim Mathies [:jimm] from comment #54)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #53)
> > When will you land the patch on m-c? It will break my patches, so, I'd like
> > you to land it soon.
> 
> I'll land the m-c bits today.

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=3f191588c7c5

Pushed to try just to be safe. This is an m-c only patch. I'll land the m-c + elm bits on elm as well.
https://hg.mozilla.org/projects/elm/rev/8f019d9009cd
Whiteboard: [metro-mvp][LOE:2][metro-it2] → [metro-mvp][LOE:2][metro-it2][completed-elm]
https://hg.mozilla.org/mozilla-central/rev/bad47c7f9239
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.