Issue 8294026: Support IMM32 reconversion on Windows

Peng

9 years, 2 months ago (2011-10-24 21:49:32 UTC) #1

jam

i'm not that familiar with this code, so i only scanned it for nits. i ...

9 years, 2 months ago (2011-10-25 00:32:13 UTC) #2

Peng

[+hbono] hbono, Could you please review this cls? Thanks. http://codereview.chromium.org/8294026/diff/22016/chrome/browser/renderer_host/render_widget_host_view_views.cc File chrome/browser/renderer_host/render_widget_host_view_views.cc (right): http://codereview.chromium.org/8294026/diff/22016/chrome/browser/renderer_host/render_widget_host_view_views.cc#newcode755 chrome/browser/renderer_host/render_widget_host_view_views.cc:755: ...

9 years, 2 months ago (2011-10-25 15:51:38 UTC) #3

James Su

http://codereview.chromium.org/8294026/diff/28001/views/ime/text_input_client.h File views/ime/text_input_client.h (right): http://codereview.chromium.org/8294026/diff/28001/views/ime/text_input_client.h#newcode12 views/ime/text_input_client.h:12: #include "base/callback.h" Remove this line. http://codereview.chromium.org/8294026/diff/28001/views/ime/text_input_client.h#newcode101 views/ime/text_input_client.h:101: // The ...

9 years, 2 months ago (2011-10-25 19:15:03 UTC) #5

Peng

http://codereview.chromium.org/8294026/diff/28001/views/ime/text_input_client.h File views/ime/text_input_client.h (right): http://codereview.chromium.org/8294026/diff/28001/views/ime/text_input_client.h#newcode12 views/ime/text_input_client.h:12: #include "base/callback.h" On 2011/10/25 19:15:03, James Su wrote: > ...

9 years, 2 months ago (2011-10-25 22:01:21 UTC) #6

Hironori Bono

Greetings, If I understand this change correctly, this change allows an IME to read text ...

9 years, 2 months ago (2011-10-26 02:39:37 UTC) #7

James Su

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc File views/ime/input_method_win.cc (right): http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc#newcode225 views/ime/input_method_win.cc:225: LRESULT InputMethodWin::OnReconvertString(RECONVERTSTRING *reconv) { On 2011/10/25 22:01:21, Peng wrote: ...

