NewWindowClient and ChromeNewWindowClient do not respect BrowserCommandController::IsReservedCommandOrKey() |
||||||
Issue descriptionA quick test shows on ChromeOS, some of the browser commands are handled directly in NewWindowClient / ChromeNewWindowClient pair without respect the return value of BrowserCommandController::IsReservedCommandOrKey() function. Steps to reproduce: - add a CHECK(false) in borwser_commands.cc: void NewTab(Browser*) function. (https://codereview.chromium.org/2922773002/diff/590001/chrome/browser/ui/browser_commands.cc) - execute a simple test to send control + t to the focused web page. (For example, https://codereview.chromium.org/2922773002/diff/590001/chrome/browser/ui/browser_command_controller_interactive_browsertest.cc) The callstack of crashing is, #0 0x000003298f8c base::debug::StackTrace::StackTrace() #1 0x0000032b1c61 logging::LogMessage::~LogMessage() #2 0x000004f89bef chrome::NewTab() #3 0x000001559eed ash::mojom::NewWindowClientStubDispatch::Accept() #4 0x0000038d1548 mojo::InterfaceEndpointClient::HandleValidatedMessage() #5 0x0000038d11e6 mojo::FilterChain::Accept() #6 0x0000038d26f5 mojo::InterfaceEndpointClient::HandleIncomingMessage() #7 0x0000038d922c mojo::internal::MultiplexRouter::ProcessIncomingMessage() #8 0x0000038d8a1f mojo::internal::MultiplexRouter::Accept() #9 0x0000038d11e6 mojo::FilterChain::Accept() #10 0x0000038cfa29 mojo::Connector::ReadSingleMessage() #11 0x0000038d0241 mojo::Connector::ReadAllAvailableMessages() #12 0x0000038d00f9 mojo::Connector::OnHandleReadyInternal() #13 0x0000038e5bf9 mojo::SimpleWatcher::OnHandleReady() #14 0x00000110534f _ZN4base8internal7InvokerINS0_9BindStateIMN7content25ServiceWorkerProviderHostEFviN5blink21WebServiceWorkerStateEEJNS_7WeakPtrIS4_EEiS6_EEEFvvEE7RunImplIRKS8_RKSt5tupleIJSA_iS6_EEJLm0ELm1ELm2EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE #15 0x0000004ab8d9 _ZNO4base8CallbackIFvvELNS_8internal8CopyModeE1ELNS2_10RepeatModeE1EE3RunEv #16 0x00000334f374 base::debug::TaskAnnotator::RunTask() #17 0x0000032b9ca9 base::MessageLoop::RunTask() #18 0x0000032b9f6b base::MessageLoop::DeferOrRunPendingTask() #19 0x0000032ba377 base::MessageLoop::DoWork() #20 0x0000032bc969 base::MessagePumpLibevent::Run() #21 0x0000032b98cb base::MessageLoop::Run() #22 0x0000032e3ada base::RunLoop::Run() #23 0x000002911f96 content::MessageLoopRunner::Run() #24 0x0000005a3235 ui_test_utils::SendKeyPressToWindowSync() i.e. The key event is sent to NewWindowClient by BrowserNewWindowClient through mojo. But the former one does not respect BrowserCommandController, instead it directly executes the NewTab() command. Same behavior can be found on Chrome book.
,
Jun 29 2017
Got you, thank you for the clear explanation.
,
Jun 30 2017
,
Jun 30 2017
,
Jul 3 2017
This issue is caught by change https://codereview.chromium.org/2922773002/.
,
Jul 27 2017
After discussed with James Cook (JamesCook@), Mitsuru Oshima (Oshima@), we believe this behavior is unexpected, i.e. fullscreen web pages should also be able to receive these key events.
,
Jul 28 2017
The root cause is in browser process, i.e. BrowserView prefers ash accelerators in ChromeOS. See Mitsuru's reply, All clients that use views toolkit (which means basically everything :) uses FocusManager. The difference is how browser handles keys/accelerators. Browser prefers ash accelerators. See https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_view.cc?rcl=10dafc8d34e727a035ad334d533d2616889d967b&l=1319
,
Jul 28 2017
To clarify #6 -- the code today is working as originally designed. Ash intercepts Ctrl-T and opens a new tab. It works that way so that ash can open a new tab when there are no browser windows open (and hence there is no browser window in the FocusManager). It's fine to change the behavior so fullscreen windows get Ctrl-T. The code just needs to be written, and the solution needs to continue to allow ash to open a new tab on Ctrl-T when no browser windows are open.
,
Aug 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a4795877b3e11ab9a1193397b13c18e402f8e7f4 commit a4795877b3e11ab9a1193397b13c18e402f8e7f4 Author: Hzj_jie <zijiehe@chromium.org> Date: Thu Aug 10 19:35:24 2017 Add tests to ensure browser shortcuts should be handled in fullscreen This change adds several tests to ensure in both browser fullscreen and HTML fullscreen, if preventDefault() is not performed, browser shortcuts should still take effect. ShortcutsShouldTakeEffectInJsFullscreen test let the about:blank page enter HTML fullscreen by using document.body.webkitRequestFullscreen(). ShortcutsShouldTakeEffectInBrowserFullscreen test uses default page and "F11" shortcut to test the behavior in browser fullscreen. After change https://codereview.chromium.org/2781633003, in fullscreen, most of the shortcuts will be sent to renderer first. So various observing-patterns are included in this change to detect the asynchronized actions. Bug: chromium:680809 , chromium:737307 Change-Id: I0e326d234bed1400882bf011331ab9b62f3adbc9 Reviewed-on: https://chromium-review.googlesource.com/592313 Commit-Queue: Zijie He <zijiehe@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#493500} [modify] https://crrev.com/a4795877b3e11ab9a1193397b13c18e402f8e7f4/chrome/browser/ui/browser_command_controller_interactive_browsertest.cc
,
Aug 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/744be8fe99a4817127e5f1ff8715556fde6ecd1a commit 744be8fe99a4817127e5f1ff8715556fde6ecd1a Author: Walter Korman <wkorman@chromium.org> Date: Thu Aug 10 21:37:49 2017 Revert "Add tests to ensure browser shortcuts should be handled in fullscreen" This reverts commit a4795877b3e11ab9a1193397b13c18e402f8e7f4. Reason for revert: <INSERT REASONING HERE> Original change's description: > Add tests to ensure browser shortcuts should be handled in fullscreen > > This change adds several tests to ensure in both browser fullscreen and HTML > fullscreen, if preventDefault() is not performed, browser shortcuts should still > take effect. > > ShortcutsShouldTakeEffectInJsFullscreen test let the about:blank page enter HTML > fullscreen by using document.body.webkitRequestFullscreen(). > ShortcutsShouldTakeEffectInBrowserFullscreen test uses default page and "F11" > shortcut to test the behavior in browser fullscreen. > > After change https://codereview.chromium.org/2781633003, in fullscreen, most of > the shortcuts will be sent to renderer first. So various observing-patterns are > included in this change to detect the asynchronized actions. > > Bug: chromium:680809 , chromium:737307 > Change-Id: I0e326d234bed1400882bf011331ab9b62f3adbc9 > Reviewed-on: https://chromium-review.googlesource.com/592313 > Commit-Queue: Zijie He <zijiehe@chromium.org> > Reviewed-by: Michael Wasserman <msw@chromium.org> > Cr-Commit-Position: refs/heads/master@{#493500} TBR=msw@chromium.org,zijiehe@chromium.org Change-Id: I7ea477c7ab0cc2b5b8e575f4490366f6c66accc5 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:680809 , chromium:737307 , chromium:754447 Reviewed-on: https://chromium-review.googlesource.com/610943 Reviewed-by: Walter Korman <wkorman@chromium.org> Commit-Queue: Walter Korman <wkorman@chromium.org> Cr-Commit-Position: refs/heads/master@{#493553} [modify] https://crrev.com/744be8fe99a4817127e5f1ff8715556fde6ecd1a/chrome/browser/ui/browser_command_controller_interactive_browsertest.cc
,
Aug 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/09b4239b6843cd936f03c4a1ffe1041194a8fdac commit 09b4239b6843cd936f03c4a1ffe1041194a8fdac Author: Hzj_jie <zijiehe@chromium.org> Date: Thu Aug 10 23:14:05 2017 Reland "Add tests to ensure browser shortcuts should be handled in fullscreen" This is a reland of a4795877b3e11ab9a1193397b13c18e402f8e7f4 Original change's description: > Add tests to ensure browser shortcuts should be handled in fullscreen > > This change adds several tests to ensure in both browser fullscreen and HTML > fullscreen, if preventDefault() is not performed, browser shortcuts should still > take effect. > > ShortcutsShouldTakeEffectInJsFullscreen test let the about:blank page enter HTML > fullscreen by using document.body.webkitRequestFullscreen(). > ShortcutsShouldTakeEffectInBrowserFullscreen test uses default page and "F11" > shortcut to test the behavior in browser fullscreen. > > After change https://codereview.chromium.org/2781633003, in fullscreen, most of > the shortcuts will be sent to renderer first. So various observing-patterns are > included in this change to detect the asynchronized actions. > > Bug: chromium:680809 , chromium:737307 > Change-Id: I0e326d234bed1400882bf011331ab9b62f3adbc9 > Reviewed-on: https://chromium-review.googlesource.com/592313 > Commit-Queue: Zijie He <zijiehe@chromium.org> > Reviewed-by: Michael Wasserman <msw@chromium.org> > Cr-Commit-Position: refs/heads/master@{#493500} Bug: chromium:680809 , chromium:737307 Change-Id: If9977bd167e9c382eeed377c01906706e895d7f3 TBR: msw@chromium.org Change-Id: If9977bd167e9c382eeed377c01906706e895d7f3 Reviewed-on: https://chromium-review.googlesource.com/611220 Commit-Queue: Zijie He <zijiehe@chromium.org> Reviewed-by: Zijie He <zijiehe@chromium.org> Cr-Commit-Position: refs/heads/master@{#493590} [modify] https://crrev.com/09b4239b6843cd936f03c4a1ffe1041194a8fdac/chrome/browser/ui/browser_command_controller_interactive_browsertest.cc
,
Aug 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4c5310e47304005d5dee153c483b2a1c22fa728d commit 4c5310e47304005d5dee153c483b2a1c22fa728d Author: Zijie He <zijiehe@chromium.org> Date: Fri Aug 11 00:48:09 2017 Revert "Reland "Add tests to ensure browser shortcuts should be handled in fullscreen"" This reverts commit 09b4239b6843cd936f03c4a1ffe1041194a8fdac. Reason for revert: <INSERT REASONING HERE> Original change's description: > Reland "Add tests to ensure browser shortcuts should be handled in fullscreen" > > This is a reland of a4795877b3e11ab9a1193397b13c18e402f8e7f4 > Original change's description: > > Add tests to ensure browser shortcuts should be handled in fullscreen > > > > This change adds several tests to ensure in both browser fullscreen and HTML > > fullscreen, if preventDefault() is not performed, browser shortcuts should still > > take effect. > > > > ShortcutsShouldTakeEffectInJsFullscreen test let the about:blank page enter HTML > > fullscreen by using document.body.webkitRequestFullscreen(). > > ShortcutsShouldTakeEffectInBrowserFullscreen test uses default page and "F11" > > shortcut to test the behavior in browser fullscreen. > > > > After change https://codereview.chromium.org/2781633003, in fullscreen, most of > > the shortcuts will be sent to renderer first. So various observing-patterns are > > included in this change to detect the asynchronized actions. > > > > Bug: chromium:680809 , chromium:737307 > > Change-Id: I0e326d234bed1400882bf011331ab9b62f3adbc9 > > Reviewed-on: https://chromium-review.googlesource.com/592313 > > Commit-Queue: Zijie He <zijiehe@chromium.org> > > Reviewed-by: Michael Wasserman <msw@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#493500} > > Bug: chromium:680809 , chromium:737307 > Change-Id: If9977bd167e9c382eeed377c01906706e895d7f3 > > TBR: msw@chromium.org > Change-Id: If9977bd167e9c382eeed377c01906706e895d7f3 > Reviewed-on: https://chromium-review.googlesource.com/611220 > Commit-Queue: Zijie He <zijiehe@chromium.org> > Reviewed-by: Zijie He <zijiehe@chromium.org> > Cr-Commit-Position: refs/heads/master@{#493590} TBR=msw@chromium.org,zijiehe@chromium.org Change-Id: Ie17ceef57a3f809f114ea73835ada7e91232ea89 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: chromium:680809 , chromium:737307 Reviewed-on: https://chromium-review.googlesource.com/610631 Reviewed-by: Zijie He <zijiehe@chromium.org> Commit-Queue: Zijie He <zijiehe@chromium.org> Cr-Commit-Position: refs/heads/master@{#493625} [modify] https://crrev.com/4c5310e47304005d5dee153c483b2a1c22fa728d/chrome/browser/ui/browser_command_controller_interactive_browsertest.cc
,
Aug 11 2017
AcceleratorTarget used on ChromeOS (AcceleratorController https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.h?type=cs&sq=package:chromium&l=40) does not execute browser command through BrowserCommandController::ExecuteCommandWithDisposition(). So the BrowserCommandController::SetBlockCommandExecution() mechanism does not take effect on Chrome OS. A potential solution is to add a GetAcceleratorCommand() function in AcceleratorTarget and AcceleratorManager. So we can get the command without executing it. But a potential risk is that some actions may not be a browser command, e.g. BRIGHTNESS_DOWN is an Ash action, but not a browser command. In this case, a simple approach is to return an invalid command integer and let BrowserView executes the accelerator immediately.
,
Aug 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/24cdbc10fbc7474216e7648a0bcb6fa300e0e578 commit 24cdbc10fbc7474216e7648a0bcb6fa300e0e578 Author: Hzj_jie <zijiehe@chromium.org> Date: Fri Aug 11 20:02:24 2017 Reland "Reland "Add tests to ensure browser shortcuts should be handled in fullscreen"" This is a reland of 09b4239b6843cd936f03c4a1ffe1041194a8fdac Executing document.body.webkitRequestFullscreen() directly in content::ExecuteScript() seems not reliable. Indeed we do not have an active example in our code base (https://cs.chromium.org/search/?q=webkitrequestfullscreen%5C(%5C)+lang:c%2B%2B&sq=package:chromium&type=cs). So I revert back to use page + keydown event. The page is too small, I directly embedded it into the source code. I executed the test case 10 times on both ChromeOS and Ubuntu. No flakiness was caught. Original change's description: > Reland "Add tests to ensure browser shortcuts should be handled in fullscreen" > > This is a reland of a4795877b3e11ab9a1193397b13c18e402f8e7f4 > Original change's description: > > Add tests to ensure browser shortcuts should be handled in fullscreen > > > > This change adds several tests to ensure in both browser fullscreen and HTML > > fullscreen, if preventDefault() is not performed, browser shortcuts should still > > take effect. > > > > ShortcutsShouldTakeEffectInJsFullscreen test let the about:blank page enter HTML > > fullscreen by using document.body.webkitRequestFullscreen(). > > ShortcutsShouldTakeEffectInBrowserFullscreen test uses default page and "F11" > > shortcut to test the behavior in browser fullscreen. > > > > After change https://codereview.chromium.org/2781633003, in fullscreen, most of > > the shortcuts will be sent to renderer first. So various observing-patterns are > > included in this change to detect the asynchronized actions. > > > > Bug: chromium:680809 , chromium:737307 > > Change-Id: I0e326d234bed1400882bf011331ab9b62f3adbc9 > > Reviewed-on: https://chromium-review.googlesource.com/592313 > > Commit-Queue: Zijie He <zijiehe@chromium.org> > > Reviewed-by: Michael Wasserman <msw@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#493500} > > Bug: chromium:680809 , chromium:737307 > Change-Id: If9977bd167e9c382eeed377c01906706e895d7f3 > > TBR: msw@chromium.org > Change-Id: If9977bd167e9c382eeed377c01906706e895d7f3 > Reviewed-on: https://chromium-review.googlesource.com/611220 > Commit-Queue: Zijie He <zijiehe@chromium.org> > Reviewed-by: Zijie He <zijiehe@chromium.org> > Cr-Commit-Position: refs/heads/master@{#493590} Bug: chromium:680809 , chromium:737307 Change-Id: Id788db366d93d45e4ab093cff6adbae2d8f6969f Reviewed-on: https://chromium-review.googlesource.com/611221 Commit-Queue: Zijie He <zijiehe@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Cr-Commit-Position: refs/heads/master@{#493838} [modify] https://crrev.com/24cdbc10fbc7474216e7648a0bcb6fa300e0e578/chrome/browser/ui/browser_command_controller_interactive_browsertest.cc
,
Aug 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/143f9cb278ea3cd9dbb0c4338fffc2f7ff1c5e32 commit 143f9cb278ea3cd9dbb0c4338fffc2f7ff1c5e32 Author: Zijie He <zijiehe@chromium.org> Date: Tue Aug 15 17:11:50 2017 Add int AcceleratorTarget::GetAcceleratorId() function Accelerator Id is an integer to represent the action of the accelerator. The meaning varies in different layers, e.g. in Chrome, it means the id of a browser command. With this function, clients of Accelerator and AcceleratorManager can predict what will happen without executing the Accelerator. So - BrowserCommandController::SetBlockCommandExecution() and related functions and variables can be removed. - BrowserView::PreHandleKeyboardEvent() can retrieve command id without executing the Accelerator. - AcceleratorTarget implementations do not need to actively check BrowserCommandController::block_command_execution() to decide whether the action should be performed. - AcceleratorTarget implementations which do not use BrowserCommandController::ExecuteCommandWithDisposition() can also be blocked in BrowserView::PreHandleKeyboardEvent(), e.g. ash::AcceleratorController. Bug: chromium:737307 Change-Id: I152bb14f2eb3bf25ac4ee54ffcd0bccaa1a01fb5 Reviewed-on: https://chromium-review.googlesource.com/612029 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Zijie He <zijiehe@chromium.org> Cr-Commit-Position: refs/heads/master@{#494421} [modify] https://crrev.com/143f9cb278ea3cd9dbb0c4338fffc2f7ff1c5e32/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/143f9cb278ea3cd9dbb0c4338fffc2f7ff1c5e32/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/143f9cb278ea3cd9dbb0c4338fffc2f7ff1c5e32/ui/base/accelerators/accelerator.cc [modify] https://crrev.com/143f9cb278ea3cd9dbb0c4338fffc2f7ff1c5e32/ui/base/accelerators/accelerator.h [modify] https://crrev.com/143f9cb278ea3cd9dbb0c4338fffc2f7ff1c5e32/ui/base/accelerators/accelerator_manager.cc [modify] https://crrev.com/143f9cb278ea3cd9dbb0c4338fffc2f7ff1c5e32/ui/base/accelerators/accelerator_manager.h [modify] https://crrev.com/143f9cb278ea3cd9dbb0c4338fffc2f7ff1c5e32/ui/base/accelerators/accelerator_manager_unittest.cc
,
Aug 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dd69ee7dae892b0024a2cb367a1c693b420f0765 commit dd69ee7dae892b0024a2cb367a1c693b420f0765 Author: Zijie He <zijiehe@chromium.org> Date: Mon Aug 21 21:12:52 2017 Remove BrowserCommandController::SetBlockCommandExecution() related functions After discussion offline, Scott suggested to directly retrieve chrome command id from BrowserView instead of using AcceleratorTarget::GetAcceleratorId(). The later one requires ash to register Accelerators within the same process as browser. So this change ensures both ash and browser accelerators are using the same keyboard shortcuts, and uses BrowserView::GetCommandIdForAccelerator() to replace BrowserCommandController::SetBlockCommandExecution() related mechanism. Meanwhile, since AcceleratorTarget::GetAcceleratorId() is not used anywhere, it has been removed. Bug: chromium:737307 Change-Id: I54025007de36d12650163d51240375195e1ba7a3 Reviewed-on: https://chromium-review.googlesource.com/620289 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Zijie He <zijiehe@chromium.org> Cr-Commit-Position: refs/heads/master@{#496061} [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/chrome/browser/ui/browser_command_controller.cc [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/chrome/browser/ui/browser_command_controller.h [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/chrome/browser/ui/browser_command_controller_interactive_browsertest.cc [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/chrome/browser/ui/views/accelerator_table.cc [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/chrome/browser/ui/views/accelerator_table_unittest.cc [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/chrome/browser/ui/views/frame/browser_view.h [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/ui/base/accelerators/accelerator.cc [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/ui/base/accelerators/accelerator.h [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/ui/base/accelerators/accelerator_manager.cc [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/ui/base/accelerators/accelerator_manager.h [modify] https://crrev.com/dd69ee7dae892b0024a2cb367a1c693b420f0765/ui/base/accelerators/accelerator_manager_unittest.cc
,
Aug 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/870bc4f9df09a3e862de149c4dc824342fd8ddb4 commit 870bc4f9df09a3e862de149c4dc824342fd8ddb4 Author: Zijie He <zijiehe@chromium.org> Date: Tue Aug 22 17:17:03 2017 BrowserView::PreHandleKeyboardEvent should not return NOT_HANDLED at the end of the function As commented in change https://chromium-review.googlesource.com/c/chromium/src/+/620289, when the command id is retrieved from |accelerator_table_|, there is no chance that the keyboard event received is a key-up event. So the condision return at the end of BrowserView::PreHandleKeyboardEvent() is not necessary. This issue exists for a long time, changing the triple operator to a DCHECK() can avoid unexpectedly returning NOT_HANDLED for key-up accelerators. Bug: chromium:737307 Change-Id: I83f8816a6179f0dcb6911909e7fe12550f512ac9 Reviewed-on: https://chromium-review.googlesource.com/624758 Commit-Queue: Zijie He <zijiehe@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#496342} [modify] https://crrev.com/870bc4f9df09a3e862de149c4dc824342fd8ddb4/chrome/browser/ui/views/frame/browser_view.cc
,
Aug 22 2017
,
Jan 22 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by e...@chromium.org
, Jun 29 2017