Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2119813002:
views::Textfield: Implement yank editing command. (Closed)
|
Created:
4 years, 5 months ago by karandeepb Modified:
4 years, 4 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, yusukes+watch_chromium.org, tfarina, shuchen+watch_chromium.org, nona+watch_chromium.org, chromium-apps-reviews_chromium.org, James Su, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionviews::Textfield: Implement yank editing command.
Like Emacs, textfields in Cocoa have a notion of a kill buffer. Certain deletion
commands (delete to beginning/end of line/paragraph) add the deleted text to the
kill buffer. All textfields in an application share the same kill buffer. This
is distinct from the system clipboard. Executing yank pastes the text in the
kill buffer at the insertion point or selection.
This CL implements the yank editing command for views::Textfield. It only has a
key binding defined on MacViews.
BUG=586985
TEST=Enable MacViews. Give focus to a MacViews textfield. Enter some text. Press
Ctrl+K to delete to end of paragraph. Verify pressing Ctrl+Y pastes the deleted
text.
Committed: https://crrev.com/7fff7c5c481c9a85495cef39be0b475dbde91da0
Cr-Commit-Position: refs/heads/master@{#407722}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add a comment. #
Total comments: 1
Patch Set 3 : Rebase. #Patch Set 4 #
Total comments: 12
Patch Set 5 : Rebase. #Patch Set 6 : Address review comments. #
Total comments: 12
Patch Set 7 : Rebase. #Patch Set 8 : Address review. #
Total comments: 3
Patch Set 9 : Address review comments. #
Total comments: 20
Patch Set 10 : Address review. #Messages
Total messages: 48 (28 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Yank2 ========== to ========== views::Textfield: Implement yank editing command. Like Emacs, textfields in Cocoa have a notion of a kill buffer. Certain deletion commands (delete to beginning/end of line/paragraph/document) add the deleted text to the kill buffer. All textfields in an application share the same kill buffer. This is distinct from the system clipboard. Executing yank pastes the text in the kill buffer at the insertion point or selection. This CL implements the yank editing command for views::Textfield. It only has a key binding defined on MacViews. BUG=586985 TEST=Enable MacViews. Give focus to a MacViews textfield. Enter some text. Press Ctrl+K to delete to end of paragraph. Verify pressing Ctrl+Y pastes the deleted text. ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. https://codereview.chromium.org/2119813002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2119813002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:1034: keyCode:ui::VKEY_Y Again is it ok to give this key code for Ctrl+Y? Before executing the scheduled command, Textfield::OnKeyPressed passes the key event to TextfieldController::HandleKeyEvent. Since Ctrl+Y can be interpreted in different ways, I may be safer to give this a dummy key code and event flags value. https://codereview.chromium.org/2119813002/diff/20001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2119813002/diff/20001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1552: add_to_kill_buffer = true; Interestingly Command+X(Cut) also adds text to the kill buffer. However I can't find this documented anywhere and also Blink does not do this. https://codereview.chromium.org/2119813002/diff/40001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2119813002/diff/40001/ui/views/controls/textf... ui/views/controls/textfield/textfield_model.cc:260: Ideally this should persist across the whole app i.e. it should have also been shared with web contents. But even Safari doesn't do it.
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2119813002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2119813002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:914: EXPECT_NSEQ_3(nil, GetExpectedText(), GetActualText()); I think this will be flaky depending on the order that the tests are run -- global state isn't cleared out, so if a test that puts something in the kill buffer runs first, then yanking here could paste it. Same below. https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:259: base::string16 g_kill_buffer_; This will add a static initializer, which isn't allowed. I don't *think* there are thread safety issues, so something like the following probably has the least overhead base::string16& GetKillBuffer() { CR_DEFINE_STATIC_LOCAL(base::string16, kill_buffer, ()); DCHECK(base::MessageLoopForUI::IsCurrent()); return kill_buffer; } An alternative to this is to place the buffer as a data member of views::ViewsDelegate. And, in fact, this may even be a requirement since toolkit-views technically has some threading support (it might get used for Mus.. but I doubt it will come up for Mac). A data member of ViewsDelegate also has the advantage of not leaving around global state in tests :) https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.h (right): https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.h:101: bool Delete(bool add_to_kill_buffer); default arguments are allowed by the style guide now - I think that's better here, and below https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1842: // Mac, send Cmd+Shift+Z. The "So, on Mac.." doesn't fit any more... something weird happened during review. It should say something like "So, on Mac, Cmd+Shift+Z is sent for the tests above and the Ctrl+Y test below is skipped." https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1866: TEST_F(TextfieldTest, Yank) { Same as before - we should test "Yank" in textfield_model_unittest, since the code is "there" for all platforms - it can be a simpler test though. It makes sense to test the keybindings, here, for Mac as well - so this is fine. https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1910: #endif nit: // defined(OS_MACOSX)
Patchset #5 (id:110001) has been deleted
Description was changed from ========== views::Textfield: Implement yank editing command. Like Emacs, textfields in Cocoa have a notion of a kill buffer. Certain deletion commands (delete to beginning/end of line/paragraph/document) add the deleted text to the kill buffer. All textfields in an application share the same kill buffer. This is distinct from the system clipboard. Executing yank pastes the text in the kill buffer at the insertion point or selection. This CL implements the yank editing command for views::Textfield. It only has a key binding defined on MacViews. BUG=586985 TEST=Enable MacViews. Give focus to a MacViews textfield. Enter some text. Press Ctrl+K to delete to end of paragraph. Verify pressing Ctrl+Y pastes the deleted text. ========== to ========== views::Textfield: Implement yank editing command. Like Emacs, textfields in Cocoa have a notion of a kill buffer. Certain deletion commands (delete to beginning/end of line/paragraph) add the deleted text to the kill buffer. All textfields in an application share the same kill buffer. This is distinct from the system clipboard. Executing yank pastes the text in the kill buffer at the insertion point or selection. This CL implements the yank editing command for views::Textfield. It only has a key binding defined on MacViews. BUG=586985 TEST=Enable MacViews. Give focus to a MacViews textfield. Enter some text. Press Ctrl+K to delete to end of paragraph. Verify pressing Ctrl+Y pastes the deleted text. ==========
Description was changed from ========== views::Textfield: Implement yank editing command. Like Emacs, textfields in Cocoa have a notion of a kill buffer. Certain deletion commands (delete to beginning/end of line/paragraph) add the deleted text to the kill buffer. All textfields in an application share the same kill buffer. This is distinct from the system clipboard. Executing yank pastes the text in the kill buffer at the insertion point or selection. This CL implements the yank editing command for views::Textfield. It only has a key binding defined on MacViews. BUG=586985 TEST=Enable MacViews. Give focus to a MacViews textfield. Enter some text. Press Ctrl+K to delete to end of paragraph. Verify pressing Ctrl+Y pastes the deleted text. ========== to ========== views::Textfield: Implement yank editing command. Like Emacs, textfields in Cocoa have a notion of a kill buffer. Certain deletion commands (delete to beginning/end of line/paragraph) add the deleted text to the kill buffer. All textfields in an application share the same kill buffer. This is distinct from the system clipboard. Executing yank pastes the text in the kill buffer at the insertion point or selection. This CL implements the yank editing command for views::Textfield. It only has a key binding defined on MacViews. BUG=586985 TEST=Enable MacViews. Give focus to a MacViews textfield. Enter some text. Press Ctrl+K to delete to end of paragraph. Verify pressing Ctrl+Y pastes the deleted text. ==========
Patchset #5 (id:130001) has been deleted
PTAL Trent. Thanks. https://codereview.chromium.org/2119813002/diff/100001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2119813002/diff/100001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:914: EXPECT_NSEQ_3(nil, GetExpectedText(), GetActualText()); On 2016/07/06 01:46:37, tapted wrote: > I think this will be flaky depending on the order that the tests are run -- > global state isn't cleared out, so if a test that puts something in the kill > buffer runs first, then yanking here could paste it. Same below. Using ViewsDelegate for storing yanked text should clear the global state. https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:259: base::string16 g_kill_buffer_; On 2016/07/06 01:46:37, tapted wrote: > This will add a static initializer, which isn't allowed. I don't *think* there > are thread safety issues, so something like the following probably has the least > overhead > > base::string16& GetKillBuffer() { > CR_DEFINE_STATIC_LOCAL(base::string16, kill_buffer, ()); > DCHECK(base::MessageLoopForUI::IsCurrent()); > return kill_buffer; > } > > An alternative to this is to place the buffer as a data member of > views::ViewsDelegate. And, in fact, this may even be a requirement since > toolkit-views technically has some threading support (it might get used for > Mus.. but I doubt it will come up for Mac). A data member of ViewsDelegate also > has the advantage of not leaving around global state in tests :) Added as a member of ViewsDelegate. https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.h (right): https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.h:101: bool Delete(bool add_to_kill_buffer); On 2016/07/06 01:46:37, tapted wrote: > default arguments are allowed by the style guide now - I think that's better > here, and below Done. https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1842: // Mac, send Cmd+Shift+Z. On 2016/07/06 01:46:37, tapted wrote: > The "So, on Mac.." doesn't fit any more... something weird happened during > review. It should say something like "So, on Mac, Cmd+Shift+Z is sent for the > tests above and the Ctrl+Y test below is skipped." Done. https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1866: TEST_F(TextfieldTest, Yank) { On 2016/07/06 01:46:37, tapted wrote: > Same as before - we should test "Yank" in textfield_model_unittest, since the > code is "there" for all platforms - it can be a simpler test though. It makes > sense to test the keybindings, here, for Mac as well - so this is fine. Done. https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1910: #endif On 2016/07/06 01:46:37, tapted wrote: > nit: // defined(OS_MACOSX) Done.
lgtm % nits https://codereview.chromium.org/2119813002/diff/170001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2119813002/diff/170001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:274: if(ViewsDelegate::GetInstance()) nit: space after `if` https://codereview.chromium.org/2119813002/diff/170001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:582: if(!ViewsDelegate::GetInstance()) nit: space after `if` (but also can this be reached in the current code or tests? it's probably better just to hit the DCHECK if something invokes Yank without a ViewsDelegate) https://codereview.chromium.org/2119813002/diff/170001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2119813002/diff/170001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1911: #endif // defined(OS_MACOSX) nit: extra space before // https://codereview.chromium.org/2119813002/diff/170001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2119813002/diff/170001/ui/views/views_delegat... ui/views/views_delegate.h:105: const base::string16& kill_buffer() { nit: declare method const https://codereview.chromium.org/2119813002/diff/170001/ui/views/views_delegat... ui/views/views_delegate.h:109: void set_kill_buffer(const base::string16& kill_buffer) { nit: swap these around and remove the blank line between to be consistent with native_widget_factory. It should have a brief comment too, like "Access the text buffer used to implement the "Yank" text edit command." https://codereview.chromium.org/2119813002/diff/170001/ui/views/views_delegat... ui/views/views_delegate.h:217: // Views::Texfield. Member of ViewsDelegate since this needs to be persisted nit: Views -> views
Patchset #8 (id:210001) has been deleted
https://codereview.chromium.org/2119813002/diff/170001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2119813002/diff/170001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:274: if(ViewsDelegate::GetInstance()) On 2016/07/20 06:32:21, tapted wrote: > nit: space after `if` Done. Forgot to run git cl format! https://codereview.chromium.org/2119813002/diff/170001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:582: if(!ViewsDelegate::GetInstance()) On 2016/07/20 06:32:21, tapted wrote: > nit: space after `if` (but also can this be reached in the current code or > tests? it's probably better just to hit the DCHECK if something invokes Yank > without a ViewsDelegate) The ViewsDelegate is initialized in ChromeBrowserMainExtraPartsViews::ToolkitInitialized() and so probably this can't be reached. But I think this is safer. Most places in the code base also use an if-check before accessing the ViewsDelegate instance. https://codereview.chromium.org/2119813002/diff/170001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2119813002/diff/170001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1911: #endif // defined(OS_MACOSX) On 2016/07/20 06:32:21, tapted wrote: > nit: extra space before // Done. https://codereview.chromium.org/2119813002/diff/170001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2119813002/diff/170001/ui/views/views_delegat... ui/views/views_delegate.h:105: const base::string16& kill_buffer() { On 2016/07/20 06:32:21, tapted wrote: > nit: declare method const Done. https://codereview.chromium.org/2119813002/diff/170001/ui/views/views_delegat... ui/views/views_delegate.h:109: void set_kill_buffer(const base::string16& kill_buffer) { On 2016/07/20 06:32:21, tapted wrote: > nit: swap these around and remove the blank line between to be consistent with > native_widget_factory. It should have a brief comment too, like "Access the text > buffer used to implement the "Yank" text edit command." Done. https://codereview.chromium.org/2119813002/diff/170001/ui/views/views_delegat... ui/views/views_delegate.h:217: // Views::Texfield. Member of ViewsDelegate since this needs to be persisted On 2016/07/20 06:32:21, tapted wrote: > nit: Views -> views Done.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
karandeepb@chromium.org changed reviewers: + msw@chromium.org, sky@chromium.org
+msw for ui/views/controls/textfield review. +sky for ui/views/views_delegate.h and ui/base review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2119813002/diff/230001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2119813002/diff/230001/ui/views/views_delegat... ui/views/views_delegate.h:106: void set_kill_buffer(const base::string16& kill_buffer) { Is there a reason this needs to be on ViewsDelegate and not a static (base::string16*) in TextfieldModel? AFAICT it's only used by TextfieldModel.
PTAL sky@. https://codereview.chromium.org/2119813002/diff/230001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2119813002/diff/230001/ui/views/views_delegat... ui/views/views_delegate.h:106: void set_kill_buffer(const base::string16& kill_buffer) { On 2016/07/20 15:45:57, sky wrote: > Is there a reason this needs to be on ViewsDelegate and not a static > (base::string16*) in TextfieldModel? AFAICT it's only used by TextfieldModel. The cpp style guide forbids static storage duration variables unless they are constexpr, owing to thread safety issues - https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#.... If thread safety is not an issue here, I can change it to a function static variable residing in textfield_model.cc. Though another benefit of adding it to ViewsDelegate is that we don't need to clear global state between different tests.
https://codereview.chromium.org/2119813002/diff/230001/ui/views/views_delegate.h File ui/views/views_delegate.h (right): https://codereview.chromium.org/2119813002/diff/230001/ui/views/views_delegat... ui/views/views_delegate.h:106: void set_kill_buffer(const base::string16& kill_buffer) { On 2016/07/21 00:44:58, karandeepb wrote: > On 2016/07/20 15:45:57, sky wrote: > > Is there a reason this needs to be on ViewsDelegate and not a static > > (base::string16*) in TextfieldModel? AFAICT it's only used by TextfieldModel. > > The cpp style guide forbids static storage duration variables unless they are > constexpr, owing to thread safety issues - > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#.... > If thread safety is not an issue here, I can change it to a function static > variable residing in textfield_model.cc. > > Though another benefit of adding it to ViewsDelegate is that we don't need to > clear global state between different tests. this came up in review earlier at https://codereview.chromium.org/2119813002/diff/100001/ui/views/controls/text... Chromium gets around the "no non-POD types" with things like CR_DEFINE_STATIC_LOCAL, which I offered in a code snippet in that review comment (a nullptr string16* `new`ed on first use would also work). So the style-guide isn't really a deal-breaker, but ViewsDelegate seemed like an elegant way to partition the sharing-between-textfields behaviour without global state leaking between tests. We could also do something like having the ViewsTestHelper destructor do ~ViewsTestHelper() { TextFieldModel::ClearKillBufferForTest(); } (i.e. along with exposing the static ClearKillBufferForTest() method somewhere). Depends which one sky prefers I guess :)
On Wed, Jul 20, 2016 at 5:44 PM, <karandeepb@chromium.org> wrote: > PTAL sky@. > > > https://codereview.chromium.org/2119813002/diff/230001/ui/views/views_delegate.h > File ui/views/views_delegate.h (right): > > https://codereview.chromium.org/2119813002/diff/230001/ui/views/views_delegat... > ui/views/views_delegate.h:106: void set_kill_buffer(const > base::string16& kill_buffer) { > On 2016/07/20 15:45:57, sky wrote: >> Is there a reason this needs to be on ViewsDelegate and not a static >> (base::string16*) in TextfieldModel? AFAICT it's only used by > TextfieldModel. > > The cpp style guide forbids static storage duration variables unless > they are constexpr, owing to thread safety issues - > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#.... > If thread safety is not an issue here, I can change it to a function > static variable residing in textfield_model.cc. Views isn't really thread safe at the moment, so the static is fine, or CR_DEFINE_STATIC_LOCAL as Trent suggests. ViewsDelegate is intended as a way to communicate with the embedder. Your usage isn't a good fit for that. A better fit is aura::Env, which implements SupportUserData, but mac isn't using aura. -Scott > > Though another benefit of adding it to ViewsDelegate is that we don't > need to clear global state between different tests. > > https://codereview.chromium.org/2119813002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Patchset #9 (id:250001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:270001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> Views isn't really thread safe at the moment, so the static is fine, > or CR_DEFINE_STATIC_LOCAL as Trent suggests. ViewsDelegate is intended > as a way to communicate with the embedder. Your usage isn't a good fit > for that. A better fit is aura::Env, which implements SupportUserData, > but mac isn't using aura. Acknowledged. Changed code to use CR_DEFINE_STATIC_LOCAL. PTAL sky@. Also ping msw@ for ui/views/controls/textfield review.
https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:273: base::string16& GetKillBuffer() { return a pointer (references are generally restricted to const). https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.h (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.h:233: static void ClearKillBufferForTesting(); How about friending the helper and moving this to private? Having a bunch of FooForTesting results in an ugly API.
https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1557: add_to_kill_buffer = true; Should this still be true if there's a selection and the command changes to DELETE_FORWARD below? Also, should this only ever be true on Mac? https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:387: size_t previous_char = gfx::UTF16OffsetToIndex(text(), cursor_position, -1); optional nit: |previous_grapheme_index| for consistency with code above. https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:590: if (!kill_buffer.empty() || HasSelection()) { Why would this try to insert an empty string if there's a selection? https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.h (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.h:101: bool Delete(bool add_to_kill_buffer = false); I thought chromium generally discouraged default parameters, but I don't see anything in our style guide, and the Google style guide says they're allowed here (albeit with a long list of cons), so I guess this is fine. https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.h:188: // selection. nit: What does "or selection" mean? Just that a current selection would be replaced, similar to the standard paste behavior? If so, you can probably nix that tidbit, otherwise please clarify. https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1903: EXPECT_STR_EQ("", textfield2->text()); nit: EXPECT_TRUE(textfield2->text().empty()) https://codereview.chromium.org/2119813002/diff/290001/ui/views/test/views_te... File ui/views/test/views_test_helper.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/test/views_te... ui/views/test/views_test_helper.cc:16: TextfieldModel::ClearKillBufferForTesting(); Would it be sufficient to clear this in Textfield[Model]Test::TearDown, or at the start/end of the yank text fixtures? Other unit test fixtures that actually use yank could do the same, and any other unit test fixtures that don't actually use yank shouldn't be affected, right?
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL msw@ and sky@. Thanks. https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1557: add_to_kill_buffer = true; On 2016/07/22 17:44:03, msw wrote: > Should this still be true if there's a selection and the command changes to > DELETE_FORWARD below? Also, should this only ever be true on Mac? Yes to both. https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:273: base::string16& GetKillBuffer() { On 2016/07/22 15:22:49, sky wrote: > return a pointer (references are generally restricted to const). Done. https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:387: size_t previous_char = gfx::UTF16OffsetToIndex(text(), cursor_position, -1); On 2016/07/22 17:44:03, msw wrote: > optional nit: |previous_grapheme_index| for consistency with code above. Done. https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:590: if (!kill_buffer.empty() || HasSelection()) { On 2016/07/22 17:44:03, msw wrote: > Why would this try to insert an empty string if there's a selection? That's how Yank works on Mac/Cocoa. Open a new Chrome app instance, select some text and press Ctrl+Y to yank. The selected text will get deleted (since kill buffer is empty). https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.h (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.h:188: // selection. On 2016/07/22 17:44:03, msw wrote: > nit: What does "or selection" mean? Just that a current selection would be > replaced, similar to the standard paste behavior? If so, you can probably nix > that tidbit, otherwise please clarify. Done. Yeah it follows the standard paste behavior. https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.h:233: static void ClearKillBufferForTesting(); On 2016/07/22 15:22:49, sky wrote: > How about friending the helper and moving this to private? Having a bunch of > FooForTesting results in an ugly API. Done. But friending BridgedNativeWidgetTest here seems slightly ugly. https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1903: EXPECT_STR_EQ("", textfield2->text()); On 2016/07/22 17:44:03, msw wrote: > nit: EXPECT_TRUE(textfield2->text().empty()) Done. https://codereview.chromium.org/2119813002/diff/290001/ui/views/test/views_te... File ui/views/test/views_test_helper.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/test/views_te... ui/views/test/views_test_helper.cc:16: TextfieldModel::ClearKillBufferForTesting(); On 2016/07/22 17:44:03, msw wrote: > Would it be sufficient to clear this in Textfield[Model]Test::TearDown, or at > the start/end of the yank text fixtures? Other unit test fixtures that actually > use yank could do the same, and any other unit test fixtures that don't actually > use yank shouldn't be affected, right? Done.
LGTM https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.h (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.h:233: static void ClearKillBufferForTesting(); On 2016/07/25 06:38:11, karandeepb wrote: > On 2016/07/22 15:22:49, sky wrote: > > How about friending the helper and moving this to private? Having a bunch of > > FooForTesting results in an ugly API. > > Done. But friending BridgedNativeWidgetTest here seems slightly ugly. Another option is to friend a class that is used by all test. These are commonly called TestApi, eg TextfieldModelTestApi.
lgtm https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:590: if (!kill_buffer.empty() || HasSelection()) { On 2016/07/25 06:38:11, karandeepb wrote: > On 2016/07/22 17:44:03, msw wrote: > > Why would this try to insert an empty string if there's a selection? > > That's how Yank works on Mac/Cocoa. Open a new Chrome app instance, select some > text and press Ctrl+Y to yank. The selected text will get deleted (since kill > buffer is empty). Ah, so it's just doing a delete in that case? Okay.
https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... File ui/views/controls/textfield/textfield_model.cc (right): https://codereview.chromium.org/2119813002/diff/290001/ui/views/controls/text... ui/views/controls/textfield/textfield_model.cc:590: if (!kill_buffer.empty() || HasSelection()) { On 2016/07/25 18:35:49, msw wrote: > On 2016/07/25 06:38:11, karandeepb wrote: > > On 2016/07/22 17:44:03, msw wrote: > > > Why would this try to insert an empty string if there's a selection? > > > > That's how Yank works on Mac/Cocoa. Open a new Chrome app instance, select > some > > text and press Ctrl+Y to yank. The selected text will get deleted (since kill > > buffer is empty). > > Ah, so it's just doing a delete in that case? Okay. Yeah!
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2119813002/#ps310001 (title: "Address review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== views::Textfield: Implement yank editing command. Like Emacs, textfields in Cocoa have a notion of a kill buffer. Certain deletion commands (delete to beginning/end of line/paragraph) add the deleted text to the kill buffer. All textfields in an application share the same kill buffer. This is distinct from the system clipboard. Executing yank pastes the text in the kill buffer at the insertion point or selection. This CL implements the yank editing command for views::Textfield. It only has a key binding defined on MacViews. BUG=586985 TEST=Enable MacViews. Give focus to a MacViews textfield. Enter some text. Press Ctrl+K to delete to end of paragraph. Verify pressing Ctrl+Y pastes the deleted text. ========== to ========== views::Textfield: Implement yank editing command. Like Emacs, textfields in Cocoa have a notion of a kill buffer. Certain deletion commands (delete to beginning/end of line/paragraph) add the deleted text to the kill buffer. All textfields in an application share the same kill buffer. This is distinct from the system clipboard. Executing yank pastes the text in the kill buffer at the insertion point or selection. This CL implements the yank editing command for views::Textfield. It only has a key binding defined on MacViews. BUG=586985 TEST=Enable MacViews. Give focus to a MacViews textfield. Enter some text. Press Ctrl+K to delete to end of paragraph. Verify pressing Ctrl+Y pastes the deleted text. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:310001)
Message was sent while issue was closed.
Description was changed from ========== views::Textfield: Implement yank editing command. Like Emacs, textfields in Cocoa have a notion of a kill buffer. Certain deletion commands (delete to beginning/end of line/paragraph) add the deleted text to the kill buffer. All textfields in an application share the same kill buffer. This is distinct from the system clipboard. Executing yank pastes the text in the kill buffer at the insertion point or selection. This CL implements the yank editing command for views::Textfield. It only has a key binding defined on MacViews. BUG=586985 TEST=Enable MacViews. Give focus to a MacViews textfield. Enter some text. Press Ctrl+K to delete to end of paragraph. Verify pressing Ctrl+Y pastes the deleted text. ========== to ========== views::Textfield: Implement yank editing command. Like Emacs, textfields in Cocoa have a notion of a kill buffer. Certain deletion commands (delete to beginning/end of line/paragraph) add the deleted text to the kill buffer. All textfields in an application share the same kill buffer. This is distinct from the system clipboard. Executing yank pastes the text in the kill buffer at the insertion point or selection. This CL implements the yank editing command for views::Textfield. It only has a key binding defined on MacViews. BUG=586985 TEST=Enable MacViews. Give focus to a MacViews textfield. Enter some text. Press Ctrl+K to delete to end of paragraph. Verify pressing Ctrl+Y pastes the deleted text. Committed: https://crrev.com/7fff7c5c481c9a85495cef39be0b475dbde91da0 Cr-Commit-Position: refs/heads/master@{#407722} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/7fff7c5c481c9a85495cef39be0b475dbde91da0 Cr-Commit-Position: refs/heads/master@{#407722} |