9 years, 2 months ago (2011-10-26 04:25:38 UTC) #8

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:225: LRESULT
InputMethodWin::OnReconvertString(RECONVERTSTRING *reconv) {
On 2011/10/25 22:01:21, Peng wrote:
> On 2011/10/25 19:15:03, James Su wrote:
> > Please keep the order of methods in .cc file same as those in the .h as much
> as
> > possible.
> 
> Done.

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

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:241: size_t need_size = sizeof(RECONVERTSTRING) +
len * sizeof(WCHAR);
On 2011/10/25 22:01:21, Peng wrote:
> On 2011/10/25 19:15:03, James Su wrote:
> > 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
> 
> 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.

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

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:244: return need_size;
On 2011/10/25 22:01:21, Peng wrote:
> On 2011/10/25 19:15:03, James Su wrote:
> > 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.
> 
> 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.

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

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:268: LRESULT
InputMethodWin::OnDocumentFeed(RECONVERTSTRING *reconv) {
On 2011/10/25 22:01:21, Peng wrote:
> On 2011/10/25 19:15:03, James Su wrote:
> > Please refer to the Microsoft sample to see how this method should be
> > implemented.
> 
> 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.
> 
I mean the extra context thing.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:287: } while (0);
On 2011/10/25 22:01:21, Peng wrote:
> On 2011/10/25 19:15:03, James Su wrote:
> > nit: I don't like do { } while(0); style myself. It's decrease the code
> > readability. Please avoid it if you can.
> 
> I tried using 'if else', but it looks worse.
> 

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

http://codereview.chromium.org/8294026/diff/29017/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/8294026/diff/29017/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.h:357: // A piece of text
which convers the selection in the web page.
How about something like: A buffer containing the text inside and around the
current selection range.

http://codereview.chromium.org/8294026/diff/29017/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.h:360: // The offset of
|selection_text_| in web page.
How about something like: The offset of the text stored in |selection_text_|
relative to the start of the web page.

http://codereview.chromium.org/8294026/diff/29017/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.h:363: // The selection
range in the web page.
How about: The current selection range relative to the start of the web page.

Peng

On 2011/10/26 02:39:37, hbono wrote: > Greetings, > > If I understand this change correctly, ...

9 years, 1 month ago (2011-10-26 15:38:26 UTC) #9

Peng

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc File views/ime/input_method_win.cc (right): http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc#newcode225 views/ime/input_method_win.cc:225: LRESULT InputMethodWin::OnReconvertString(RECONVERTSTRING *reconv) { On 2011/10/26 04:25:38, James Su ...

9 years, 1 month ago (2011-10-26 16:20:15 UTC) #10

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:225: LRESULT
InputMethodWin::OnReconvertString(RECONVERTSTRING *reconv) {
On 2011/10/26 04:25:38, James Su wrote:
> On 2011/10/25 22:01:21, Peng wrote:
> > On 2011/10/25 19:15:03, James Su wrote:
> > > Please keep the order of methods in .cc file same as those in the .h as
much
> > as
> > > possible.
> > 
> > Done.
> 
> It's not done yet. OnReconvertString() and OnDocumentFeed() should be put
after
> OnDeadChar() according to the declaration order in the header file.

Sorry. Done again.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:241: size_t need_size = sizeof(RECONVERTSTRING) +
len * sizeof(WCHAR);
On 2011/10/26 04:25:38, James Su wrote:
> On 2011/10/25 22:01:21, Peng wrote:
> > On 2011/10/25 19:15:03, James Su wrote:
> > > 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
> > 
> > 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.
> 
> 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().

Done.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:244: return need_size;
On 2011/10/26 04:25:38, James Su wrote:
> On 2011/10/25 22:01:21, Peng wrote:
> > On 2011/10/25 19:15:03, James Su wrote:
> > > 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.
> > 
> > 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.
> 
> You can't assume it here, especially if you want to retrieve some extra
context.

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?

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:268: LRESULT
InputMethodWin::OnDocumentFeed(RECONVERTSTRING *reconv) {
On 2011/10/26 04:25:38, James Su wrote:
> On 2011/10/25 22:01:21, Peng wrote:
> > On 2011/10/25 19:15:03, James Su wrote:
> > > Please refer to the Microsoft sample to see how this method should be
> > > implemented.
> > 
> > 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.
> > 
> I mean the extra context thing.

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

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:287: } while (0);
On 2011/10/26 04:25:38, James Su wrote:
> On 2011/10/25 22:01:21, Peng wrote:
> > On 2011/10/25 19:15:03, James Su wrote:
> > > nit: I don't like do { } while(0); style myself. It's decrease the code
> > > readability. Please avoid it if you can.
> > 
> > I tried using 'if else', but it looks worse.
> > 
> 
> Please try your best to rewrite this piece of code to eliminate this do {}
> while(0); thing. I really don't like it.

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

http://codereview.chromium.org/8294026/diff/29017/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (left):

http://codereview.chromium.org/8294026/diff/29017/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_views.cc:169: void
RenderWidgetHostViewViews::InitAsFullscreen(
On 2011/10/26 02:39:37, hbono wrote:
> nit: I'm wondering why this change deletes this function. (I do not think this
> function does not have connection with IMM32.)

Done.

http://codereview.chromium.org/8294026/diff/29017/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_views.h (right):

http://codereview.chromium.org/8294026/diff/29017/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_views.h:95: const
ui::Range& range) OVRERRIDE;
On 2011/10/26 02:39:37, hbono wrote:
> nit: OVRERRIDE -> OVERRIDE?

Done.

http://codereview.chromium.org/8294026/diff/29017/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/8294026/diff/29017/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.h:357: // A piece of text
which convers the selection in the web page.
On 2011/10/26 04:25:38, James Su wrote:
> How about something like: A buffer containing the text inside and around the
> current selection range.

Done.

http://codereview.chromium.org/8294026/diff/29017/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.h:360: // The offset of
|selection_text_| in web page.
On 2011/10/26 04:25:38, James Su wrote:
> How about something like: The offset of the text stored in |selection_text_|
> relative to the start of the web page.

Done.

http://codereview.chromium.org/8294026/diff/29017/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.h:363: // The selection
range in the web page.
On 2011/10/26 04:25:38, James Su wrote:
> How about: The current selection range relative to the start of the web page.

Done.

James Su

I'll review your CL again once you fix all these feedbacks. http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc File views/ime/input_method_win.cc (right): ...

9 years, 1 month ago (2011-10-26 18:41:56 UTC) #11

I'll review your CL again once you fix all these feedbacks.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:244: return need_size;
On 2011/10/26 16:20:16, Peng wrote:
> On 2011/10/26 04:25:38, James Su wrote:
> > On 2011/10/25 22:01:21, Peng wrote:
> > > On 2011/10/25 19:15:03, James Su wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > You can't assume it here, especially if you want to retrieve some extra
> context.
> 
> 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?
> 

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

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:268: LRESULT
InputMethodWin::OnDocumentFeed(RECONVERTSTRING *reconv) {
On 2011/10/26 16:20:16, Peng wrote:
> On 2011/10/26 04:25:38, James Su wrote:
> > On 2011/10/25 22:01:21, Peng wrote:
> > > On 2011/10/25 19:15:03, James Su wrote:
> > > > Please refer to the Microsoft sample to see how this method should be
> > > > implemented.
> > > 
> > > 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.
> > > 
> > I mean the extra context thing.
> 
> This function returns all text which can be accessed. So it should be enough. 

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.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:287: } while (0);
On 2011/10/26 16:20:16, Peng wrote:
> On 2011/10/26 04:25:38, James Su wrote:
> > On 2011/10/25 22:01:21, Peng wrote:
> > > On 2011/10/25 19:15:03, James Su wrote:
> > > > nit: I don't like do { } while(0); style myself. It's decrease the code
> > > > readability. Please avoid it if you can.
> > > 
> > > I tried using 'if else', but it looks worse.
> > > 
> > 
> > Please try your best to rewrite this piece of code to eliminate this do {}
> > while(0); thing. I really don't like it.
> 
> I think it is not a big deal. It is very common for avoiding goto statement.
At
> same time, it may help reduce {} levels.

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

http://codereview.chromium.org/8294026/diff/35027/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/35027/views/ime/input_method_win....
views/ime/input_method_win.cc:230: ui::TextInputType type = GetTextInputType();
nit: const ui::TextInputType

http://codereview.chromium.org/8294026/diff/35027/views/ime/input_method_win....
views/ime/input_method_win.cc:363: size_t len = text_range.length();
It's better not to return all text content, because it'll be wasteful if it's
too large.

Peng

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc File views/ime/input_method_win.cc (right): http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc#newcode244 views/ime/input_method_win.cc:244: return need_size; On 2011/10/26 18:41:57, James Su wrote: > ...

9 years, 1 month ago (2011-10-26 19:36:53 UTC) #12

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:244: return need_size;
On 2011/10/26 18:41:57, James Su wrote:
> On 2011/10/26 16:20:16, Peng wrote:
> > On 2011/10/26 04:25:38, James Su wrote:
> > > On 2011/10/25 22:01:21, Peng wrote:
> > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > You can't assume it here, especially if you want to retrieve some extra
> > context.
> > 
> > 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?
> > 
> 
> I don't think it's ok. The current api design looks more reasonable.

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.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:268: LRESULT
InputMethodWin::OnDocumentFeed(RECONVERTSTRING *reconv) {
On 2011/10/26 18:41:57, James Su wrote:
> On 2011/10/26 16:20:16, Peng wrote:
> > On 2011/10/26 04:25:38, James Su wrote:
> > > On 2011/10/25 22:01:21, Peng wrote:
> > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > Please refer to the Microsoft sample to see how this method should be
> > > > > implemented.
> > > > 
> > > > 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.
> > > > 
> > > I mean the extra context thing.
> > 
> > This function returns all text which can be accessed. So it should be
enough. 
> 
> 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.

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.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:287: } while (0);
On 2011/10/26 18:41:57, James Su wrote:
> On 2011/10/26 16:20:16, Peng wrote:
> > On 2011/10/26 04:25:38, James Su wrote:
> > > On 2011/10/25 22:01:21, Peng wrote:
> > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > nit: I don't like do { } while(0); style myself. It's decrease the
code
> > > > > readability. Please avoid it if you can.
> > > > 
> > > > I tried using 'if else', but it looks worse.
> > > > 
> > > 
> > > Please try your best to rewrite this piece of code to eliminate this do {}
> > > while(0); thing. I really don't like it.
> > 
> > I think it is not a big deal. It is very common for avoiding goto statement.
> At
> > same time, it may help reduce {} levels.
> 
> I'd prefer something like:
> 
> ui::Range target_range;
> if (client->HasCompositionText())
>   client->GetCompositionTextRange(&target_range);

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.

> if (target_range.is_empty())
>   client->GetSelectionRange(&target_range);
> ...

James Su

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc File views/ime/input_method_win.cc (right): http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc#newcode244 views/ime/input_method_win.cc:244: return need_size; On 2011/10/26 19:36:53, Peng wrote: > On ...

9 years, 1 month ago (2011-10-26 19:57:17 UTC) #13

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:244: return need_size;
On 2011/10/26 19:36:53, Peng wrote:
> On 2011/10/26 18:41:57, James Su wrote:
> > On 2011/10/26 16:20:16, Peng wrote:
> > > On 2011/10/26 04:25:38, James Su wrote:
> > > > On 2011/10/25 22:01:21, Peng wrote:
> > > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > > 
> > > > You can't assume it here, especially if you want to retrieve some extra
> > > context.
> > > 
> > > 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?
> > > 
> > 
> > I don't think it's ok. The current api design looks more reasonable.
> 
> 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.

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.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:268: LRESULT
InputMethodWin::OnDocumentFeed(RECONVERTSTRING *reconv) {
On 2011/10/26 19:36:53, Peng wrote:
> On 2011/10/26 18:41:57, James Su wrote:
> > On 2011/10/26 16:20:16, Peng wrote:
> > > On 2011/10/26 04:25:38, James Su wrote:
> > > > On 2011/10/25 22:01:21, Peng wrote:
> > > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > > Please refer to the Microsoft sample to see how this method should
be
> > > > > > implemented.
> > > > > 
> > > > > 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.
> > > > > 
> > > > I mean the extra context thing.
> > > 
> > > This function returns all text which can be accessed. So it should be
> enough. 
> > 
> > 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.
> 
> 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.

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.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:287: } while (0);
On 2011/10/26 19:36:53, Peng wrote:
> On 2011/10/26 18:41:57, James Su wrote:
> > On 2011/10/26 16:20:16, Peng wrote:
> > > On 2011/10/26 04:25:38, James Su wrote:
> > > > On 2011/10/25 22:01:21, Peng wrote:
> > > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > > nit: I don't like do { } while(0); style myself. It's decrease the
> code
> > > > > > readability. Please avoid it if you can.
> > > > > 
> > > > > I tried using 'if else', but it looks worse.
> > > > > 
> > > > 
> > > > Please try your best to rewrite this piece of code to eliminate this do
{}
> > > > while(0); thing. I really don't like it.
> > > 
> > > I think it is not a big deal. It is very common for avoiding goto
statement.
> > At
> > > same time, it may help reduce {} levels.
> > 
> > I'd prefer something like:
> > 
> > ui::Range target_range;
> > if (client->HasCompositionText())
> >   client->GetCompositionTextRange(&target_range);
> 
> 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.
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.

> 
> > if (target_range.is_empty())
> >   client->GetSelectionRange(&target_range);
> > ...
>

Peng

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc File views/ime/input_method_win.cc (right): http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc#newcode244 views/ime/input_method_win.cc:244: return need_size; On 2011/10/26 19:57:17, James Su wrote: > ...

9 years, 1 month ago (2011-10-27 00:28:00 UTC) #14

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:244: return need_size;
On 2011/10/26 19:57:17, James Su wrote:
> On 2011/10/26 19:36:53, Peng wrote:
> > On 2011/10/26 18:41:57, James Su wrote:
> > > On 2011/10/26 16:20:16, Peng wrote:
> > > > On 2011/10/26 04:25:38, James Su wrote:
> > > > > On 2011/10/25 22:01:21, Peng wrote:
> > > > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > You can't assume it here, especially if you want to retrieve some
extra
> > > > context.
> > > > 
> > > > 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?
> > > > 
> > > 
> > > I don't think it's ok. The current api design looks more reasonable.
> > 
> > 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.
> 
> 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.

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.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:268: LRESULT
InputMethodWin::OnDocumentFeed(RECONVERTSTRING *reconv) {
On 2011/10/26 19:57:17, James Su wrote:
> On 2011/10/26 19:36:53, Peng wrote:
> > On 2011/10/26 18:41:57, James Su wrote:
> > > On 2011/10/26 16:20:16, Peng wrote:
> > > > On 2011/10/26 04:25:38, James Su wrote:
> > > > > On 2011/10/25 22:01:21, Peng wrote:
> > > > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > > > Please refer to the Microsoft sample to see how this method should
> be
> > > > > > > implemented.
> > > > > > 
> > > > > > 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.
> > > > > > 
> > > > > I mean the extra context thing.
> > > > 
> > > > This function returns all text which can be accessed. So it should be
> > enough. 
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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)

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:287: } while (0);
On 2011/10/26 19:57:17, James Su wrote:
> On 2011/10/26 19:36:53, Peng wrote:
> > On 2011/10/26 18:41:57, James Su wrote:
> > > On 2011/10/26 16:20:16, Peng wrote:
> > > > On 2011/10/26 04:25:38, James Su wrote:
> > > > > On 2011/10/25 22:01:21, Peng wrote:
> > > > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > > > nit: I don't like do { } while(0); style myself. It's decrease the
> > code
> > > > > > > readability. Please avoid it if you can.
> > > > > > 
> > > > > > I tried using 'if else', but it looks worse.
> > > > > > 
> > > > > 
> > > > > Please try your best to rewrite this piece of code to eliminate this
do
> {}
> > > > > while(0); thing. I really don't like it.
> > > > 
> > > > I think it is not a big deal. It is very common for avoiding goto
> statement.
> > > At
> > > > same time, it may help reduce {} levels.
> > > 
> > > I'd prefer something like:
> > > 
> > > ui::Range target_range;
> > > if (client->HasCompositionText())
> > >   client->GetCompositionTextRange(&target_range);
> > 
> > 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.
> 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.
> 
> > 
> > > if (target_range.is_empty())
> > >   client->GetSelectionRange(&target_range);
> > > ...
> > 
> 

