Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3142008:
Model, View and Controller for a Gap Buffer based one line text field in view... (Closed)
|
Created:
10 years, 4 months ago by varunjain Modified:
9 years, 7 months ago CC:
chromium-reviews, ben+cc_chromium.org, Paweł Hajdan Jr. Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionModel, View and Controller for a Gap Buffer based one line text field in views.
BUG=none
TEST=unittests added for new textfield
Patch Set 1 #
Total comments: 10
Patch Set 2 : '' #
Total comments: 19
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 52
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : seperated gap buffer into another CL #Patch Set 8 : added files that were missed in the last patch #
Total comments: 37
Messages
Total messages: 12 (0 generated)
I need some explanation I think. http://codereview.chromium.org/3142008/diff/1/2 File base/gap_buffer.h (right): http://codereview.chromium.org/3142008/diff/1/2#newcode19 base/gap_buffer.h:19: template<class T> why templated? intended application for this will be over a small string. So, you need an "insert T*" method I would think. also, need some way to get metrics... http://codereview.chromium.org/3142008/diff/1/2#newcode25 base/gap_buffer.h:25: GapBuffer(int size = 10) G3 style implies that default parameters are undesirable. probably need a constructor of T[] to fill the buffer at start? http://codereview.chromium.org/3142008/diff/1/2#newcode37 base/gap_buffer.h:37: fprintf(stderr, ">>error creating gap buffer\n"); turn these all into Chrome standard logging-style statement. http://codereview.chromium.org/3142008/diff/1/2#newcode60 base/gap_buffer.h:60: for (int i = 0; i < count && gap_end_ < buffer_size_; i++) { typically, you would want to use memcopy for this sort of thing. http://codereview.chromium.org/3142008/diff/1/2#newcode139 base/gap_buffer.h:139: // FIXME: explore more efficient resizing. use doubling up to 64kB and then grow in 16kB increments? http://codereview.chromium.org/3142008/diff/1/2#newcode158 base/gap_buffer.h:158: scoped_array<T> buffer_; what's T? http://codereview.chromium.org/3142008/diff/1/2#newcode159 base/gap_buffer.h:159: int buffer_size_; comments on these. http://codereview.chromium.org/3142008/diff/1/4 File views/controls/textfield/native_textfield.cc (right): http://codereview.chromium.org/3142008/diff/1/4#newcode5 views/controls/textfield/native_textfield.cc:5: #include "native_textfield.h" so, I'm not sure that this is the right way... http://codereview.chromium.org/3142008/diff/1/5 File views/controls/textfield/native_textfield.h (right): http://codereview.chromium.org/3142008/diff/1/5#newcode51 views/controls/textfield/native_textfield.h:51: scoped_ptr<base::GapBuffer<wchar_t> > buffer; does it need to be a scoped_ptr? http://codereview.chromium.org/3142008/diff/1/5#newcode56 views/controls/textfield/native_textfield.h:56: class NativeTextfieldController { most of this code shouldn't be in an .h?
Hi Rob, updated the changelist. Please have a look again. -V. On 2010/08/12 16:36:49, rjkroege wrote: > I need some explanation I think. > > http://codereview.chromium.org/3142008/diff/1/2 > File base/gap_buffer.h (right): > > http://codereview.chromium.org/3142008/diff/1/2#newcode19 > base/gap_buffer.h:19: template<class T> > why templated? intended application for this will be over a small string. So, > you need an "insert T*" method I would think. > > also, need some way to get metrics... > > http://codereview.chromium.org/3142008/diff/1/2#newcode25 > base/gap_buffer.h:25: GapBuffer(int size = 10) > G3 style implies that default parameters are undesirable. > > probably need a constructor of T[] to fill the buffer at start? > > http://codereview.chromium.org/3142008/diff/1/2#newcode37 > base/gap_buffer.h:37: fprintf(stderr, ">>error creating gap buffer\n"); > turn these all into Chrome standard logging-style statement. > > http://codereview.chromium.org/3142008/diff/1/2#newcode60 > base/gap_buffer.h:60: for (int i = 0; i < count && gap_end_ < buffer_size_; i++) > { > typically, you would want to use memcopy for this sort of thing. > > http://codereview.chromium.org/3142008/diff/1/2#newcode139 > base/gap_buffer.h:139: // FIXME: explore more efficient resizing. > use doubling up to 64kB and then grow in 16kB increments? > > http://codereview.chromium.org/3142008/diff/1/2#newcode158 > base/gap_buffer.h:158: scoped_array<T> buffer_; > what's T? > > http://codereview.chromium.org/3142008/diff/1/2#newcode159 > base/gap_buffer.h:159: int buffer_size_; > comments on these. > > http://codereview.chromium.org/3142008/diff/1/4 > File views/controls/textfield/native_textfield.cc (right): > > http://codereview.chromium.org/3142008/diff/1/4#newcode5 > views/controls/textfield/native_textfield.cc:5: #include "native_textfield.h" > so, I'm not sure that this is the right way... > > http://codereview.chromium.org/3142008/diff/1/5 > File views/controls/textfield/native_textfield.h (right): > > http://codereview.chromium.org/3142008/diff/1/5#newcode51 > views/controls/textfield/native_textfield.h:51: > scoped_ptr<base::GapBuffer<wchar_t> > buffer; > does it need to be a scoped_ptr? > > http://codereview.chromium.org/3142008/diff/1/5#newcode56 > views/controls/textfield/native_textfield.h:56: class NativeTextfieldController > { > most of this code shouldn't be in an .h?
I'd make some minor fixes and try to get this upstreamed. You'll want sky to review and anyone else he recommends. http://codereview.chromium.org/3142008/diff/8001/9002 File base/base.gypi (right): http://codereview.chromium.org/3142008/diff/8001/9002#newcode91 base/base.gypi:91: 'gap_buffer.h', you'll need to exclude this when not using touchui. http://codereview.chromium.org/3142008/diff/8001/9003 File base/gap_buffer.h (right): http://codereview.chromium.org/3142008/diff/8001/9003#newcode9 base/gap_buffer.h:9: #define BASE_GAP_BUFFER_H_ #pragma once ... http://codereview.chromium.org/3142008/diff/8001/9003#newcode64 base/gap_buffer.h:64: for (int i = 0; i < count && gap_start_ > 0; i++) { replace these loops with memmove/memcpy http://codereview.chromium.org/3142008/diff/8001/9003#newcode183 base/gap_buffer.h:183: probably no space http://codereview.chromium.org/3142008/diff/8001/9003#newcode188 base/gap_buffer.h:188: void ResizeBuffer() { isn't there a stl thinger that could provide a lot of this functionality. http://codereview.chromium.org/3142008/diff/8001/9005 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/3142008/diff/8001/9005#newcode1266 chrome/chrome_tests.gypi:1266: '../views/controls/textfield/textfield_view_unittest.cc', you'll probably want to exclude this if touchui isn't defined. http://codereview.chromium.org/3142008/diff/8001/9008 File views/controls/textfield/textfield_view.cc (right): http://codereview.chromium.org/3142008/diff/8001/9008#newcode46 views/controls/textfield/textfield_view.cc:46: bool TextfieldView::OnMousePressed(const views::MouseEvent& e) { these aren't done... you need to add some TODOs to take care of this. http://codereview.chromium.org/3142008/diff/8001/9008#newcode69 views/controls/textfield/textfield_view.cc:69: void TextfieldView::WillGainFocus() { this almost certainly needs contents... http://codereview.chromium.org/3142008/diff/8001/9008#newcode91 views/controls/textfield/textfield_view.cc:91: void TextfieldView::PaintTextAndCursor(gfx::Canvas* canvas) { 1. does it work with bidi 2. you repaint the whole text field every time. Yes? Add a TODO() for correctly handling the partial paint. http://codereview.chromium.org/3142008/diff/8001/9008#newcode286 views/controls/textfield/textfield_view.cc:286: wchar_t TextfieldView::GetWritableChar(base::KeyboardCode key_code, is this functionality not provided elsewhere? Could it be nicer implemented via map? http://codereview.chromium.org/3142008/diff/8001/9009 File views/controls/textfield/textfield_view.h (right): http://codereview.chromium.org/3142008/diff/8001/9009#newcode6 views/controls/textfield/textfield_view.h:6: #define VIEWS_CONTROLS_TEXTFIELD_TEXTFIELD_VIEW_H_ #pragma once http://codereview.chromium.org/3142008/diff/8001/9009#newcode10 views/controls/textfield/textfield_view.h:10: #include "base/keyboard_codes.h" can you forward declare instead of including? http://codereview.chromium.org/3142008/diff/8001/9009#newcode37 views/controls/textfield/textfield_view.h:37: virtual void WillGainFocus(); you'll want to add an OnTouchEvent at some point. :-) But not until later. http://codereview.chromium.org/3142008/diff/8001/9010 File views/controls/textfield/textfield_view_unittest.cc (right): http://codereview.chromium.org/3142008/diff/8001/9010#newcode48 views/controls/textfield/textfield_view_unittest.cc:48: for (int i = 0; i < 4; i++) { i think that you need a unit test for paint http://codereview.chromium.org/3142008/diff/8001/9011 File views/views.gyp (right): http://codereview.chromium.org/3142008/diff/8001/9011#newcode220 views/views.gyp:220: 'controls/textfield/textfield_model.cc', you'll need an exclude for these somewhere.
Added sky as reviewer Also addressed all of Rob's comments http://codereview.chromium.org/3142008/diff/8001/9003 File base/gap_buffer.h (right): http://codereview.chromium.org/3142008/diff/8001/9003#newcode9 base/gap_buffer.h:9: #define BASE_GAP_BUFFER_H_ On 2010/08/30 17:26:24, rjkroege wrote: > #pragma once ... Done. http://codereview.chromium.org/3142008/diff/8001/9003#newcode64 base/gap_buffer.h:64: for (int i = 0; i < count && gap_start_ > 0; i++) { On 2010/08/30 17:26:24, rjkroege wrote: > replace these loops with memmove/memcpy Done. http://codereview.chromium.org/3142008/diff/8001/9003#newcode183 base/gap_buffer.h:183: On 2010/08/30 17:26:24, rjkroege wrote: > probably no space Done. http://codereview.chromium.org/3142008/diff/8001/9009 File views/controls/textfield/textfield_view.h (right): http://codereview.chromium.org/3142008/diff/8001/9009#newcode6 views/controls/textfield/textfield_view.h:6: #define VIEWS_CONTROLS_TEXTFIELD_TEXTFIELD_VIEW_H_ On 2010/08/30 17:26:24, rjkroege wrote: > #pragma once Done.
I focused on just the GapBuffer. It would be better if you separate that out and once it looks good, move on. http://codereview.chromium.org/3142008/diff/30001/31003 File base/gap_buffer.h (right): http://codereview.chromium.org/3142008/diff/30001/31003#newcode5 base/gap_buffer.h:5: // This class implements a standard GapBuffer Put the comment above the class name. http://codereview.chromium.org/3142008/diff/30001/31003#newcode14 base/gap_buffer.h:14: #include <string> Do you need this include? http://codereview.chromium.org/3142008/diff/30001/31003#newcode16 base/gap_buffer.h:16: #include "base/basictypes.h" newline between 15 and 16. http://codereview.chromium.org/3142008/diff/30001/31003#newcode22 base/gap_buffer.h:22: template<class T> Is it really worth parameterizing the type? At some point we want to move views off of wstring, and at that point we would presumably convert this too. http://codereview.chromium.org/3142008/diff/30001/31003#newcode25 base/gap_buffer.h:25: no newline here. http://codereview.chromium.org/3142008/diff/30001/31003#newcode35 base/gap_buffer.h:35: if (buffer_.get()) { We don't do checks like this. OOM triggers a crash. http://codereview.chromium.org/3142008/diff/30001/31003#newcode49 base/gap_buffer.h:49: if (buffer_.get()) { Same comment as on line 35. http://codereview.chromium.org/3142008/diff/30001/31003#newcode61 base/gap_buffer.h:61: void MoveGapLeft(int count) { Do we really need all these MoveGapFoo methods? Can't we just have MoveGapTo ? Also, shouldn't these DCHECK if attempted to move to an invalid location? http://codereview.chromium.org/3142008/diff/30001/31003#newcode62 base/gap_buffer.h:62: if (buffer_.get() && gap_start_ > 0) { Don't check buffer_.get() at all in this class. http://codereview.chromium.org/3142008/diff/30001/31003#newcode116 base/gap_buffer.h:116: buffer_.get()[gap_start_] = object; buffer_[gap_start] http://codereview.chromium.org/3142008/diff/30001/31003#newcode157 base/gap_buffer.h:157: (*contents).clear(); contents-> http://codereview.chromium.org/3142008/diff/30001/31003#newcode176 base/gap_buffer.h:176: int GetBufferSize() { Is there a case where someone cares about GetBufferSize()? Seems like an implementation detail, and if you keep public it's easy to mistake it for GetCurrentSize. http://codereview.chromium.org/3142008/diff/30001/31003#newcode196 base/gap_buffer.h:196: for (i = 0; i < gap_start_; i++) { How come you don't memmove these two blocks? http://codereview.chromium.org/3142008/diff/30001/31003#newcode230 base/gap_buffer.h:230: const int kMinResizeAmount; This should be static. http://codereview.chromium.org/3142008/diff/30001/31003#newcode231 base/gap_buffer.h:231: }; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/3142008/diff/30001/31004 File base/gap_buffer_unittest.cc (right): http://codereview.chromium.org/3142008/diff/30001/31004#newcode6 base/gap_buffer_unittest.cc:6: #include "base/gap_buffer.h" newline between 5 and 6. http://codereview.chromium.org/3142008/diff/30001/31004#newcode13 base/gap_buffer_unittest.cc:13: GapBufferTester(int a = 1, char b = 'a') no default params. http://codereview.chromium.org/3142008/diff/30001/31004#newcode14 base/gap_buffer_unittest.cc:14: : a_(a), b_(b) { } each member on its own line. http://codereview.chromium.org/3142008/diff/30001/31004#newcode15 base/gap_buffer_unittest.cc:15: bool operator!=(const GapBufferTester& that) { Style guides says you should use explicit methods for this. http://codereview.chromium.org/3142008/diff/30001/31004#newcode30 base/gap_buffer_unittest.cc:30: bool CompareVectorWithArray(const T* array, int size, std::vector<T> vect) { const std::vector<T>& But I believe you can use EXPECT_THAT(.., ElementsAre()) here, but I haven't tried it. If that doesn't work, you should make sure your failing is more descriptive. http://codereview.chromium.org/3142008/diff/30001/31004#newcode31 base/gap_buffer_unittest.cc:31: if ((size_t) size == vect.size()) { Use C++ style casts. http://codereview.chromium.org/3142008/diff/30001/31004#newcode45 base/gap_buffer_unittest.cc:45: void Expectations(int current_size, int buffer_size, int contents_size, each param on its own line. http://codereview.chromium.org/3142008/diff/30001/31004#newcode46 base/gap_buffer_unittest.cc:46: int gap_start, const T* contents_array, GapBuffer<T>& gap_buffer) { const GapBuffer&. If it can't be const&, then it should be a pointer. http://codereview.chromium.org/3142008/diff/30001/31004#newcode48 base/gap_buffer_unittest.cc:48: gap_buffer.GetContents(&(contents)); No parens around contents, just &contents http://codereview.chromium.org/3142008/diff/30001/31004#newcode52 base/gap_buffer_unittest.cc:52: EXPECT_EQ((size_t) contents_size, contents.size()); C++ style casts http://codereview.chromium.org/3142008/diff/30001/31004#newcode53 base/gap_buffer_unittest.cc:53: EXPECT_TRUE(CompareVectorWithArray(contents_array, contents_size, contents)); If this test fails the generated failure isn't going to be particular helpful. It would be much more readable if you generated a string description and compared that.
I have a meta question about this patch. Should we look at making it easy to host a WebKit edit control rather than going this route? TextFields are extremely difficult to get right and we don't actually use that many textfields in Chrome's UI. -Scott
Similarly, I'd like to see a proof of concept wrapping a WebKit text control... since it depends on WebKit and our OOP WebKit wrappers currently live above src/views/ in the layering hierarchy this POC would have to live in src/chrome/browser/views/ to start with. It seems like it should only take a few days-a week for someone to make something like this that would work. And you'd get a whole lot of things (RTL, IME, etc) for free.
On 2010/08/31 23:26:51, Ben Goodger wrote: > Similarly, I'd like to see a proof of concept wrapping a WebKit text control... > since it depends on WebKit and our OOP WebKit wrappers currently live above > src/views/ in the layering hierarchy this POC would have to live in > src/chrome/browser/views/ to start with. It seems like it should only take a few > days-a week for someone to make something like this that would work. And you'd > get a whole lot of things (RTL, IME, etc) for free. Ben, Scott: can you expand on this some more. I like this approach better but am not sure how we would get started with it.
Hi Scott and others, As suggested by Scott, I have moved the gap buffer into a separate CL: http://codereview.chromium.org/5158004 Also addressed all of Scott's comments. Please have a look at the other CL and also this one for the views textfield. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h File base/gap_buffer.h (right): http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode5 base/gap_buffer.h:5: // This class implements a standard GapBuffer On 2010/08/31 23:19:47, sky wrote: > Put the comment above the class name. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode14 base/gap_buffer.h:14: #include <string> On 2010/08/31 23:19:47, sky wrote: > Do you need this include? Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode16 base/gap_buffer.h:16: #include "base/basictypes.h" On 2010/08/31 23:19:47, sky wrote: > newline between 15 and 16. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode22 base/gap_buffer.h:22: template<class T> On 2010/08/31 23:19:47, sky wrote: > Is it really worth parameterizing the type? At some point we want to move views > off of wstring, and at that point we would presumably convert this too. my intention was exactly to avoid changes to this class when changing string implementations. This is fairly small class and should not bloat too much due to parameterization. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode25 base/gap_buffer.h:25: On 2010/08/31 23:19:47, sky wrote: > no newline here. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode35 base/gap_buffer.h:35: if (buffer_.get()) { On 2010/08/31 23:19:47, sky wrote: > We don't do checks like this. OOM triggers a crash. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode49 base/gap_buffer.h:49: if (buffer_.get()) { On 2010/08/31 23:19:47, sky wrote: > Same comment as on line 35. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode61 base/gap_buffer.h:61: void MoveGapLeft(int count) { On 2010/08/31 23:19:47, sky wrote: > Do we really need all these MoveGapFoo methods? Can't we just have MoveGapTo ? Since gap buffers are really only used in text editing, I meant to provide the api to reflect the actions one might do in a text editor. > Also, shouldn't these DCHECK if attempted to move to an invalid location? As in a text editor if you try to move the cursor (gap) out of bounds, say to a position after the end of text then we simply move it to the end of text. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode62 base/gap_buffer.h:62: if (buffer_.get() && gap_start_ > 0) { On 2010/08/31 23:19:47, sky wrote: > Don't check buffer_.get() at all in this class. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode116 base/gap_buffer.h:116: buffer_.get()[gap_start_] = object; On 2010/08/31 23:19:47, sky wrote: > buffer_[gap_start] Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode157 base/gap_buffer.h:157: (*contents).clear(); On 2010/08/31 23:19:47, sky wrote: > contents-> Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode176 base/gap_buffer.h:176: int GetBufferSize() { On 2010/08/31 23:19:47, sky wrote: > Is there a case where someone cares about GetBufferSize()? Seems like an > implementation detail, and if you keep public it's easy to mistake it for > GetCurrentSize. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode196 base/gap_buffer.h:196: for (i = 0; i < gap_start_; i++) { On 2010/08/31 23:19:47, sky wrote: > How come you don't memmove these two blocks? Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode230 base/gap_buffer.h:230: const int kMinResizeAmount; On 2010/08/31 23:19:47, sky wrote: > This should be static. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode231 base/gap_buffer.h:231: }; On 2010/08/31 23:19:47, sky wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc File base/gap_buffer_unittest.cc (right): http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:6: #include "base/gap_buffer.h" On 2010/08/31 23:19:47, sky wrote: > newline between 5 and 6. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:13: GapBufferTester(int a = 1, char b = 'a') On 2010/08/31 23:19:47, sky wrote: > no default params. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:14: : a_(a), b_(b) { } On 2010/08/31 23:19:47, sky wrote: > each member on its own line. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:15: bool operator!=(const GapBufferTester& that) { On 2010/08/31 23:19:47, sky wrote: > Style guides says you should use explicit methods for this. these operators are no longer required. However, I have added == because it is required by the matcher. Added commetn to explain this. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:30: bool CompareVectorWithArray(const T* array, int size, std::vector<T> vect) { On 2010/08/31 23:19:47, sky wrote: > const std::vector<T>& > But I believe you can use EXPECT_THAT(.., ElementsAre()) here, but I haven't > tried it. If that doesn't work, you should make sure your failing is more > descriptive. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:31: if ((size_t) size == vect.size()) { On 2010/08/31 23:19:47, sky wrote: > Use C++ style casts. removed http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:45: void Expectations(int current_size, int buffer_size, int contents_size, On 2010/08/31 23:19:47, sky wrote: > each param on its own line. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:46: int gap_start, const T* contents_array, GapBuffer<T>& gap_buffer) { On 2010/08/31 23:19:47, sky wrote: > const GapBuffer&. If it can't be const&, then it should be a pointer. Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:48: gap_buffer.GetContents(&(contents)); On 2010/08/31 23:19:47, sky wrote: > No parens around contents, just &contents Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:52: EXPECT_EQ((size_t) contents_size, contents.size()); On 2010/08/31 23:19:47, sky wrote: > C++ style casts Done. http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... base/gap_buffer_unittest.cc:53: EXPECT_TRUE(CompareVectorWithArray(contents_array, contents_size, contents)); On 2010/08/31 23:19:47, sky wrote: > If this test fails the generated failure isn't going to be particular helpful. > It would be much more readable if you generated a string description and > compared that. converted to ExpectThat
http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... File views/controls/textfield/textfield_model.cc (right): http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.cc:12: this->buffer_.reset(new base::GapBuffer<wchar_t>(50)); Please, remove all the usages of |this->| in this file. We don't use it on Chromium. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.cc:43: buffer_->MoveGapLeft(1); extract this constant? You can put it before the namespace views above, like this (no need to wrap it in an anonymous namespace or put static before const): // Describe what it means, etc. const int kMyConstant = 1; http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.cc:78: std::vector<wchar_t>::const_iterator iter; Can you move this inside the for loop ? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.cc:79: text_ = L""; text_.clear() instead? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.cc:80: for (iter = contents.begin(); iter != contents.end(); iter++) { ++iter http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.cc:84: }; Please add a blank line above. // namespace views Also don't need the ; here http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... File views/controls/textfield/textfield_model.h (right): http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.h:10: #include <vector> It seems you don't need this include also. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.h:13: #include "views/event.h" It seems you don't need this include here. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.h:59: std::wstring text(); Since this is not a trivial getter, can you name this as GetText? Also, since you are returning |text_| can you write this as: const std::wstring& GetText() const; http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.h:70: }; DISALLOW_COPY_AND_ASSIGN? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.h:72: class TextfieldController { Can you name this TextfieldDelegate or even TextfieldListener? So this match with other examples in views. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.h:72: class TextfieldController { I'd also move this above, before the TextfieldModel class, or move inside it. Your call. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.h:74: virtual ~TextfieldController(); add a {} here? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.h:75: virtual void TextChanged(const std::wstring new_text); It looks like this class is an interface. I think this virtual method needs to be a pure virtual method instead, so please add = 0; here. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.h:77: }; // namespace views Also don't need the ; here http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... File views/controls/textfield/textfield_view.cc (right): http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.cc:129: std::wstring str = L""; I think when you do: std::wstring str; it is already initialized with empty value no? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.cc:131: for (int pos = cursor_pos; pos > 0; pos--) { --pos http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.cc:138: for (unsigned i = 0; i < str.length() / 2.0; i++) { ++i http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.cc:151: for (unsigned pos = cursor_pos; pos < text.length(); pos++) { ++pos http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.cc:240: < bounds().width() - View::GetInsets().width()) { put this < in the line above? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.cc:262: break; I think the break is not necessary here, it won't be reached. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.cc:270: || (key_code >= app::VKEY_NUMPAD0 && key_code <= app::VKEY_DIVIDE) Please, fix all the occurrences in this file. This || should be in the end of the previous line. Always. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.cc:289: bool shift) { You can align shift with app::KeyboardCode. Also please, go to this file and match the order of the implementation of all methods with the order of the definitions in the header file. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.cc:322: // case app::VKEY_BACK: Why this big block of comment? If it isn't used. Please remove it. Same thing below. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... File views/controls/textfield/textfield_view.h (right): http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.h:9: #include <algorithm> Do you need this include here? Or just in the .cc file? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.h:11: #include "gfx/canvas.h" can't you forward declare this? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.h:14: #include "views/event.h" can't you forward declare this? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.h:15: #include "views/view.h" I think you don't need this include. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.h:25: explicit TextfieldView(TextfieldModel* model); Document that you take the ownership of |model|? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.h:27: virtual void SetController(TextfieldController* controller); The virtual is not needed, right? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.h:33: // View Overrides. Refer to view.h for documentation. Please, rewrite this comment as just: // Overridden from views::View: http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.h:46: // Getters remove this comment. It's redundant. http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.h:86: }; DISALLOW_COPY_AND_ASSIGN? http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view.h:87: }; // namespace views Also don't need the ; here http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... File views/controls/textfield/textfield_view_unittest.cc (right): http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_view_unittest.cc:103: } // namespace views http://codereview.chromium.org/3142008/diff/64001/views/views.gyp File views/views.gyp (right): http://codereview.chromium.org/3142008/diff/64001/views/views.gyp#newcode220 views/views.gyp:220: 'controls/textfield/textfield.cc', Since you are here, can you fix the order here too? It should be alphabetical.
http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... File views/controls/textfield/textfield_model.h (right): http://codereview.chromium.org/3142008/diff/64001/views/controls/textfield/te... views/controls/textfield/textfield_model.h:72: class TextfieldController { TextfieldDelegate doesn't make sense... it's not doing work on behalf of the Textfield. Listener perhaps, but I am not sure what other responsibilities this interface will take on as this code evolves.
On Wed, Nov 17, 2010 at 7:45 PM, <varunjain@chromium.org> wrote: > Hi Scott and others, > As suggested by Scott, I have moved the gap buffer into a separate CL: > http://codereview.chromium.org/5158004 I'll review this patch after 5158004 is done. -Scott > > Also addressed all of Scott's comments. Please have a look at the other CL > and > also this one for the views textfield. > > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h > File base/gap_buffer.h (right): > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode5 > base/gap_buffer.h:5: // This class implements a standard GapBuffer > On 2010/08/31 23:19:47, sky wrote: >> >> Put the comment above the class name. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode14 > base/gap_buffer.h:14: #include <string> > On 2010/08/31 23:19:47, sky wrote: >> >> Do you need this include? > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode16 > base/gap_buffer.h:16: #include "base/basictypes.h" > On 2010/08/31 23:19:47, sky wrote: >> >> newline between 15 and 16. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode22 > base/gap_buffer.h:22: template<class T> > On 2010/08/31 23:19:47, sky wrote: >> >> Is it really worth parameterizing the type? At some point we want to > > move views >> >> off of wstring, and at that point we would presumably convert this > > too. > > my intention was exactly to avoid changes to this class when changing > string implementations. This is fairly small class and should not bloat > too much due to parameterization. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode25 > base/gap_buffer.h:25: > On 2010/08/31 23:19:47, sky wrote: >> >> no newline here. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode35 > base/gap_buffer.h:35: if (buffer_.get()) { > On 2010/08/31 23:19:47, sky wrote: >> >> We don't do checks like this. OOM triggers a crash. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode49 > base/gap_buffer.h:49: if (buffer_.get()) { > On 2010/08/31 23:19:47, sky wrote: >> >> Same comment as on line 35. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode61 > base/gap_buffer.h:61: void MoveGapLeft(int count) { > On 2010/08/31 23:19:47, sky wrote: >> >> Do we really need all these MoveGapFoo methods? Can't we just have > > MoveGapTo ? > > Since gap buffers are really only used in text editing, I meant to > provide the api to reflect the actions one might do in a text editor. > >> Also, shouldn't these DCHECK if attempted to move to an invalid > > location? > > As in a text editor if you try to move the cursor (gap) out of bounds, > say to a position after the end of text then we simply move it to the > end of text. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode62 > base/gap_buffer.h:62: if (buffer_.get() && gap_start_ > 0) { > On 2010/08/31 23:19:47, sky wrote: >> >> Don't check buffer_.get() at all in this class. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode116 > base/gap_buffer.h:116: buffer_.get()[gap_start_] = object; > On 2010/08/31 23:19:47, sky wrote: >> >> buffer_[gap_start] > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode157 > base/gap_buffer.h:157: (*contents).clear(); > On 2010/08/31 23:19:47, sky wrote: >> >> contents-> > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode176 > base/gap_buffer.h:176: int GetBufferSize() { > On 2010/08/31 23:19:47, sky wrote: >> >> Is there a case where someone cares about GetBufferSize()? Seems like > > an >> >> implementation detail, and if you keep public it's easy to mistake it > > for >> >> GetCurrentSize. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode196 > base/gap_buffer.h:196: for (i = 0; i < gap_start_; i++) { > On 2010/08/31 23:19:47, sky wrote: >> >> How come you don't memmove these two blocks? > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode230 > base/gap_buffer.h:230: const int kMinResizeAmount; > On 2010/08/31 23:19:47, sky wrote: >> >> This should be static. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer.h#newcode231 > base/gap_buffer.h:231: }; > On 2010/08/31 23:19:47, sky wrote: >> >> DISALLOW_COPY_AND_ASSIGN > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc > File base/gap_buffer_unittest.cc (right): > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:6: #include "base/gap_buffer.h" > On 2010/08/31 23:19:47, sky wrote: >> >> newline between 5 and 6. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:13: GapBufferTester(int a = 1, char b = 'a') > On 2010/08/31 23:19:47, sky wrote: >> >> no default params. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:14: : a_(a), b_(b) { } > On 2010/08/31 23:19:47, sky wrote: >> >> each member on its own line. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:15: bool operator!=(const GapBufferTester& > that) { > On 2010/08/31 23:19:47, sky wrote: >> >> Style guides says you should use explicit methods for this. > > these operators are no longer required. However, I have added == because > it is required by the matcher. Added commetn to explain this. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:30: bool CompareVectorWithArray(const T* > array, int size, std::vector<T> vect) { > On 2010/08/31 23:19:47, sky wrote: >> >> const std::vector<T>& >> But I believe you can use EXPECT_THAT(.., ElementsAre()) here, but I > > haven't >> >> tried it. If that doesn't work, you should make sure your failing is > > more >> >> descriptive. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:31: if ((size_t) size == vect.size()) { > On 2010/08/31 23:19:47, sky wrote: >> >> Use C++ style casts. > > removed > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:45: void Expectations(int current_size, int > buffer_size, int contents_size, > On 2010/08/31 23:19:47, sky wrote: >> >> each param on its own line. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:46: int gap_start, const T* contents_array, > GapBuffer<T>& gap_buffer) { > On 2010/08/31 23:19:47, sky wrote: >> >> const GapBuffer&. If it can't be const&, then it should be a pointer. > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:48: gap_buffer.GetContents(&(contents)); > On 2010/08/31 23:19:47, sky wrote: >> >> No parens around contents, just &contents > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:52: EXPECT_EQ((size_t) contents_size, > contents.size()); > On 2010/08/31 23:19:47, sky wrote: >> >> C++ style casts > > Done. > > http://codereview.chromium.org/3142008/diff/30001/base/gap_buffer_unittest.cc... > base/gap_buffer_unittest.cc:53: > EXPECT_TRUE(CompareVectorWithArray(contents_array, contents_size, > contents)); > On 2010/08/31 23:19:47, sky wrote: >> >> If this test fails the generated failure isn't going to be particular > > helpful. >> >> It would be much more readable if you generated a string description > > and >> >> compared that. > > converted to ExpectThat > > http://codereview.chromium.org/3142008/ > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
