Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
329463002:
MacViews: Implement text input. (Closed)
|
Created:
6 years, 6 months ago by Andre Modified:
3 years, 5 months ago CC:
chromium-reviews, tfarina, ben+views_chromium.org, tdanderson+views_chromium.org, mac-views-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMacViews: Implement text input.
Implement text input for MacViews by making BridgedContentView conform
to NSTextInputClient protocol. Keyboard events are sent to the system input
method to be interpreted, and the resulting NSTextInputClient calls are
translated and forwarded to the ui::TextInputClient of the widget's focused View.
BUG=374077
TEST=BridgedNativeWidgetTest
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279806
Patch Set 1 #
Total comments: 66
Patch Set 2 : Fixes for tapted #
Total comments: 13
Patch Set 3 : More changes for tapted. #
Total comments: 7
Patch Set 4 #Patch Set 5 : Rebase. Fix FocusChangeListener #
Total comments: 1
Patch Set 6 : Rebase to master #
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.h:25: ui::TextInputClient* textInputClient_; This needs a comment, along with something about who owns it. // Weak. If non-null the current something. Owned by a View in the hierarchy rooted at |hostedView_|. (if that's all true! I'm guessing ;)) https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.h:36: - (void)setTextInputClient:(ui::TextInputClient*)textInputClient; you could probably make this a property. If it's a function, it needs a comment (go/objcstyle says so ;) https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:50: textInputClient_ = textInputClient; Does toolkit-views guarantee to send OnDidChangeFocus away from the View when it's destroyed and takes its TextInputClient away with it? https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:71: - (void)doCommandByID:(int)cmd_id { This should be declared in BridgedContentView () with a comment and, nit: cmd_id -> commandId (no abbreviations). Or, just `id` is probably fine too. resourceId might be another option, to make it clear that it's an ID coming from grit https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:100: [self interpretKeyEvents:@[ theEvent ]]; Do you think interpretKeyEvents will be needed for some cases where textInputClient_ is NULL? I actually couldn't decide how to tie in interpretKeyEvents when I added keyUp/keyDown. Is your approach similar to how the renderer does it? https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:139: - (NSAttributedString*)attributedSubstringForProposedRange:(NSRange)range ugh longmethodnames. is this what clang-format does? I'd probably try breaking after the return type and aligning colons https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:141: base::string16 text; is there a better name for `text` - `substring` maybe? https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:159: - (void)doCommandBySelector:(SEL)selector { Should be under `// NSResponder implementation.`. (but why was this needed - I thought the AppKit NSResponder dispatcher took care of this https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:166: // FIXME(andresantoso): ui::TextInputClient does not expose this. I don't typically see FIXME comments. TODO is usually fine though https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:176: - (void)insertText:(id)text replacementRange:(NSRange)replacementRange { nit: I think method signatures "always" linebreak between arguments in chromium objc https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:177: if ([text isKindOfClass:[NSAttributedString class]]) Before this, you should do if (!textInputClient_) return; https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:186: gfx::Range range; if (!textInputClient_) return NSMakeRange(0, 0); more below. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... File ui/views/cocoa/bridged_native_widget.h (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... ui/views/cocoa/bridged_native_widget.h:41: // FocusChangeListener: convention around views seems to lean towards // Overridden from FocusChangeListener: (I wish there was a style rule for this :/) https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... ui/views/cocoa/bridged_native_widget.mm:8: #include "ui/views/cocoa/bridged_content_view.h" include -> import. Typically import is used if a header happens to be one that can't be compiled into a .cc file https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... ui/views/cocoa/bridged_native_widget.mm:30: [bridged_view_ view]->GetFocusManager()->RemoveFocusChangeListener(this); I think this belongs in [BridgedContentView clearView] (which can get an early return if already cleared). Of course, [BridgedContentView initWithView] should be symmetric with that, so the initializer will need to take a `BridgedNativeWidget* parent` argument. That was always the plan - it just wouldn't have been used yet the first time it landed. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... ui/views/cocoa/bridged_native_widget.mm:46: // FocusChangeListener implementation. I usually skip these comments for C++ implementations, since it's redundant from the declaration. Just need to keep everything in the same order. However, really big classes like views::View and NativeWidgetFoo tend to get a different style of comment. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... ui/views/cocoa/bridged_native_widget.mm:54: if (focused_now) nit: I'd probably write this ui::TextInputClient* input_client = focused_now ? focused_now->GetTextInputClient() : NULL; [bridged_view_ setTextInputClient:input_client]; I think that pattern is more common. And, since there's an NSTestInputClient and a ui::TextInputClient it helps clear things up a bit. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... File ui/views/cocoa/text_input_unittest.mm (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:5: #include "base/mac/scoped_nsobject.h" nit: import this and the other objc-only headers https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:16: static NSRange EmptyRange = NSMakeRange(NSNotFound, 0); non-primitive statics aren't usually allowed - http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo.... I'd probably make this a function in an anonymous namespace `const NSRange kEmptyRange = {NSNotFound, 0};` might avoid a static initializer too. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:19: EXPECT_EQ(a.location, b.location); \ I've seen EXPECT_TRUE(NSEqualRanges(a, b)) for stuff like this in Chrome https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:29: bridge_.reset([[BridgedContentView alloc] initWithView:textfield_.get()]); Somehting to consider - I've recently been finding out that event dispatch will be a lot different this way -- views::internal::RootView has a lot of logic that tweaks the way events are delivered to subviews, so it might be more authentic with a real RootView. Easiest way will be just to make a Widget I think. There are some functions in widget_test.h But, this direct approach might be fine too, since it's testing at a lower level. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:36: [bridge_ attributedSubstringForProposedRange:NSMakeRange(0, 1000) this 1000 should be a constant somewhere https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:44: }; nit: private: DISALLOW_COPY_AND_ASSIGN(..) (you'll need a default constructor too) https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:48: TEST_F(TextInputMacTest, AttributedSubstring) { nit: it's sometimes nice to have a comment before this describing each test too. But if the name of the test is descriptive enough it's sometimes skipped. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:49: NSRange range; Unless it's a constant, declarations are usually deferred to their first use and assigned at the same time. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:50: NSRange actualRange; actualRange -> actual_range, since it's not an objective C method https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:53: const char* str = "foo bar baz"; I'd usually see `const char kTestString[] = "foo bar baz";`. Or, since you use the length later, const std::string kTestString = "foo bar baz"; I don't see strlen too often in tests ;) https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:60: EXPECT_EQ(str, SysNSStringToUTF8([text string])); There's also EXPECT_NSEQ from testing/gtest_mac.h . Not sure if it gives better output here. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:88: NSString* str = @"foo"; `str` probably counts as an abbreviation, so not allowed by the style guide https://codereview.chromium.org/329463002/diff/1/ui/views/views.gyp File ui/views/views.gyp (right): https://codereview.chromium.org/329463002/diff/1/ui/views/views.gyp#newcode681 ui/views/views.gyp:681: 'cocoa/text_input_unittest.mm', hm - where's 'cocoa/bridged_native_widget_unittest.mm', gone? Since you're not adding a new file called text_input, it's probably more consistent to just add tests to cocoa/bridged_native_widget_unittest.mm, or add cocoa/bridged_content_view_unittest.mm - I'd probably lean towards the former, so you can get a bit more coverage. https://codereview.chromium.org/329463002/diff/1/ui/views/widget/native_widge... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/329463002/diff/1/ui/views/widget/native_widge... ui/views/widget/native_widget_mac.mm:212: return gfx::ScreenRectFromNSRect( hm - my next patch is looking like it won't need this again - it would end up being a "rider", (orthogonal to the CL). So, if you need it for this, I'd probably include it now - probably just ScreenRectFromNSRect with a comment in the header for it - since that was missing.
https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.h:25: ui::TextInputClient* textInputClient_; On 2014/06/17 13:23:56, tapted wrote: > This needs a comment, along with something about who owns it. > > // Weak. If non-null the current something. Owned by a View in the hierarchy > rooted at |hostedView_|. > > (if that's all true! I'm guessing ;)) Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.h:36: - (void)setTextInputClient:(ui::TextInputClient*)textInputClient; On 2014/06/17 13:23:56, tapted wrote: > you could probably make this a property. > > If it's a function, it needs a comment (go/objcstyle says so ;) Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:50: textInputClient_ = textInputClient; On 2014/06/17 13:23:56, tapted wrote: > Does toolkit-views guarantee to send OnDidChangeFocus away from the View when > it's destroyed and takes its TextInputClient away with it? Yes, that should be handled through Widget::ViewHierarchyChanged and FocusManager::ViewRemoved. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:71: - (void)doCommandByID:(int)cmd_id { On 2014/06/17 13:23:56, tapted wrote: > This should be declared in BridgedContentView () with a comment > > and, nit: cmd_id -> commandId (no abbreviations). Or, just `id` is probably fine > too. resourceId might be another option, to make it clear that it's an ID coming > from grit Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:100: [self interpretKeyEvents:@[ theEvent ]]; On 2014/06/17 13:23:56, tapted wrote: > Do you think interpretKeyEvents will be needed for some cases where > textInputClient_ is NULL? I actually couldn't decide how to tie in > interpretKeyEvents when I added keyUp/keyDown. Is your approach similar to how > the renderer does it? If textInputClient_ is NULL, I think we should forward the key event to the next responder by calling super. It will give other responders in the chain to process the event, and the event is not handled the system will emit a beep. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:139: - (NSAttributedString*)attributedSubstringForProposedRange:(NSRange)range On 2014/06/17 13:23:56, tapted wrote: > ugh longmethodnames. is this what clang-format does? I'd probably try breaking > after the return type and aligning colons Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:141: base::string16 text; On 2014/06/17 13:23:56, tapted wrote: > is there a better name for `text` - `substring` maybe? Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:159: - (void)doCommandBySelector:(SEL)selector { On 2014/06/17 13:23:56, tapted wrote: > Should be under `// NSResponder implementation.`. (but why was this needed - I > thought the AppKit NSResponder dispatcher took care of this doCommandBySelector is part of both NSResponder and NSTextInputClient. In this case, we are implementing the NSTextInputClient one so I don't think it should be under NSResponder implementation. I think you're right that NSResponder's default implementation probably takes care of this, but this is a required method in NSTextInputClient protocol, and the docs state that it should not call super. It probably should forward unhandled selectors to nextResponder though, so I've added code for that. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:166: // FIXME(andresantoso): ui::TextInputClient does not expose this. On 2014/06/17 13:23:56, tapted wrote: > I don't typically see FIXME comments. TODO is usually fine though Yep, that was a temporary experiment. I've changed it to NOTIMPLEMENTED for now. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:176: - (void)insertText:(id)text replacementRange:(NSRange)replacementRange { On 2014/06/17 13:23:56, tapted wrote: > nit: I think method signatures "always" linebreak between arguments in chromium > objc Hmm, I don't see that in objcstyle, and cocoa code in chrome/browser does not seem to do that. Where do you see the always linebreak convention? https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:177: if ([text isKindOfClass:[NSAttributedString class]]) On 2014/06/17 13:23:56, tapted wrote: > Before this, you should do > > if (!textInputClient_) > return; Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:186: gfx::Range range; On 2014/06/17 13:23:56, tapted wrote: > if (!textInputClient_) > return NSMakeRange(0, 0); > > more below. Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... File ui/views/cocoa/bridged_native_widget.h (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... ui/views/cocoa/bridged_native_widget.h:41: // FocusChangeListener: On 2014/06/17 13:23:56, tapted wrote: > convention around views seems to lean towards > > // Overridden from FocusChangeListener: > > (I wish there was a style rule for this :/) Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... ui/views/cocoa/bridged_native_widget.mm:8: #include "ui/views/cocoa/bridged_content_view.h" On 2014/06/17 13:23:56, tapted wrote: > include -> import. Typically import is used if a header happens to be one that > can't be compiled into a .cc file Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... ui/views/cocoa/bridged_native_widget.mm:30: [bridged_view_ view]->GetFocusManager()->RemoveFocusChangeListener(this); On 2014/06/17 13:23:56, tapted wrote: > I think this belongs in [BridgedContentView clearView] (which can get an early > return if already cleared). > > Of course, [BridgedContentView initWithView] should be symmetric with that, so > the initializer will need to take a `BridgedNativeWidget* parent` argument. That > was always the plan - it just wouldn't have been used yet the first time it > landed. Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... ui/views/cocoa/bridged_native_widget.mm:46: // FocusChangeListener implementation. On 2014/06/17 13:23:56, tapted wrote: > I usually skip these comments for C++ implementations, since it's redundant from > the declaration. Just need to keep everything in the same order. > > However, really big classes like views::View and NativeWidgetFoo tend to get a > different style of comment. Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_nativ... ui/views/cocoa/bridged_native_widget.mm:54: if (focused_now) On 2014/06/17 13:23:56, tapted wrote: > nit: I'd probably write this > > ui::TextInputClient* input_client = > focused_now ? focused_now->GetTextInputClient() : NULL; > [bridged_view_ setTextInputClient:input_client]; > > I think that pattern is more common. And, since there's an NSTestInputClient and > a ui::TextInputClient it helps clear things up a bit. Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... File ui/views/cocoa/text_input_unittest.mm (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:5: #include "base/mac/scoped_nsobject.h" On 2014/06/17 13:23:56, tapted wrote: > nit: import this and the other objc-only headers Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:16: static NSRange EmptyRange = NSMakeRange(NSNotFound, 0); On 2014/06/17 13:23:57, tapted wrote: > non-primitive statics aren't usually allowed - > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo.... > I'd probably make this a function in an anonymous namespace `const NSRange > kEmptyRange = {NSNotFound, 0};` might avoid a static initializer too. Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:19: EXPECT_EQ(a.location, b.location); \ On 2014/06/17 13:23:57, tapted wrote: > I've seen EXPECT_TRUE(NSEqualRanges(a, b)) for stuff like this in Chrome I had it like that, but the failure output is much more helpful with the EQ variants since it prints out the actual and expected values. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:36: [bridge_ attributedSubstringForProposedRange:NSMakeRange(0, 1000) On 2014/06/17 13:23:57, tapted wrote: > this 1000 should be a constant somewhere Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:44: }; On 2014/06/17 13:23:57, tapted wrote: > nit: private: > DISALLOW_COPY_AND_ASSIGN(..) > > (you'll need a default constructor too) Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:48: TEST_F(TextInputMacTest, AttributedSubstring) { On 2014/06/17 13:23:57, tapted wrote: > nit: it's sometimes nice to have a comment before this describing each test too. > But if the name of the test is descriptive enough it's sometimes skipped. Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:49: NSRange range; On 2014/06/17 13:23:57, tapted wrote: > Unless it's a constant, declarations are usually deferred to their first use and > assigned at the same time. Yeah, I did this way because the first use became different and quite long. I think it's better to split this up into shorter tests, so I've done that. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:50: NSRange actualRange; On 2014/06/17 13:23:57, tapted wrote: > actualRange -> actual_range, since it's not an objective C method Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:53: const char* str = "foo bar baz"; On 2014/06/17 13:23:57, tapted wrote: > I'd usually see `const char kTestString[] = "foo bar baz";`. Or, since you use > the length later, > > const std::string kTestString = "foo bar baz"; I don't see strlen too often in > tests ;) Done. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:60: EXPECT_EQ(str, SysNSStringToUTF8([text string])); On 2014/06/17 13:23:57, tapted wrote: > There's also EXPECT_NSEQ from testing/gtest_mac.h . Not sure if it gives better > output here. Thanks, I don't think it will reduce the number of conversions in this case. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/text_input_un... ui/views/cocoa/text_input_unittest.mm:88: NSString* str = @"foo"; On 2014/06/17 13:23:57, tapted wrote: > `str` probably counts as an abbreviation, so not allowed by the style guide Done. https://codereview.chromium.org/329463002/diff/1/ui/views/views.gyp File ui/views/views.gyp (right): https://codereview.chromium.org/329463002/diff/1/ui/views/views.gyp#newcode681 ui/views/views.gyp:681: 'cocoa/text_input_unittest.mm', On 2014/06/17 13:23:57, tapted wrote: > hm - where's 'cocoa/bridged_native_widget_unittest.mm', gone? > > Since you're not adding a new file called text_input, it's probably more > consistent to just add tests to cocoa/bridged_native_widget_unittest.mm, or add > cocoa/bridged_content_view_unittest.mm - I'd probably lean towards the former, > so you can get a bit more coverage. Done. https://codereview.chromium.org/329463002/diff/1/ui/views/widget/native_widge... File ui/views/widget/native_widget_mac.mm (right): https://codereview.chromium.org/329463002/diff/1/ui/views/widget/native_widge... ui/views/widget/native_widget_mac.mm:212: return gfx::ScreenRectFromNSRect( On 2014/06/17 13:23:57, tapted wrote: > hm - my next patch is looking like it won't need this again - it would end up > being a "rider", (orthogonal to the CL). So, if you need it for this, I'd > probably include it now - probably just ScreenRectFromNSRect with a comment in > the header for it - since that was missing. I don't need this to get the tests to pass, so I won't make it part of this CL.
lgtm with some nits. Some questions too, but I think it's ok as-is - at least until there is a bigger picture Make sure you put a summary of "how" the CL implements its summary - there's finally a guide: http://dev.chromium.org/developers/contributing-code#TOC-Change-List-Descript... https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:100: [self interpretKeyEvents:@[ theEvent ]]; On 2014/06/18 21:48:35, Andre wrote: > On 2014/06/17 13:23:56, tapted wrote: > > Do you think interpretKeyEvents will be needed for some cases where > > textInputClient_ is NULL? I actually couldn't decide how to tie in > > interpretKeyEvents when I added keyUp/keyDown. Is your approach similar to how > > the renderer does it? > > If textInputClient_ is NULL, I think we should forward the key event to the next > responder by calling super. It will give other responders in the chain to > process the > event, and the event is not handled the system will emit a beep. yep - lg for now. For things like menus/lists we'll want arrow keys for navigation - i don't think they will ahve a textInputClient, but we should be going through interpretKeyEvents for those anyway to ensure we pick up things a user has reconfigured. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:159: - (void)doCommandBySelector:(SEL)selector { On 2014/06/18 21:48:34, Andre wrote: > On 2014/06/17 13:23:56, tapted wrote: > > Should be under `// NSResponder implementation.`. (but why was this needed - I > > thought the AppKit NSResponder dispatcher took care of this > > doCommandBySelector is part of both NSResponder and NSTextInputClient. > In this case, we are implementing the NSTextInputClient one so I don't think it > should > be under NSResponder implementation. > I think you're right that NSResponder's default implementation probably takes > care of > this, but this is a required method in NSTextInputClient protocol, and the docs > state > that it should not call super. > It probably should forward unhandled selectors to nextResponder though, so I've > added code for that. It's still overriding a method on NSResponder (whereas it's just conforming to a protocol on NSTextInputClient). I guess either is fine. https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:176: - (void)insertText:(id)text replacementRange:(NSRange)replacementRange { On 2014/06/18 21:48:34, Andre wrote: > On 2014/06/17 13:23:56, tapted wrote: > > nit: I think method signatures "always" linebreak between arguments in > chromium > > objc > > Hmm, I don't see that in objcstyle, and cocoa code in chrome/browser does not > seem > to do that. Where do you see the always linebreak convention? Oh wow - I've just always done it - but you're right. I guess objC argument names are usually so long it hardly ever comes up https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:177: if ([text isKindOfClass:[NSAttributedString class]]) On 2014/06/18 21:48:34, Andre wrote: > On 2014/06/17 13:23:56, tapted wrote: > > Before this, you should do > > > > if (!textInputClient_) > > return; > > Done. `if (!textInputClient_)` should be the first line of the function - it just keeps the nesting down to a minimum. https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.h:16: class BridgedNativeWidget; nit: sort https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:25: // Execute a command on the currently focused TextInputClient. nit: mention that |commandId| should be a resource ID from ui_strings.grd https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:48: hostedView_->GetFocusManager()->AddFocusChangeListener(parent_); Hm - I just looked at this some more. Does this always end up calling Widget::GetFocusManager() ? It's a virtual method on View, but after poking around I think that might just be for tests. Then, since this is called from GetWidget()->GetRootView() I think the widget (and therefore the FocusManager) is guaranteed to be non-null. Also, while it makes sense for BridgedContentView to add/remove listeners if it's a thing that comes from hostedView_, if it always *actually* comes from Widget::GetFocusManager(), it might be cleaner for BridgedNativeWidget to add/remove listeners, and to not change it if the bridge_ ever gets recreated due to the rootview changing. That is, the Add/RemoveFocusChangeListener(..) could be called in the BridgedNativeWidget constructor/destructor (BridgedNativeWidget would need the NativeWidgetMac passed in as a |parent|, but that was also the plan). Putting this lifetime on a C++ object would make this cleaner I think. But if we need the flexibility that View::GetFocusManager gives, this is fine. WDYT? https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:54: if (hostedView_) { if (!hostedView_) return; or perhaps even DCHECK(hostedView_); if we want to guarantee that this is called exactly once - I think that's reasonable. https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:55: if (hostedView_->GetFocusManager()) note that we'll have to ensure the lifetime of this stays correct as we add more stuff. That is, if the view is removed from a widget before being cleared from this, then GetFocusManager could return null where it returned non-null in the initializer. But FocusManager will DCHECK if we leave a dangling observer, so it must be correct at least for now. https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:224: return NSMakeRange(0, 0); oops - my bad. this should be NSMakeRange(NSNotFound, 0); (or gfx::Range().ToNSRange(). another below https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:243: if (textInputClient_) { same here - prefer early return / less indenting
https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/329463002/diff/1/ui/views/cocoa/bridged_conte... ui/views/cocoa/bridged_content_view.mm:177: if ([text isKindOfClass:[NSAttributedString class]]) On 2014/06/19 01:29:15, tapted wrote: > On 2014/06/18 21:48:34, Andre wrote: > > On 2014/06/17 13:23:56, tapted wrote: > > > Before this, you should do > > > > > > if (!textInputClient_) > > > return; > > > > Done. > > `if (!textInputClient_)` should be the first line of the function - it just > keeps the nesting down to a minimum. Done. https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.h (right): https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.h:16: class BridgedNativeWidget; On 2014/06/19 01:29:15, tapted wrote: > nit: sort Deleted, no longer needed. https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:25: // Execute a command on the currently focused TextInputClient. On 2014/06/19 01:29:16, tapted wrote: > nit: mention that |commandId| should be a resource ID from ui_strings.grd Done. https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:48: hostedView_->GetFocusManager()->AddFocusChangeListener(parent_); On 2014/06/19 01:29:15, tapted wrote: > Hm - I just looked at this some more. Does this always end up calling > Widget::GetFocusManager() ? It's a virtual method on View, but after poking > around I think that might just be for tests. Then, since this is called from > GetWidget()->GetRootView() I think the widget (and therefore the FocusManager) > is guaranteed to be non-null. > > Also, while it makes sense for BridgedContentView to add/remove listeners if > it's a thing that comes from hostedView_, if it always *actually* comes from > Widget::GetFocusManager(), it might be cleaner for BridgedNativeWidget to > add/remove listeners, and to not change it if the bridge_ ever gets recreated > due to the rootview changing. > > That is, the Add/RemoveFocusChangeListener(..) could be called in the > BridgedNativeWidget constructor/destructor (BridgedNativeWidget would need the > NativeWidgetMac passed in as a |parent|, but that was also the plan). Putting > this lifetime on a C++ object would make this cleaner I think. > > But if we need the flexibility that View::GetFocusManager gives, this is fine. > > WDYT? I like it, moved this to BridgedNativeWidget. https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:54: if (hostedView_) { On 2014/06/19 01:29:16, tapted wrote: > if (!hostedView_) > return; > > or perhaps even > > DCHECK(hostedView_); > > if we want to guarantee that this is called exactly once - I think that's > reasonable. Removed this change from the patch. https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:224: return NSMakeRange(0, 0); On 2014/06/19 01:29:16, tapted wrote: > oops - my bad. this should be NSMakeRange(NSNotFound, 0); (or > gfx::Range().ToNSRange(). another below Done. https://codereview.chromium.org/329463002/diff/40001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:243: if (textInputClient_) { On 2014/06/19 01:29:16, tapted wrote: > same here - prefer early return / less indenting Done.
https://codereview.chromium.org/329463002/diff/80001/ui/views/widget/native_w... File ui/views/widget/native_widget_mac.h (left): https://codereview.chromium.org/329463002/diff/80001/ui/views/widget/native_w... ui/views/widget/native_widget_mac.h:20: protected: This looks to be unnecessarily protected. It matches DesktopNativeWidgetAura, but in NativeWidgetAura they are public. I need to call GetWidget() from BridgedNativeWidget.
lgtm - don't forget to expand the CL description https://codereview.chromium.org/329463002/diff/80001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/329463002/diff/80001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:24: @interface BridgedContentView () merge with above https://codereview.chromium.org/329463002/diff/80001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:142: [self interpretKeyEvents:@[ theEvent ]]; (just curious) If keyDown: is removed completely, does the default responder handling end up calling interpretKeyEvents: for us? https://codereview.chromium.org/329463002/diff/80001/ui/views/cocoa/bridged_n... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/329463002/diff/80001/ui/views/cocoa/bridged_n... ui/views/cocoa/bridged_native_widget.mm:84: Widget* widget = [bridged_view_ hostedView]->GetWidget(); I'd probably do DCHECK(parent_); DCHECK(focus_manager_); parent_->GetWidget()->OnKeyEvent(const_cast<ui::KeyEvent*>(&key)); if (!key.handled()) focus_manager_->OnKeyEvent(key); (i.e. if a test calls this, it's not a great test if that stuff is missing, and would probably crash when bridged_view_ is nil anyway and [.. hostedView] return NULL.. although it's possible there's a test "somewhere" that relies on this codepath too).
https://codereview.chromium.org/329463002/diff/80001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/329463002/diff/80001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:24: @interface BridgedContentView () On 2014/06/20 00:40:59, tapted wrote: > merge with above Oops, bad merge. Done. https://codereview.chromium.org/329463002/diff/80001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:142: [self interpretKeyEvents:@[ theEvent ]]; On 2014/06/20 00:40:59, tapted wrote: > (just curious) If keyDown: is removed completely, does the default responder > handling end up calling interpretKeyEvents: for us? No, I think NSResponder's implementation simply forwards it to the next responder. Usually only text handling responders will call interpretKeyEvents. https://codereview.chromium.org/329463002/diff/80001/ui/views/cocoa/bridged_n... File ui/views/cocoa/bridged_native_widget.mm (right): https://codereview.chromium.org/329463002/diff/80001/ui/views/cocoa/bridged_n... ui/views/cocoa/bridged_native_widget.mm:84: Widget* widget = [bridged_view_ hostedView]->GetWidget(); On 2014/06/20 00:40:59, tapted wrote: > I'd probably do > > DCHECK(parent_); > DCHECK(focus_manager_); > parent_->GetWidget()->OnKeyEvent(const_cast<ui::KeyEvent*>(&key)); > if (!key.handled()) > focus_manager_->OnKeyEvent(key); > > (i.e. if a test calls this, it's not a great test if that stuff is missing, and > would probably crash when bridged_view_ is nil anyway and [.. hostedView] return > NULL.. although it's possible there's a test "somewhere" that relies on this > codepath too). Done.
+sky for OWNERS
LGTM
Uploaded patch #5 with a fix for focus change listener registration. The focus manager is created in Widget::OnNativeWidgetCreated, so calling AddFocusChangeListener from BridgedNativeWidget::BridgedNativeWidget() is too early. Added BridgedNativeWidget::SetFocusManager and call it after the widget is fully initialized.
slgtm https://codereview.chromium.org/329463002/diff/120001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/329463002/diff/120001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:105: - (void)keyUp:(NSEvent*)theEvent { note you'll need to rebase against master (this chunk isn't in yet, so you'll get a conflict). When I resolved it locally everything still looks good.
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/329463002/14...
Message was sent while issue was closed.
Change committed as 279806 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