Done.

http://codereview.chromium.org/8294026/diff/42002/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/42002/views/ime/input_method_win....
views/ime/input_method_win.cc:358: size_t need_size = sizeof(RECONVERTSTRING) +
len * sizeof(WCHAR);
I checked document of IMR_RECONVERTSTRING & IMR_DOCUMENTFEED. It is just for IME
requiring a piece of text which application want to reconvert. If IME want to
make it more accurate, the IME could use IMR_DOCUMENTFEED for more content.

James Su

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc File views/ime/input_method_win.cc (right): http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc#newcode244 views/ime/input_method_win.cc:244: return need_size; On 2011/10/27 00:28:00, Peng wrote: > On ...

9 years, 1 month ago (2011-10-27 04:56:39 UTC) #15

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:244: return need_size;
On 2011/10/27 00:28:00, Peng wrote:
> On 2011/10/26 19:57:17, James Su wrote:
> > On 2011/10/26 19:36:53, Peng wrote:
> > > On 2011/10/26 18:41:57, James Su wrote:
> > > > On 2011/10/26 16:20:16, Peng wrote:
> > > > > On 2011/10/26 04:25:38, James Su wrote:
> > > > > > On 2011/10/25 22:01:21, Peng wrote:
> > > > > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > > > > 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.
> > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > You can't assume it here, especially if you want to retrieve some
> extra
> > > > > context.
> > > > > 
> > > > > 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?
> > > > > 
> > > > 
> > > > I don't think it's ok. The current api design looks more reasonable.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:268: LRESULT
InputMethodWin::OnDocumentFeed(RECONVERTSTRING *reconv) {
On 2011/10/27 00:28:00, Peng wrote:
> On 2011/10/26 19:57:17, James Su wrote:
> > On 2011/10/26 19:36:53, Peng wrote:
> > > On 2011/10/26 18:41:57, James Su wrote:
> > > > On 2011/10/26 16:20:16, Peng wrote:
> > > > > On 2011/10/26 04:25:38, James Su wrote:
> > > > > > On 2011/10/25 22:01:21, Peng wrote:
> > > > > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > > > > Please refer to the Microsoft sample to see how this method
should
> > be
> > > > > > > > implemented.
> > > > > > > 
> > > > > > > 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.
> > > > > > > 
> > > > > > I mean the extra context thing.
> > > > > 
> > > > > This function returns all text which can be accessed. So it should be
> > > enough. 
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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)
> 

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.

