Issue 5857002: no native implementation of Textfield.

Issue 5857002: no native implementation of Textfield. (Closed)

Created:
10 years ago by oshima

Modified:
9 years, 6 months ago

Reviewers:
rjkroege, beng (no_code_reviews), Paweł Hajdan Jr., sky, Ben Goodger (Google)

CC:
chromium-reviews, brettw-cc_chromium.org, varunjain, nkostylev+cc_chromium.org, davemoore+watch_chromium.org, Paweł Hajdan Jr.

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

no native implementation of Textfield. This is based on the original CL http://codereview.chromium.org/3142008. The key difference is * This uses Textfield framework and NativeTextfieldView implements NativeTextfieldWrapper. This allows us to swap the implementation without recompling the tree and can start testing on bots. * Changed the name of the model to TextfieldViewModel as TextfieldModel may be confusing as other Textfield implementations are not using it. I also changed to use string16 instead of gap buffer as it's enough for single line text. We can update the model to use GapBuffer when necessary. * Changed to use string16 as that's what chrome codebase should use. * Added a switch to turn on TextfieldView. I also filled a couple of features such as: * selection by key * mouse actions (move cursor, selection) * used WordIterator, which is i18n compatible, to move cursor by word * blinking cursor This is only for linux based build due to KeyStroke difference. I'm going to move some of test utlity function in chrome/browser/automation/ui_controls to app/test and will add more test once the migration is done. BUG=none TEST=new unit tests are added : NativeTestfieldViewTest and TextfieldViewModelTest. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69523

Patch Set 1 #

Patch Set 2 : fix test #

Patch Set 3 : " #

Total comments: 73

Patch Set 4 : addressed comments. fix touch build #

Patch Set 5 : "remove chrome switch. always use TextfieldView for touch" #

Patch Set 6 : " #

Patch Set 7 : view -> views rename #

Total comments: 1

Patch Set 8 : fix test #

Patch Set 9 : " #

Total comments: 1

Patch Set 10 : remove selection_end and add tests #

Total comments: 18

Patch Set 11 : " #

Patch Set 12 : " #

Total comments: 4

Patch Set 13 : fix comment #

Created: 10 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1866 lines, -32 lines)			Patch
M	chrome/browser/chromeos/login/screen_locker_tester.cc	View	1	2 chunks	+3 lines, -1 line	0 comments	Download
M	chrome/browser/ui/views/dropdown_bar_host_gtk.cc	View	1 2 3 4 5 6 7 8 9 10 11 12	2 chunks	+21 lines, -0 lines	0 comments	Download
M	chrome/chrome_tests.gypi	View	1 2 3 4 5 6 7 8 9	2 chunks	+4 lines, -0 lines	0 comments	Download
M	views/controls/textfield/native_textfield_gtk.cc	View	1 2 3	3 chunks	+4 lines, -11 lines	0 comments	Download
A	views/controls/textfield/native_textfield_views.h	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+172 lines, -0 lines	0 comments	Download
A	views/controls/textfield/native_textfield_views.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+698 lines, -0 lines	0 comments	Download
A	views/controls/textfield/native_textfield_views_unittest.cc	View	1 2 3 4 5 6 7 8	1 chunk	+226 lines, -0 lines	0 comments	Download
M	views/controls/textfield/textfield.h	View	1 2 3	4 chunks	+9 lines, -5 lines	0 comments	Download
M	views/controls/textfield/textfield.cc	View	1	2 chunks	+3 lines, -6 lines	0 comments	Download
A	views/controls/textfield/textfield_views_model.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+158 lines, -0 lines	0 comments	Download
A	views/controls/textfield/textfield_views_model.cc	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+247 lines, -0 lines	0 comments	Download
A	views/controls/textfield/textfield_views_model_unittest.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+285 lines, -0 lines	0 comments	Download
M	views/event.h	View	1 2 3	3 chunks	+7 lines, -2 lines	0 comments	Download
M	views/event_gtk.cc	View	1 2 3	1 chunk	+6 lines, -2 lines	0 comments	Download
M	views/event_x.cc	View	1 2 3	1 chunk	+2 lines, -1 line	0 comments	Download
M	views/views.gyp	View	1 2 3 4 5 6	4 chunks	+14 lines, -0 lines	0 comments	Download
M	views/widget/root_view.cc	View	1 2 3 4 5 6	2 chunks	+7 lines, -4 lines	0 comments	Download

