Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2748993004:
Don't call NOTIMPLEMENTED for caps lock state on non-Mac/Win platforms. (Closed)
DescriptionDon't call NOTIMPLEMENTED for caps lock state on non-Mac/Win platforms.
BUG=618739
Review-Url: https://codereview.chromium.org/2748993004
Cr-Commit-Position: refs/heads/master@{#457043}
Committed: https://chromium.googlesource.com/chromium/src/+/7599eb3eee0daef81cef0bdf79294849b5d52e6d
Patch Set 1 #
Total comments: 4
Patch Set 2 : Feedback. #Messages
Total messages: 18 (8 generated)
wkorman@chromium.org changed reviewers: + dtapuska@chromium.org, stevenjb@chromium.org
https://codereview.chromium.org/2748993004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right): https://codereview.chromium.org/2748993004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:440: // Note that the caps lock state provided by this method is only Two alternatives to this patch: 1. modify LayoutTextControlSingleLine event-forwarding logic (intended to handle paint invalidation for the control on caps-lock state change) to also consider the platform in some manner. For example, we could add a method to KeyboardEventManager that provides visibility into whether caps lock state is available for the current platform, and have LTCSL check that before calling currentCapsLockState(). ref: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... This seems like more of a maintenance headache than the benefit it would provide, but I could be convinced if you feel strongly. 2. finish implementing currentCapsLockState for all platforms. This doesn't seem worthwhile currently since we only need/use this for Mac theme painting.
https://codereview.chromium.org/2748993004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right): https://codereview.chromium.org/2748993004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:440: // Note that the caps lock state provided by this method is only On 2017/03/15 00:03:14, wkorman wrote: > Two alternatives to this patch: > > 1. modify LayoutTextControlSingleLine event-forwarding logic (intended to handle > paint invalidation for the control on caps-lock state change) to also consider > the platform in some manner. For example, we could add a method to > KeyboardEventManager that provides visibility into whether caps lock state is > available for the current platform, and have LTCSL check that before calling > currentCapsLockState(). > > ref: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > This seems like more of a maintenance headache than the benefit it would > provide, but I could be convinced if you feel strongly. > > 2. finish implementing currentCapsLockState for all platforms. This doesn't seem > worthwhile currently since we only need/use this for Mac theme painting. this lgtm. This NOTIMPLEMENTED use to be a no-op before the unified logging was added to blink. It then changed to a warning that was always generated, so I'm fine ignoring it for now.
wkorman@chromium.org changed reviewers: + pdr@chromium.org
+pdr for OWNERS.
On 2017/03/15 at 00:20:40, wkorman wrote: > +pdr for OWNERS. LGTM
The CQ bit was checked by wkorman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for tackling this, lgtm! https://codereview.chromium.org/2748993004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right): https://codereview.chromium.org/2748993004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:447: // no current plans or need to use this. This is helpful, but maybe a little bit verbose :) You could put this text in the bug, simplify it here, and refer to the bug (e.g. see crbug.com/1234).
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, dtapuska@chromium.org, pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2748993004/#ps20001 (title: "Feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2748993004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right): https://codereview.chromium.org/2748993004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:447: // no current plans or need to use this. On 2017/03/15 01:25:33, stevenjb wrote: > This is helpful, but maybe a little bit verbose :) You could put this text in > the bug, simplify it here, and refer to the bug (e.g. see crbug.com/1234). Done.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489565821821840,
"parent_rev": "850916c69f040842d11473f02474de45b7f57e0c", "commit_rev":
"7599eb3eee0daef81cef0bdf79294849b5d52e6d"}
Message was sent while issue was closed.
Description was changed from ========== Don't call NOTIMPLEMENTED for caps lock state on non-Mac/Win platforms. BUG=618739 ========== to ========== Don't call NOTIMPLEMENTED for caps lock state on non-Mac/Win platforms. BUG=618739 Review-Url: https://codereview.chromium.org/2748993004 Cr-Commit-Position: refs/heads/master@{#457043} Committed: https://chromium.googlesource.com/chromium/src/+/7599eb3eee0daef81cef0bdf7929... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7599eb3eee0daef81cef0bdf7929... |