http://codereview.chromium.org/8294026/diff/42002/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/42002/views/ime/input_method_win....
views/ime/input_method_win.cc:358: size_t need_size = sizeof(RECONVERTSTRING) +
len * sizeof(WCHAR);
On 2011/10/27 00:28:00, Peng wrote:
> I checked document of IMR_RECONVERTSTRING & IMR_DOCUMENTFEED. It is just for
IME
> requiring a piece of text which application want to reconvert. If IME want to
> make it more accurate, the IME could use IMR_DOCUMENTFEED for more content.

When doing reconversion, the IME can use the surrounding context provided by
IMR_RECONVERTSTRING to improve the accuracy.
IMR_DOCUMENTFEED is used when the IME wants to improve the accuracy of normal
input, e.g. inserting new text in the middle of existing document.

Hironori Bono

Greetings, Thanks for your response. I do understand Chrome disables IMEs on password fields and ...

9 years, 1 month ago (2011-10-27 06:17:47 UTC) #16

Peng

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc File views/ime/input_method_win.cc (right): http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc#newcode244 views/ime/input_method_win.cc:244: return need_size; On 2011/10/27 04:56:39, James Su wrote: > ...

9 years, 1 month ago (2011-10-27 16:12:24 UTC) #17

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:244: return need_size;
On 2011/10/27 04:56:39, James Su wrote:
> On 2011/10/27 00:28:00, Peng wrote:
> > On 2011/10/26 19:57:17, James Su wrote:
> > > On 2011/10/26 19:36:53, Peng wrote:
> > > > On 2011/10/26 18:41:57, James Su wrote:
> > > > > On 2011/10/26 16:20:16, Peng wrote:
> > > > > > On 2011/10/26 04:25:38, James Su wrote:
> > > > > > > On 2011/10/25 22:01:21, Peng wrote:
> > > > > > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > > > > > 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.
> > > > > > > > 
> > > > > > > > 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.
> > > > > > > 
> > > > > > > You can't assume it here, especially if you want to retrieve some
> > extra
> > > > > > context.
> > > > > > 
> > > > > > 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?
> > > > > > 
> > > > > 
> > > > > I don't think it's ok. The current api design looks more reasonable.
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.

http://codereview.chromium.org/8294026/diff/32001/views/ime/input_method_win....
views/ime/input_method_win.cc:268: LRESULT
InputMethodWin::OnDocumentFeed(RECONVERTSTRING *reconv) {
On 2011/10/27 04:56:39, James Su wrote:
> On 2011/10/27 00:28:00, Peng wrote:
> > On 2011/10/26 19:57:17, James Su wrote:
> > > On 2011/10/26 19:36:53, Peng wrote:
> > > > On 2011/10/26 18:41:57, James Su wrote:
> > > > > On 2011/10/26 16:20:16, Peng wrote:
> > > > > > On 2011/10/26 04:25:38, James Su wrote:
> > > > > > > On 2011/10/25 22:01:21, Peng wrote:
> > > > > > > > On 2011/10/25 19:15:03, James Su wrote:
> > > > > > > > > Please refer to the Microsoft sample to see how this method
> should
> > > be
> > > > > > > > > implemented.
> > > > > > > > 
> > > > > > > > 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.
> > > > > > > > 
> > > > > > > I mean the extra context thing.
> > > > > > 
> > > > > > This function returns all text which can be accessed. So it should
be
> > > > enough. 
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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)
> > 
> 
> 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.

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.