Messages

Total messages: 20 (0 generated)

Expand Messages | Collapse Messages

oshima

http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/textfield.h File views/controls/textfield/textfield.h (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/textfield.h#newcode46 views/controls/textfield/textfield.h:46: class Keystroke { I wonder if we need this ...

10 years ago (2010-12-15 17:29:06 UTC) #1

sky

http://codereview.chromium.org/5857002/diff/15001/chrome/common/chrome_switches.cc File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/5857002/diff/15001/chrome/common/chrome_switches.cc#newcode1282 chrome/common/chrome_switches.cc:1282: const char kEnableTextfieldView[] = "enable-textfield-view"; nit: align '=' with ...

10 years ago (2010-12-15 20:31:27 UTC) #2

beng (no_code_reviews)

http://codereview.chromium.org/5857002/diff/15001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/5857002/diff/15001/chrome/browser/browser_main.cc#newcode901 chrome/browser/browser_main.cc:901: if (parameters.command_line_.HasSwitch(switches::kEnableTextfieldView)) This check should be done inside CreateWrapper, ...

10 years ago (2010-12-15 20:41:59 UTC) #3

rjkroege

http://codereview.chromium.org/5857002/diff/15001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/5857002/diff/15001/chrome/browser/browser_main.cc#newcode158 chrome/browser/browser_main.cc:158: #if defined(OS_LINUX) Why OS_LINUX? We have been TOUCH_UI for ...

10 years ago (2010-12-15 21:23:31 UTC) #4

oshima

http://codereview.chromium.org/5857002/diff/15001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/5857002/diff/15001/chrome/browser/browser_main.cc#newcode158 chrome/browser/browser_main.cc:158: #if defined(OS_LINUX) On 2010/12/15 21:23:31, rjkroege wrote: > Why ...

10 years ago (2010-12-16 01:15:18 UTC) #5

oshima

Hello, Seems like I was missing something. Other views only classes are using prefix _views ...

10 years ago (2010-12-16 02:00:19 UTC) #6

oshima

On Wed, Dec 15, 2010 at 6:00 PM, <oshima@chromium.org> wrote: > Hello, > > Seems ...

10 years ago (2010-12-16 02:20:44 UTC) #7

Paweł Hajdan Jr.

Drive-by with a test comment. http://codereview.chromium.org/5857002/diff/35001/views/controls/textfield/native_textfield_views_unittest.cc File views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/5857002/diff/35001/views/controls/textfield/native_textfield_views_unittest.cc#newcode58 views/controls/textfield/native_textfield_views_unittest.cc:58: DCHECK(!textfield_); Could you convert ...

10 years ago (2010-12-16 09:02:21 UTC) #8

oshima

On Thu, Dec 16, 2010 at 1:02 AM, <phajdan.jr@chromium.org> wrote: > Drive-by with a test ...

10 years ago (2010-12-16 14:18:51 UTC) #9

sky

http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/textfield_view_model.h File views/controls/textfield/textfield_view_model.h (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/textfield_view_model.h#newcode150 views/controls/textfield/textfield_view_model.h:150: size_t begin_, end_; On 2010/12/16 01:15:19, oshima wrote: > ...

10 years ago (2010-12-16 17:29:43 UTC) #10

http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te...
File views/controls/textfield/textfield_view_model.h (right):

http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te...
views/controls/textfield/textfield_view_model.h:150: size_t begin_, end_;
On 2010/12/16 01:15:19, oshima wrote:
> On 2010/12/15 20:31:27, sky wrote:
> > Could you combine cursor_pos_ and begin_ end_ ? Is the cursor_pos always one
> of
> > begin_ or end_?
> 
> Only if something is selected. There are several options:
> 1) current impl
>    - pros: canonical. has selection iff begin != end.
>    - cons: need extra variable
> 2) have one selection_start_, and move selection_start_ along with cursor if
> there is no selection
>    - pros: canonical. has selection iff begin != end.
>    - cons: needs to update selection_start_ for every regular cursor move.

I don't understand this con. You have to update something when the cursor moves.
In (1) it's cursor_pos_, in (2) its selection_begin_. Maybe you mean (2) also
has to update selection_end_?

