Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2922773002:
Add BrowserCommandController Interactive Test (Closed)
DescriptionAdd BrowserCommandController Interactive Test
BrowserCommandController interactive test ensures the recent changes of browser
shortcuts in fullscreen won't be broken by unexpected changes. It starts a full
screen browser window, sends several key events normally consumed by browser,
and checks whether these events are caught by the web page.
BUG=680809
Review-Url: https://codereview.chromium.org/2922773002
Cr-Commit-Position: refs/heads/master@{#486949}
Committed: https://chromium.googlesource.com/chromium/src/+/bb317b9cc0055f7abebcd014dfb6c193548b4d88
Patch Set 1 #
Total comments: 28
Patch Set 2 : Resolve review comments #Patch Set 3 : msvc does not support recursive #if #
Total comments: 68
Patch Set 4 : Resolve review comments #Patch Set 5 : Resolve review comments #Patch Set 6 : Update bug id #Patch Set 7 : Remove debug log #Patch Set 8 : Build break on platforms other than MacOSX #
Total comments: 12
Patch Set 9 : Resolve review comments #Patch Set 10 : Do not exiting fullscreen on OS10.9 or earlier #
Total comments: 6
Patch Set 11 : git cl format #
Total comments: 7
Patch Set 12 : Resolve review comments #Patch Set 13 : Update getKeyEventReport() logic #
Total comments: 22
Patch Set 14 : Try to remove "output = nullptr;" #Patch Set 15 : Resolve review comments #Patch Set 16 : Resolve review comments #
Messages
Total messages: 260 (228 generated)
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by zijiehe@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...
Patchset #1 (id:1) has been deleted
Description was changed from ========== BrowserCommandControllerInteractiveBrowserTest BUG= ========== to ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. BUG=680809 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Description was changed from ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. BUG=680809 ========== to ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. These tests currently cannot be executed on Mac OSX: the shortcuts are not handled by RenderWidgetHostViewCocoa, thus sending them by ui_test_utils::SendKeyPress() is not consistent with user inputs. BUG=680809 ==========
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Description was changed from ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. These tests currently cannot be executed on Mac OSX: the shortcuts are not handled by RenderWidgetHostViewCocoa, thus sending them by ui_test_utils::SendKeyPress() is not consistent with user inputs. BUG=680809 ========== to ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. These tests currently cannot be executed on Mac OSX: the shortcuts are not handled by RenderWidgetHostViewCocoa, thus sending them by ui_test_utils::SendKeyPress() is not consistent with user inputs. Executing these tests on ChromeOS triggers crash error, BrowserTestBase received signal: Terminated. Backtrace: #0 0x000003363eec base::debug::StackTrace::StackTrace() #1 0x0000029b7f62 content::(anonymous namespace)::DumpStackTraceSignalHandler() #2 0x7f0297078cb0 <unknown> #3 0x7f029713284d __poll #4 0x7f029ade5fe4 <unknown> #5 0x7f029ade60ec g_main_context_iteration #6 0x000003386825 base::MessagePumpGlib::Run() #7 0x0000033840eb base::MessageLoop::Run() #8 0x0000033ae25a base::RunLoop::Run() #9 0x0000029c2726 content::MessageLoopRunner::Run() #10 0x0000029c2b9d content::WindowedNotificationObserver::Wait() BUG=680809 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. These tests currently cannot be executed on Mac OSX: the shortcuts are not handled by RenderWidgetHostViewCocoa, thus sending them by ui_test_utils::SendKeyPress() is not consistent with user inputs. Executing these tests on ChromeOS triggers crash error, BrowserTestBase received signal: Terminated. Backtrace: #0 0x000003363eec base::debug::StackTrace::StackTrace() #1 0x0000029b7f62 content::(anonymous namespace)::DumpStackTraceSignalHandler() #2 0x7f0297078cb0 <unknown> #3 0x7f029713284d __poll #4 0x7f029ade5fe4 <unknown> #5 0x7f029ade60ec g_main_context_iteration #6 0x000003386825 base::MessagePumpGlib::Run() #7 0x0000033840eb base::MessageLoop::Run() #8 0x0000033ae25a base::RunLoop::Run() #9 0x0000029c2726 content::MessageLoopRunner::Run() #10 0x0000029c2b9d content::WindowedNotificationObserver::Wait() BUG=680809 ========== to ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. These tests currently can only be executed on Linux. On Mac OSX: The shortcuts are not handled by RenderWidgetHostViewCocoa, thus sending them by ui_test_utils::SendKeyPress() is not consistent with user inputs. On ChromeOS: These tests trigger crash error, BrowserTestBase received signal: Terminated. Backtrace: #0 0x000003363eec base::debug::StackTrace::StackTrace() #1 0x0000029b7f62 content::(anonymous namespace)::DumpStackTraceSignalHandler() #2 0x7f0297078cb0 <unknown> #3 0x7f029713284d __poll #4 0x7f029ade5fe4 <unknown> #5 0x7f029ade60ec g_main_context_iteration #6 0x000003386825 base::MessagePumpGlib::Run() #7 0x0000033840eb base::MessageLoop::Run() #8 0x0000033ae25a base::RunLoop::Run() #9 0x0000029c2726 content::MessageLoopRunner::Run() #10 0x0000029c2b9d content::WindowedNotificationObserver::Wait() On Windows: Flaky. The events may not be able to send to the browser window. See test case https://cs.chromium.org/chromium/src/chrome/browser/browser_keyevents_browser... for detail. BUG=680809 ==========
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run
Patchset #1 (id:60001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
zijiehe@chromium.org changed reviewers: + msw@chromium.org
Sorry I don't have any great ideas for making these tests more reliable and consistent, but I suggest you investigate a bit more, ping others for help with that, and maybe file/cite some bugs if you can't resolve the flakiness and platform-applicability before landing. FYI, I'll be out June 8-27, so you might want to find another reviewer to help here. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:22: constexpr char kFullscreenKeyboardLockHTML[] = nit: comment on what this is https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:25: class FullscreenNotificationObserver Avoid this subclass, just construct a WindowedNotificationObserver instance with the arguments you need and call Wait() on that, right? https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:26: : public content::WindowedNotificationObserver { nit: include content/public/test/test_utils.h to use this https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:43: // TODO(zijiehe): Make SendKeyPressSync() respect shortcuts on Mac OSX. File a bug; cite its number here. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:47: // BrowserTestBase received signal: Terminated. Backtrace: Don't include a stack trace here; address this before landing or investigate sufficiently to file an actionable bug. ie. why does this crash? (if nothing else, use a binary search of removing test code and commenting out support code to determine what triggers this, we can obviously run some browser_tests on chrome os) https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:62: // Tests depending on keyboard inputs are flaky on Windows, see test case Citing that file isn't sufficient (I don't see what 'test case' you mean there), cite/file a bug, or address this by finding a non-flaky way of doing this on Windows, I can't believe that all keyboard inputs in browser tests are flaky on windows, otherwise there would be a Pri-0 bug to fix that... https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:65: #define MAYBE(X) DISABLED_##X Sorry I don't have any great ideas for making these tests more reliable and consistent. Maybe try event generators, look through other tests in the interactive_ui_tests target, or ping chrome/test/OWNERS? https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:71: MAYBE(InF11Fullscreen)) { nit: use a better name here, what is this test checking? ie. KeyEventsSentToPageWithShortcutTriggeredFullscreen or use a simpler name like EventsInShortcutTriggeredFullscreen and add a comment above the test like "Test which key events are sent to the page when fullscreen is triggered by the user pressing the fullscreen shortcut.", ditto for other tests below (use a consistent naming scheme and add explanatory comments) https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:76: #if defined(OS_MACOSX) nit: add a local helper: void PressFullscreenShortcut() { #if... } https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:86: #if defined(OS_MACOSX) nit: add a local helper: bool PressKeySync(ui::KeyboardCode key) { #if... } Optionally pass a string pointer for it to append the expected keydown/keyup report sequences, to avoid hardcoding the super long string literals below (that should facilitate test readability/maintainability/extensibility) https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:181: MAYBE(ExitHTMLFullscreenWithEsc)) { nit: try to use a consistent naming pattern for these test fixtures ie. KeyEventsSentToPageWithHtmlTriggeredFullscreen https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:188: browser(), ui::VKEY_S, false, false, false, false)); q: Is 's' another magic key to make the page trigger fullscreen? Add a comment. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:223: // Esc. nit: "// Esc should exit fullscreen, when ..." (explain the condition under which Esc will exit fullscreen, like "when the page triggers fullscreen itself"... I don't know exactly when that's the case...) Maybe also note that the webpage should not receive the Esc keypress (since it's not in the expected string) https://codereview.chromium.org/2922773002/diff/80001/chrome/test/data/fullsc... File chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html (right): https://codereview.chromium.org/2922773002/diff/80001/chrome/test/data/fullsc... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:1: <!DOCTYPE html> Please find a good reviewer for test html/js, I'm not so experienced here. It would probably help if you added some comments throughout.
Oh and thank you for adding tests!!!
The CQ bit was checked by zijiehe@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.
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 zijiehe@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: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 zijiehe@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: 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 zijiehe@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 checked by zijiehe@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: 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 zijiehe@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: 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 zijiehe@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: Try jobs failed on following builders: linux_chromium_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 zijiehe@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: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by zijiehe@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: 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 zijiehe@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...
Description was changed from ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. These tests currently can only be executed on Linux. On Mac OSX: The shortcuts are not handled by RenderWidgetHostViewCocoa, thus sending them by ui_test_utils::SendKeyPress() is not consistent with user inputs. On ChromeOS: These tests trigger crash error, BrowserTestBase received signal: Terminated. Backtrace: #0 0x000003363eec base::debug::StackTrace::StackTrace() #1 0x0000029b7f62 content::(anonymous namespace)::DumpStackTraceSignalHandler() #2 0x7f0297078cb0 <unknown> #3 0x7f029713284d __poll #4 0x7f029ade5fe4 <unknown> #5 0x7f029ade60ec g_main_context_iteration #6 0x000003386825 base::MessagePumpGlib::Run() #7 0x0000033840eb base::MessageLoop::Run() #8 0x0000033ae25a base::RunLoop::Run() #9 0x0000029c2726 content::MessageLoopRunner::Run() #10 0x0000029c2b9d content::WindowedNotificationObserver::Wait() On Windows: Flaky. The events may not be able to send to the browser window. See test case https://cs.chromium.org/chromium/src/chrome/browser/browser_keyevents_browser... for detail. BUG=680809 ========== to ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. BUG=680809 ==========
The CQ bit was checked by zijiehe@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 checked by zijiehe@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 checked by zijiehe@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 checked by zijiehe@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: Try jobs failed on following builders: linux_chromium_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 zijiehe@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 zijiehe@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 zijiehe@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: 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 zijiehe@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 zijiehe@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 checked by zijiehe@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 zijiehe@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 checked by zijiehe@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 zijiehe@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...
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #25 (id:590001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #16 (id:450001) has been deleted
Patchset #21 (id:570001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #18 (id:550001) has been deleted
Patchset #3 (id:240001) has been deleted
Patchset #16 (id:530001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #14 (id:510001) has been deleted
Patchset #13 (id:490001) has been deleted
Patchset #12 (id:470001) has been deleted
Patchset #11 (id:430001) has been deleted
Patchset #10 (id:410001) has been deleted
Patchset #9 (id:390007) has been deleted
Patchset #8 (id:380001) has been deleted
Patchset #7 (id:360001) has been deleted
Patchset #6 (id:340001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Patchset #5 (id:320001) has been deleted
Patchset #4 (id:300001) has been deleted
Patchset #3 (id:280001) has been deleted
Patchset #2 (id:260001) has been deleted
Though there are some inconsistent behavior on MacOSX and ChromeOS, they are covered by bugs or #if defined. Now the tests work on all platforms. But unfortunately most of the logic are changed already. So of your original comments are fixed or out of date, would you mind to have a look again? Thank you. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:22: constexpr char kFullscreenKeyboardLockHTML[] = On 2017/06/06 20:32:07, msw wrote: > nit: comment on what this is Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:25: class FullscreenNotificationObserver On 2017/06/06 20:32:07, msw wrote: > Avoid this subclass, just construct a WindowedNotificationObserver instance with > the arguments you need and call Wait() on that, right? Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:26: : public content::WindowedNotificationObserver { On 2017/06/06 20:32:06, msw wrote: > nit: include content/public/test/test_utils.h to use this Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:43: // TODO(zijiehe): Make SendKeyPressSync() respect shortcuts on Mac OSX. On 2017/06/06 20:32:06, msw - vacation june 8-27 wrote: > File a bug; cite its number here. Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:47: // BrowserTestBase received signal: Terminated. Backtrace: On 2017/06/06 20:32:06, msw - vacation june 8-27 wrote: > Don't include a stack trace here; address this before landing or investigate > sufficiently to file an actionable bug. ie. why does this crash? (if nothing > else, use a binary search of removing test code and commenting out support code > to determine what triggers this, we can obviously run some browser_tests on > chrome os) Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:62: // Tests depending on keyboard inputs are flaky on Windows, see test case On 2017/06/06 20:32:07, msw - vacation june 8-27 wrote: > Citing that file isn't sufficient (I don't see what 'test case' you mean there), > cite/file a bug, or address this by finding a non-flaky way of doing this on > Windows, I can't believe that all keyboard inputs in browser tests are flaky on > windows, otherwise there would be a Pri-0 bug to fix that... Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:65: #define MAYBE(X) DISABLED_##X On 2017/06/06 20:32:06, msw - vacation june 8-27 wrote: > Sorry I don't have any great ideas for making these tests more reliable and > consistent. Maybe try event generators, look through other tests in the > interactive_ui_tests target, or ping chrome/test/OWNERS? Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:71: MAYBE(InF11Fullscreen)) { On 2017/06/06 20:32:06, msw - vacation june 8-27 wrote: > nit: use a better name here, what is this test checking? ie. > KeyEventsSentToPageWithShortcutTriggeredFullscreen or use a simpler name like > EventsInShortcutTriggeredFullscreen and add a comment above the test like "Test > which key events are sent to the page when fullscreen is triggered by the user > pressing the fullscreen shortcut.", ditto for other tests below (use a > consistent naming scheme and add explanatory comments) Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:76: #if defined(OS_MACOSX) On 2017/06/06 20:32:06, msw - vacation june 8-27 wrote: > nit: add a local helper: void PressFullscreenShortcut() { #if... } Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:86: #if defined(OS_MACOSX) On 2017/06/06 20:32:07, msw - vacation june 8-27 wrote: > nit: add a local helper: bool PressKeySync(ui::KeyboardCode key) { #if... } > Optionally pass a string pointer for it to append the expected keydown/keyup > report sequences, to avoid hardcoding the super long string literals below (that > should facilitate test readability/maintainability/extensibility) Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:181: MAYBE(ExitHTMLFullscreenWithEsc)) { On 2017/06/06 20:32:06, msw - vacation june 8-27 wrote: > nit: try to use a consistent naming pattern for these test fixtures ie. > KeyEventsSentToPageWithHtmlTriggeredFullscreen Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:188: browser(), ui::VKEY_S, false, false, false, false)); On 2017/06/06 20:32:07, msw - vacation june 8-27 wrote: > q: Is 's' another magic key to make the page trigger fullscreen? Add a comment. Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:223: // Esc. On 2017/06/06 20:32:06, msw - vacation june 8-27 wrote: > nit: "// Esc should exit fullscreen, when ..." (explain the condition under > which Esc will exit fullscreen, like "when the page triggers fullscreen > itself"... I don't know exactly when that's the case...) Maybe also note that > the webpage should not receive the Esc keypress (since it's not in the expected > string) Done. https://codereview.chromium.org/2922773002/diff/80001/chrome/test/data/fullsc... File chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html (right): https://codereview.chromium.org/2922773002/diff/80001/chrome/test/data/fullsc... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:1: <!DOCTYPE html> On 2017/06/06 20:32:07, msw wrote: > Please find a good reviewer for test html/js, I'm not so experienced here. > It would probably help if you added some comments throughout. Done.
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by zijiehe@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.
msw@chromium.org changed reviewers: + tapted@chromium.org
Thanks for working on these tests, it will be good to have this coverage. tapted: please take a look for mac comments/qs mentioning your ldap. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:30: "/fullscreen_keyboardlock/fullscreen_keyboardlock.html"; It seems a bit silly to create a new subdirectory for this one html file, but I guess that's already the case for fullscreen_mouselock... I'd suggest moving both to the parent chrome/test/data/ directory, but that's not critical. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:32: class TabCountObserver : public TabStripModelObserver { Could you replace this class with a simpler helper function, like: // Wait for the browser to have the expected tab count, or timeout. bool WaitForTabCount(Browser* browser, int tab_count) { const int kMaxTries = 10; for (int i = 0; i < kMaxTries && browser->tab_strip_model()->count() != tab_count; ++i) content::RunAllPendingInMessageLoop(); return browser->tab_strip_model()->count() == tab_count; } https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:39: void TabInsertedAt(TabStripModel* tab_strip_model, Since you use this class to also track the closing of tabs, is it necessary to watch for TabClosingAt, in case the tab isn't closed synchronously with the accelerator (and the initial tab count in TabCountObserver's ctor is greater than the expected number of tabs)? Presumably it's safe to ignore TabDetachedAt (etc.), since nothing should be detaching tabs in this test (etc.). https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:45: bool seen_ = false; nit: rename |seen_| to something like |has_expected_tab_count_| https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:51: seen_ = (browser->tab_strip_model()->count() == expected_); nit: parens not needed https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:52: if (!seen_) { nit: curlies not needed for one line conditional and body https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:56: TabCountObserver::~TabCountObserver() { nit: blank line above (or inline the definitions in the declarations above) https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:57: while (!seen_) { ditto nit: curlies not needed for one line conditional and body https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:66: if (tab_strip_model->count() == expected_) { ditto nit: curlies not needed for one line conditional and body, or do: seen_ |= tab_strip_model->count() == expected_; https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:83: // modifier will be added once |shift| is true. nit: s/once/if/ https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:87: void SendShiftShortcut(ui::KeyboardCode key); nit: I'd opt to just use "SendShortcut(foo, true);" and remove this helper. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:107: }; nit: add the private: DISSALLOW_COPY_AND_ASSIGN(BrowserCommandControllerInteractiveTest); https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:144: // Command + Shift + F shortcut is not registered to interactive_ui_tests, so nit: indent these two comment lines https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:145: // sending directly a fullscreen command instead. nit: "send a fullscreen command directly instead." https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:146: ASSERT_TRUE(chrome::ExecuteCommand(browser(), IDC_FULLSCREEN)); Strange, can you point to a bug or more info about why accelerators (eg. this and Command+W) are not registered on Mac interactive test environments? +tapted for advice https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:148: // A dedicated fullscreen key is used on Chrome OS, so sending directly a nit: "send a fullscreen command directly instead, to avoid constructing the key press." https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:236: // TODO(zijiehe): Investigate why inconsistent key event sequence has been Can you clarify what's unexpected about the reported key event sequences here and elsewhere? Is it the lack of the "keyup KeyP" events (and keyup for other non-modifier accelerator keys)? +tapted for thoughts. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:268: // TODO(zijiehe): Figure out why this test crashes on Mac OSX. The suspecious nit: suspicious https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:270: #define MAYBE(X) DISABLED_##X Hmm, bummer this entire test is broken on Mac... tapted? https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:272: #define MAYBE(X) X Please follow the more common pattern of: #if defined(OS_MACOSX) #define MAYBE_KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen DISABLED_KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen #else #define MAYBE_KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen #endif to avoid defining and undefining a more generic MAYBE() macro. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:281: // Close tab. Rather than just commenting on what the shortcut typically does, can you explain and/or test the expected outcomes after pressing these accelerators? ie. instead of say "Close tab." say something like "The tab should not close.", and then assert that the tab count hasn't changed. Ditto for all the other similar comments, like "New window." -> "A new window should not be opened.", or put something above these comments like "None of the accelerators should have an effect on the browser or tab strip when the page is in fullscreen mode." ditto in the other test fixtures. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:285: // TODO(zijiehe): ChromeOS has special shortcuts for the following four nit: "// TODO(zijiehe): ChromeOS incorrectly handles these; see crbug.com/737307." ditto elsewhere below. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:425: KeyEventsShouldBeConsumedByWebPageInJsFullscreenExceptForEsc) { Can you split these tests up a bit to avoid redundancy: 1) KeyEventsShouldBeConsumedByWebPageInJsFullscreen (tests all the shortcuts in JS) 2) EscShouldNotBeConsumedByWebPageInJsFullscreen (just tests Esc) 3) F11ShouldNotBeConsumedByWebPageInJsFullscreen (just tests F11) Bonus points if you manage to factor out a single helper that tests all the shortcuts, used by the browser and js fullscreen test fixtures (so we don't have two big blocks of "keydown..."). https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... File chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:1: <!DOCTYPE html> Someone more familiar with test html helper files should take a look here. In particular, I'm not familiar with promises and the timeout functionality. https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:13: function report() { nit: Is "report" a common name for this type of function? Consider "getKeyEventReport" or something similarly more explanatory? https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:55: function consumeEvent(t, e) { nit: give a better name for the type/title than 't' https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:61: document.addEventListener('keydown', (e) => { nit: add a comment explaining the purpose of this handler (ie. "The web content triggers fullscreen on the 's' key press event") https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:64: } else if (isBrowserFullscreenShortcut(e)) { aside q: Can websites preventDefault on the fullscreen shortcuts like F11 and Meta+Ctrl+F? Are accelerators like Ctrl+W also sent to the page first and eligible for preventDefault? (sorry for forgetting the details of your recent behavior change). Could a page could trap users in fullscreen (making their only resort to Ctrl+Alt+Delete or similar to kill the tab/browser)?
The CQ bit was checked by zijiehe@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.
https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:30: "/fullscreen_keyboardlock/fullscreen_keyboardlock.html"; On 2017/06/30 20:17:29, msw wrote: > It seems a bit silly to create a new subdirectory for this one html file, but I > guess that's already the case for fullscreen_mouselock... I'd suggest moving > both to the parent chrome/test/data/ directory, but that's not critical. Yes, I followed the fullscreen_mouselock. Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:32: class TabCountObserver : public TabStripModelObserver { On 2017/06/30 20:17:28, msw wrote: > Could you replace this class with a simpler helper function, like: > // Wait for the browser to have the expected tab count, or timeout. > bool WaitForTabCount(Browser* browser, int tab_count) { > const int kMaxTries = 10; > for (int i = 0; i < kMaxTries && browser->tab_strip_model()->count() != > tab_count; ++i) > content::RunAllPendingInMessageLoop(); > return browser->tab_strip_model()->count() == tab_count; > } Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:39: void TabInsertedAt(TabStripModel* tab_strip_model, On 2017/06/30 20:17:29, msw wrote: > Since you use this class to also track the closing of tabs, is it necessary to > watch for TabClosingAt, in case the tab isn't closed synchronously with the > accelerator (and the initial tab count in TabCountObserver's ctor is greater > than the expected number of tabs)? Presumably it's safe to ignore TabDetachedAt > (etc.), since nothing should be detaching tabs in this test (etc.). Since a function is used to replace the observer, following several comments are out-of-date. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:45: bool seen_ = false; On 2017/06/30 20:17:29, msw wrote: > nit: rename |seen_| to something like |has_expected_tab_count_| Acknowledged. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:51: seen_ = (browser->tab_strip_model()->count() == expected_); On 2017/06/30 20:17:28, msw wrote: > nit: parens not needed Acknowledged. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:52: if (!seen_) { On 2017/06/30 20:17:28, msw wrote: > nit: curlies not needed for one line conditional and body Acknowledged. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:56: TabCountObserver::~TabCountObserver() { On 2017/06/30 20:17:28, msw wrote: > nit: blank line above (or inline the definitions in the declarations above) Acknowledged. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:57: while (!seen_) { On 2017/06/30 20:17:29, msw wrote: > ditto nit: curlies not needed for one line conditional and body Acknowledged. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:66: if (tab_strip_model->count() == expected_) { On 2017/06/30 20:17:28, msw wrote: > ditto nit: curlies not needed for one line conditional and body, or do: > seen_ |= tab_strip_model->count() == expected_; Acknowledged. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:83: // modifier will be added once |shift| is true. On 2017/06/30 20:17:28, msw wrote: > nit: s/once/if/ Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:87: void SendShiftShortcut(ui::KeyboardCode key); On 2017/06/30 20:17:28, msw wrote: > nit: I'd opt to just use "SendShortcut(foo, true);" and remove this helper. My concern is SendShortcut(ui::VKEY_A, true); is not as clear as SendShirtShortcut(ui::VKEY_A); If you do not have a very strong opinion, I would like to keep it. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:107: }; On 2017/06/30 20:17:28, msw wrote: > nit: add the private: > DISSALLOW_COPY_AND_ASSIGN(BrowserCommandControllerInteractiveTest); Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:144: // Command + Shift + F shortcut is not registered to interactive_ui_tests, so On 2017/06/30 20:17:29, msw wrote: > nit: indent these two comment lines Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:145: // sending directly a fullscreen command instead. On 2017/06/30 20:17:28, msw wrote: > nit: "send a fullscreen command directly instead." Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:146: ASSERT_TRUE(chrome::ExecuteCommand(browser(), IDC_FULLSCREEN)); On 2017/06/30 20:17:28, msw wrote: > Strange, can you point to a bug or more info about why accelerators (eg. this > and Command+W) are not registered on Mac interactive test environments? +tapted > for advice I have very limited understanding to MacOSX. But it looks like all the shortcuts are defined in src/chrome/app/nibs/MainMenu.xib, which is included in to "chrome_xibs" target, and referred by "chrome_framework" target only. The conclusion is based on my test: run the test, and types shortcuts when the window of interactive_ui_tests is showing. Some takes effect, such as ctrl + t, some not, such as ctrl + w and ctrl + shift + f. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:148: // A dedicated fullscreen key is used on Chrome OS, so sending directly a On 2017/06/30 20:17:28, msw wrote: > nit: "send a fullscreen command directly instead, to avoid constructing the key > press." Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:236: // TODO(zijiehe): Investigate why inconsistent key event sequence has been On 2017/06/30 20:17:28, msw wrote: > Can you clarify what's unexpected about the reported key event sequences here > and elsewhere? Is it the lack of the "keyup KeyP" events (and keyup for other > non-modifier accelerator keys)? +tapted for thoughts. IMO, this is an issue in test utilities: the test web page works well on my machine. Though it does not look like a trivial fix. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:268: // TODO(zijiehe): Figure out why this test crashes on Mac OSX. The suspecious On 2017/06/30 20:17:28, msw wrote: > nit: suspicious Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:272: #define MAYBE(X) X On 2017/06/30 20:17:28, msw wrote: > Please follow the more common pattern of: > #if defined(OS_MACOSX) > #define MAYBE_KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen > DISABLED_KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen > #else > #define MAYBE_KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen > KeyEventsShouldBeConsumedByWebPageInBrowserFullscreen > #endif > to avoid defining and undefining a more generic MAYBE() macro. Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:281: // Close tab. On 2017/06/30 20:17:28, msw wrote: > Rather than just commenting on what the shortcut typically does, can you explain > and/or test the expected outcomes after pressing these accelerators? ie. instead > of say "Close tab." say something like "The tab should not close.", and then > assert that the tab count hasn't changed. Ditto for all the other similar > comments, like "New window." -> "A new window should not be opened.", or put > something above these comments like "None of the accelerators should have an > effect on the browser or tab strip when the page is in fullscreen mode." ditto > in the other test fixtures. Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:285: // TODO(zijiehe): ChromeOS has special shortcuts for the following four On 2017/06/30 20:17:29, msw wrote: > nit: "// TODO(zijiehe): ChromeOS incorrectly handles these; see > crbug.com/737307." ditto elsewhere below. Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:425: KeyEventsShouldBeConsumedByWebPageInJsFullscreenExceptForEsc) { On 2017/06/30 20:17:28, msw wrote: > Can you split these tests up a bit to avoid redundancy: > 1) KeyEventsShouldBeConsumedByWebPageInJsFullscreen (tests all the shortcuts in > JS) > 2) EscShouldNotBeConsumedByWebPageInJsFullscreen (just tests Esc) > 3) F11ShouldNotBeConsumedByWebPageInJsFullscreen (just tests F11) > Bonus points if you manage to factor out a single helper that tests all the > shortcuts, used by the browser and js fullscreen test fixtures (so we don't have > two big blocks of "keydown..."). Yes, maybe I can use Send* functions to generate the result instead of write them manually. https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... File chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:13: function report() { On 2017/06/30 20:17:29, msw wrote: > nit: Is "report" a common name for this type of function? Consider > "getKeyEventReport" or something similarly more explanatory? Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:55: function consumeEvent(t, e) { On 2017/06/30 20:17:29, msw wrote: > nit: give a better name for the type/title than 't' Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:61: document.addEventListener('keydown', (e) => { On 2017/06/30 20:17:29, msw wrote: > nit: add a comment explaining the purpose of this handler (ie. "The web content > triggers fullscreen on the 's' key press event") Done. https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:64: } else if (isBrowserFullscreenShortcut(e)) { On 2017/06/30 20:17:29, msw wrote: > aside q: Can websites preventDefault on the fullscreen shortcuts like F11 and > Meta+Ctrl+F? Are accelerators like Ctrl+W also sent to the page first and > eligible for preventDefault? (sorry for forgetting the details of your recent > behavior change). Could a page could trap users in fullscreen (making their only > resort to Ctrl+Alt+Delete or similar to kill the tab/browser)? No, otherwise the "ExceptForF11" test will fail. But web page can preventDefault of an F11 fullscreen request in window mode, which is the reason of this condition.
https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:146: ASSERT_TRUE(chrome::ExecuteCommand(browser(), IDC_FULLSCREEN)); On 2017/07/01 01:56:47, Hzj_jie wrote: > On 2017/06/30 20:17:28, msw wrote: > > Strange, can you point to a bug or more info about why accelerators (eg. this > > and Command+W) are not registered on Mac interactive test environments? > +tapted > > for advice > > I have very limited understanding to MacOSX. But it looks like all the shortcuts > are defined in src/chrome/app/nibs/MainMenu.xib, which is included in to > "chrome_xibs" target, and referred by "chrome_framework" target only. > > The conclusion is based on my test: run the test, and types shortcuts when the > window of interactive_ui_tests is showing. Some takes effect, such as ctrl + t, > some not, such as ctrl + w and ctrl + shift + f. This doesn't match my experience. There is an interactive UI test running on Mac that successfully sends Cmd+w and other shortcuts. E.g. PermissionBubbleInteractiveUITest.CmdWClosesWindow -- https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/permission_bubbl... https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:161: chrome::NOTIFICATION_FULLSCREEN_CHANGED, This might not be enough for Mac. We have a NSWindowFullscreenNotificationWaiter, but it only works in .mm files. A good first step would be to get the test working using ui::test::ScopedFakeNSWindowFullscreen -- this _can_ be used in .cc files (still guard with #if defined(OS_MACOSX)) Tests that don't use ScopedFakeNSWindowFullscreen tend to be flaky on Mac. There are only a few tests, specifically for testing fullscreen, which don't use it (and are not disabled). We probably do want a test that _doesn't_ use ScopedFakeNSWindowFullscreen, but it might need to be run manually rather than on the bots. There's a MANUAL_ prefix that may help -- https://cs.chromium.org/chromium/src/content/public/test/test_launcher.cc?typ... -- but usually there's an associated .py script to run the test with an appropriate --gtest_filter But if you can get these test passing on Mac using ScopedFakeNSWindowFullscreen then that is preferable to having the tests disabled or MANUAL_ - prefixed. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:236: // TODO(zijiehe): Investigate why inconsistent key event sequence has been On 2017/07/01 01:56:47, Hzj_jie wrote: > On 2017/06/30 20:17:28, msw wrote: > > Can you clarify what's unexpected about the reported key event sequences here > > and elsewhere? Is it the lack of the "keyup KeyP" events (and keyup for other > > non-modifier accelerator keys)? +tapted for thoughts. > > IMO, this is an issue in test utilities: the test web page works well on my > machine. Though it does not look like a trivial fix. This probably has something to do with how the test events are generated under SendKeyPressSync - going into this much detail is probably testing the test event generator rather than actual production code. Can you perhaps filter out modifier keys and just check the modifier flags on the events for the non-control keys? I think this much detail make the test fragile. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:270: #define MAYBE(X) DISABLED_##X On 2017/06/30 20:17:29, msw wrote: > Hmm, bummer this entire test is broken on Mac... tapted? urp - yeah what's the crash stacktrace? There are a bunch fullscreen tests that are disabled, but they still run fine on Mac when using --gtest_also_run_disabled_tests E.g. $ ./out/gn_Debug/interactive_ui_tests --gtest_filter=FullscreenControllerInteractiveTest.* --gtest_also_run_disabled_tests / *snip */ [19/19] FullscreenControllerInteractiveTest.DISABLED_ToggleFullscreenModeForTab (2373 ms) SUCCESS: all tests passed.
Thank you for the comments. I will investigate the IN_PROC_BROWSER_TEST_P(PermissionBubbleInteractiveUITest, CmdWClosesWindow) test tomorrow. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:161: chrome::NOTIFICATION_FULLSCREEN_CHANGED, On 2017/07/03 00:57:47, tapted wrote: > This might not be enough for Mac. We have a > NSWindowFullscreenNotificationWaiter, but it only works in .mm files. A good > first step would be to get the test working using > ui::test::ScopedFakeNSWindowFullscreen -- this _can_ be used in .cc files (still > guard with #if defined(OS_MACOSX)) > > Tests that don't use ScopedFakeNSWindowFullscreen tend to be flaky on Mac. There > are only a few tests, specifically for testing fullscreen, which don't use it > (and are not disabled). > > We probably do want a test that _doesn't_ use ScopedFakeNSWindowFullscreen, but > it might need to be run manually rather than on the bots. > > There's a MANUAL_ prefix that may help -- > https://cs.chromium.org/chromium/src/content/public/test/test_launcher.cc?typ... > -- but usually there's an associated .py script to run the test with an > appropriate --gtest_filter > > > But if you can get these test passing on Mac using ScopedFakeNSWindowFullscreen > then that is preferable to having the tests disabled or MANUAL_ - prefixed. Emmm, indeed this test passes on Mac OSX, though I believe ui::test::ScopedFakeNSWindowFullscreen has a totally different implementation. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:236: // TODO(zijiehe): Investigate why inconsistent key event sequence has been On 2017/07/03 00:57:47, tapted wrote: > On 2017/07/01 01:56:47, Hzj_jie wrote: > > On 2017/06/30 20:17:28, msw wrote: > > > Can you clarify what's unexpected about the reported key event sequences > here > > > and elsewhere? Is it the lack of the "keyup KeyP" events (and keyup for > other > > > non-modifier accelerator keys)? +tapted for thoughts. > > > > IMO, this is an issue in test utilities: the test web page works well on my > > machine. Though it does not look like a trivial fix. > > This probably has something to do with how the test events are generated under > SendKeyPressSync - going into this much detail is probably testing the test > event generator rather than actual production code. Can you perhaps filter out > modifier keys and just check the modifier flags on the events for the > non-control keys? I think this much detail make the test fragile. > Besides the order of modifier keys, the keyup event cannot be caught by interactive_ui_tests on Mac OSX. The order of modifier keys can be easily fixed, since it's generated at https://cs.chromium.org/chromium/src/ui/base/test/ui_controls_mac.mm?type=cs&.... Though I think it may not safe to do so as I cannot guarantee whether there are other tests which depend on the predefined order. I will try it tomorrow. But I do not know the reason of lacking keyup event. https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:270: #define MAYBE(X) DISABLED_##X On 2017/07/03 00:57:47, tapted wrote: > On 2017/06/30 20:17:29, msw wrote: > > Hmm, bummer this entire test is broken on Mac... tapted? > > urp - yeah what's the crash stacktrace? There are a bunch fullscreen tests that > are disabled, but they still run fine on Mac when using > --gtest_also_run_disabled_tests > > E.g. > > $ ./out/gn_Debug/interactive_ui_tests > --gtest_filter=FullscreenControllerInteractiveTest.* > --gtest_also_run_disabled_tests > / *snip */ > [19/19] FullscreenControllerInteractiveTest.DISABLED_ToggleFullscreenModeForTab > (2373 ms) > SUCCESS: all tests passed. > I tend to believe combining content::WindowedNotificationObserver observer( chrome::NOTIFICATION_FULLSCREEN_CHANGED, content::NotificationService::AllSources()); with SendKeyPressSync() will trigger the crash. The crash randomly happens after several SendKeyPressSync() function call. I will provide a callstack tomorrow, and try if ScopedFakeNSWindowFullscreen resolves the issue.
https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:236: // TODO(zijiehe): Investigate why inconsistent key event sequence has been On 2017/07/03 05:08:14, Hzj_jie wrote: > On 2017/07/03 00:57:47, tapted wrote: > > On 2017/07/01 01:56:47, Hzj_jie wrote: > > > On 2017/06/30 20:17:28, msw wrote: > > > > Can you clarify what's unexpected about the reported key event sequences > > here > > > > and elsewhere? Is it the lack of the "keyup KeyP" events (and keyup for > > other > > > > non-modifier accelerator keys)? +tapted for thoughts. > > > > > > IMO, this is an issue in test utilities: the test web page works well on my > > > machine. Though it does not look like a trivial fix. > > > > This probably has something to do with how the test events are generated under > > SendKeyPressSync - going into this much detail is probably testing the test > > event generator rather than actual production code. Can you perhaps filter out > > modifier keys and just check the modifier flags on the events for the > > non-control keys? I think this much detail make the test fragile. > > > > Besides the order of modifier keys, the keyup event cannot be caught by > interactive_ui_tests on Mac OSX. > The order of modifier keys can be easily fixed, since it's generated at > https://cs.chromium.org/chromium/src/ui/base/test/ui_controls_mac.mm?type=cs&.... > Though I think it may not safe to do so as I cannot guarantee whether there are > other tests which depend on the predefined order. I will try it tomorrow. But I > do not know the reason of lacking keyup event. The point is that if a result in a test is not relevant for what you're trying to test, you shouldn't check for it. It makes the test fragile without adding extra code coverage or any benefit for catching real regressions. The print preview dialog triggers on key *down* on all platforms. The key up is not relevant: Chrome ignores it. Likewise, Chrome does not react to changes in Command/Control/Shift modifier keys. Here, it looks like you can test for two specific events to occur: 1) A KeyDown for "p" with modifier flags indicating that Control is down at the same time 2) A KeyDown for "p" with modifier flags indicating that both Control and Shift were down at the same time As well as adding fragility, testing for other things makes it harder to tell why the test exists - i.e. why it's important that it shouldn't fail. I this case, the behaviour for keup on Mac seems to be intentional - http://crbug.com/28089 mentions it. Basically Safari doesn't send keyup when a command modifier is present, so Chrome doesn't either. See also http://crbug.com/126282 and a few others if you search for OS=Mac + "keyup"
https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:236: // TODO(zijiehe): Investigate why inconsistent key event sequence has been On 2017/07/03 05:52:41, tapted wrote: > On 2017/07/03 05:08:14, Hzj_jie wrote: > > On 2017/07/03 00:57:47, tapted wrote: > > > On 2017/07/01 01:56:47, Hzj_jie wrote: > > > > On 2017/06/30 20:17:28, msw wrote: > > > > > Can you clarify what's unexpected about the reported key event sequences > > > here > > > > > and elsewhere? Is it the lack of the "keyup KeyP" events (and keyup for > > > other > > > > > non-modifier accelerator keys)? +tapted for thoughts. > > > > > > > > IMO, this is an issue in test utilities: the test web page works well on > my > > > > machine. Though it does not look like a trivial fix. > > > > > > This probably has something to do with how the test events are generated > under > > > SendKeyPressSync - going into this much detail is probably testing the test > > > event generator rather than actual production code. Can you perhaps filter > out > > > modifier keys and just check the modifier flags on the events for the > > > non-control keys? I think this much detail make the test fragile. > > > > > > > Besides the order of modifier keys, the keyup event cannot be caught by > > interactive_ui_tests on Mac OSX. > > The order of modifier keys can be easily fixed, since it's generated at > > > https://cs.chromium.org/chromium/src/ui/base/test/ui_controls_mac.mm?type=cs&.... > > Though I think it may not safe to do so as I cannot guarantee whether there > are > > other tests which depend on the predefined order. I will try it tomorrow. But > I > > do not know the reason of lacking keyup event. > > The point is that if a result in a test is not relevant for what you're trying > to test, you shouldn't check for it. It makes the test fragile without adding > extra code coverage or any benefit for catching real regressions. > > The print preview dialog triggers on key *down* on all platforms. The key up is > not relevant: Chrome ignores it. Likewise, Chrome does not react to changes in > Command/Control/Shift modifier keys. > > Here, it looks like you can test for two specific events to occur: > 1) A KeyDown for "p" with modifier flags indicating that Control is down at the > same time > 2) A KeyDown for "p" with modifier flags indicating that both Control and Shift > were down at the same time > > > As well as adding fragility, testing for other things makes it harder to tell > why the test exists - i.e. why it's important that it shouldn't fail. > > I this case, the behaviour for keup on Mac seems to be intentional - > http://crbug.com/28089 mentions it. Basically Safari doesn't send keyup when a > command modifier is present, so Chrome doesn't either. See also > http://crbug.com/126282 and a few others if you search for OS=Mac + "keyup" Thank you for the information. Since the keyup is expected to be ignored and the order of control / shift is also controlled by test utils, the TODO can be replaced by bug # and link to ui_controls_mac.mm. This "#if defined(OS_MACOSX)" does not add any fragility: only an inconsistent behavior on Mac OSX and other platforms.
https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:236: // TODO(zijiehe): Investigate why inconsistent key event sequence has been On 2017/07/03 20:08:15, Hzj_jie wrote: > On 2017/07/03 05:52:41, tapted wrote: > > On 2017/07/03 05:08:14, Hzj_jie wrote: > > > On 2017/07/03 00:57:47, tapted wrote: > > > > On 2017/07/01 01:56:47, Hzj_jie wrote: > > > > > On 2017/06/30 20:17:28, msw wrote: > > > > > > Can you clarify what's unexpected about the reported key event > sequences > > > > here > > > > > > and elsewhere? Is it the lack of the "keyup KeyP" events (and keyup > for > > > > other > > > > > > non-modifier accelerator keys)? +tapted for thoughts. > > > > > > > > > > IMO, this is an issue in test utilities: the test web page works well on > > my > > > > > machine. Though it does not look like a trivial fix. > > > > > > > > This probably has something to do with how the test events are generated > > under > > > > SendKeyPressSync - going into this much detail is probably testing the > test > > > > event generator rather than actual production code. Can you perhaps filter > > out > > > > modifier keys and just check the modifier flags on the events for the > > > > non-control keys? I think this much detail make the test fragile. > > > > > > > > > > Besides the order of modifier keys, the keyup event cannot be caught by > > > interactive_ui_tests on Mac OSX. > > > The order of modifier keys can be easily fixed, since it's generated at > > > > > > https://cs.chromium.org/chromium/src/ui/base/test/ui_controls_mac.mm?type=cs&.... > > > Though I think it may not safe to do so as I cannot guarantee whether there > > are > > > other tests which depend on the predefined order. I will try it tomorrow. > But > > I > > > do not know the reason of lacking keyup event. > > > > The point is that if a result in a test is not relevant for what you're trying > > to test, you shouldn't check for it. It makes the test fragile without adding > > extra code coverage or any benefit for catching real regressions. > > > > The print preview dialog triggers on key *down* on all platforms. The key up > is > > not relevant: Chrome ignores it. Likewise, Chrome does not react to changes in > > Command/Control/Shift modifier keys. > > > > Here, it looks like you can test for two specific events to occur: > > 1) A KeyDown for "p" with modifier flags indicating that Control is down at > the > > same time > > 2) A KeyDown for "p" with modifier flags indicating that both Control and > Shift > > were down at the same time > > > > > > As well as adding fragility, testing for other things makes it harder to tell > > why the test exists - i.e. why it's important that it shouldn't fail. > > > > I this case, the behaviour for keup on Mac seems to be intentional - > > http://crbug.com/28089 mentions it. Basically Safari doesn't send keyup when > a > > command modifier is present, so Chrome doesn't either. See also > > http://crbug.com/126282 and a few others if you search for OS=Mac + "keyup" > > Thank you for the information. Since the keyup is expected to be ignored and the > order of control / shift is also controlled by test utils, the TODO can be > replaced by bug # and link to ui_controls_mac.mm. > > This "#if defined(OS_MACOSX)" does not add any fragility: only an inconsistent > behavior on Mac OSX and other platforms. The kind of fragility I'm talking about here comes from over-specifying the expectation. Ideally, a test only fails if there is a regression in the thing it is testing, or someone modifies the behavior of the specific thing being tested. In this case, someone may fix http://crbug.com/28089 so that Chrome's behaviour is a closer match to Safari (e.g. compare http://unixpapa.com/js/testkey.html with Chrome and Safari). Fixing http://crbug.com/28089 would involve additional events appearing on Mac and these expectations to fail. The author then needs to reason about whether the change represents a regression to your feature or not. Testing the full key sequence is OK, but it should be in a test called "BrowserCommandControllerInteractiveTest.VerifyKeyEventSequence". For the other tests, the full sequence is incidental to whether a regression has occurred: a change in the sequence is not necessary to detect a regression.
The CQ bit was checked by zijiehe@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 checked by zijiehe@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 checked by zijiehe@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/07/03 23:22:17, tapted wrote: > https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... > File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc > (right): > > https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:236: // > TODO(zijiehe): Investigate why inconsistent key event sequence has been > On 2017/07/03 20:08:15, Hzj_jie wrote: > > On 2017/07/03 05:52:41, tapted wrote: > > > On 2017/07/03 05:08:14, Hzj_jie wrote: > > > > On 2017/07/03 00:57:47, tapted wrote: > > > > > On 2017/07/01 01:56:47, Hzj_jie wrote: > > > > > > On 2017/06/30 20:17:28, msw wrote: > > > > > > > Can you clarify what's unexpected about the reported key event > > sequences > > > > > here > > > > > > > and elsewhere? Is it the lack of the "keyup KeyP" events (and keyup > > for > > > > > other > > > > > > > non-modifier accelerator keys)? +tapted for thoughts. > > > > > > > > > > > > IMO, this is an issue in test utilities: the test web page works well > on > > > my > > > > > > machine. Though it does not look like a trivial fix. > > > > > > > > > > This probably has something to do with how the test events are generated > > > under > > > > > SendKeyPressSync - going into this much detail is probably testing the > > test > > > > > event generator rather than actual production code. Can you perhaps > filter > > > out > > > > > modifier keys and just check the modifier flags on the events for the > > > > > non-control keys? I think this much detail make the test fragile. > > > > > > > > > > > > > Besides the order of modifier keys, the keyup event cannot be caught by > > > > interactive_ui_tests on Mac OSX. > > > > The order of modifier keys can be easily fixed, since it's generated at > > > > > > > > > > https://cs.chromium.org/chromium/src/ui/base/test/ui_controls_mac.mm?type=cs&.... > > > > Though I think it may not safe to do so as I cannot guarantee whether > there > > > are > > > > other tests which depend on the predefined order. I will try it tomorrow. > > But > > > I > > > > do not know the reason of lacking keyup event. > > > > > > The point is that if a result in a test is not relevant for what you're > trying > > > to test, you shouldn't check for it. It makes the test fragile without > adding > > > extra code coverage or any benefit for catching real regressions. > > > > > > The print preview dialog triggers on key *down* on all platforms. The key up > > is > > > not relevant: Chrome ignores it. Likewise, Chrome does not react to changes > in > > > Command/Control/Shift modifier keys. > > > > > > Here, it looks like you can test for two specific events to occur: > > > 1) A KeyDown for "p" with modifier flags indicating that Control is down at > > the > > > same time > > > 2) A KeyDown for "p" with modifier flags indicating that both Control and > > Shift > > > were down at the same time > > > > > > > > > As well as adding fragility, testing for other things makes it harder to > tell > > > why the test exists - i.e. why it's important that it shouldn't fail. > > > > > > I this case, the behaviour for keup on Mac seems to be intentional - > > > http://crbug.com/28089 mentions it. Basically Safari doesn't send keyup > when > > a > > > command modifier is present, so Chrome doesn't either. See also > > > http://crbug.com/126282 and a few others if you search for OS=Mac + "keyup" > > > > Thank you for the information. Since the keyup is expected to be ignored and > the > > order of control / shift is also controlled by test utils, the TODO can be > > replaced by bug # and link to ui_controls_mac.mm. > > > > This "#if defined(OS_MACOSX)" does not add any fragility: only an inconsistent > > behavior on Mac OSX and other platforms. > > The kind of fragility I'm talking about here comes from over-specifying the > expectation. Ideally, a test only fails if there is a regression in the thing it > is testing, or someone modifies the behavior of the specific thing being tested. > In this case, someone may fix http://crbug.com/28089 so that Chrome's behaviour > is a closer match to Safari (e.g. compare http://unixpapa.com/js/testkey.html > with Chrome and Safari). Fixing http://crbug.com/28089 would involve additional > events appearing on Mac and these expectations to fail. The author then needs to > reason about whether the change represents a regression to your feature or not. > > Testing the full key sequence is OK, but it should be in a test called > "BrowserCommandControllerInteractiveTest.VerifyKeyEventSequence". For the other > tests, the full sequence is incidental to whether a regression has occurred: a > change in the sequence is not necessary to detect a regression. I believe http://crbug.com/28089 is talking about key press event instead of key up :) Meanwhile key up is expected to be ignored to match safari, which is special handled by the test cases. And when keydown is prevented, keypress should not be fired (https://www.w3.org/TR/DOM-Level-3-Events/#event-type-keydown), so even http://crbug.com/28089 is fixed, these test cases should not be impacted: keydown events have been prevented already. So in short these test assertions are following a "correct" behavior of the web browser on MacOSX. But I do have concern about the reordering of MetaLeft and ShiftLeft: putting MetaLeft before ShiftLeft seems more reasonable on MacOSX, it replaces ControlLeft for all accelerators. I am trying to reorder them in ui_controls::SendKeyPressNotifyWhenDone() in change https://codereview.chromium.org/2970673003/. Hope it won't introduce other failures. I still think key sequences are useful for these test cases: web page in fullscreen mode should be able to receive and preventDefault() for these keys. Even Chrome does not react on keyup events, lacking keyup events in webpage is still a regression.
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thank you for continuing to refine these tests! https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... File chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:64: } else if (isBrowserFullscreenShortcut(e)) { On 2017/07/01 01:56:48, Hzj_jie wrote: > On 2017/06/30 20:17:29, msw wrote: > > aside q: Can websites preventDefault on the fullscreen shortcuts like F11 and > > Meta+Ctrl+F? Are accelerators like Ctrl+W also sent to the page first and > > eligible for preventDefault? (sorry for forgetting the details of your recent > > behavior change). Could a page could trap users in fullscreen (making their > only > > resort to Ctrl+Alt+Delete or similar to kill the tab/browser)? > > No, otherwise the "ExceptForF11" test will fail. But web page can preventDefault > of an F11 fullscreen request in window mode, which is the reason of this > condition. Actually, it might be interesting to construct a test where the html/js attempts to 'consumeEvent'/|e.preventDefault()| when the event is the F11 key (and the Meta+Ctrl+F). This code does not attempt to preventDefault for those keys. https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:37: #if defined(OS_MACOSX) nit: move the #if block inside the function, all platforms can share one function declaration: // On MacOSX command key is used for most of the shortcuts, so replace it with // control to reduce the complexity of comparison of the results. void NormalizeMetaKeyForMacOS(std::string* output) { #if defined(OS_MACOSX) base::ReplaceSubstringsAfterOffset(output, 0, "MetaLeft", "ControlLeft"); #endif } https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:83: // the result and verifies if it equals to |expected_result_|. nit: "if it equals |expected_result_|." of "if it is equal to..." https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:122: // 1. ui_controls_mac.mm puts shift key before meta (command) key. Trent's point about test fragility is very much worth considering. Instead of the html/js reporting the entire sequence of down and up events (like "keydown ShiftLeft\nkeydown ControlLeft\nkeydown keyW\nkeyup keyW\n\nkeyup ControlLeft\nkeyup ShiftLeft\n"), can the it just report the state of the modifiers at the time of the non-modifier keys (like "key:w ctrl:1 shift:1 alt:0 meta:0\n" or similar)? That would make this command controller test completely agnostic to differences in generated sequences and robust against bugs/changes with unreported keyup events, etc. https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:232: ASSERT_EQ(result, expected_result_); nit: ASSERT_EQ and EXPECTED_EQ macros should have the expected value as the first argument, and the actual value as the second argument. https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:289: // The tab should not be closed. Can you make a helper function that sends these commands, since the code is repeated three times? The code should also test for the expected behavior. For example: void CheckShortcutsInFullscreen() { // Add a second tab for counting and focus purposes. AddTabAtIndex(1, GURL("about:blank"), ui::PAGE_TRANSITION_LINK); ASSERT_EQ(2, browser()->tab_strip_model()->count()); ASSERT_EQ(0, browser()->tab_strip_model()->active_index()); ASSERT_EQ(1, BrowserList::GetInstance()->size()); // The tab should not be closed. SendShortcut(ui::VKEY_W); ASSERT_EQ(2, browser()->tab_strip_model()->count()); // The window should not be closed. SendShiftShortcut(ui::VKEY_W); ASSERT_EQ(1, BrowserList::GetInstance()->size()); // TODO(zijiehe): ChromeOS incorrectly handles these; // see http://crbug.com/737307. #if !defined(OS_CHROMEOS) // A new tab should not be created. SendShortcut(ui::VKEY_T); ASSERT_EQ(2, browser()->tab_strip_model()->count()); // A new window should not be created. SendShortcut(ui::VKEY_N); ASSERT_EQ(1, BrowserList::GetInstance()->size()); // A new incognito window should not be created. SendShiftShortcut(ui::VKEY_N); ASSERT_EQ(1, BrowserList::GetInstance()->size()); // Last closed tab should not be restored. SendShiftShortcut(ui::VKEY_T); ASSERT_EQ(2, browser()->tab_strip_model()->count()); #endif // Browser should not switch to the next tab. SendShortcut(ui::VKEY_TAB); ASSERT_EQ(0, browser()->tab_strip_model()->active_index()); // Browser should not switch to the previous tab. SendShiftShortcut(ui::VKEY_TAB); ASSERT_EQ(0, browser()->tab_strip_model()->active_index()); }
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zijiehe@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.
https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:146: ASSERT_TRUE(chrome::ExecuteCommand(browser(), IDC_FULLSCREEN)); On 2017/07/03 00:57:47, tapted wrote: > On 2017/07/01 01:56:47, Hzj_jie wrote: > > On 2017/06/30 20:17:28, msw wrote: > > > Strange, can you point to a bug or more info about why accelerators (eg. > this > > > and Command+W) are not registered on Mac interactive test environments? > > +tapted > > > for advice > > > > I have very limited understanding to MacOSX. But it looks like all the > shortcuts > > are defined in src/chrome/app/nibs/MainMenu.xib, which is included in to > > "chrome_xibs" target, and referred by "chrome_framework" target only. > > > > The conclusion is based on my test: run the test, and types shortcuts when the > > window of interactive_ui_tests is showing. Some takes effect, such as ctrl + > t, > > some not, such as ctrl + w and ctrl + shift + f. > > This doesn't match my experience. > > There is an interactive UI test running on Mac that successfully sends Cmd+w and > other shortcuts. E.g. PermissionBubbleInteractiveUITest.CmdWClosesWindow -- > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/permission_bubbl... > Found the root cause: interactive_ui_tests window won't be able to receive most of the shortcuts without executing BringBrowserWindowToFront(). https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:270: #define MAYBE(X) DISABLED_##X On 2017/07/03 05:08:15, Hzj_jie wrote: > On 2017/07/03 00:57:47, tapted wrote: > > On 2017/06/30 20:17:29, msw wrote: > > > Hmm, bummer this entire test is broken on Mac... tapted? > > > > urp - yeah what's the crash stacktrace? There are a bunch fullscreen tests > that > > are disabled, but they still run fine on Mac when using > > --gtest_also_run_disabled_tests > > > > E.g. > > > > $ ./out/gn_Debug/interactive_ui_tests > > --gtest_filter=FullscreenControllerInteractiveTest.* > > --gtest_also_run_disabled_tests > > / *snip */ > > [19/19] > FullscreenControllerInteractiveTest.DISABLED_ToggleFullscreenModeForTab > > (2373 ms) > > SUCCESS: all tests passed. > > > > I tend to believe combining > content::WindowedNotificationObserver observer( > chrome::NOTIFICATION_FULLSCREEN_CHANGED, > content::NotificationService::AllSources()); > with > SendKeyPressSync() > will trigger the crash. > The crash randomly happens after several SendKeyPressSync() function call. > I will provide a callstack tomorrow, and try if ScopedFakeNSWindowFullscreen > resolves the issue. Crash stack traces are included in bug http://crbug.com/738949. https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... File chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html (right): https://codereview.chromium.org/2922773002/diff/630001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock/fullscreen_keyboardlock.html:64: } else if (isBrowserFullscreenShortcut(e)) { On 2017/07/05 19:11:44, msw wrote: > On 2017/07/01 01:56:48, Hzj_jie wrote: > > On 2017/06/30 20:17:29, msw wrote: > > > aside q: Can websites preventDefault on the fullscreen shortcuts like F11 > and > > > Meta+Ctrl+F? Are accelerators like Ctrl+W also sent to the page first and > > > eligible for preventDefault? (sorry for forgetting the details of your > recent > > > behavior change). Could a page could trap users in fullscreen (making their > > only > > > resort to Ctrl+Alt+Delete or similar to kill the tab/browser)? > > > > No, otherwise the "ExceptForF11" test will fail. But web page can > preventDefault > > of an F11 fullscreen request in window mode, which is the reason of this > > condition. > > Actually, it might be interesting to construct a test where the html/js attempts > to 'consumeEvent'/|e.preventDefault()| when the event is the F11 key (and the > Meta+Ctrl+F). This code does not attempt to preventDefault for those keys. Yes, I can do it in a separated change. https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:37: #if defined(OS_MACOSX) On 2017/07/05 19:11:45, msw wrote: > nit: move the #if block inside the function, all platforms can share one > function declaration: > // On MacOSX command key is used for most of the shortcuts, so replace it with > // control to reduce the complexity of comparison of the results. > void NormalizeMetaKeyForMacOS(std::string* output) { > #if defined(OS_MACOSX) > base::ReplaceSubstringsAfterOffset(output, 0, "MetaLeft", "ControlLeft"); > #endif > } Done. https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:83: // the result and verifies if it equals to |expected_result_|. On 2017/07/05 19:11:45, msw wrote: > nit: "if it equals |expected_result_|." of "if it is equal to..." Done. https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:122: // 1. ui_controls_mac.mm puts shift key before meta (command) key. On 2017/07/05 19:11:45, msw wrote: > Trent's point about test fragility is very much worth considering. Instead of > the html/js reporting the entire sequence of down and up events (like "keydown > ShiftLeft\nkeydown ControlLeft\nkeydown keyW\nkeyup keyW\n\nkeyup > ControlLeft\nkeyup ShiftLeft\n"), can the it just report the state of the > modifiers at the time of the non-modifier keys (like "key:w ctrl:1 shift:1 alt:0 > meta:0\n" or similar)? That would make this command controller test completely > agnostic to differences in generated sequences and robust against bugs/changes > with unreported keyup events, etc. A good suggestion to avoid addressing the order of command key. But I still have concerns regarding to ignoring keyup events: they should not happen at all. Since you two have the same opinion, I will modify the html page to apply your suggestion. https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:232: ASSERT_EQ(result, expected_result_); On 2017/07/05 19:11:45, msw wrote: > nit: ASSERT_EQ and EXPECTED_EQ macros should have the expected value as the > first argument, and the actual value as the second argument. Done. https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:289: // The tab should not be closed. On 2017/07/05 19:11:45, msw wrote: > Can you make a helper function that sends these commands, since the code is > repeated three times? The code should also test for the expected behavior. For > example: > > void CheckShortcutsInFullscreen() { > // Add a second tab for counting and focus purposes. > AddTabAtIndex(1, GURL("about:blank"), ui::PAGE_TRANSITION_LINK); > ASSERT_EQ(2, browser()->tab_strip_model()->count()); > ASSERT_EQ(0, browser()->tab_strip_model()->active_index()); > ASSERT_EQ(1, BrowserList::GetInstance()->size()); > > // The tab should not be closed. > SendShortcut(ui::VKEY_W); > ASSERT_EQ(2, browser()->tab_strip_model()->count()); > // The window should not be closed. > SendShiftShortcut(ui::VKEY_W); > ASSERT_EQ(1, BrowserList::GetInstance()->size()); > // TODO(zijiehe): ChromeOS incorrectly handles these; > // see http://crbug.com/737307. > #if !defined(OS_CHROMEOS) > // A new tab should not be created. > SendShortcut(ui::VKEY_T); > ASSERT_EQ(2, browser()->tab_strip_model()->count()); > // A new window should not be created. > SendShortcut(ui::VKEY_N); > ASSERT_EQ(1, BrowserList::GetInstance()->size()); > // A new incognito window should not be created. > SendShiftShortcut(ui::VKEY_N); > ASSERT_EQ(1, BrowserList::GetInstance()->size()); > // Last closed tab should not be restored. > SendShiftShortcut(ui::VKEY_T); > ASSERT_EQ(2, browser()->tab_strip_model()->count()); > #endif > // Browser should not switch to the next tab. > SendShortcut(ui::VKEY_TAB); > ASSERT_EQ(0, browser()->tab_strip_model()->active_index()); > // Browser should not switch to the previous tab. > SendShiftShortcut(ui::VKEY_TAB); > ASSERT_EQ(0, browser()->tab_strip_model()->active_index()); > } AddTabAtIndex() always creates a focused tab and cause other tabs to exit fullscreen mode. So I moved the logic into StartTestPage() to ensure the test page can be correctly loaded in the focused tab.
lgtm with minor comments, thanks! https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:122: // 1. ui_controls_mac.mm puts shift key before meta (command) key. On 2017/07/07 23:18:10, Hzj_jie wrote: > On 2017/07/05 19:11:45, msw wrote: > > Trent's point about test fragility is very much worth considering. Instead of > > the html/js reporting the entire sequence of down and up events (like "keydown > > ShiftLeft\nkeydown ControlLeft\nkeydown keyW\nkeyup keyW\n\nkeyup > > ControlLeft\nkeyup ShiftLeft\n"), can the it just report the state of the > > modifiers at the time of the non-modifier keys (like "key:w ctrl:1 shift:1 > alt:0 > > meta:0\n" or similar)? That would make this command controller test completely > > agnostic to differences in generated sequences and robust against bugs/changes > > with unreported keyup events, etc. > > A good suggestion to avoid addressing the order of command key. > > But I still have concerns regarding to ignoring keyup events: they should not > happen at all. Since you two have the same opinion, I will modify the html page > to apply your suggestion. Feel free to add a separate keyup behavior test in a followup CL. https://codereview.chromium.org/2922773002/diff/770001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/770001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:108: AddTabAtIndex(1, GURL("about:blank"), ui::PAGE_TRANSITION_LINK); nit: check the active index after adding the tab. https://codereview.chromium.org/2922773002/diff/770001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:162: ASSERT_TRUE(ui_test_utils::SendKeyPressSync( Why doesn't this modify |expected_result_| anymore? Maybe comment? If it should be added, maybe use Send[Shift]Shortcut? https://codereview.chromium.org/2922773002/diff/770001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:204: const auto initial_active_index = browser()->tab_strip_model()->active_index(); nit: 'git cl format', avoid 'auto' here and below.
The CQ bit was checked by zijiehe@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...
zijiehe@chromium.org changed reviewers: + phajdan.jr@chromium.org
Add Pawel for chrome/test/ changes. https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/730001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:122: // 1. ui_controls_mac.mm puts shift key before meta (command) key. On 2017/07/10 18:48:17, msw wrote: > On 2017/07/07 23:18:10, Hzj_jie wrote: > > On 2017/07/05 19:11:45, msw wrote: > > > Trent's point about test fragility is very much worth considering. Instead > of > > > the html/js reporting the entire sequence of down and up events (like > "keydown > > > ShiftLeft\nkeydown ControlLeft\nkeydown keyW\nkeyup keyW\n\nkeyup > > > ControlLeft\nkeyup ShiftLeft\n"), can the it just report the state of the > > > modifiers at the time of the non-modifier keys (like "key:w ctrl:1 shift:1 > > alt:0 > > > meta:0\n" or similar)? That would make this command controller test > completely > > > agnostic to differences in generated sequences and robust against > bugs/changes > > > with unreported keyup events, etc. > > > > A good suggestion to avoid addressing the order of command key. > > > > But I still have concerns regarding to ignoring keyup events: they should not > > happen at all. Since you two have the same opinion, I will modify the html > page > > to apply your suggestion. > > Feel free to add a separate keyup behavior test in a followup CL. Acknowledged. https://codereview.chromium.org/2922773002/diff/770001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/770001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:108: AddTabAtIndex(1, GURL("about:blank"), ui::PAGE_TRANSITION_LINK); On 2017/07/10 18:48:17, msw wrote: > nit: check the active index after adding the tab. Though it should switch to the newly added tab, it won't really relate to this test I think. https://codereview.chromium.org/2922773002/diff/770001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:162: ASSERT_TRUE(ui_test_utils::SendKeyPressSync( On 2017/07/10 18:48:17, msw wrote: > Why doesn't this modify |expected_result_| anymore? Maybe comment? > If it should be added, maybe use Send[Shift]Shortcut? This command won't impact |expected_result_| :) https://codereview.chromium.org/2922773002/diff/770001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:204: const auto initial_active_index = browser()->tab_strip_model()->active_index(); On 2017/07/10 18:48:17, msw wrote: > nit: 'git cl format', avoid 'auto' here and below. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
phajdan.jr@chromium.org changed reviewers: + jam@chromium.org
+jam to advise on making the tests robust Some initial review comments seem to suggest flakiness issues with tests. These could be related to suspicious patterns identified below. https://codereview.chromium.org/2922773002/diff/790001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/790001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:108: AddTabAtIndex(1, GURL("about:blank"), ui::PAGE_TRANSITION_LINK); nit: Use url::kAboutBlankURL . https://codereview.chromium.org/2922773002/diff/790001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:120: while (browser()->tab_strip_model()->count() != tab_count) Loops like this make me suspicious. Can we wait for an event instead? https://codereview.chromium.org/2922773002/diff/790001/chrome/test/data/fulls... File chrome/test/data/fullscreen_keyboardlock.html (right): https://codereview.chromium.org/2922773002/diff/790001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock.html:16: window.setTimeout(() => { What's the purpose of this delay? Looks like a polling loop where in the "else" branch we run "register" (and thus setTimeout) again. Is there an event we can wait for?
The CQ bit was checked by zijiehe@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...
https://codereview.chromium.org/2922773002/diff/790001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/790001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:108: AddTabAtIndex(1, GURL("about:blank"), ui::PAGE_TRANSITION_LINK); On 2017/07/11 11:59:57, Paweł Hajdan Jr. wrote: > nit: Use url::kAboutBlankURL . Done. https://codereview.chromium.org/2922773002/diff/790001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:120: while (browser()->tab_strip_model()->count() != tab_count) On 2017/07/11 11:59:57, Paweł Hajdan Jr. wrote: > Loops like this make me suspicious. Can we wait for an event instead? In an early iteration, a TabCountObserver has been added (https://codereview.chromium.org/2922773002/diff/650001/chrome/browser/ui/brow...). It's much more complex than the while-loop, but internally they do exactly the same thing to loop until condition meets. Mike suggested to remove the observer and use the while-loop directly instead. https://codereview.chromium.org/2922773002/diff/790001/chrome/test/data/fulls... File chrome/test/data/fullscreen_keyboardlock.html (right): https://codereview.chromium.org/2922773002/diff/790001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock.html:16: window.setTimeout(() => { On 2017/07/11 11:59:57, Paweł Hajdan Jr. wrote: > What's the purpose of this delay? Looks like a polling loop where in the "else" > branch we run "register" (and thus setTimeout) again. > > Is there an event we can wait for? The key events are sent to renderer process through IPC channel, i.e. SendKeyPressSync() function cannot guarantee the events have been sent to web page / javascript. But we cannot blindly send the result to window.domAutomationController in document.onkeyup event, because the window.domAutomationController needs to be initialized in ExecuteScriptAndExtractString(). (https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.c...) So here, getKeyEventReport() function continually polls until |done| is set by document.onkeyup event.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_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 zijiehe@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...
https://codereview.chromium.org/2922773002/diff/790001/chrome/test/data/fulls... File chrome/test/data/fullscreen_keyboardlock.html (right): https://codereview.chromium.org/2922773002/diff/790001/chrome/test/data/fulls... chrome/test/data/fullscreen_keyboardlock.html:16: window.setTimeout(() => { On 2017/07/11 17:47:26, Hzj_jie wrote: > On 2017/07/11 11:59:57, Paweł Hajdan Jr. wrote: > > What's the purpose of this delay? Looks like a polling loop where in the > "else" > > branch we run "register" (and thus setTimeout) again. > > > > Is there an event we can wait for? > > The key events are sent to renderer process through IPC channel, i.e. > SendKeyPressSync() function cannot guarantee the events have been sent to web > page / javascript. > But we cannot blindly send the result to window.domAutomationController in > document.onkeyup event, because the window.domAutomationController needs to be > initialized in ExecuteScriptAndExtractString(). > (https://cs.chromium.org/chromium/src/content/public/test/browser_test_utils.c...) > > So here, getKeyEventReport() function continually polls until |done| is set by > document.onkeyup event. But yes, this logic is over-complex, I have updated it to a simpler approach.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping
zijiehe@chromium.org changed reviewers: + sky@chromium.org - jam@chromium.org, phajdan.jr@chromium.org
Pawel and John may not have time these days to review this change. Scott, would you mind to have a look? Thank you.
https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:47: output = nullptr; Are you sure this is really necessary? https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:97: std::string expected_result_; Please add a comment. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:102: void BrowserCommandControllerInteractiveTest::StartTestPage() { You need to wrap all calls to this in ASSERT_NO_FATAL_FAILURE, otherwise tests continue execution even if the assertions in this function fail. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:119: void BrowserCommandControllerInteractiveTest::WaitForTabCount( Why do you need the wait here? AFAICT you wait for the key events to be processed synchronously, so shouldn't the tab count already have changed? Assuming you do need to wait, use a TabStripModelObserver to wait for the count to be what you want. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:125: void BrowserCommandControllerInteractiveTest::SendShortcut( Similar comment about ASSERT_NO_FATAL_FAILURE. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:159: // Enter fullscreen. fix indentation. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:169: ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_F11, false, Similar comment here about the need to wait. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:213: // TODO(zijiehe): ChromeOS incorrectly handles these; fix indentation. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:239: EXPECT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_X, false, Why is this necessary?
The CQ bit was checked by zijiehe@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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by zijiehe@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...
https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:47: output = nullptr; On 2017/07/13 20:33:25, sky wrote: > Are you sure this is really necessary? My fault, removed. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:97: std::string expected_result_; On 2017/07/13 20:33:25, sky wrote: > Please add a comment. Done. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:102: void BrowserCommandControllerInteractiveTest::StartTestPage() { On 2017/07/13 20:33:25, sky wrote: > You need to wrap all calls to this in ASSERT_NO_FATAL_FAILURE, otherwise tests > continue execution even if the assertions in this function fail. Done. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:119: void BrowserCommandControllerInteractiveTest::WaitForTabCount( On 2017/07/13 20:33:26, sky wrote: > Why do you need the wait here? AFAICT you wait for the key events to be > processed synchronously, so shouldn't the tab count already have changed? > Assuming you do need to wait, use a TabStripModelObserver to wait for the count > to be what you want. Done. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:125: void BrowserCommandControllerInteractiveTest::SendShortcut( On 2017/07/13 20:33:25, sky wrote: > Similar comment about ASSERT_NO_FATAL_FAILURE. Done. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:159: // Enter fullscreen. On 2017/07/13 20:33:25, sky wrote: > fix indentation. It's triple weird, this is done by git cl format. But anyway, I will revert it. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:169: ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_F11, false, On 2017/07/13 20:33:25, sky wrote: > Similar comment here about the need to wait. At least on Mac OSX, entering and exiting fullscreen are not synchronous. (https://www.google.com/search?q=toggleFullScreen&oq=toggleFullScreen&aqs=chro...) https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/browser_window_c... https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:213: // TODO(zijiehe): ChromeOS incorrectly handles these; On 2017/07/13 20:33:26, sky wrote: > fix indentation. Done. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:239: EXPECT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_X, false, On 2017/07/13 20:33:25, sky wrote: > Why is this necessary? If my understanding is correct, the keys are sent to renderer process through IPC channel. So SendKeyPressSync() is only "synchronous" in browser process, but not renderer process. Since these keys need to be handled by JS in renderer process, the KeyX indicates the end of the tests, i.e. test html page can report the recorded keys to window.domAutomationController. Without the magic KeyX, the getKeyEventReport() may receive partial key sequence. This happens at least on Mac OSX.
I prefer the TabCountObserver. It's clearer what you are waiting for and results in running the message lop for exactly the amount of time you need (I do have some comments on it though). https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:169: ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_F11, false, On 2017/07/13 23:05:11, Hzj_jie wrote: > On 2017/07/13 20:33:25, sky wrote: > > Similar comment here about the need to wait. > > At least on Mac OSX, entering and exiting fullscreen are not synchronous. > (https://www.google.com/search?q=toggleFullScreen&oq=toggleFullScreen&aqs=chro...) > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/browser_window_c... Ok, please add a comment to that effect then. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:239: EXPECT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_X, false, On 2017/07/13 23:05:11, Hzj_jie wrote: > On 2017/07/13 20:33:25, sky wrote: > > Why is this necessary? > > If my understanding is correct, the keys are sent to renderer process through > IPC channel. So SendKeyPressSync() is only "synchronous" in browser process, but > not renderer process. Since these keys need to be handled by JS in renderer > process, the KeyX indicates the end of the tests, i.e. test html page can report > the recorded keys to window.domAutomationController. > > Without the magic KeyX, the getKeyEventReport() may receive partial key > sequence. This happens at least on Mac OSX. Pleaes add a better comment (the current comment is not at all clear).
Also, TabCountObserver is similar to other patterns in the code (and in the test you are creating), it just so happens there isn't one already created for you. On Thu, Jul 13, 2017 at 5:03 PM, <sky@chromium.org> wrote: > I prefer the TabCountObserver. It's clearer what you are waiting for and > results > in running the message lop for exactly the amount of time you need (I do > have > some comments on it though). > > > https://codereview.chromium.org/2922773002/diff/830001/ > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc > File > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc > (right): > > https://codereview.chromium.org/2922773002/diff/830001/ > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc# > newcode169 > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc: > 169: > ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_F11, > false, > On 2017/07/13 23:05:11, Hzj_jie wrote: > > On 2017/07/13 20:33:25, sky wrote: > > > Similar comment here about the need to wait. > > > > At least on Mac OSX, entering and exiting fullscreen are not > synchronous. > > > (https://www.google.com/search?q=toggleFullScreen&oq= > toggleFullScreen&aqs=chrome..69i57j0j69i60j0l3.164j0j4& > sourceid=chrome&ie=UTF-8#newwindow=1&q=nswindow. > togglefullscreen+asynchronous) > > > https://cs.chromium.org/chromium/src/chrome/browser/ > ui/cocoa/browser_window_controller_private.mm?rcl= > 4087baf3e09ba62dff9968ed43b693dc9a59c728&l=830 > > Ok, please add a comment to that effect then. > > https://codereview.chromium.org/2922773002/diff/830001/ > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc# > newcode239 > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc: > 239: > EXPECT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_X, > false, > On 2017/07/13 23:05:11, Hzj_jie wrote: > > On 2017/07/13 20:33:25, sky wrote: > > > Why is this necessary? > > > > If my understanding is correct, the keys are sent to renderer process > through > > IPC channel. So SendKeyPressSync() is only "synchronous" in browser > process, but > > not renderer process. Since these keys need to be handled by JS in > renderer > > process, the KeyX indicates the end of the tests, i.e. test html page > can report > > the recorded keys to window.domAutomationController. > > > > Without the magic KeyX, the getKeyEventReport() may receive partial > key > > sequence. This happens at least on Mac OSX. > > Pleaes add a better comment (the current comment is not at all clear). > > https://codereview.chromium.org/2922773002/ > -- 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 zijiehe@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...
There was a TabCountObserver (https://codereview.chromium.org/2922773002/diff/630001/chrome/browser/ui/brow...). But since adding tab is synchronous, do you still prefer to add one? https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... File chrome/browser/ui/browser_command_controller_interactive_browsertest.cc (right): https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:169: ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_F11, false, On 2017/07/14 00:03:37, sky wrote: > On 2017/07/13 23:05:11, Hzj_jie wrote: > > On 2017/07/13 20:33:25, sky wrote: > > > Similar comment here about the need to wait. > > > > At least on Mac OSX, entering and exiting fullscreen are not synchronous. > > > (https://www.google.com/search?q=toggleFullScreen&oq=toggleFullScreen&aqs=chro...) > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/browser_window_c... > > Ok, please add a comment to that effect then. Done. https://codereview.chromium.org/2922773002/diff/830001/chrome/browser/ui/brow... chrome/browser/ui/browser_command_controller_interactive_browsertest.cc:239: EXPECT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_X, false, On 2017/07/14 00:03:37, sky wrote: > On 2017/07/13 23:05:11, Hzj_jie wrote: > > On 2017/07/13 20:33:25, sky wrote: > > > Why is this necessary? > > > > If my understanding is correct, the keys are sent to renderer process through > > IPC channel. So SendKeyPressSync() is only "synchronous" in browser process, > but > > not renderer process. Since these keys need to be handled by JS in renderer > > process, the KeyX indicates the end of the tests, i.e. test html page can > report > > the recorded keys to window.domAutomationController. > > > > Without the magic KeyX, the getKeyEventReport() may receive partial key > > sequence. This happens at least on Mac OSX. > > Pleaes add a better comment (the current comment is not at all clear). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
My mistake. As adding tabs is sync no need for the extra complexity. -Scott On Thu, Jul 13, 2017 at 5:24 PM, <zijiehe@chromium.org> wrote: > There was a TabCountObserver > (https://codereview.chromium.org/2922773002/diff/630001/ > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc). > But since adding tab is synchronous, do you still prefer to add one? > > > https://codereview.chromium.org/2922773002/diff/830001/ > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc > File > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc > (right): > > https://codereview.chromium.org/2922773002/diff/830001/ > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc# > newcode169 > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc: > 169: > ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_F11, > false, > On 2017/07/14 00:03:37, sky wrote: > > On 2017/07/13 23:05:11, Hzj_jie wrote: > > > On 2017/07/13 20:33:25, sky wrote: > > > > Similar comment here about the need to wait. > > > > > > At least on Mac OSX, entering and exiting fullscreen are not > synchronous. > > > > > > (https://www.google.com/search?q=toggleFullScreen&oq= > toggleFullScreen&aqs=chrome..69i57j0j69i60j0l3.164j0j4& > sourceid=chrome&ie=UTF-8#newwindow=1&q=nswindow. > togglefullscreen+asynchronous) > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ > ui/cocoa/browser_window_controller_private.mm?rcl= > 4087baf3e09ba62dff9968ed43b693dc9a59c728&l=830 > > > > Ok, please add a comment to that effect then. > > Done. > > https://codereview.chromium.org/2922773002/diff/830001/ > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc# > newcode239 > chrome/browser/ui/browser_command_controller_interactive_browsertest.cc: > 239: > EXPECT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_X, > false, > On 2017/07/14 00:03:37, sky wrote: > > On 2017/07/13 23:05:11, Hzj_jie wrote: > > > On 2017/07/13 20:33:25, sky wrote: > > > > Why is this necessary? > > > > > > If my understanding is correct, the keys are sent to renderer > process through > > > IPC channel. So SendKeyPressSync() is only "synchronous" in browser > process, > > but > > > not renderer process. Since these keys need to be handled by JS in > renderer > > > process, the KeyX indicates the end of the tests, i.e. test html > page can > > report > > > the recorded keys to window.domAutomationController. > > > > > > Without the magic KeyX, the getKeyEventReport() may receive partial > key > > > sequence. This happens at least on Mac OSX. > > > > Pleaes add a better comment (the current comment is not at all clear). > > Done. > > https://codereview.chromium.org/2922773002/ > -- 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.
Scott, is this change LGTU now?
Yes, LGTM
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2922773002/#ps890001 (title: "Resolve review comments")
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": 890001, "attempt_start_ts": 1500076565665710,
"parent_rev": "f6a16073fd8b433d6f3cea80dfdccf1bbe6813b4", "commit_rev":
"bb317b9cc0055f7abebcd014dfb6c193548b4d88"}
Message was sent while issue was closed.
Description was changed from ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. BUG=680809 ========== to ========== Add BrowserCommandController Interactive Test BrowserCommandController interactive test ensures the recent changes of browser shortcuts in fullscreen won't be broken by unexpected changes. It starts a full screen browser window, sends several key events normally consumed by browser, and checks whether these events are caught by the web page. BUG=680809 Review-Url: https://codereview.chromium.org/2922773002 Cr-Commit-Position: refs/heads/master@{#486949} Committed: https://chromium.googlesource.com/chromium/src/+/bb317b9cc0055f7abebcd014dfb6... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:890001) as https://chromium.googlesource.com/chromium/src/+/bb317b9cc0055f7abebcd014dfb6...
Message was sent while issue was closed.
On 2017/07/14 23:47:37, sky wrote: > Yes, LGTM Thank you all. Go ahead to submit. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