http://codereview.chromium.org/8294026/diff/42002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/8294026/diff/42002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_views.cc:751: bool
RenderWidgetHostViewViews::GetTextRange(ui::Range* range) {
On 2011/10/27 06:17:47, hbono wrote:
> nit: I prefer adding 'DCHECK(range)' here.

I used those kind of DHCECK() for input arguments. But some reviewer told me it
is not necessary. They think crash in function can be found and resolved easily,
even if without DCHECK(). So I removed them.

http://codereview.chromium.org/8294026/diff/22016/chrome/browser/renderer_hos...

http://codereview.chromium.org/8294026/diff/42002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_views.cc:752: *range =
ui::Range(selection_text_offset_,
On 2011/10/27 06:17:47, hbono wrote:
> nit: can we use operator= for ui::Range? In general, Google coding style does
> not encourage using copy constructors
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...>.

Done.

http://codereview.chromium.org/8294026/diff/42002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_views.cc:752: *range =
ui::Range(selection_text_offset_,
On 2011/10/27 06:17:47, hbono wrote:
> nit: can we use operator= for ui::Range? In general, Google coding style does
> not encourage using copy constructors
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C...>.

Done.

http://codereview.chromium.org/8294026/diff/42002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_views.cc:763: bool
RenderWidgetHostViewViews::GetSelectionRange(ui::Range* range) {
On 2011/10/27 06:17:47, hbono wrote:
> nit: I prefer adding 'DCHECK(range)' here.

ditto

http://codereview.chromium.org/8294026/diff/42002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_views.cc:764: *range =
selection_range_;
On 2011/10/27 06:17:47, hbono wrote:
> nit: if ui::Range explicitly allows using copy constructors, I prefer
replacing
> it with set_start() and set_end().

Done.

http://codereview.chromium.org/8294026/diff/42002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_views.cc:782: string16*
text) {
On 2011/10/27 06:17:47, hbono wrote:
> nit: I prefer adding 'DCHECK(text)' here.
ditto

http://codereview.chromium.org/8294026/diff/42002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_views.h (right):

http://codereview.chromium.org/8294026/diff/42002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_views.h:180: string16*
text) OVERRIDE;
On 2011/10/27 06:17:47, hbono wrote:
> nit: can you add '#include "base/compiler_specific.h"' and '#include
> "base/string16.h"' at the beginning of this file?

Done.

http://codereview.chromium.org/8294026/diff/42002/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view.cc (right):

http://codereview.chromium.org/8294026/diff/42002/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.cc:68: selection_range_ =
range;
On 2011/10/27 06:17:47, hbono wrote:
> nit: if ui::Range explicitly allows using copy constructors, I prefer
replacing
> it with set_start() and set_end().

Done.

http://codereview.chromium.org/8294026/diff/42002/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/42002/views/ime/input_method_win....
views/ime/input_method_win.cc:358: size_t need_size = sizeof(RECONVERTSTRING) +
len * sizeof(WCHAR);
On 2011/10/27 04:56:39, James Su wrote:
> On 2011/10/27 00:28:00, Peng wrote:
> > I checked document of IMR_RECONVERTSTRING & IMR_DOCUMENTFEED. It is just for
> IME
> > requiring a piece of text which application want to reconvert. If IME want
to
> > make it more accurate, the IME could use IMR_DOCUMENTFEED for more content.
> 
> When doing reconversion, the IME can use the surrounding context provided by
> IMR_RECONVERTSTRING to improve the accuracy.

Because we do not implement the logic to allow IME adjust the reconvert range.
And providing extra text to IME can also cause information leaking. So I decide
to just return the text in the selection range right now until we can support
adjusting the reconvert range and figure out the privacy impact.

I am also wondering if I should remove OnDocumentFeed(). I tested with some
IMEs, did not find any IME use this message. But this function may have security
issue.

James Su

http://codereview.chromium.org/8294026/diff/42002/views/ime/input_method_win.cc File views/ime/input_method_win.cc (right): http://codereview.chromium.org/8294026/diff/42002/views/ime/input_method_win.cc#newcode358 views/ime/input_method_win.cc:358: size_t need_size = sizeof(RECONVERTSTRING) + len * sizeof(WCHAR); On ...

9 years, 1 month ago (2011-10-27 18:39:49 UTC) #18

http://codereview.chromium.org/8294026/diff/42002/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/42002/views/ime/input_method_win....
views/ime/input_method_win.cc:358: size_t need_size = sizeof(RECONVERTSTRING) +
len * sizeof(WCHAR);
On 2011/10/27 16:12:24, Peng wrote:
> On 2011/10/27 04:56:39, James Su wrote:
> > On 2011/10/27 00:28:00, Peng wrote:
> > > I checked document of IMR_RECONVERTSTRING & IMR_DOCUMENTFEED. It is just
for
> > IME
> > > requiring a piece of text which application want to reconvert. If IME want
> to
> > > make it more accurate, the IME could use IMR_DOCUMENTFEED for more
content.
> > 
> > When doing reconversion, the IME can use the surrounding context provided by
> > IMR_RECONVERTSTRING to improve the accuracy.
> 
> Because we do not implement the logic to allow IME adjust the reconvert range.
> And providing extra text to IME can also cause information leaking. So I
decide
> to just return the text in the selection range right now until we can support
> adjusting the reconvert range and figure out the privacy impact.
You still don't get the point.
1) This method has nothing to do with the "set reconvert range" thing. They are
independent functionalities.
2) Returning some extra surrounding context along with the selection text can
improve the reconversion accuracy if the input method can make use the
information.
3) I don't think the privacy impact is a big issue, as we already return the
text inside the selection range. Returning a little more text won't make the
situation worse.

> 
> I am also wondering if I should remove OnDocumentFeed(). I tested with some
> IMEs, did not find any IME use this message. But this function may have
security
> issue.

It may not be very easy to find out whether an IME uses this feature or not.
Because the IME method will still work without this feature. it'll only affect
the conversion accuracy.
I think this method is nice to have. And the privacy issue should not be a big
deal. As we already support similar feature on Mac.