> 3) change selection_start_ to int and use -1 as an indication of not selected.
>    - pros: no need for extra variable
>    - cons: this has different type than other indexes.
>            not canonical. need to check cursor != end.
> 4) have boolean "has_selection_".
>    - pros: more explicit
>    - cons: not canonical. still to check if curosr == end.
> I ended up 1), but if you don't like 1), 2) or 4) is fine. I think 3) is worst
> of 4.

I prefer (2). If you go with (1) it is easier to accidentally get things out of
sync.

> 
>  I would also suggest renaming to selection_begin_,
> > selection_end_. Also, make it clear that begin_ may be > end_
> 
> done.
>

http://codereview.chromium.org/5857002/diff/22002/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/5857002/diff/22002/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.h:36: class NativeTextfieldViews
: public views::View,
The reason I'm not too keen on this name is that this class is completely
different from NativeButton. But I see why you're going with it because this is
an implementation of NativeTextfieldWrapper. Did you consider just
TextfieldViews? Or ViewsTextfield?

oshima

On 2010/12/16 17:29:43, sky wrote: > http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/textfield_view_model.h > File views/controls/textfield/textfield_view_model.h (right): > > http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/textfield_view_model.h#newcode150 > ...

10 years ago (2010-12-16 20:57:07 UTC) #11

On 2010/12/16 17:29:43, sky wrote:
>
http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te...
> File views/controls/textfield/textfield_view_model.h (right):
> 
>
http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te...
> views/controls/textfield/textfield_view_model.h:150: size_t begin_, end_;
> On 2010/12/16 01:15:19, oshima wrote:
> > On 2010/12/15 20:31:27, sky wrote:
> > > Could you combine cursor_pos_ and begin_ end_ ? Is the cursor_pos always
one
> > of
> > > begin_ or end_?
> > 
> > Only if something is selected. There are several options:
> > 1) current impl
> >    - pros: canonical. has selection iff begin != end.
> >    - cons: need extra variable
> > 2) have one selection_start_, and move selection_start_ along with cursor if
> > there is no selection
> >    - pros: canonical. has selection iff begin != end.
> >    - cons: needs to update selection_start_ for every regular cursor move.
> 
> I don't understand this con. You have to update something when the cursor
moves.
> In (1) it's cursor_pos_, in (2) its selection_begin_. Maybe you mean (2) also
> has to update selection_end_?
> 
> > 3) change selection_start_ to int and use -1 as an indication of not
selected.
> >    - pros: no need for extra variable
> >    - cons: this has different type than other indexes.
> >            not canonical. need to check cursor != end.
> > 4) have boolean "has_selection_".
> >    - pros: more explicit
> >    - cons: not canonical. still to check if curosr == end.
> > I ended up 1), but if you don't like 1), 2) or 4) is fine. I think 3) is
worst
> > of 4.
> 
> I prefer (2). If you go with (1) it is easier to accidentally get things out
of
> sync.

It has to update selection_begin_ even if it's not selecting which isn't
intuitive,
and i think it has its own chance to become out of sync. Anyway, I'm fine with
2)
as well and updated the CL so PTAL.


> 
> > 
> >  I would also suggest renaming to selection_begin_,
> > > selection_end_. Also, make it clear that begin_ may be > end_
> > 
> > done.
> >
> 
>
http://codereview.chromium.org/5857002/diff/22002/views/controls/textfield/na...
> File views/controls/textfield/native_textfield_views.h (right):
> 
>
http://codereview.chromium.org/5857002/diff/22002/views/controls/textfield/na...
> views/controls/textfield/native_textfield_views.h:36: class
NativeTextfieldViews
> : public views::View,
> The reason I'm not too keen on this name is that this class is completely
> different from NativeButton.

Yes, I understand 100% and i think the root issue is this NativeXXX naming.
Please see my proposal at the bottom.

> But I see why you're going with it because this is
> an implementation of NativeTextfieldWrapper. Did you consider just
> TextfieldViews? Or ViewsTextfield?

I did consider TextfieldViews, but this is confusing for a user because
this is just an implementation behind Textfield and not something a user should
use.
ViewsTextfield is probably not an option because it's not consistent with the
suffix pattern
that other classes are using. I think the consistency is more important for
readability and
discover-ability than ugly-ness.

