Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2535553002:
MacViews: Fix GlobalKeyboardShortcuts.ShortcutsToWindowCommand. (Closed)
DescriptionMacViews: Fix GlobalKeyboardShortcuts.ShortcutsToWindowCommand.
After crbug.com/620315 global_keyboard_shortcuts_mac.mm uses two different
shortcut tables: one for Cocoa and one for MacViews. Ensure that the test works
on both browser implementations.
BUG=620315
Committed: https://crrev.com/4ffb0e227b4b006a85475a368187dd96e63d7996
Cr-Commit-Position: refs/heads/master@{#436169}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #ifdef out failing test. #Messages
Total messages: 19 (8 generated)
mblsha@yandex-team.ru changed reviewers: + mark@chromium.org
Description was changed from ========== MacViews: Fix GlobalKeyboardShortcuts.ShortcutsToWindowCommand. After crbug.com/620315 global_keyboard_shortcuts_mac.mm uses two different shortcut tables: one for Cocoa and one for MacViews. Ensure that the test works on both browser implementations. ========== to ========== MacViews: Fix GlobalKeyboardShortcuts.ShortcutsToWindowCommand. After crbug.com/620315 global_keyboard_shortcuts_mac.mm uses two different shortcut tables: one for Cocoa and one for MacViews. Ensure that the test works on both browser implementations. BUG=620315 ==========
mblsha@yandex-team.ru changed reviewers: + thakis@chromium.org
+thakis Mark hasn't responded to this CL yet :(
mark@chromium.org changed reviewers: + tapted@chromium.org
Well, you sent it during a holiday weekend where we are. tapted should review this. Nico or I can give the LG for OWNERS purposes, but since I see you’ve been working most on bug 620315 with tapted, he’s probably the best choice.
On 2016/11/30 13:30:44, Mark Mentovai wrote: > Well, you sent it during a holiday weekend where we are. > > tapted should review this. Nico or I can give the LG for OWNERS purposes, but > since I see you’ve been working most on bug 620315 with tapted, he’s probably > the best choice. Ah, thanks!
Sorry for the delay! My schedule was a bit disrupted the last couple of days. I think the right thing to do is to just skip this EXPECT_EQ on MacViews, which is easier to do since r435207. See comment below. https://codereview.chromium.org/2535553002/diff/1/chrome/browser/global_keybo... File chrome/browser/global_keyboard_shortcuts_mac_unittest.mm (right): https://codereview.chromium.org/2535553002/diff/1/chrome/browser/global_keybo... chrome/browser/global_keyboard_shortcuts_mac_unittest.mm:74: EXPECT_EQ(IDC_SELECT_TAB_0, CommandForTestShortcut( I poked around the code, and I think this expectation simply doesn't make sense on MacViews. IDC_SELECT_TAB_0 is mapped via the accelerator_table.cc which _only_ supports mapping via keycodes. It's only the curly braces above that needed different treatment. So I think all we need is a good comment here (e.g. paraphrase the above paragraph), and #if !BUILDFLAG(MAC_VIEWS_BROWSER) around this EXPECT You'll need #include "ui/base/ui_features.h" then the other changes can be dropped. Ideally we also point to where equivalent coverage of this exists via accelerator_table, but the test would just be checking whether the accelerator table contains { ui::VKEY_1, kPlatformModifier, IDC_SELECT_TAB_0 }, which it obviously does, so I don't think that's interesting.
https://codereview.chromium.org/2535553002/diff/1/chrome/browser/global_keybo... File chrome/browser/global_keyboard_shortcuts_mac_unittest.mm (right): https://codereview.chromium.org/2535553002/diff/1/chrome/browser/global_keybo... chrome/browser/global_keyboard_shortcuts_mac_unittest.mm:74: EXPECT_EQ(IDC_SELECT_TAB_0, CommandForTestShortcut( On 2016/12/02 07:26:09, tapted wrote: > I poked around the code, and I think this expectation simply doesn't make sense > on MacViews. IDC_SELECT_TAB_0 is mapped via the accelerator_table.cc which > _only_ supports mapping via keycodes. It's only the curly braces above that > needed different treatment. > > So I think all we need is a good comment here (e.g. paraphrase the above > paragraph), and > > #if !BUILDFLAG(MAC_VIEWS_BROWSER) > > around this EXPECT > > > You'll need #include "ui/base/ui_features.h" > > > then the other changes can be dropped. > > Ideally we also point to where equivalent coverage of this exists via > accelerator_table, but the test would just be checking whether the accelerator > table contains > { ui::VKEY_1, kPlatformModifier, IDC_SELECT_TAB_0 }, > which it obviously does, so I don't think that's interesting. Woohoo! This BUILDFLAG will make dealing with failing test like this much easier, thanks!
lgtm
rs-lgtm
The CQ bit was checked by mblsha@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480768892744360,
"parent_rev": "a13a155b7e1cbe095d4080045b39ec174b1e7f99", "commit_rev":
"6eac8185ffe132905e986a0feaa6f8eab88366cf"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix GlobalKeyboardShortcuts.ShortcutsToWindowCommand. After crbug.com/620315 global_keyboard_shortcuts_mac.mm uses two different shortcut tables: one for Cocoa and one for MacViews. Ensure that the test works on both browser implementations. BUG=620315 ========== to ========== MacViews: Fix GlobalKeyboardShortcuts.ShortcutsToWindowCommand. After crbug.com/620315 global_keyboard_shortcuts_mac.mm uses two different shortcut tables: one for Cocoa and one for MacViews. Ensure that the test works on both browser implementations. BUG=620315 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix GlobalKeyboardShortcuts.ShortcutsToWindowCommand. After crbug.com/620315 global_keyboard_shortcuts_mac.mm uses two different shortcut tables: one for Cocoa and one for MacViews. Ensure that the test works on both browser implementations. BUG=620315 ========== to ========== MacViews: Fix GlobalKeyboardShortcuts.ShortcutsToWindowCommand. After crbug.com/620315 global_keyboard_shortcuts_mac.mm uses two different shortcut tables: one for Cocoa and one for MacViews. Ensure that the test works on both browser implementations. BUG=620315 Committed: https://crrev.com/4ffb0e227b4b006a85475a368187dd96e63d7996 Cr-Commit-Position: refs/heads/master@{#436169} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/4ffb0e227b4b006a85475a368187dd96e63d7996 Cr-Commit-Position: refs/heads/master@{#436169} |