http://codereview.chromium.org/8294026/diff/42003/views/ime/text_input_client.h
File views/ime/text_input_client.h (right):

http://codereview.chromium.org/8294026/diff/42003/views/ime/text_input_client...
views/ime/text_input_client.h:77: // the View. It must cover the composition and
selection range.
nit: accessabled -> accessible.

http://codereview.chromium.org/8294026/diff/42003/views/ime/text_input_client...
views/ime/text_input_client.h:101: // The result will be written in text.
nit: The result will be stored into |*text|.

http://codereview.chromium.org/8294026/diff/42003/views/ime/text_input_client...
views/ime/text_input_client.h:103: // given range.
nit: ... or the specified range is out of the text range returned by
GetTextRange().

Now I understand your intention to remove the |actual_text| parameter. I agree
that it's also a feasible way. Comparing the original API, your proposal moves
the implementation burden to the input method side.

With the original API, the input method can simply call GetTextFromRange() with
a range it wants and rely on the text input client to adjust the range to a
supported range. The input method does not need to care about if the specified
range is out of the text range returned by GetTextRange() or not. Because the
text input client ensures the validity of the actual_range value.
In another word, in the original API, GetTextRange() method is actually not
mandatory.

With your proposal, the input method must make sure the required range is inside
the text range returned by GetTextRange() before calling GetTextFromRange(),
otherwise the call will fail.

So you can see that, your proposal actually does not simplify the code as you
thought. It just moves the code around.

I'd like to see how the code looks with the original API, so that we can compare
these two approach to see which one is better.

James Su

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc#newcode2141 content/browser/renderer_host/render_widget_host_view_win.cc:2141: return reinterpret_cast<LRESULT>(reconv); Please consider to extract the common code ...

9 years, 1 month ago (2011-10-28 04:20:45 UTC) #19

Peng

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc#newcode2141 content/browser/renderer_host/render_widget_host_view_win.cc:2141: return reinterpret_cast<LRESULT>(reconv); On 2011/10/28 04:20:45, James Su wrote: > ...

9 years, 1 month ago (2011-10-28 15:42:19 UTC) #20

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2141: return
reinterpret_cast<LRESULT>(reconv);
On 2011/10/28 04:20:45, James Su wrote:
> Please consider to extract the common code from this method and
> InputMethodWin::OnReconvertString() and put them into a shared function in
> ime_input.cc

I didn't find a easy way to do it. Because ime_input.cc do not have a good way
to get the text, text_range, selection_range and etc from TextInputClient and
RWHVWin.

Probably the only way I can figure out is let RWHVWin implements the
TextInputClient interface too, and pass the textinputclient to ime_input.cc. I
think it is not must to do, right?

http://codereview.chromium.org/8294026/diff/42003/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8294026/diff/42003/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:798: *text =
model_->GetTextFromRange(range);
On 2011/10/28 04:20:45, James Su wrote:
> You need to make sure the range is inside the content range and return false
if
> it's not.

Done.

http://codereview.chromium.org/8294026/diff/42003/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/42003/views/ime/input_method_win....
views/ime/input_method_win.cc:13: static const size_t kExtraNumberOfChars = 100;
On 2011/10/28 04:20:45, James Su wrote:
> I think 10~20 should be enough.
> And how about to add a comment to describe the purpose of this constant?

Done.

http://codereview.chromium.org/8294026/diff/42003/views/ime/input_method_win....
views/ime/input_method_win.cc:365: size_t len = selection_range.length();
On 2011/10/28 04:20:45, James Su wrote:
> If you really don't want to return extra surrounding context, please add a
> comment to explain the reason and add a TODO, in case we want to support it in
> the future to help improve IMEs' reconversion accuracy.

Done.

http://codereview.chromium.org/8294026/diff/42003/views/ime/input_method_win....
views/ime/input_method_win.cc:375: if
(!GetTextInputClient()->GetTextFromRange(selection_range, &text))
On 2011/10/28 04:20:45, James Su wrote:
> nit: check the text length in case it's shorter than len?

The API guarantee the text has right size with given range. So checking it is
not necessary.

http://codereview.chromium.org/8294026/diff/42003/views/ime/input_method_win....
views/ime/input_method_win.cc:389: return reinterpret_cast<LRESULT>(reconv);
On 2011/10/28 04:20:45, James Su wrote:
> Though msdn says that this method should return the structure on success, but
> I'd rather believe it's a mistake of the document, because any implementations
I
> can find on the internet return the structure size instead, even the sample
> provided by Microsoft.

I am not sure. Most IME just tests the return value is not zero. So both are OK.
But if some IMEs test the return value with the structure pointer, it may has
problem. So I guess following API document is safer.

http://codereview.chromium.org/8294026/diff/42003/views/ime/text_input_client.h
File views/ime/text_input_client.h (right):

http://codereview.chromium.org/8294026/diff/42003/views/ime/text_input_client...
views/ime/text_input_client.h:77: // the View. It must cover the composition and
selection range.
On 2011/10/27 18:39:49, James Su wrote:
> nit: accessabled -> accessible.

Done.

http://codereview.chromium.org/8294026/diff/42003/views/ime/text_input_client...
views/ime/text_input_client.h:101: // The result will be written in text.
On 2011/10/27 18:39:49, James Su wrote:
> nit: The result will be stored into |*text|.

Done.

http://codereview.chromium.org/8294026/diff/42003/views/ime/text_input_client...
views/ime/text_input_client.h:103: // given range.
On 2011/10/27 18:39:49, James Su wrote:
> nit: ... or the specified range is out of the text range returned by
> GetTextRange().
> 
> Now I understand your intention to remove the |actual_text| parameter. I agree
> that it's also a feasible way. Comparing the original API, your proposal moves
> the implementation burden to the input method side.
> 
> With the original API, the input method can simply call GetTextFromRange()
with
> a range it wants and rely on the text input client to adjust the range to a
> supported range. The input method does not need to care about if the specified
> range is out of the text range returned by GetTextRange() or not. Because the
> text input client ensures the validity of the actual_range value.
> In another word, in the original API, GetTextRange() method is actually not
> mandatory.
> 
> With your proposal, the input method must make sure the required range is
inside
> the text range returned by GetTextRange() before calling GetTextFromRange(),
> otherwise the call will fail.
> 
> So you can see that, your proposal actually does not simplify the code as you
> thought. It just moves the code around.
> 
> I'd like to see how the code looks with the original API, so that we can
compare
> these two approach to see which one is better.