Being said, I think we can improve naming. I think "Native" isn't really
important here so
how about the following?

NativeTextfieldWrapper -> TextfieldWrapper
NativeTextfieldWin/Gtk/Views ->
TextfieldWrapperWin/TextfieldWrapperGtk/TextfieldWrapperViews

This is basically PIMPL pattern, so alternative option is TextfieldImpl but
looks like views
isn't using "Impl", so it's probably better to stick to Wrapper. What do you
think?

- oshima

oshima

On 2010/12/16 20:57:07, oshima wrote: > On 2010/12/16 17:29:43, sky wrote: > > > http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/textfield_view_model.h ...

10 years ago (2010-12-16 21:17:34 UTC) #12

On 2010/12/16 20:57:07, oshima wrote:
> On 2010/12/16 17:29:43, sky wrote:
> >
>
http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te...
> > File views/controls/textfield/textfield_view_model.h (right):
> > 
> >
>
http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te...
> > views/controls/textfield/textfield_view_model.h:150: size_t begin_, end_;
> > On 2010/12/16 01:15:19, oshima wrote:
> > > On 2010/12/15 20:31:27, sky wrote:
> > > > Could you combine cursor_pos_ and begin_ end_ ? Is the cursor_pos always
> one
> > > of
> > > > begin_ or end_?
> > > 
> > > Only if something is selected. There are several options:
> > > 1) current impl
> > >    - pros: canonical. has selection iff begin != end.
> > >    - cons: need extra variable
> > > 2) have one selection_start_, and move selection_start_ along with cursor
if
> > > there is no selection
> > >    - pros: canonical. has selection iff begin != end.
> > >    - cons: needs to update selection_start_ for every regular cursor move.
> > 
> > I don't understand this con. You have to update something when the cursor
> moves.
> > In (1) it's cursor_pos_, in (2) its selection_begin_. Maybe you mean (2)
also
> > has to update selection_end_?
> > 
> > > 3) change selection_start_ to int and use -1 as an indication of not
> selected.
> > >    - pros: no need for extra variable
> > >    - cons: this has different type than other indexes.
> > >            not canonical. need to check cursor != end.
> > > 4) have boolean "has_selection_".
> > >    - pros: more explicit
> > >    - cons: not canonical. still to check if curosr == end.
> > > I ended up 1), but if you don't like 1), 2) or 4) is fine. I think 3) is
> worst
> > > of 4.
> > 
> > I prefer (2). If you go with (1) it is easier to accidentally get things out
> of
> > sync.
> 
> It has to update selection_begin_ even if it's not selecting which isn't
> intuitive,
> and i think it has its own chance to become out of sync. Anyway, I'm fine with
> 2)
> as well and updated the CL so PTAL.
> 
> 
> > 
> > > 
> > >  I would also suggest renaming to selection_begin_,
> > > > selection_end_. Also, make it clear that begin_ may be > end_
> > > 
> > > done.
> > >
> > 
> >
>
http://codereview.chromium.org/5857002/diff/22002/views/controls/textfield/na...
> > File views/controls/textfield/native_textfield_views.h (right):
> > 
> >
>
http://codereview.chromium.org/5857002/diff/22002/views/controls/textfield/na...
> > views/controls/textfield/native_textfield_views.h:36: class
> NativeTextfieldViews
> > : public views::View,
> > The reason I'm not too keen on this name is that this class is completely
> > different from NativeButton.
> 
> Yes, I understand 100% and i think the root issue is this NativeXXX naming.
> Please see my proposal at the bottom.
> 
> > But I see why you're going with it because this is
> > an implementation of NativeTextfieldWrapper. Did you consider just
> > TextfieldViews? Or ViewsTextfield?
> 
> I did consider TextfieldViews, but this is confusing for a user because
> this is just an implementation behind Textfield and not something a user
should
> use.
> ViewsTextfield is probably not an option because it's not consistent with the
> suffix pattern
> that other classes are using. I think the consistency is more important for
> readability and
> discover-ability than ugly-ness.
> 
> Being said, I think we can improve naming. I think "Native" isn't really
> important here so
> how about the following?
> 
> NativeTextfieldWrapper -> TextfieldWrapper
> NativeTextfieldWin/Gtk/Views ->
> TextfieldWrapperWin/TextfieldWrapperGtk/TextfieldWrapperViews
> 
> This is basically PIMPL pattern, so alternative option is TextfieldImpl but
> looks like views
> isn't using "Impl", so it's probably better to stick to Wrapper. What do you
> think?

