Support IMM32 reconversion on Windows

views/ime/input_method_win.cc

Index: views/ime/input_method_win.cc
diff --git a/views/ime/input_method_win.cc b/views/ime/input_method_win.cc
index 4ab1a187378d389ba70bf8dc6d3131b107afa847..1cd01f535c6f6f55ddd224443fb00907d431aa36 100644
--- a/views/ime/input_method_win.cc
+++ b/views/ime/input_method_win.cc
@@ -115,6 +115,9 @@ LRESULT InputMethodWin::OnImeMessages(
     case WM_IME_ENDCOMPOSITION:
       result = OnImeEndComposition(message, w_param, l_param, handled);
       break;
+    case WM_IME_REQUEST:
+      result = OnImeRequest(message, w_param, l_param, handled);
+      break;
     case WM_CHAR:
     case WM_SYSCHAR:
       result = OnChar(message, w_param, l_param, handled);
@@ -219,6 +222,126 @@ LRESULT InputMethodWin::OnImeEndComposition(
   return 0;
 }
 
+LRESULT InputMethodWin::OnReconvertString(RECONVERTSTRING *reconv) {

[James Su, 2011/10/25 19:15:03]

Please keep the order of methods in .cc file same as those in the .h as much as
possible.

[Peng, 2011/10/25 22:01:21]

Done.

[James Su, 2011/10/26 04:25:38]

It's not done yet. OnReconvertString() and OnDocumentFeed() should be put after
OnDeadChar() according to the declaration order in the header file.

[Peng, 2011/10/26 16:20:16]

Sorry. Done again.

+  TextInputClient* client = GetTextInputClient();
+  if (!client)
+    return 0;
+
+  // If there is a composition string already, we don't allow reconversion.
+  if (client->HasCompositionText())
+    return 0;
+
+  ui::Range selection_range;
+  if (!client->GetSelectionRange(&selection_range) ||
+      selection_range.is_empty()) {
+    return 0;
+  }
+
+  size_t len = selection_range.length();
+  size_t need_size = sizeof(RECONVERTSTRING) + len * sizeof(WCHAR);

[James Su, 2011/10/25 19:15:03]

It's better to return some extra content around selection text. The sample
provided in Microsoft Windows SDK sends 40 characters surrounding text totally
(20 before selection, 20 after selection).

See the sample in C:\Program Files\Microsoft
SDKs\Windows\v7.1\Samples\winui\input\tsf\tsfapps\immpad-level3-step3

[Peng, 2011/10/25 22:01:21]

This logic is same with firefox. And currently we do not implement
SetSelectionRange in RHWVV. It is needed when IME want to reconvert piece of
text which is not current selection. So returning extra text is not very useful
right now. So I would like not support getting extra context right now. And when
RHWVV can support SetSelectionRange(), I will change this function.

[James Su, 2011/10/26 04:25:38]

I think you misunderstood the purpose of those extra context, which is to help
the input method improve reconversion accuracy (if the input method supports it,
e.g. by feeding the extra context into its language model). It's nothing to do
with SetSelectionRange().

[Peng, 2011/10/26 16:20:16]

Done.

+
+  if (!reconv)
+    return need_size;

[James Su, 2011/10/25 19:15:03]

The actual_range returned by TextInputClient::GetTextFromRange() may be
different than the request selection range. You need to calculate need_size
according to the actual_range rather than the selection range.

[Peng, 2011/10/25 22:01:21]

Because text range must cover the selection range, so GetTextFromRange will
return the same range of selection range. So I think we can use selection_range
here. And it could avoid calling GetTextFromRange() and a useless string
copying.

[James Su, 2011/10/26 04:25:38]

You can't assume it here, especially if you want to retrieve some extra context.

[Peng, 2011/10/26 16:20:16]

Actually, I would like to remove actual_range from GetTextFromRange() which will
simply return false, if it can not get whole text in the range. Is OK?

[James Su, 2011/10/26 18:41:57]

I don't think it's ok. The current api design looks more reasonable.

[Peng, 2011/10/26 19:36:53]

Remove acutal_range, can make both caller and text input client implementation
code simpler. And we do not need call GetTextFromRange twice. It is wasteful.

[James Su, 2011/10/26 19:57:17]

actual_range is necessary from the api design point of view. You can make
GetTextFromRange() to accept a NULL text pointer to avoid copying the content
when not necessary.

[Peng, 2011/10/27 00:28:00]

I didn't get it. Why we must have it from the API design point of view. I remove
it from TextInputClient and refined the document for APIs. That makes code
simpler.

[James Su, 2011/10/27 04:56:39]

Without |actual_range| it's impossible for the client to fulfill the IME's
requirement partially. But in some cases, if the IME wants to retrieve some
extra context to help increase conversion accuracy, it's ok for the client to
return less context, which may affect the conversion accuracy, but won't break
the functionality completely. Without the parameter, the functionality will be
broken completely.

[Peng, 2011/10/27 16:12:24]

I refined the document. The text input client must return the whole text in the
requested range. It makes the logic simpler. If the client can return text in
another range, it will make the code complex. API users have to check if the
returned text can cover the range which is needed.

And I also refined the document of GetTextRange(), it will return a text range
in the document which can be accessed by GetTextFromRange(). So API users can
use GetTextRange() to get the accessible range before calling
GetTextFrameRange(). It will not break anything.

Thanks for your suggestions. But I like define the API in this way which I think
is better.

+
+  if (reconv->dwSize < need_size)
+    return 0;
+
+  string16 text;
+  ui::Range actual_range;
+  if (!GetTextInputClient()->GetTextFromRange(
+      selection_range, &text, &actual_range))
+    return 0;
+
+  reconv->dwVersion = 0;
+  reconv->dwStrLen = len;
+  reconv->dwStrOffset = sizeof(RECONVERTSTRING);
+  reconv->dwCompStrLen = len;
+  reconv->dwCompStrOffset = 0;
+  reconv->dwTargetStrLen = len;
+  reconv->dwTargetStrOffset = 0;
+  memcpy(reinterpret_cast<char*>(reconv) + sizeof(RECONVERTSTRING),
+      text.c_str(), len * sizeof(WCHAR));
+
+  return need_size;
+}
+
+LRESULT InputMethodWin::OnDocumentFeed(RECONVERTSTRING *reconv) {

[James Su, 2011/10/25 19:15:03]

Please refer to the Microsoft sample to see how this method should be
implemented.

[Peng, 2011/10/25 22:01:21]

It is almost same with MS sample. Only a little different - the dwTarget* is set
with range of selection, when chrome does not have composition. This logic is
same with firefox.

[James Su, 2011/10/26 04:25:38]

I mean the extra context thing.

[Peng, 2011/10/26 16:20:16]

This function returns all text which can be accessed. So it should be enough. 

[James Su, 2011/10/26 18:41:57]

I suggest to do the same thing as the Microsoft SDK sample, as you don't know
how large is the whole content will be. It'll be wasteful to return the whole
content if it's too large.

[Peng, 2011/10/26 19:36:53]

It is not easy task to decide how large context is enough. MS is just a simple
sample to show how api works, do not mean it is the suitable for us (just an API
demo). And in our implementation, the context is from renderer, it is already
truncated to reasonable size. We do not need to it again.

For omni-box, the content should be in one line. It is not necessary to truncate
it.

[James Su, 2011/10/26 19:57:17]

The content in omnibox might be huge, especially when dealing with data url (~KB
or even ~MB in a single line). And you don't know how a new TextInputClient will
be implemented in the future.  And somebody else might change the behavior of
existing TextInputClient implementations in the future. So please do not assume
that you know their internal behavior. You should solely program your part by
using the interface exposed to you.

[Peng, 2011/10/27 00:28:00]

So I think currently it is better to just return the all document right now.
Event if in some very minor cases, the size of document could be several KBs or
more. In those case, return all document, I don't see any problem. And IMEs
could refuse it, if the IMEs think the required size is too big. (Mozc has this
logic)

[James Su, 2011/10/27 04:56:39]

As I said, the document might be huge, and if the IME has logic to reject huge
document, then the functionality might be broken completely. Even if the IME
doesn't reject the huge document, copying the content each time is still
wasteful.

[Peng, 2011/10/27 16:12:24]

I would like to implement this function as firefox does. It only returns the
fewest number of paragraphs which can cover the target range. And it searches
the '\n' in the text as  borders of paragraphs.

But I think it is not too necessary to do it now, because our text clients RHWVV
returns truncated text, and omnibox returns single line text.

As you wish, I truncated the text here. Done.

+  TextInputClient* client = GetTextInputClient();
+  if (!client)
+    return 0;
+
+  ui::Range text_range;
+  if (!client->GetTextRange(&text_range) || text_range.is_empty())
+    return 0;
+
+  ui::Range target_range;
+  do {
+    if (client->HasCompositionText() &&
+        client->GetCompositionTextRange(&target_range) &&
+        !target_range.is_empty()) {
+      break;
+    }
+    if (client->GetSelectionRange(&target_range))
+      break;
+    return 0;
+  } while (0);

[James Su, 2011/10/25 19:15:03]

nit: I don't like do { } while(0); style myself. It's decrease the code
readability. Please avoid it if you can.

[Peng, 2011/10/25 22:01:21]

I tried using 'if else', but it looks worse.

[James Su, 2011/10/26 04:25:38]

Please try your best to rewrite this piece of code to eliminate this do {}
while(0); thing. I really don't like it.

[Peng, 2011/10/26 16:20:16]

I think it is not a big deal. It is very common for avoiding goto statement. At
same time, it may help reduce {} levels.

[James Su, 2011/10/26 18:41:57]

I'd prefer something like:

ui::Range target_range;
if (client->HasCompositionText())
  client->GetCompositionTextRange(&target_range);
if (target_range.is_empty())
  client->GetSelectionRange(&target_range);
...

[Peng, 2011/10/26 19:36:53]

Do not need check the return value of GetCompositionTextRange() and
GetSelectionRange()? I don't think it is a big deal within reviewing. We do not
need wast too much time on it.

[James Su, 2011/10/26 19:57:17]

The return value is actually not a big deal here. The TextInputClient
implementation should not alter the value of target_range if it returns false.
Perhaps we should state it explicitly in the method comment.

If you do want to check the return value, you can use a local boolean variable
to keep the method result and check it after calling the method. It helps keep
the code readability. 

On the contrary, the code readability IS a big deal. Please do whatever you can
to keep it.

[Peng, 2011/10/27 00:28:00]

Done.

+
+  if (!text_range.Contains(target_range))
+    return 0;
+
+  size_t len = text_range.length();
+  size_t need_size = sizeof(RECONVERTSTRING) + len * sizeof(WCHAR);
+
+  if (!reconv)
+    return need_size;
+
+  if (reconv->dwSize < need_size)
+    return 0;
+
+  string16 text;
+  ui::Range actual_range;
+  if (!GetTextInputClient()->GetTextFromRange(
+      text_range, &text, &actual_range)) {
+    return 0;
+  }
+
+  DCHECK(text_range == actual_range);
+
+  reconv->dwVersion = 0;
+  reconv->dwStrLen = len;
+  reconv->dwStrOffset = sizeof(RECONVERTSTRING);
+  reconv->dwCompStrLen =
+      client->HasCompositionText() ? reconv->dwTargetStrOffset : 0;
+  reconv->dwCompStrOffset =
+      (target_range.GetMin() - text_range.start()) * sizeof(WCHAR);
+  reconv->dwTargetStrLen = target_range.length();
+  reconv->dwTargetStrOffset = reconv->dwCompStrOffset;
+
+  memcpy((char*)reconv + sizeof(RECONVERTSTRING),
+      text.c_str(), len * sizeof(WCHAR));
+
+  return need_size;
+}
+
+LRESULT InputMethodWin::OnImeRequest(
+    UINT message, WPARAM wparam, LPARAM lparam, BOOL* handled) {
+  *handled = FALSE;
+
+  if (IsTextInputTypeNone())
+    return 0;
+
+  switch (wparam) {
+    case IMR_RECONVERTSTRING:
+      *handled = TRUE;

[James Su, 2011/10/25 19:15:03]

should we set *handled to FALSE if any method of TextInputClient returns false?

[Peng, 2011/10/25 22:01:21]

I think we should set *handled to TRUE, because we handled the request, even if
it failed.

+      return OnReconvertString(reinterpret_cast<RECONVERTSTRING*>(lparam));
+    case IMR_DOCUMENTFEED:
+      *handled = TRUE;
+      return OnDocumentFeed(reinterpret_cast<RECONVERTSTRING*>(lparam));
+    default:
+      return 0;
+  }
+}
+
 LRESULT InputMethodWin::OnChar(
     UINT message, WPARAM wparam, LPARAM lparam, BOOL* handled) {
   *handled = TRUE;