Done.

James Su

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc#newcode2141 content/browser/renderer_host/render_widget_host_view_win.cc:2141: return reinterpret_cast<LRESULT>(reconv); On 2011/10/28 15:42:19, Peng wrote: > On ...

9 years, 1 month ago (2011-10-28 18:16:03 UTC) #21

Peng

http://codereview.chromium.org/8294026/diff/39015/content/browser/renderer_host/render_widget_host_view_win.cc File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/8294026/diff/39015/content/browser/renderer_host/render_widget_host_view_win.cc#newcode2072 content/browser/renderer_host/render_widget_host_view_win.cc:2072: return 0; On 2011/10/28 18:16:04, James Su wrote: > ...

9 years, 1 month ago (2011-10-28 19:13:05 UTC) #22

James Su

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc#newcode2141 content/browser/renderer_host/render_widget_host_view_win.cc:2141: return reinterpret_cast<LRESULT>(reconv); On 2011/10/28 18:16:04, James Su wrote: > ...

9 years, 1 month ago (2011-10-28 22:26:31 UTC) #23

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2141: return
reinterpret_cast<LRESULT>(reconv);
On 2011/10/28 18:16:04, James Su wrote:
> On 2011/10/28 15:42:19, Peng wrote:
> > On 2011/10/28 04:20:45, James Su wrote:
> > > Please consider to extract the common code from this method and
> > > InputMethodWin::OnReconvertString() and put them into a shared function in
> > > ime_input.cc
> > 
> > I didn't find a easy way to do it. Because ime_input.cc do not have a good
way
> > to get the text, text_range, selection_range and etc from TextInputClient
and
> > RWHVWin.
> > 
> > Probably the only way I can figure out is let RWHVWin implements the
> > TextInputClient interface too, and pass the textinputclient to ime_input.cc.
I
> > think it is not must to do, right?
> > 
> 
> I think at least the part of filling reconv structure can be extracted (line
> 2121~2141). It helps make the code more maintainable.

I check the code again and do think they can be extracted and unified into a
shared function. The function prototype could be:

LRESULT FillReconvertString(const string16& text, size_t comp_len, size_t
comp_offset, size_t target_len, size_t target_offset, RECONVERTSTRING* reconv);

http://codereview.chromium.org/8294026/diff/50003/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8294026/diff/50003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2101: // and return
reconv, but some applications return need_size.
nit: How about: According to Microsoft API document, ... and ... should return
reconv, but ...

http://codereview.chromium.org/8294026/diff/50003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2144: // and return
reconv, but some applications return need_size.
ditto.

http://codereview.chromium.org/8294026/diff/50003/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/50003/views/ime/input_method_win....
views/ime/input_method_win.cc:345: // and return reconv, but some applications
return need_size.
ditto.

http://codereview.chromium.org/8294026/diff/50003/views/ime/input_method_win....
views/ime/input_method_win.cc:398: // and return reconv, but some applications
return need_size.
ditto.

Peng

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc#newcode2141 content/browser/renderer_host/render_widget_host_view_win.cc:2141: return reinterpret_cast<LRESULT>(reconv); On 2011/10/28 22:26:32, James Su wrote: > ...

9 years, 1 month ago (2011-10-29 03:28:09 UTC) #24

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2141: return
reinterpret_cast<LRESULT>(reconv);
On 2011/10/28 22:26:32, James Su wrote:
> On 2011/10/28 18:16:04, James Su wrote:
> > On 2011/10/28 15:42:19, Peng wrote:
> > > On 2011/10/28 04:20:45, James Su wrote:
> > > > Please consider to extract the common code from this method and
> > > > InputMethodWin::OnReconvertString() and put them into a shared function
in
> > > > ime_input.cc
> > > 
> > > I didn't find a easy way to do it. Because ime_input.cc do not have a good
> way
> > > to get the text, text_range, selection_range and etc from TextInputClient
> and
> > > RWHVWin.
> > > 
> > > Probably the only way I can figure out is let RWHVWin implements the
> > > TextInputClient interface too, and pass the textinputclient to
ime_input.cc.
> I
> > > think it is not must to do, right?
> > > 
> > 
> > I think at least the part of filling reconv structure can be extracted (line
> > 2121~2141). It helps make the code more maintainable.
> 
> I check the code again and do think they can be extracted and unified into a
> shared function. The function prototype could be:
> 
> LRESULT FillReconvertString(const string16& text, size_t comp_len, size_t
> comp_offset, size_t target_len, size_t target_offset, RECONVERTSTRING*
reconv);

I agree. Filling structure can be a function. But I will be on a trip next whole
week. And can not test any changes. How about I do  it later? So I can also
think about how to extract more common code. Thanks.

http://codereview.chromium.org/8294026/diff/50003/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8294026/diff/50003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2101: // and return
reconv, but some applications return need_size.
On 2011/10/28 22:26:32, James Su wrote:
> nit: How about: According to Microsoft API document, ... and ... should return
> reconv, but ...

Done.

http://codereview.chromium.org/8294026/diff/50003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2144: // and return
reconv, but some applications return need_size.
On 2011/10/28 22:26:32, James Su wrote:
> ditto.

Done.

http://codereview.chromium.org/8294026/diff/50003/views/ime/input_method_win.cc
File views/ime/input_method_win.cc (right):

http://codereview.chromium.org/8294026/diff/50003/views/ime/input_method_win....
views/ime/input_method_win.cc:345: // and return reconv, but some applications
return need_size.
On 2011/10/28 22:26:32, James Su wrote:
> ditto.

Done.

http://codereview.chromium.org/8294026/diff/50003/views/ime/input_method_win....
views/ime/input_method_win.cc:398: // and return reconv, but some applications
return need_size.
On 2011/10/28 22:26:32, James Su wrote:
> ditto.

Done.