Hmm, other controls are using native_ pattern, so this may not be good idea. Or
as ben's comment implies in native_menu_gtk.cc/win.cc, we shouldn't have used
"Native" for these class?

> 
> - oshima

sky

On Thu, Dec 16, 2010 at 12:57 PM, <oshima@chromium.org> wrote: > On 2010/12/16 17:29:43, sky ...

10 years ago (2010-12-16 22:09:31 UTC) #13

On Thu, Dec 16, 2010 at 12:57 PM,  <oshima@chromium.org> wrote:
> On 2010/12/16 17:29:43, sky wrote:
>
>
http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te...
>>
>> File views/controls/textfield/textfield_view_model.h (right):
>
>
>
http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te...
>>
>> views/controls/textfield/textfield_view_model.h:150: size_t begin_, end_;
>> On 2010/12/16 01:15:19, oshima wrote:
>> > On 2010/12/15 20:31:27, sky wrote:
>> > > Could you combine cursor_pos_ and begin_ end_ ? Is the cursor_pos
>> > > always
>
> one
>>
>> > of
>> > > begin_ or end_?
>> >
>> > Only if something is selected. There are several options:
>> > 1) current impl
>> >    - pros: canonical. has selection iff begin != end.
>> >    - cons: need extra variable
>> > 2) have one selection_start_, and move selection_start_ along with
>> > cursor if
>> > there is no selection
>> >    - pros: canonical. has selection iff begin != end.
>> >    - cons: needs to update selection_start_ for every regular cursor
>> > move.
>
>> I don't understand this con. You have to update something when the cursor
>
> moves.
>>
>> In (1) it's cursor_pos_, in (2) its selection_begin_. Maybe you mean (2)
>> also
>> has to update selection_end_?
>
>> > 3) change selection_start_ to int and use -1 as an indication of not
>
> selected.
>>
>> >    - pros: no need for extra variable
>> >    - cons: this has different type than other indexes.
>> >            not canonical. need to check cursor != end.
>> > 4) have boolean "has_selection_".
>> >    - pros: more explicit
>> >    - cons: not canonical. still to check if curosr == end.
>> > I ended up 1), but if you don't like 1), 2) or 4) is fine. I think 3) is
>
> worst
>>
>> > of 4.
>
>> I prefer (2). If you go with (1) it is easier to accidentally get things
>> out
>
> of
>>
>> sync.
>
> It has to update selection_begin_ even if it's not selecting which isn't
> intuitive,
> and i think it has its own chance to become out of sync. Anyway, I'm fine
> with
> 2)
> as well and updated the CL so PTAL.
>
>
>
>> >
>> >  I would also suggest renaming to selection_begin_,
>> > > selection_end_. Also, make it clear that begin_ may be > end_
>> >
>> > done.
>> >
>
>
>
http://codereview.chromium.org/5857002/diff/22002/views/controls/textfield/na...
>>
>> File views/controls/textfield/native_textfield_views.h (right):
>
>
>
http://codereview.chromium.org/5857002/diff/22002/views/controls/textfield/na...
>>
>> views/controls/textfield/native_textfield_views.h:36: class
>
> NativeTextfieldViews
>>
>> : public views::View,
>> The reason I'm not too keen on this name is that this class is completely
>> different from NativeButton.
>
> Yes, I understand 100% and i think the root issue is this NativeXXX naming.
> Please see my proposal at the bottom.
>
>> But I see why you're going with it because this is
>> an implementation of NativeTextfieldWrapper. Did you consider just
>> TextfieldViews? Or ViewsTextfield?
>
> I did consider TextfieldViews, but this is confusing for a user because
> this is just an implementation behind Textfield and not something a user
> should
> use.
> ViewsTextfield is probably not an option because it's not consistent with
> the
> suffix pattern
> that other classes are using. I think the consistency is more important for
> readability and
> discover-ability than ugly-ness.
>
> Being said, I think we can improve naming. I think "Native" isn't really
> important here so
> how about the following?
>
> NativeTextfieldWrapper -> TextfieldWrapper
> NativeTextfieldWin/Gtk/Views ->
> TextfieldWrapperWin/TextfieldWrapperGtk/TextfieldWrapperViews

