Add common data types for input method support.

Add common data types for input method support.

This CL adds following common data types for input method support:
1. struct CompositionUnderline
2. struct Composition
3. enum TextInputType

BUG=75003
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78793

[James Su, 2011-03-17 18:19:55]

Hi,
  It's the first CL of the new Views input method API. By putting these common
data types into ui/base/ime, some code can be shared among Linux/Gtk, Windows
native and Views implementations, noticeably, code used in
chrome/browser/ime_input.* and
chrome/browser/renderer_host/gtk_im_context_wrapper.*

Next step: I'll move shareable code from these files into ui/base/.

Regards
James Su

[Peng, 2011-03-18 18:01:11]

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc
File ui/base/ime/composition.cc (right):

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc#newc...
ui/base/ime/composition.cc:8: 
It is better to put those struct's functions in header file, and remove this
file?

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h
File ui/base/ime/composition.h (right):

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h#newco...
ui/base/ime/composition.h:29: // if the range length is zero.
What's the selection range in the composition text. If the length is not zero,
what's right behavior of the textfield and etc? Please add more detail in here.

[James Su, 2011-03-18 18:11:24]

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc
File ui/base/ime/composition.cc (right):

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc#newc...
ui/base/ime/composition.cc:8: 
On 2011/03/18 18:01:11, Peng wrote:
> It is better to put those struct's functions in header file, and remove this
> file?

According to chromium code style, we shouldn't inline constructors.

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h
File ui/base/ime/composition.h (right):

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h#newco...
ui/base/ime/composition.h:29: // if the range length is zero.
On 2011/03/18 18:01:11, Peng wrote:
> What's the selection range in the composition text. If the length is not zero,
> what's right behavior of the textfield and etc? Please add more detail in
here.

It's the text range selected in the composition text, just like normal
selection. It usually indicates the target clause. Gtk doesn't have such
concept, background color is usually used instead. Both Windows and Mac has such
concept and WebKit is supposed to support it, though there is a bug. See:
https://bugs.webkit.org/show_bug.cgi?id=37788

[Peng, 2011-03-18 18:50:11]

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc
File ui/base/ime/composition.cc (right):

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc#newc...
ui/base/ime/composition.cc:8: 
But it still looks not much reasonable for me, to add a .cc file for a such
simple struct. Maybe you should use class instead of struct to emphasize this is
not a struct, it has some member functions.

On 2011/03/18 18:11:24, James Su wrote:
> On 2011/03/18 18:01:11, Peng wrote:
> > It is better to put those struct's functions in header file, and remove this
> > file?
> 
> According to chromium code style, we shouldn't inline constructors.

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h
File ui/base/ime/composition.h (right):

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h#newco...
ui/base/ime/composition.h:29: // if the range length is zero.
Please add detail about the behavior in comment. If it is used only for display
(as Pango background), please write them down. It may confuse Client developers.

On 2011/03/18 18:11:24, James Su wrote:
> On 2011/03/18 18:01:11, Peng wrote:
> > What's the selection range in the composition text. If the length is not
zero,
> > what's right behavior of the textfield and etc? Please add more detail in
> here.
> 
> It's the text range selected in the composition text, just like normal
> selection. It usually indicates the target clause. Gtk doesn't have such
> concept, background color is usually used instead. Both Windows and Mac has
such
> concept and WebKit is supposed to support it, though there is a bug. See:
> https://bugs.webkit.org/show_bug.cgi?id=37788

[oshima, 2011-03-18 19:03:25]

LGTM on my side

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition_underli...
File ui/base/ime/composition_underline.h (right):

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition_underli...
ui/base/ime/composition_underline.h:31: unsigned end_offset;
use of unsigned is discouraged. I'm guess this is because WebKit uses it, so
please mention in the comment.

[James Su, 2011-03-18 19:18:35]

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc
File ui/base/ime/composition.cc (right):

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc#newc...
ui/base/ime/composition.cc:8: 
On 2011/03/18 18:50:11, Peng wrote:
> But it still looks not much reasonable for me, to add a .cc file for a such
> simple struct. Maybe you should use class instead of struct to emphasize this
is
> not a struct, it has some member functions.
> 
> On 2011/03/18 18:11:24, James Su wrote:
> > On 2011/03/18 18:01:11, Peng wrote:
> > > It is better to put those struct's functions in header file, and remove
this
> > > file?
> > 
> > According to chromium code style, we shouldn't inline constructors.
> 

Just removed it. We actually don't need it.

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h
File ui/base/ime/composition.h (right):

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h#newco...
ui/base/ime/composition.h:29: // if the range length is zero.
Added some comments.

On 2011/03/18 18:50:11, Peng wrote:
> Please add detail about the behavior in comment. If it is used only for
display
> (as Pango background), please write them down. It may confuse Client
developers.
> 
> On 2011/03/18 18:11:24, James Su wrote:
> > On 2011/03/18 18:01:11, Peng wrote:
> > > What's the selection range in the composition text. If the length is not
> zero,
> > > what's right behavior of the textfield and etc? Please add more detail in
> > here.
> > 
> > It's the text range selected in the composition text, just like normal
> > selection. It usually indicates the target clause. Gtk doesn't have such
> > concept, background color is usually used instead. Both Windows and Mac has
> such
> > concept and WebKit is supposed to support it, though there is a bug. See:
> > https://bugs.webkit.org/show_bug.cgi?id=37788
>

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition_underli...
File ui/base/ime/composition_underline.h (right):

http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition_underli...
ui/base/ime/composition_underline.h:31: unsigned end_offset;
On 2011/03/18 19:03:25, oshima wrote:
> use of unsigned is discouraged. I'm guess this is because WebKit uses it, so
> please mention in the comment.
> 

