Chromium Code Reviews
|
|
Chromium Code Reviews|
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. |
Descriptionno 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
Messages
Total messages: 20 (0 generated)
http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... File views/controls/textfield/textfield.h (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield.h:46: class Keystroke { I wonder if we need this now. Is it possible to replace this with KeyEvent? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield.h:65: } This seems to be safe as Keystroke is used on stack and the lifetime of keystroke is shorter than keyevent. Please let me know if I should copy the event.
http://codereview.chromium.org/5857002/diff/15001/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/5857002/diff/15001/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:1282: const char kEnableTextfieldView[] = "enable-textfield-view"; nit: align '=' with '=' on 1278 http://codereview.chromium.org/5857002/diff/15001/chrome/common/chrome_switch... File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/5857002/diff/15001/chrome/common/chrome_switch... chrome/common/chrome_switches.h:367: extern const char kEnableTextfieldView[]; This name isn't very descriptive. How about kEnableViewsBasedTextfield or kNonNativeTextfield? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... File views/controls/textfield/native_textfield_view.cc (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:69: return false; This is the default return value. Did you mean to override this? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:75: if (model_->MoveCursorTo(pos, false)) { What about a double click? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:316: if (bounds().IsEmpty()) return; nit: don't do single ifs like this. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:323: gfx::Font font = GetFont(); const gfx::Font& http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:336: text_offset_ = width - cursor_bounds_.right(); Wouldn't this trigger scrolling on every cursor change? Shouldn't you try to minimize scrolling? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:374: string16 text = model_->GetVisibleText((*iter).begin, (*iter).end); iter->begin, iter->end http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:379: if ((*iter).selected) { iter->selected http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:628: } else { What about == ? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... File views/controls/textfield/native_textfield_view.h (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.h:34: class NativeTextfieldView : public views::View, This name is confusing given the class description says non-native. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.h:87: class TextfieldBorder : public Border { nit: put this in the .cc? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.h:111: gfx::Font GetFont() const; const gfx::Font& http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.h:156: // The drawing state of cursor. True to draw. How about naming this is_cursor_visible_? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... File views/controls/textfield/native_textfield_view_unittest.cc (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view_unittest.cc:17: #define EXPECT_STR_EQ(ascii, utf16) EXPECT_EQ(ASCIIToUTF16(ascii), utf16) If this fails the output isn't very helpful (meaning you just see a bunch of code points). If you convert both to wide you'll get better output on failure. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... File views/controls/textfield/textfield_view_model.cc (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.cc:60: text_.insert(cursor_pos_, 1, c); Meta question. Isn't it possible for a visual character (glyph) to be comprised of more than one char16? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.cc:122: // middle of string and adnvace backwards. adnvace -> advance http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.cc:219: std::abs(static_cast<long>(end_ - begin_))); static_cast<size_t> http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.cc:224: end_ = text_.length(); Select(0, text_.length()) http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.cc:251: size_t n = std::abs(static_cast<long>(end_ - begin_)); static_cast<size_t> 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:21: class TextfieldViewModel { Description? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:24: virtual ~TextfieldViewModel(); Why virtual? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:28: // in the future to supportmulti-color text support multi http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:42: void PopulateFragments(TextFragments* elements) const; How about GetFragments. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:76: int cursor_pos() const { return cursor_pos_; } size_t http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:113: string16 GetSelectedText() const; Does this intentionally not return the password character? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:131: // end is the new corsor position. corsor->cursor http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:150: size_t begin_, end_; Could you combine cursor_pos_ and begin_ end_ ? Is the cursor_pos always one of begin_ or end_? I would also suggest renaming to selection_begin_, selection_end_. Also, make it clear that begin_ may be > end_
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... chrome/browser/browser_main.cc:901: if (parameters.command_line_.HasSwitch(switches::kEnableTextfieldView)) This check should be done inside CreateWrapper, rather than here. If we add every such experiment to browser_main it'll become unreadable.
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... chrome/browser/browser_main.cc:158: #if defined(OS_LINUX) Why OS_LINUX? We have been TOUCH_UI for the touch changes? This change ought to work under win too? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... File views/controls/textfield/native_textfield_view.cc (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:495: char16 NativeTextfieldView::GetPrintableChar(app::KeyboardCode key_code, Could this merge with keyboard_code_conversion_x.cc somehow? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:622: while (std::abs(static_cast<long>(right_pos - left_pos) > 1)) { AFAIK the binary search approach won't necessarily work for languages needing shaping such as Arabic. You are including this issue in your TODO? http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... File views/controls/textfield/textfield.h (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield.h:66: const GdkEventKey* event() const { return event_->native_event(); } I think we should exclude this in a touchui=1 context yes? http://codereview.chromium.org/5857002/diff/15001/views/event.h File views/event.h (right): http://codereview.chromium.org/5857002/diff/15001/views/event.h#newcode334 views/event.h:334: #if defined(OS_LINUX) and !touchui http://codereview.chromium.org/5857002/diff/15001/views/event.h#newcode370 views/event.h:370: #if defined(OS_LINUX) and !touchui http://codereview.chromium.org/5857002/diff/15001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/5857002/diff/15001/views/widget/root_view.cc#n... views/widget/root_view.cc:622: #if defined(OS_LINUX) probably should stay TOUCH_UI? IMO, TOUCH_UI can imply the views text field always.
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... chrome/browser/browser_main.cc:158: #if defined(OS_LINUX) On 2010/12/15 21:23:31, rjkroege wrote: > Why OS_LINUX? We have been TOUCH_UI for the touch changes? This change ought to > work under win too? This currently does not compile on Windows due to the difference of keystroke class. I'm going to enable this in the follow up CL. http://codereview.chromium.org/5857002/diff/15001/chrome/browser/browser_main... chrome/browser/browser_main.cc:901: if (parameters.command_line_.HasSwitch(switches::kEnableTextfieldView)) On 2010/12/15 20:42:00, beng wrote: > This check should be done inside CreateWrapper, rather than here. If we add > every such experiment to browser_main it'll become unreadable. I originally had it in CreateWrapper, but I moved here to put the switch in chrome/common/chrome_switch.h. I put the switch back to native_textfield_view.cc. Let me know if it's okay, or if you have alternative suggestion. http://codereview.chromium.org/5857002/diff/15001/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/5857002/diff/15001/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:1282: const char kEnableTextfieldView[] = "enable-textfield-view"; On 2010/12/15 20:31:27, sky wrote: > nit: align '=' with '=' on 1278 this has been moved and fixed. http://codereview.chromium.org/5857002/diff/15001/chrome/common/chrome_switch... File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/5857002/diff/15001/chrome/common/chrome_switch... chrome/common/chrome_switches.h:367: extern const char kEnableTextfieldView[]; On 2010/12/15 20:31:27, sky wrote: > This name isn't very descriptive. How about kEnableViewsBasedTextfield or > kNonNativeTextfield? Moved to native_textfield_view.cc http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... File views/controls/textfield/native_textfield_view.cc (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:69: return false; On 2010/12/15 20:31:27, sky wrote: > This is the default return value. Did you mean to override this? No, I was going to remove this, but forgot to do so. This was overridden in the original CL and was testing if this works fine. Removed. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:75: if (model_->MoveCursorTo(pos, false)) { On 2010/12/15 20:31:27, sky wrote: > What about a double click? Not yet supported. I'll implement it in follow up CL. Added to the unsupported feature list. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:316: if (bounds().IsEmpty()) return; On 2010/12/15 20:31:27, sky wrote: > nit: don't do single ifs like this. Done. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:323: gfx::Font font = GetFont(); On 2010/12/15 20:31:27, sky wrote: > const gfx::Font& Done. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:336: text_offset_ = width - cursor_bounds_.right(); On 2010/12/15 20:31:27, sky wrote: > Wouldn't this trigger scrolling on every cursor change? Shouldn't you try to > minimize scrolling? This is executed only if the cursor moves out of the bounds. If the cursor stays in the bounds, it won't scroll. (in else below) http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:374: string16 text = model_->GetVisibleText((*iter).begin, (*iter).end); On 2010/12/15 20:31:27, sky wrote: > iter->begin, iter->end It does not work. * is an operator that returns TextFragment type. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:379: if ((*iter).selected) { On 2010/12/15 20:31:27, sky wrote: > iter->selected same here. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:495: char16 NativeTextfieldView::GetPrintableChar(app::KeyboardCode key_code, On 2010/12/15 21:23:31, rjkroege wrote: > Could this merge with keyboard_code_conversion_x.cc somehow? That's xevent -> keyboard code right? what we need is keyboardcode -> utf16 character and should be cross platform code. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:622: while (std::abs(static_cast<long>(right_pos - left_pos) > 1)) { On 2010/12/15 21:23:31, rjkroege wrote: > AFAIK the binary search approach won't necessarily work for languages needing > shaping such as Arabic. You are including this issue in your TODO? Thank you for the info. Updated the comment above. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.cc:628: } else { On 2010/12/15 20:31:27, sky wrote: > What about == ? Added shortcut for == case. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... File views/controls/textfield/native_textfield_view.h (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.h:34: class NativeTextfieldView : public views::View, On 2010/12/15 20:31:27, sky wrote: > This name is confusing given the class description says non-native. I've been going back and forth, and this name sounded better. I updated the description instead. PTAL and let me know what you think. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.h:111: gfx::Font GetFont() const; On 2010/12/15 20:31:27, sky wrote: > const gfx::Font& Done. It was cut&paste from Textfield. Updated Textfield as well. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view.h:156: // The drawing state of cursor. True to draw. On 2010/12/15 20:31:27, sky wrote: > How about naming this is_cursor_visible_? Done. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... File views/controls/textfield/native_textfield_view_unittest.cc (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/na... views/controls/textfield/native_textfield_view_unittest.cc:17: #define EXPECT_STR_EQ(ascii, utf16) EXPECT_EQ(ASCIIToUTF16(ascii), utf16) On 2010/12/15 20:31:27, sky wrote: > If this fails the output isn't very helpful (meaning you just see a bunch of > code points). If you convert both to wide you'll get better output on failure. Done. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... File views/controls/textfield/textfield.h (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield.h:66: const GdkEventKey* event() const { return event_->native_event(); } On 2010/12/15 21:23:31, rjkroege wrote: > I think we should exclude this in a touchui=1 context yes? we still need this to compile. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... File views/controls/textfield/textfield_view_model.cc (right): http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.cc:60: text_.insert(cursor_pos_, 1, c); On 2010/12/15 20:31:27, sky wrote: > Meta question. Isn't it possible for a visual character (glyph) to be comprised > of more than one char16? Yes, you're correct. This only works in UTF16/UCS-2. Full i18n will be handled later. I updated the comments. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.cc:122: // middle of string and adnvace backwards. On 2010/12/15 20:31:27, sky wrote: > adnvace -> advance Done. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.cc:219: std::abs(static_cast<long>(end_ - begin_))); On 2010/12/15 20:31:27, sky wrote: > static_cast<size_t> size_t is unsigned. using long to make it signed for abs to work. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.cc:224: end_ = text_.length(); On 2010/12/15 20:31:27, sky wrote: > Select(0, text_.length()) Done. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.cc:251: size_t n = std::abs(static_cast<long>(end_ - begin_)); On 2010/12/15 20:31:27, sky wrote: > static_cast<size_t> same here. 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:21: class TextfieldViewModel { On 2010/12/15 20:31:27, sky wrote: > Description? Done. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:24: virtual ~TextfieldViewModel(); On 2010/12/15 20:31:27, sky wrote: > Why virtual? it's just my habit to always use virtual destructor. I understand it's not necessary, but i had better experience in the past. I can remove if you don't like. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:28: // in the future to supportmulti-color text On 2010/12/15 20:31:27, sky wrote: > support multi Done. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:42: void PopulateFragments(TextFragments* elements) const; On 2010/12/15 20:31:27, sky wrote: > How about GetFragments. Done. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:76: int cursor_pos() const { return cursor_pos_; } On 2010/12/15 20:31:27, sky wrote: > size_t good catch. Done http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:113: string16 GetSelectedText() const; On 2010/12/15 20:31:27, sky wrote: > Does this intentionally not return the password character? I believe this is now gtk version works and expected. Cut/Copy will take care of password visibility. http://codereview.chromium.org/5857002/diff/15001/views/controls/textfield/te... views/controls/textfield/textfield_view_model.h:131: // end is the new corsor position. On 2010/12/15 20:31:27, sky wrote: > corsor->cursor Done. 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/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. 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 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/15001/views/event.h File views/event.h (right): http://codereview.chromium.org/5857002/diff/15001/views/event.h#newcode334 views/event.h:334: #if defined(OS_LINUX) On 2010/12/15 21:23:31, rjkroege wrote: > and !touchui touch still need this to compile http://codereview.chromium.org/5857002/diff/15001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/5857002/diff/15001/views/widget/root_view.cc#n... views/widget/root_view.cc:622: #if defined(OS_LINUX) On 2010/12/15 21:23:31, rjkroege wrote: > probably should stay TOUCH_UI? IMO, TOUCH_UI can imply the views text field > always. This is necessary for NativeTextfieldView to work on linux/views build. IsTextfieldViewEnabled always returns true for touch now, so that case should be covered.
Hello, Seems like I was missing something. Other views only classes are using prefix _views for file and Views for class name. My bad. So i think I should rename classes and files to use views instead of views (NativeTextfieldViews and TextfieldViewsModel respectively). Does this make sense to you guys? want to make sure everyone agrees before doing massive class/file name change.
On Wed, Dec 15, 2010 at 6:00 PM, <oshima@chromium.org> wrote: > Hello, > > Seems like I was missing something. Other views only classes are using > prefix > _views for file and Views for class name. My bad. So i think I should > rename > classes and files to use views instead of views (NativeTextfieldViews and > TextfieldViewsModel respectively). > > Does this make sense to you guys? want to make sure everyone agrees before > doing massive class/file name change. I chatted with Robert and we agreed that Views prefix is right. I'm going to update class/file names and will let you know when uploaded. > > > http://codereview.chromium.org/5857002/ >
Drive-by with a test comment. http://codereview.chromium.org/5857002/diff/35001/views/controls/textfield/na... File views/controls/textfield/native_textfield_views_unittest.cc (right): http://codereview.chromium.org/5857002/diff/35001/views/controls/textfield/na... views/controls/textfield/native_textfield_views_unittest.cc:58: DCHECK(!textfield_); Could you convert those DCHECKs to ASSERT_TRUEs? The former crashes entire test binary and leads to disabling tests in case of failures.
On Thu, Dec 16, 2010 at 1:02 AM, <phajdan.jr@chromium.org> wrote: > Drive-by with a test comment. > > > > http://codereview.chromium.org/5857002/diff/35001/views/controls/textfield/na... > File views/controls/textfield/native_textfield_views_unittest.cc > (right): > > > http://codereview.chromium.org/5857002/diff/35001/views/controls/textfield/na... > views/controls/textfield/native_textfield_views_unittest.cc:58: > DCHECK(!textfield_); > Could you convert those DCHECKs to ASSERT_TRUEs? > Done > > The former crashes entire test binary and leads to disabling tests in > case of failures. > > > http://codereview.chromium.org/5857002/ >
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?
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
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
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
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 >
Have you considered adding a delegate to the model that is notified when the contents change? Seems like doing that would avoid having to remember to invoke update bounds/controller/paint. http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:49: const char NativeTextfieldViews::kViewClassName[] = "views/NativeTextfieldViews"; nit: > 80 http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:104: handled = handled || HandleKeyEvent(e); No point in assigning and just returning. Make 104 be: return handled || HandleKeyEvent(e); http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:127: MessageLoop::current()->PostDelayedTask( Any reason you're not using a RepeatingTimer? http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:130: 800); make this is a constant. http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:159: Textfield::Controller* controller = textfield_->GetController(); Are you sure we want to notify the controller here? It doesn't seem like we do on gtk/windows, but I didn't try with an actual debugger to verify that so that I could be wrong. http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:601: gfx::Font font = GetFont(); Doesn't this need to take into consideration text_offset_? http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:651: : insets_(4, 4, 4, 4) { Initialize has_focus_ to false http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:89: } else if(cursor_pos_ > 0) { nit: space between if and ( http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:192: } Do you need an else case if !select that clears selection?
http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... File views/controls/textfield/native_textfield_views.cc (right): http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:49: const char NativeTextfieldViews::kViewClassName[] = "views/NativeTextfieldViews"; On 2010/12/16 22:43:04, sky wrote: > nit: > 80 Done. http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:104: handled = handled || HandleKeyEvent(e); On 2010/12/16 22:43:04, sky wrote: > No point in assigning and just returning. Make 104 be: > > return handled || HandleKeyEvent(e); Done. http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:127: MessageLoop::current()->PostDelayedTask( On 2010/12/16 22:43:04, sky wrote: > Any reason you're not using a RepeatingTimer? because its delay is alternativing 800 and 500 ms (just picked from looking at how gtk handles cursor). Can repeating timer handle it? http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:130: 800); On 2010/12/16 22:43:04, sky wrote: > make this is a constant. Overlooked. Done. http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:159: Textfield::Controller* controller = textfield_->GetController(); On 2010/12/16 22:43:04, sky wrote: > Are you sure we want to notify the controller here? It doesn't seem like we do > on gtk/windows, but I didn't try with an actual debugger to verify that so that > I could be wrong. gtk is using "changed" signal, which is called when content has been changed. I'm not sure if this is consistent with Win's behavior though. http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:601: gfx::Font font = GetFont(); On 2010/12/16 22:43:04, sky wrote: > Doesn't this need to take into consideration text_offset_? good catch. Thanks. done. http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... views/controls/textfield/native_textfield_views.cc:651: : insets_(4, 4, 4, 4) { On 2010/12/16 22:43:04, sky wrote: > Initialize has_focus_ to false Done. http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/te... File views/controls/textfield/textfield_views_model.cc (right): http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:89: } else if(cursor_pos_ > 0) { On 2010/12/16 22:43:04, sky wrote: > nit: space between if and ( Done. http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/te... views/controls/textfield/textfield_views_model.cc:192: } On 2010/12/16 22:43:04, sky wrote: > Do you need an else case if !select that clears selection? Done.
LGTM
On 2010/12/16 22:43:04, sky wrote: > Have you considered adding a delegate to the model that is notified when the > contents change? Seems like doing that would avoid having to remember to invoke > update bounds/controller/paint. JFTR: here is what I chatted with scott offline. Not for TextfieldView, but I was thinking notification mechanism for AutocompleteEditView. I'll incorporate Scott's suggestion when I add notification mechanism. > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... > File views/controls/textfield/native_textfield_views.cc (right): > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... > views/controls/textfield/native_textfield_views.cc:49: const char > NativeTextfieldViews::kViewClassName[] = "views/NativeTextfieldViews"; > nit: > 80 > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... > views/controls/textfield/native_textfield_views.cc:104: handled = handled || > HandleKeyEvent(e); > No point in assigning and just returning. Make 104 be: > > return handled || HandleKeyEvent(e); > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... > views/controls/textfield/native_textfield_views.cc:127: > MessageLoop::current()->PostDelayedTask( > Any reason you're not using a RepeatingTimer? > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... > views/controls/textfield/native_textfield_views.cc:130: 800); > make this is a constant. > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... > views/controls/textfield/native_textfield_views.cc:159: Textfield::Controller* > controller = textfield_->GetController(); > Are you sure we want to notify the controller here? It doesn't seem like we do > on gtk/windows, but I didn't try with an actual debugger to verify that so that > I could be wrong. > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... > views/controls/textfield/native_textfield_views.cc:601: gfx::Font font = > GetFont(); > Doesn't this need to take into consideration text_offset_? > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/na... > views/controls/textfield/native_textfield_views.cc:651: : insets_(4, 4, 4, 4) { > Initialize has_focus_ to false > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/te... > File views/controls/textfield/textfield_views_model.cc (right): > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/te... > views/controls/textfield/textfield_views_model.cc:89: } else if(cursor_pos_ > 0) > { > nit: space between if and ( > > http://codereview.chromium.org/5857002/diff/46001/views/controls/textfield/te... > views/controls/textfield/textfield_views_model.cc:192: } > Do you need an else case if !select that clears selection?
LGTM with nit and worry. 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(). RenderWidgetHostViewViews 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) are you sure you don't want to make this TOUCH_UI?
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. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