I like this.

> This is basically PIMPL pattern, so alternative option is TextfieldImpl but
> looks like views
> isn't using "Impl", so it's probably better to stick to Wrapper. What do you
> think?

Ben has been talking about a way to structure code so that
implementation specific classes like this exist in a separate place.
Or more correctly we have headers for the 'public api' in one place,
and the non-public parts some where else. You can see this with Skia
where there is an include directory and source directory. The source
directory has some headers that aren't intended for public
consumption.

Ben is most likely on vacation now, so we can't really get his input.
Leave the names you have and we can revisit this when he is back.

  -Scott

oshima

On Thu, Dec 16, 2010 at 2:09 PM, Scott Violet <sky@chromium.org> wrote: > On Thu, ...

10 years ago (2010-12-16 22:24:40 UTC) #14

On Thu, Dec 16, 2010 at 2:09 PM, Scott Violet <sky@chromium.org> wrote:

> On Thu, Dec 16, 2010 at 12:57 PM,  <oshima@chromium.org> wrote:
> > On 2010/12/16 17:29:43, sky wrote:
> >
> >
>
http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te...
> >>
> >> File views/controls/textfield/textfield_view_model.h (right):
> >
> >
> >
>
http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te...
> >>
> >> views/controls/textfield/textfield_view_model.h:150: size_t begin_,
> end_;
> >> On 2010/12/16 01:15:19, oshima wrote:
> >> > On 2010/12/15 20:31:27, sky wrote:
> >> > > Could you combine cursor_pos_ and begin_ end_ ? Is the cursor_pos
> >> > > always
> >
> > one
> >>
> >> > of
> >> > > begin_ or end_?
> >> >
> >> > Only if something is selected. There are several options:
> >> > 1) current impl
> >> >    - pros: canonical. has selection iff begin != end.
> >> >    - cons: need extra variable
> >> > 2) have one selection_start_, and move selection_start_ along with
> >> > cursor if
> >> > there is no selection
> >> >    - pros: canonical. has selection iff begin != end.
> >> >    - cons: needs to update selection_start_ for every regular cursor
> >> > move.
> >
> >> I don't understand this con. You have to update something when the
> cursor
> >
> > moves.
> >>
> >> In (1) it's cursor_pos_, in (2) its selection_begin_. Maybe you mean (2)
> >> also
> >> has to update selection_end_?
> >
> >> > 3) change selection_start_ to int and use -1 as an indication of not
> >
> > selected.
> >>
> >> >    - pros: no need for extra variable
> >> >    - cons: this has different type than other indexes.
> >> >            not canonical. need to check cursor != end.
> >> > 4) have boolean "has_selection_".
> >> >    - pros: more explicit
> >> >    - cons: not canonical. still to check if curosr == end.
> >> > I ended up 1), but if you don't like 1), 2) or 4) is fine. I think 3)
> is
> >
> > worst
> >>
> >> > of 4.
> >
> >> I prefer (2). If you go with (1) it is easier to accidentally get things
> >> out
> >
> > of
> >>
> >> sync.
> >
> > It has to update selection_begin_ even if it's not selecting which isn't
> > intuitive,
> > and i think it has its own chance to become out of sync. Anyway, I'm fine
> > with
> > 2)
> > as well and updated the CL so PTAL.
> >
> >
> >
> >> >
> >> >  I would also suggest renaming to selection_begin_,
> >> > > selection_end_. Also, make it clear that begin_ may be > end_
> >> >
> >> > done.
> >> >
> >
> >
> >
>
http://codereview.chromium.org/5857002/diff/22002/views/controls/textfield/na...
> >>
> >> File views/controls/textfield/native_textfield_views.h (right):
> >
> >
> >
>
http://codereview.chromium.org/5857002/diff/22002/views/controls/textfield/na...
> >>
> >> views/controls/textfield/native_textfield_views.h:36: class
> >
> > NativeTextfieldViews
> >>
> >> : public views::View,
> >> The reason I'm not too keen on this name is that this class is
> completely
> >> different from NativeButton.
> >
> > Yes, I understand 100% and i think the root issue is this NativeXXX
> naming.
> > Please see my proposal at the bottom.
> >
> >> But I see why you're going with it because this is
> >> an implementation of NativeTextfieldWrapper. Did you consider just
> >> TextfieldViews? Or ViewsTextfield?
> >
> > I did consider TextfieldViews, but this is confusing for a user because
> > this is just an implementation behind Textfield and not something a user
> > should
> > use.
> > ViewsTextfield is probably not an option because it's not consistent with
> > the
> > suffix pattern
> > that other classes are using. I think the consistency is more important
> for
> > readability and
> > discover-ability than ugly-ness.
> >
> > Being said, I think we can improve naming. I think "Native" isn't really
> > important here so
> > how about the following?
> >
> > NativeTextfieldWrapper -> TextfieldWrapper
> > NativeTextfieldWin/Gtk/Views ->
> > TextfieldWrapperWin/TextfieldWrapperGtk/TextfieldWrapperViews
>
> I like this.
>
> > This is basically PIMPL pattern, so alternative option is TextfieldImpl
> but
> > looks like views
> > isn't using "Impl", so it's probably better to stick to Wrapper. What do
> you
> > think?
>
> Ben has been talking about a way to structure code so that
> implementation specific classes like this exist in a separate place.
> Or more correctly we have headers for the 'public api' in one place,
> and the non-public parts some where else. You can see this with Skia
> where there is an include directory and source directory. The source
> directory has some headers that aren't intended for public
> consumption.
>
> Ben is most likely on vacation now, so we can't really get his input.
> Leave the names you have and we can revisit this when he is back.
>

