Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2019233003:
MacViews: Fix crash caused due to nil TextInputClient. (Closed)
|
Created:
4 years, 6 months ago by karandeepb Modified:
4 years, 6 months ago Reviewers:
tapted CC:
chromium-reviews, tfarina, 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. |
DescriptionMacViews: Fix crash caused due to nil TextInputClient.
All key events in MacViews are currently passed to interpretKeyEvents, which
generates appropriate action messages for the BridgedContentView, as per the
system key bindings. Some of these action messages use IsTextRTL without
checking if the textInputClient is nil or not. This CL resolves the crash casued
due to this, by changing the IsTextRTL method to correctly handle the case where
TextInputClient is nil.
BUG=615745
Committed: https://crrev.com/5f965b799f22a580675113556d38217c0d8abe96
Cr-Commit-Position: refs/heads/master@{#396769}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Address review and add test. #
Total comments: 2
Patch Set 3 : Address review comments. #
Total comments: 4
Patch Set 4 : Address nits. #
Messages
Total messages: 20 (10 generated)
Description was changed from ========== --- BUG= ========== to ========== MacViews: Fix crash caused due to nil TextInputClient. All key events in MacViews are currently passed to interpretKeyEvents, which generates appropriate action messages for the BridgedContentView, as per the system key bindings. Some of these action messages use IsTextRTL without checking if the textInputClient is nil or not. This CL resolves the crash casued due to this by returning early for these action messages if textInputClient is nil. BUG=615745 ==========
Description was changed from ========== MacViews: Fix crash caused due to nil TextInputClient. All key events in MacViews are currently passed to interpretKeyEvents, which generates appropriate action messages for the BridgedContentView, as per the system key bindings. Some of these action messages use IsTextRTL without checking if the textInputClient is nil or not. This CL resolves the crash casued due to this by returning early for these action messages if textInputClient is nil. BUG=615745 ========== to ========== MacViews: Fix crash caused due to nil TextInputClient. All key events in MacViews are currently passed to interpretKeyEvents, which generates appropriate action messages for the BridgedContentView, as per the system key bindings. Some of these action messages use IsTextRTL without checking if the textInputClient is nil or not. This CL resolves the crash casued due to this, by returning early for these action messages if textInputClient is nil. BUG=615745 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Thanks.
https://codereview.chromium.org/2019233003/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2019233003/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:82: DCHECK(client); I think it's nicer for this just to return false, then rely on the checks in handleAction: We then just need to ensure there is some appropriate test coverage to ensure we don't regress.
Description was changed from ========== MacViews: Fix crash caused due to nil TextInputClient. All key events in MacViews are currently passed to interpretKeyEvents, which generates appropriate action messages for the BridgedContentView, as per the system key bindings. Some of these action messages use IsTextRTL without checking if the textInputClient is nil or not. This CL resolves the crash casued due to this, by returning early for these action messages if textInputClient is nil. BUG=615745 ========== to ========== MacViews: Fix crash caused due to nil TextInputClient. All key events in MacViews are currently passed to interpretKeyEvents, which generates appropriate action messages for the BridgedContentView, as per the system key bindings. Some of these action messages use IsTextRTL without checking if the textInputClient is nil or not. This CL resolves the crash casued due to this, by changing the IsTextRTL method to correctly handle the case where TextInputClient is nil. BUG=615745 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
PTAL Trent. Thanks.
https://codereview.chromium.org/2019233003/diff/50001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2019233003/diff/50001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:1018: NSArray* selectors = @[ We shouldn't repeat these - can we do something like NSArray* selectors = @[ @"insertText:" ]; selectors = [selectors arrayByAddingObjectsFromArray:kMoveActions]; selectors = [selectors arrayByAddingObjectsFromArray:kSelectActions]; selectors = [selectors arrayByAddingObjectsFromArray:kDeleteActions];
PTAL Trent. https://codereview.chromium.org/2019233003/diff/50001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2019233003/diff/50001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:1018: NSArray* selectors = @[ On 2016/05/31 01:17:10, tapted wrote: > We shouldn't repeat these - can we do something like > > NSArray* selectors = @[ @"insertText:" ]; > selectors = [selectors arrayByAddingObjectsFromArray:kMoveActions]; > selectors = [selectors arrayByAddingObjectsFromArray:kSelectActions]; > selectors = [selectors arrayByAddingObjectsFromArray:kDeleteActions]; > Done.
lgtm % nits https://codereview.chromium.org/2019233003/diff/70001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2019233003/diff/70001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:113: // Implemented NSResponder action messages for use in tests. nit: constants should come before functions in each section, so this should probably go to the top of the anonymous namespace. https://codereview.chromium.org/2019233003/diff/70001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:114: NSArray* kMoveActions = @[ nit: NSArray* const kMoveActions - same below (it's OK to have `const` on the variable itself, we just wouldn't use `const NSArray*` or [equivalent] `NSArray const*` for ObjectiveC types since that would be asking the NSArray to be const which is weird).
https://codereview.chromium.org/2019233003/diff/70001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2019233003/diff/70001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:113: // Implemented NSResponder action messages for use in tests. On 2016/05/31 01:49:14, tapted wrote: > nit: constants should come before functions in each section, so this should > probably go to the top of the anonymous namespace. Done. https://codereview.chromium.org/2019233003/diff/70001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:114: NSArray* kMoveActions = @[ On 2016/05/31 01:49:14, tapted wrote: > nit: NSArray* const kMoveActions - same below (it's OK to have `const` on the > variable itself, we just wouldn't use `const NSArray*` or [equivalent] `NSArray > const*` for ObjectiveC types since that would be asking the NSArray to be const > which is weird). Done.
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/2019233003/#ps90001 (title: "Address nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2019233003/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2019233003/90001
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix crash caused due to nil TextInputClient. All key events in MacViews are currently passed to interpretKeyEvents, which generates appropriate action messages for the BridgedContentView, as per the system key bindings. Some of these action messages use IsTextRTL without checking if the textInputClient is nil or not. This CL resolves the crash casued due to this, by changing the IsTextRTL method to correctly handle the case where TextInputClient is nil. BUG=615745 ========== to ========== MacViews: Fix crash caused due to nil TextInputClient. All key events in MacViews are currently passed to interpretKeyEvents, which generates appropriate action messages for the BridgedContentView, as per the system key bindings. Some of these action messages use IsTextRTL without checking if the textInputClient is nil or not. This CL resolves the crash casued due to this, by changing the IsTextRTL method to correctly handle the case where TextInputClient is nil. BUG=615745 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:90001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix crash caused due to nil TextInputClient. All key events in MacViews are currently passed to interpretKeyEvents, which generates appropriate action messages for the BridgedContentView, as per the system key bindings. Some of these action messages use IsTextRTL without checking if the textInputClient is nil or not. This CL resolves the crash casued due to this, by changing the IsTextRTL method to correctly handle the case where TextInputClient is nil. BUG=615745 ========== to ========== MacViews: Fix crash caused due to nil TextInputClient. All key events in MacViews are currently passed to interpretKeyEvents, which generates appropriate action messages for the BridgedContentView, as per the system key bindings. Some of these action messages use IsTextRTL without checking if the textInputClient is nil or not. This CL resolves the crash casued due to this, by changing the IsTextRTL method to correctly handle the case where TextInputClient is nil. BUG=615745 Committed: https://crrev.com/5f965b799f22a580675113556d38217c0d8abe96 Cr-Commit-Position: refs/heads/master@{#396769} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5f965b799f22a580675113556d38217c0d8abe96 Cr-Commit-Position: refs/heads/master@{#396769} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