Done.

[Peng, 2011-03-18 20:01:31]

It looks weird a .cc  file just for a simple struct function. Anyway. It LGTM.

On 2011/03/18 19:18:35, James Su wrote:
> http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc
> File ui/base/ime/composition.cc (right):
> 
>
http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc#newc...
> ui/base/ime/composition.cc:8: 
> On 2011/03/18 18:50:11, Peng wrote:
> > But it still looks not much reasonable for me, to add a .cc file for a such
> > simple struct. Maybe you should use class instead of struct to emphasize
this
> is
> > not a struct, it has some member functions.
> > 
> > On 2011/03/18 18:11:24, James Su wrote:
> > > On 2011/03/18 18:01:11, Peng wrote:
> > > > It is better to put those struct's functions in header file, and remove
> this
> > > > file?
> > > 
> > > According to chromium code style, we shouldn't inline constructors.
> > 
> 
> Just removed it. We actually don't need it.
> 
> http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h
> File ui/base/ime/composition.h (right):
> 
>
http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h#newco...
> ui/base/ime/composition.h:29: // if the range length is zero.
> Added some comments.
> 
> On 2011/03/18 18:50:11, Peng wrote:
> > Please add detail about the behavior in comment. If it is used only for
> display
> > (as Pango background), please write them down. It may confuse Client
> developers.
> > 
> > On 2011/03/18 18:11:24, James Su wrote:
> > > On 2011/03/18 18:01:11, Peng wrote:
> > > > What's the selection range in the composition text. If the length is not
> > zero,
> > > > what's right behavior of the textfield and etc? Please add more detail
in
> > > here.
> > > 
> > > It's the text range selected in the composition text, just like normal
> > > selection. It usually indicates the target clause. Gtk doesn't have such
> > > concept, background color is usually used instead. Both Windows and Mac
has
> > such
> > > concept and WebKit is supposed to support it, though there is a bug. See:
> > > https://bugs.webkit.org/show_bug.cgi?id=37788
> >
> 
>
http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition_underli...
> File ui/base/ime/composition_underline.h (right):
> 
>
http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition_underli...
> ui/base/ime/composition_underline.h:31: unsigned end_offset;
> On 2011/03/18 19:03:25, oshima wrote:
> > use of unsigned is discouraged. I'm guess this is because WebKit uses it, so
> > please mention in the comment.
> > 
> 
> Done.

[oshima, 2011-03-18 22:00:49]

On Fri, Mar 18, 2011 at 1:01 PM, <penghuang@chromium.org> wrote:

> It looks weird a .cc  file just for a simple struct function. Anyway. It
> LGTM.


This is actually not simple because it calls constructors of
string16, CompositionUnderlines (which is vector of another struct), and
Range.
clang bot may give you warning.. We're also trying to reduce the size,
and avoiding inline is recommended. Just FYI.

- oshima


>
>
> On 2011/03/18 19:18:35, James Su wrote:
>
>> http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc
>> File ui/base/ime/composition.cc (right):
>>
>
>
>
>
http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.cc#newc...
>
>> ui/base/ime/composition.cc:8:
>> On 2011/03/18 18:50:11, Peng wrote:
>> > But it still looks not much reasonable for me, to add a .cc file for a
>> such
>> > simple struct. Maybe you should use class instead of struct to emphasize
>>
> this
>
>> is
>> > not a struct, it has some member functions.
>> >
>> > On 2011/03/18 18:11:24, James Su wrote:
>> > > On 2011/03/18 18:01:11, Peng wrote:
>> > > > It is better to put those struct's functions in header file, and
>> remove
>> this
>> > > > file?
>> > >
>> > > According to chromium code style, we shouldn't inline constructors.
>> >
>>
>
>  Just removed it. We actually don't need it.
>>
>
>  http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h
>> File ui/base/ime/composition.h (right):
>>
>
>
>
>
http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition.h#newco...
>
>> ui/base/ime/composition.h:29: // if the range length is zero.
>> Added some comments.
>>
>
>  On 2011/03/18 18:50:11, Peng wrote:
>> > Please add detail about the behavior in comment. If it is used only for
>> display
>> > (as Pango background), please write them down. It may confuse Client
>> developers.
>> >
>> > On 2011/03/18 18:11:24, James Su wrote:
>> > > On 2011/03/18 18:01:11, Peng wrote:
>> > > > What's the selection range in the composition text. If the length is
>> not
>> > zero,
>> > > > what's right behavior of the textfield and etc? Please add more
>> detail
>>
> in
>
>> > > here.
>> > >
>> > > It's the text range selected in the composition text, just like normal
>> > > selection. It usually indicates the target clause. Gtk doesn't have
>> such
>> > > concept, background color is usually used instead. Both Windows and
>> Mac
>>
> has
>
>> > such
>> > > concept and WebKit is supposed to support it, though there is a bug.
>> See:
>> > > https://bugs.webkit.org/show_bug.cgi?id=37788
>> >
>>
>
>
>
>
http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition_underli...
>
>> File ui/base/ime/composition_underline.h (right):
>>
>
>
>
>
http://codereview.chromium.org/6711008/diff/1/ui/base/ime/composition_underli...
>
>> ui/base/ime/composition_underline.h:31: unsigned end_offset;
>> On 2011/03/18 19:03:25, oshima wrote:
>> > use of unsigned is discouraged. I'm guess this is because WebKit uses
>> it, so
>> > please mention in the comment.
>> >
>>
>
>  Done.
>>
>
>
>
> http://codereview.chromium.org/6711008/
>