Got it. Please let me know if there is any issue/concern I should address.

- oshima


>
>  -Scott
>

sky

Have you considered adding a delegate to the model that is notified when the contents ...

10 years ago (2010-12-16 22:43:04 UTC) #15

oshima

http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/native_textfield_views.cc File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/native_textfield_views.cc#newcode49 views/controls/textfield/native_textfield_views.cc:49: const char NativeTextfieldViews::kViewClassName[] = "views/NativeTextfieldViews"; On 2010/12/16 22:43:04, sky ...

10 years ago (2010-12-16 23:50:02 UTC) #16

oshima

On 2010/12/16 22:43:04, sky wrote: > Have you considered adding a delegate to the model ...

10 years ago (2010-12-16 23:57:29 UTC) #18

rjkroege

LGTM with nit and worry. http://codereview.chromium.org/5857002/diff/12002/chrome/browser/ui/views/dropdown_bar_host_gtk.cc File chrome/browser/ui/views/dropdown_bar_host_gtk.cc (right): http://codereview.chromium.org/5857002/diff/12002/chrome/browser/ui/views/dropdown_bar_host_gtk.cc#newcode36 chrome/browser/ui/views/dropdown_bar_host_gtk.cc:36: // RenderWidgetHostViewVeiwe::OnKeyPressed(). RenderWidgetHostViewViews http://codereview.chromium.org/5857002/diff/12002/views/widget/root_view.cc ...

10 years ago (2010-12-17 02:01:21 UTC) #19

oshima

10 years ago (2010-12-17 07:48:41 UTC) #20

http://codereview.chromium.org/5857002/diff/12002/chrome/browser/ui/views/dro...
File chrome/browser/ui/views/dropdown_bar_host_gtk.cc (right):

http://codereview.chromium.org/5857002/diff/12002/chrome/browser/ui/views/dro...
chrome/browser/ui/views/dropdown_bar_host_gtk.cc:36: //
RenderWidgetHostViewVeiwe::OnKeyPressed().
On 2010/12/17 02:01:22, rjkroege wrote:
> RenderWidgetHostViewViews

Done.

http://codereview.chromium.org/5857002/diff/12002/views/widget/root_view.cc
File views/widget/root_view.cc (right):

http://codereview.chromium.org/5857002/diff/12002/views/widget/root_view.cc#n...
views/widget/root_view.cc:622: #if defined(OS_LINUX)
On 2010/12/17 02:01:22, rjkroege wrote:
> are you sure you don't want to make this TOUCH_UI?

Yes. This allow me to text it with linux_views(or chromeos) build as well as
touch. And this won't affect chromeos unless textfieldviews is enabled.

Expand Messages | Collapse Messages