James Su

LGTM. http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc#newcode2141 content/browser/renderer_host/render_widget_host_view_win.cc:2141: return reinterpret_cast<LRESULT>(reconv); On 2011/10/29 03:28:10, Peng wrote: > ...

9 years, 1 month ago (2011-10-30 11:48:39 UTC) #25

LGTM.

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2141: return
reinterpret_cast<LRESULT>(reconv);
On 2011/10/29 03:28:10, Peng wrote:
> On 2011/10/28 22:26:32, James Su wrote:
> > On 2011/10/28 18:16:04, James Su wrote:
> > > On 2011/10/28 15:42:19, Peng wrote:
> > > > On 2011/10/28 04:20:45, James Su wrote:
> > > > > Please consider to extract the common code from this method and
> > > > > InputMethodWin::OnReconvertString() and put them into a shared
function
> in
> > > > > ime_input.cc
> > > > 
> > > > I didn't find a easy way to do it. Because ime_input.cc do not have a
good
> > way
> > > > to get the text, text_range, selection_range and etc from
TextInputClient
> > and
> > > > RWHVWin.
> > > > 
> > > > Probably the only way I can figure out is let RWHVWin implements the
> > > > TextInputClient interface too, and pass the textinputclient to
> ime_input.cc.
> > I
> > > > think it is not must to do, right?
> > > > 
> > > 
> > > I think at least the part of filling reconv structure can be extracted
(line
> > > 2121~2141). It helps make the code more maintainable.
> > 
> > I check the code again and do think they can be extracted and unified into a
> > shared function. The function prototype could be:
> > 
> > LRESULT FillReconvertString(const string16& text, size_t comp_len, size_t
> > comp_offset, size_t target_len, size_t target_offset, RECONVERTSTRING*
> reconv);
> 
> I agree. Filling structure can be a function. But I will be on a trip next
whole
> week. And can not test any changes. How about I do  it later? So I can also
> think about how to extract more common code. Thanks.
>  

Alright, but please add a TODO to make sure you won't forget it.

Peng

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_host/render_widget_host_view_win.cc#newcode2141 content/browser/renderer_host/render_widget_host_view_win.cc:2141: return reinterpret_cast<LRESULT>(reconv); On 2011/10/30 11:48:39, James Su wrote: > ...

9 years, 1 month ago (2011-10-30 12:38:15 UTC) #26

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8294026/diff/42003/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:2141: return
reinterpret_cast<LRESULT>(reconv);
On 2011/10/30 11:48:39, James Su wrote:
> On 2011/10/29 03:28:10, Peng wrote:
> > On 2011/10/28 22:26:32, James Su wrote:
> > > On 2011/10/28 18:16:04, James Su wrote:
> > > > On 2011/10/28 15:42:19, Peng wrote:
> > > > > On 2011/10/28 04:20:45, James Su wrote:
> > > > > > Please consider to extract the common code from this method and
> > > > > > InputMethodWin::OnReconvertString() and put them into a shared
> function
> > in
> > > > > > ime_input.cc
> > > > > 
> > > > > I didn't find a easy way to do it. Because ime_input.cc do not have a
> good
> > > way
> > > > > to get the text, text_range, selection_range and etc from
> TextInputClient
> > > and
> > > > > RWHVWin.
> > > > > 
> > > > > Probably the only way I can figure out is let RWHVWin implements the
> > > > > TextInputClient interface too, and pass the textinputclient to
> > ime_input.cc.
> > > I
> > > > > think it is not must to do, right?
> > > > > 
> > > > 
> > > > I think at least the part of filling reconv structure can be extracted
> (line
> > > > 2121~2141). It helps make the code more maintainable.
> > > 
> > > I check the code again and do think they can be extracted and unified into
a
> > > shared function. The function prototype could be:
> > > 
> > > LRESULT FillReconvertString(const string16& text, size_t comp_len, size_t
> > > comp_offset, size_t target_len, size_t target_offset, RECONVERTSTRING*
> > reconv);
> > 
> > I agree. Filling structure can be a function. But I will be on a trip next
> whole
> > week. And can not test any changes. How about I do  it later? So I can also
> > think about how to extract more common code. Thanks.
> >  
> 
> Alright, but please add a TODO to make sure you won't forget it.

Done. filed a bug for it.

Peng

On 2011/10/27 06:17:47, hbono wrote: > Greetings, > > Thanks for your response. > I ...

9 years, 1 month ago (2011-10-30 13:43:55 UTC) #27

Hironori Bono

LGTM. Thanks for your change. Regards, Hironori Bono

9 years, 1 month ago (2011-10-31 05:52:06 UTC) #28

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/8294026/55001

9 years, 1 month ago (2011-10-31 07:55:07 UTC) #29

commit-bot: I haz the power

Try job failure for 8294026-55001 on linux_clang for steps "update_scripts, update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clang&number=162 Step "update" is ...

9 years, 1 month ago (2011-10-31 07:55:52 UTC) #30

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/penghuang@chromium.org/8294026/55001

9 years, 1 month ago (2011-10-31 08:04:27 UTC) #31

Change committed as 107934

Issue 8294026: Support IMM32 reconversion on Windows (Closed)

Description

Patch Set 1 #

Patch Set 2 : WIP #

Patch Set 3 : wip #

Patch Set 4 : wip #

Patch Set 5 : wip #

Patch Set 6 : wip #

Patch Set 7 : wip #

Patch Set 8 : wip #

Patch Set 9 : wip #

Patch Set 10 : wip #

Patch Set 11 : Rebase #

Patch Set 12 : wip #

Patch Set 13 : wip #

Patch Set 14 : wip #

Patch Set 15 : Fix style issues #

Patch Set 16 : wip #

Patch Set 17 : Fix review issues #

Patch Set 18 : Remove unused header file #

Patch Set 19 : Fix review issues #

Patch Set 20 : Fix review issues #

Patch Set 21 : Fix review issues #

Patch Set 22 : update #

Patch Set 23 : Fix return values #

Patch Set 24 : update #

Patch Set 25 : Fix review issues #

Patch Set 26 : Fix review issues #

Patch Set 27 : Refine some inline comments #

Patch Set 28 : Rebase #

Messages