Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831403:
TsfTextStore implementation. (Closed)
|
Created:
8 years, 4 months ago by horo Modified:
8 years, 3 months ago CC:
chrome-tsf_google.com Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionTsfTextStore implementation.
BUG=141611
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153453
Patch Set 1 #Patch Set 2 : fix misstype #
Total comments: 65
Patch Set 3 : Add SendOnLayoutChange() method. #Patch Set 4 : #
Total comments: 40
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : reflected code review meeting #
Total comments: 14
Patch Set 8 : #
Total comments: 16
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : Add unit tests. #
Total comments: 2
Patch Set 12 : #
Total comments: 26
Patch Set 13 : #
Total comments: 4
Patch Set 14 : #
Messages
Total messages: 25 (0 generated)
initial scan http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:19: nit: remove line. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:21: CLSCTX_INPROC_SERVER))) { nit: align http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:26: NULL, nit:align http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:187: type == TEXT_INPUT_TYPE_PASSWORD) { I think DCHECK(type != TEXT_INPUT_TYPE_NONE && type != TEXT_INPUT_TYPE_PASSWORD) is okay because if the text input type is none or password, this text store is not used. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:271: return E_FAIL; How about using GetCaretBounds if the start_pos == end_pos == 0 http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:290: nit: remove line http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:326: if (!WriterLocked()) { nit: remove braces http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:339: status_.InsertText(start_pos, DCHECK(text_buffer)? http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:354: text_store_acp_sink_->OnSelectionChange(); I'm not sure but is it okay to call OnSelectionChange under write locking? http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:401: STDMETHODIMP TsfTextStore::RequestLock(DWORD lock_flags, HRESULT* result) { I think it is clear that just returning E_FAIL if the text_input_client is null. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:445: new_commited_size - last_commited_size); nit: align http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:531: < selection_buffer[0].acpEnd)){ Plz be consistent the operator position. BTW, this condition is complicated. How about split these conditions? http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:562: if (SetSelection(1, &selection) != S_OK) { nit: remove braces. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:624: if (FAILED(category_mgr_->GetGUID(guid_atom, &guid))) { nit: remove braces. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:629: guid, display_attribute_info.Receive(), NULL))) { nit:align http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:636: ITfContext* context, const TfEditCookie read_only_edit_cookie, nit: one line one variable. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:641: const GUID *rgGuids[2] = {&GUID_PROP_COMPOSING, &GUID_PROP_ATTRIBUTE}; nit: /GUID */GUID* / http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:652: context->GetStart(read_only_edit_cookie, start_to_end_range.Receive()); Plz check the returned value, and plz do elsewhere http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:676: TfGuidAtom guid_atom = (TfGuidAtom) property_value.varValue.lVal; nit: static_cast? http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:677: if (GetDisplayAttribute(guid_atom, &display_attribute)) { nit: remove braces. and plz do elsewhere http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:708: HWND focused_window, TextInputClient* text_input_client) { nit: one line one variable http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:13: #include "base/memory/scoped_ptr.h" We can omit this line. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:16: #include "ui/base/ime/text_input_client.h" plz use forward declaration instead. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:17: #include "ui/base/ui_export.h" Do you need UI_EXPORT? http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:170: // HWND of the current view window which is set in SetFocus(). nit: SetFocusedTextInputClient http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:173: // Current TextInputClien which is set in SetFocus(). /TextInputClien/TextInputClient/ /SetFocus/SetFocusedTextInputClient/ http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:179: TextStoreStatus() : commited_size_(0), selection_start_(0), Plz initialize one line one variable. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:182: void InsertText(int nPos, const string16 &buffer){ /string16 &/string16& / http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:182: void InsertText(int nPos, const string16 &buffer){ nit: /nPos/position/ http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:213: size_t selection_end_; How about introduce ui::Range object? http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:236: // Queue of the lock request used in RequestLock(). Please add one line before line comment. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:245: base::win::ScopedComPtr<ITfCategoryMgr> category_mgr_; Plz do not abbreviate the variables. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...
http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:19: On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: remove line. Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:21: CLSCTX_INPROC_SERVER))) { On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: align Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:26: NULL, On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit:align Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:187: type == TEXT_INPUT_TYPE_PASSWORD) { On 2012/08/21 12:22:41, Seigo Nonaka wrote: > I think DCHECK(type != TEXT_INPUT_TYPE_NONE && type != TEXT_INPUT_TYPE_PASSWORD) > is okay because if the text input type is none or password, this text store is > not used. Oh I see. Then, status->dwDynamicFlags = 0; status->dwStaticFlags = TS_SS_NOHIDDENTEXT; is ok http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:271: return E_FAIL; On 2012/08/21 12:22:41, Seigo Nonaka wrote: > How about using GetCaretBounds if the start_pos == end_pos == 0 Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:290: On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: remove line Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:326: if (!WriterLocked()) { On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: remove braces Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:339: status_.InsertText(start_pos, On 2012/08/21 12:22:41, Seigo Nonaka wrote: > DCHECK(text_buffer)? Added if (!text_buffer) return E_INVALIDARG; http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:354: text_store_acp_sink_->OnSelectionChange(); On 2012/08/21 12:22:41, Seigo Nonaka wrote: > I'm not sure but is it okay to call OnSelectionChange under write locking? I think it is ok. The sample application "TsfPad" provided by MS also calls OnSelectionChange() in InsertTextAtSelection. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:401: STDMETHODIMP TsfTextStore::RequestLock(DWORD lock_flags, HRESULT* result) { On 2012/08/21 12:22:41, Seigo Nonaka wrote: > I think it is clear that just returning E_FAIL if the text_input_client is null. Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:445: new_commited_size - last_commited_size); On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: align Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:531: < selection_buffer[0].acpEnd)){ On 2012/08/21 12:22:41, Seigo Nonaka wrote: > Plz be consistent the operator position. > BTW, this condition is complicated. > How about split these conditions? Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:562: if (SetSelection(1, &selection) != S_OK) { On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: remove braces. Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:624: if (FAILED(category_mgr_->GetGUID(guid_atom, &guid))) { On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: remove braces. Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:629: guid, display_attribute_info.Receive(), NULL))) { On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit:align Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:636: ITfContext* context, const TfEditCookie read_only_edit_cookie, On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: one line one variable. Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:641: const GUID *rgGuids[2] = {&GUID_PROP_COMPOSING, &GUID_PROP_ATTRIBUTE}; On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: /GUID */GUID* / Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:652: context->GetStart(read_only_edit_cookie, start_to_end_range.Receive()); On 2012/08/21 12:22:41, Seigo Nonaka wrote: > Plz check the returned value, and plz do elsewhere Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:676: TfGuidAtom guid_atom = (TfGuidAtom) property_value.varValue.lVal; On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: static_cast? Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:677: if (GetDisplayAttribute(guid_atom, &display_attribute)) { On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: remove braces. and plz do elsewhere Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:708: HWND focused_window, TextInputClient* text_input_client) { On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: one line one variable Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:13: #include "base/memory/scoped_ptr.h" On 2012/08/21 12:22:41, Seigo Nonaka wrote: > We can omit this line. Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:16: #include "ui/base/ime/text_input_client.h" On 2012/08/21 12:22:41, Seigo Nonaka wrote: > plz use forward declaration instead. Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:17: #include "ui/base/ui_export.h" On 2012/08/21 12:22:41, Seigo Nonaka wrote: > Do you need UI_EXPORT? deleted http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:170: // HWND of the current view window which is set in SetFocus(). On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: SetFocusedTextInputClient Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:173: // Current TextInputClien which is set in SetFocus(). On 2012/08/21 12:22:41, Seigo Nonaka wrote: > /TextInputClien/TextInputClient/ > /SetFocus/SetFocusedTextInputClient/ Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:179: TextStoreStatus() : commited_size_(0), selection_start_(0), On 2012/08/21 12:22:41, Seigo Nonaka wrote: > Plz initialize one line one variable. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:182: void InsertText(int nPos, const string16 &buffer){ On 2012/08/21 12:22:41, Seigo Nonaka wrote: > /string16 &/string16& / Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:182: void InsertText(int nPos, const string16 &buffer){ On 2012/08/21 12:22:41, Seigo Nonaka wrote: > nit: /nPos/position/ Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:213: size_t selection_end_; On 2012/08/21 12:22:41, Seigo Nonaka wrote: > How about introduce ui::Range object? Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:236: // Queue of the lock request used in RequestLock(). On 2012/08/21 12:22:41, Seigo Nonaka wrote: > Please add one line before line comment. Done. http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:245: base::win::ScopedComPtr<ITfCategoryMgr> category_mgr_; On 2012/08/21 12:22:41, Seigo Nonaka wrote: > Plz do not abbreviate the variables. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... Done.
http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/2003/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:354: text_store_acp_sink_->OnSelectionChange(); Got it, thanks. On 2012/08/21 13:59:57, horo wrote: > On 2012/08/21 12:22:41, Seigo Nonaka wrote: > > I'm not sure but is it okay to call OnSelectionChange under write locking? > > I think it is ok. > The sample application "TsfPad" provided by MS also calls OnSelectionChange() in > InsertTextAtSelection. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:229: LONG acp_end, I'm confusing the |acp_end| spec. Please let me known whether the |acp_end| is inclusive? or exclusive? I read the reference again but it is not explicitly documented. I think this |acp_end| seems inclusive, do you know the standard behavior? (I think exclusive range has bidi or RTL issue, but I think it is okay to ignore bidi issue for the first implementation) And according to your code, you assume the |asp_end| as exclusive... right? http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:245: uint32 start_pos = acp_start - status_.commited_size_; nit: const? http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:246: uint32 end_pos = acp_end - status_.commited_size_; nit: const? http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:249: if (text_input_client_->GetCompositionCharacterBounds(start_pos + 1, /start_pos + 1/start_pos/ ? GetCompositionCharacterBounds accepts zero based index. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:253: } else if (text_input_client_->GetCompositionCharacterBounds(start_pos, /start_pos/start_pos -1/ ? http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:260: result = text_input_client_->GetCaretBounds(); Sorry this is my mistake. Caret does not always exist at 0th position. I think we can use CaretBounds only if the string buffer is empty. HOw about if (start_pos == end_pos) { if (start_pos == 0) { if (text_input_client_->GetCompositionCharacterBounds(0, &tmp_rect)) { result = tmp_rect; result.set_width(0); } else if (string_buffer_.size() == status_.committed_size_) { result = text_input_client_->GetCaretBounds(); } else { return TS_E_NOLAYOUT; } } else if (text_input_client_->GetCompositionCharacterBounds(start_pos -1 , &tmp_rect)) { result.set_x(tmp_rect.right()); result.set_y(tmp_rect.y()); result.set_width(0); result.set_height(tmp_rect.height()); } } http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:269: for (uint32 i = start_pos + 1; i < end_pos - 1; ++i) { I think |i| should go |end_pos| -1. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:270: if (!text_input_client_->GetCompositionCharacterBounds(start_pos, /start_pos/i/ ? http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:274: result.Union(tmp_rect); Union function returns new rectangle. result = result.Union(tmp_rect); ? http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:274: result.Union(tmp_rect); BTW, do we concern multiple line composition string? I think it's okay for minimum implementation, but I think we should check the standard behavior. Anyway please leave TODO comment for multiple line handling. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:319: LONG start_pos = status_.selection_.start(); nit: const? http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:320: LONG end_pos = status_.selection_.end(); nit: const? http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:334: if (acp_start) { nit: remove braces http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:337: if (acp_end) { nit: remove braces http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:415: size_t last_commited_size = status_.commited_size_; nit: const and /commited/committed/ http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:424: if(lock_queue_.size() > 0) { nit: /if(/if (/ http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:443: if (text_input_client_) { nit: remove braces http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:468: if (text_input_client_) { nit: remove braces http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:523: (static_cast<LONG>(status_.string_buffer_.length()) < end_pos)) nit: please do not remove braces if the condition become multiple lines. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:180: struct TextStoreStatus { Do you think this struct should be exposed? If not, I think it is good to move to .cc file with anonymous namespace. Also good to move member function implementation to .cc file.
http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:229: LONG acp_end, On 2012/08/21 16:14:06, Seigo Nonaka wrote: > I'm confusing the |acp_end| spec. > Please let me known whether the |acp_end| is inclusive? or exclusive? > I read the reference again but it is not explicitly documented. > I think this |acp_end| seems inclusive, do you know the standard behavior? > (I think exclusive range has bidi or RTL issue, but I think it is okay to ignore > bidi issue for the first implementation) > And according to your code, you assume the |asp_end| as exclusive... right? Sorry my code was wrong. I think |acp_end| should be inclusive. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:245: uint32 start_pos = acp_start - status_.commited_size_; On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: const? Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:246: uint32 end_pos = acp_end - status_.commited_size_; On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: const? Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:249: if (text_input_client_->GetCompositionCharacterBounds(start_pos + 1, On 2012/08/21 16:14:06, Seigo Nonaka wrote: > /start_pos + 1/start_pos/ ? > GetCompositionCharacterBounds accepts zero based index. ah yes, it is my mistake. Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:253: } else if (text_input_client_->GetCompositionCharacterBounds(start_pos, On 2012/08/21 16:14:06, Seigo Nonaka wrote: > /start_pos/start_pos -1/ ? Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:260: result = text_input_client_->GetCaretBounds(); On 2012/08/21 16:14:06, Seigo Nonaka wrote: > Sorry this is my mistake. > Caret does not always exist at 0th position. > I think we can use CaretBounds only if the string buffer is empty. > HOw about > > if (start_pos == end_pos) { > if (start_pos == 0) { > if (text_input_client_->GetCompositionCharacterBounds(0, &tmp_rect)) { > result = tmp_rect; > result.set_width(0); > } else if (string_buffer_.size() == status_.committed_size_) { > result = text_input_client_->GetCaretBounds(); > } else { > return TS_E_NOLAYOUT; > } > } else if (text_input_client_->GetCompositionCharacterBounds(start_pos -1 , > &tmp_rect)) { > result.set_x(tmp_rect.right()); > result.set_y(tmp_rect.y()); > result.set_width(0); > result.set_height(tmp_rect.height()); > } > } Done. And added } else { return TS_E_NOLAYOUT; } http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:269: for (uint32 i = start_pos + 1; i < end_pos - 1; ++i) { On 2012/08/21 16:14:06, Seigo Nonaka wrote: > I think |i| should go |end_pos| -1. Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:270: if (!text_input_client_->GetCompositionCharacterBounds(start_pos, On 2012/08/21 16:14:06, Seigo Nonaka wrote: > /start_pos/i/ ? Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:274: result.Union(tmp_rect); On 2012/08/21 16:14:06, Seigo Nonaka wrote: > Union function returns new rectangle. > result = result.Union(tmp_rect); ? Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:274: result.Union(tmp_rect); On 2012/08/21 16:14:06, Seigo Nonaka wrote: > BTW, do we concern multiple line composition string? > I think it's okay for minimum implementation, but I think we should check the > standard behavior. > Anyway please leave TODO comment for multiple line handling. There is no problem in the implementation of GetTextExt, which returns the bounding box of the text. IME (text service) should concern about the multiple line composition. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:319: LONG start_pos = status_.selection_.start(); On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: const? Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:320: LONG end_pos = status_.selection_.end(); On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: const? Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:334: if (acp_start) { On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: remove braces Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:337: if (acp_end) { On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: remove braces Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:415: size_t last_commited_size = status_.commited_size_; On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: const and /commited/committed/ Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:424: if(lock_queue_.size() > 0) { On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: /if(/if (/ Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:443: if (text_input_client_) { On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: remove braces Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:468: if (text_input_client_) { On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: remove braces Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.cc:523: (static_cast<LONG>(status_.string_buffer_.length()) < end_pos)) On 2012/08/21 16:14:06, Seigo Nonaka wrote: > nit: please do not remove braces if the condition become multiple lines. Done. http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/9001/ui/base/win/tsf_text_store.... ui/base/win/tsf_text_store.h:180: struct TextStoreStatus { On 2012/08/21 16:14:06, Seigo Nonaka wrote: > Do you think this struct should be exposed? > If not, I think it is good to move to .cc file with anonymous namespace. Also > good to move member function implementation to .cc file. It is exposed because I would like to access to the status_ in unit tests. I've moved the implementation to .cc file.
I have updated the code according to the code review meeting.
lgtm lgtm with style nits, but please wait other reviewer's lgtm. http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:259: // equal values of |acp_start| and |acp_end|. So we handles this condition. handle or should handle http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:309: // We does not support any embedded objects. /does not/don't/ and plz do elsewhere. http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:349: DCHECK(start_pos <= end_pos); I recommend using DCHECK_LE instead http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:217: // Check if the document has a read-only lock. /Check/Checks/ and move to L163 http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:221: bool HasReadWriteLock() const; ditto http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:230: TF_DISPLAYATTRIBUTE* attribute); move to L163 http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:236: CompositionUnderlines* undelines); ditto
http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:259: // equal values of |acp_start| and |acp_end|. So we handles this condition. On 2012/08/22 16:44:19, Seigo Nonaka wrote: > handle or should handle Done. http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:309: // We does not support any embedded objects. On 2012/08/22 16:44:19, Seigo Nonaka wrote: > /does not/don't/ and plz do elsewhere. Done. http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:349: DCHECK(start_pos <= end_pos); On 2012/08/22 16:44:19, Seigo Nonaka wrote: > I recommend using DCHECK_LE instead Done. http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:217: // Check if the document has a read-only lock. On 2012/08/22 16:44:19, Seigo Nonaka wrote: > /Check/Checks/ and move to L163 Done. http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:221: bool HasReadWriteLock() const; On 2012/08/22 16:44:19, Seigo Nonaka wrote: > ditto Done. http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:230: TF_DISPLAYATTRIBUTE* attribute); On 2012/08/22 16:44:19, Seigo Nonaka wrote: > move to L163 Done. http://codereview.chromium.org/10831403/diff/18001/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:236: CompositionUnderlines* undelines); On 2012/08/22 16:44:19, Seigo Nonaka wrote: > ditto Done.
http://codereview.chromium.org/10831403/diff/18002/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/18002/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:16: } } // namespace http://codereview.chromium.org/10831403/diff/18002/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:27: CLSID_TF_CategoryMgr, NULL, CLSCTX_INPROC_SERVER))) { Can we omit parameters as follows? category_manager_.CreateInstance(CLSID_TF_CategoryMgr) http://codereview.chromium.org/10831403/diff/18002/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:31: if (FAILED(display_attribute_manager_.CreateInstance( ditto. http://codereview.chromium.org/10831403/diff/18002/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:213: acp_end = std::min(acp_end, acp_start + (int)text_buffer_size); static_cast<LONG>(text_buffer_size) http://codereview.chromium.org/10831403/diff/18002/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:214: *text_buffer_copied = acp_end - acp_start; Please check if acp_start <= acp_end. http://codereview.chromium.org/10831403/diff/18002/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:216: const string16 result = string_buffer_.substr(acp_start, *text_buffer_copied); const string16& result http://codereview.chromium.org/10831403/diff/18002/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:332: const LONG new_end_pos = start_pos + text_buffer_size; http://codereview.chromium.org/10831403/diff/18002/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/18002/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:190: HWND hwnd_; window_handle_ might be better.
https://chromiumcodereview.appspot.com/10831403/diff/18002/ui/base/win/tsf_te... File ui/base/win/tsf_text_store.cc (right): https://chromiumcodereview.appspot.com/10831403/diff/18002/ui/base/win/tsf_te... ui/base/win/tsf_text_store.cc:16: } On 2012/08/23 04:10:27, Yohei Yukawa wrote: > } // namespace Done. https://chromiumcodereview.appspot.com/10831403/diff/18002/ui/base/win/tsf_te... ui/base/win/tsf_text_store.cc:27: CLSID_TF_CategoryMgr, NULL, CLSCTX_INPROC_SERVER))) { On 2012/08/23 04:10:27, Yohei Yukawa wrote: > Can we omit parameters as follows? > category_manager_.CreateInstance(CLSID_TF_CategoryMgr) Done. https://chromiumcodereview.appspot.com/10831403/diff/18002/ui/base/win/tsf_te... ui/base/win/tsf_text_store.cc:31: if (FAILED(display_attribute_manager_.CreateInstance( On 2012/08/23 04:10:27, Yohei Yukawa wrote: > ditto. Done. https://chromiumcodereview.appspot.com/10831403/diff/18002/ui/base/win/tsf_te... ui/base/win/tsf_text_store.cc:213: acp_end = std::min(acp_end, acp_start + (int)text_buffer_size); On 2012/08/23 04:10:27, Yohei Yukawa wrote: > static_cast<LONG>(text_buffer_size) Done. https://chromiumcodereview.appspot.com/10831403/diff/18002/ui/base/win/tsf_te... ui/base/win/tsf_text_store.cc:214: *text_buffer_copied = acp_end - acp_start; On 2012/08/23 04:10:27, Yohei Yukawa wrote: > Please check if acp_start <= acp_end. Done. https://chromiumcodereview.appspot.com/10831403/diff/18002/ui/base/win/tsf_te... ui/base/win/tsf_text_store.cc:216: const string16 result = string_buffer_.substr(acp_start, *text_buffer_copied); On 2012/08/23 04:10:27, Yohei Yukawa wrote: > const string16& result Done. https://chromiumcodereview.appspot.com/10831403/diff/18002/ui/base/win/tsf_te... ui/base/win/tsf_text_store.cc:332: On 2012/08/23 04:10:27, Yohei Yukawa wrote: > const LONG new_end_pos = start_pos + text_buffer_size; Done. https://chromiumcodereview.appspot.com/10831403/diff/18002/ui/base/win/tsf_te... File ui/base/win/tsf_text_store.h (right): https://chromiumcodereview.appspot.com/10831403/diff/18002/ui/base/win/tsf_te... ui/base/win/tsf_text_store.h:190: HWND hwnd_; On 2012/08/23 04:10:27, Yohei Yukawa wrote: > window_handle_ might be better. Done.
lgtm http://codereview.chromium.org/10831403/diff/12004/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/12004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:365: text_change->acpNewEnd = start_pos + text_buffer_size; text_change->acpNewEnd = new_end_pos;
https://chromiumcodereview.appspot.com/10831403/diff/12004/ui/base/win/tsf_te... File ui/base/win/tsf_text_store.cc (right): https://chromiumcodereview.appspot.com/10831403/diff/12004/ui/base/win/tsf_te... ui/base/win/tsf_text_store.cc:365: text_change->acpNewEnd = start_pos + text_buffer_size; On 2012/08/23 06:23:16, Yohei Yukawa wrote: > text_change->acpNewEnd = new_end_pos; Done.
lgtm http://codereview.chromium.org/10831403/diff/19005/ui/base/win/tsf_text_store... File ui/base/win/tsf_text_store_unittest.cc (right): http://codereview.chromium.org/10831403/diff/19005/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store_unittest.cc:107: text_store_->AdviseSink(IID_ITextStoreACPSink, Indent?
https://chromiumcodereview.appspot.com/10831403/diff/19005/ui/base/win/tsf_te... File ui/base/win/tsf_text_store_unittest.cc (right): https://chromiumcodereview.appspot.com/10831403/diff/19005/ui/base/win/tsf_te... ui/base/win/tsf_text_store_unittest.cc:107: text_store_->AdviseSink(IID_ITextStoreACPSink, On 2012/08/23 10:16:44, Yohei Yukawa wrote: > Indent? Done.
cpu@ and sky@ Could you please review this CL. Thank you.
LGTM
http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:7: #include <OleCtl.h> space between the system include and the other includes, please look at the style guide. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:15: const TsViewCookie kViewCookie = 1; spaces in the braces for namespace, please look at the style guide. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:102: return S_OK; if we don't support why return S_OK? http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:146: // We don't support GetFormattedText. remove this comment http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:186: status->dwStaticFlags = TS_SS_NOHIDDENTEXT; a comment about why we return this would be nice. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:209: acp_end = static_cast<LONG>(string_buffer_.size()); please use a LONG variable with string_buffer_.size() http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:216: acp_end = std::min(acp_end, acp_start + static_cast<LONG>(text_buffer_size)); is the cast of text_buffer_size necessary? http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:421: if (!text_store_acp_sink_.get()) this code is not really touched by multiple threads, right? http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:22: // TsfTextStore is used to interact with the system input method via TSF. can you please put an extended comment about what this class does and which is the general flow of members called? Also some basic explanation about the locking scheme that you implement would be helpful. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:198: // example: "aoi" is committed, and "umi" is under composition. Example: http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:205: // example: "iue" is selected Example:
http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... File ui/base/win/tsf_text_store_unittest.cc (right): http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store_unittest.cc:264: } how aboout testing also when you call RequestLock(TS_LF_READWRITE, ) RequestLock(TS_LF_READ, ) Also looking at the code it seems to support nesting, like TS_LF_READ TS_LF_READ TS_LF_READWRITE http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store_unittest.cc:426: TEST_F(TsfTextStoreTest, SenarioTest) { Scenario ?
Thank you for your review! http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store.cc File ui/base/win/tsf_text_store.cc (right): http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:7: #include <OleCtl.h> On 2012/08/23 18:37:47, cpu wrote: > space between the system include and the other includes, please look at the > style guide. Done. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:15: const TsViewCookie kViewCookie = 1; On 2012/08/23 18:37:47, cpu wrote: > spaces in the braces for namespace, please look at the style guide. Done. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:102: return S_OK; On 2012/08/23 18:37:47, cpu wrote: > if we don't support why return S_OK? This return value means "Not found". http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:146: // We don't support GetFormattedText. On 2012/08/23 18:37:47, cpu wrote: > remove this comment Done. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:186: status->dwStaticFlags = TS_SS_NOHIDDENTEXT; On 2012/08/23 18:37:47, cpu wrote: > a comment about why we return this would be nice. Done. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:209: acp_end = static_cast<LONG>(string_buffer_.size()); On 2012/08/23 18:37:47, cpu wrote: > please use a LONG variable with string_buffer_.size() Done. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:216: acp_end = std::min(acp_end, acp_start + static_cast<LONG>(text_buffer_size)); On 2012/08/23 18:37:47, cpu wrote: > is the cast of text_buffer_size necessary? We need this cast because text_buffer_size is unsigned. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.cc:421: if (!text_store_acp_sink_.get()) On 2012/08/23 18:37:47, cpu wrote: > this code is not really touched by multiple threads, right? TsfTextStore is in Single-Threaded Apartment. So this code is touched by only one thread. But RequestLock() may be called recursively in OnLockGranted() or OnSelectionChange() or OnLayoutChange() or OnTextChange(). http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:22: // TsfTextStore is used to interact with the system input method via TSF. On 2012/08/23 18:37:47, cpu wrote: > can you please put an extended comment about what this class does and which is > the general flow of members called? > > Also some basic explanation about the locking scheme that you implement would be > helpful. Done. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:198: // example: "aoi" is committed, and "umi" is under composition. On 2012/08/23 18:37:47, cpu wrote: > Example: Done. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:205: // example: "iue" is selected On 2012/08/23 18:37:47, cpu wrote: > Example: Done. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... File ui/base/win/tsf_text_store_unittest.cc (right): http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store_unittest.cc:264: } On 2012/08/23 19:36:45, cpu wrote: > how aboout testing also when you call > > RequestLock(TS_LF_READWRITE, ) > RequestLock(TS_LF_READ, ) > > Also looking at the code it seems to support nesting, like > TS_LF_READ > TS_LF_READ > TS_LF_READWRITE Done. http://codereview.chromium.org/10831403/diff/20004/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store_unittest.cc:426: TEST_F(TsfTextStoreTest, SenarioTest) { On 2012/08/23 19:36:45, cpu wrote: > Scenario ? Done.
lgtm Thanks for the explanation on the .h file. http://codereview.chromium.org/10831403/diff/21005/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/21005/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:31: // - The user enter "a". the user enters 'a". same for line 44, 55 http://codereview.chromium.org/10831403/diff/21005/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:70: // About locking scheme: About the locking scheme:
Thank you very much! http://codereview.chromium.org/10831403/diff/21005/ui/base/win/tsf_text_store.h File ui/base/win/tsf_text_store.h (right): http://codereview.chromium.org/10831403/diff/21005/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:31: // - The user enter "a". On 2012/08/24 21:12:44, cpu wrote: > the user enters 'a". > > same for line 44, 55 Done. http://codereview.chromium.org/10831403/diff/21005/ui/base/win/tsf_text_store... ui/base/win/tsf_text_store.h:70: // About locking scheme: On 2012/08/24 21:12:44, cpu wrote: > About the locking scheme: Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/horo@chromium.org/10831403/12006
Change committed as 153453 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
