New issue
Advanced search Search tips

Issue 631836 link

Starred by 3 users

Issue metadata

Status: Archived
Owner: ----
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 665179
issue 612331



Sign in to add a comment

Replace AppDriver with explicit interface exported by ash and implemented by chrome

Project Member Reported by sky@chromium.org, Jul 26 2016

Issue description

AppDriver is launched as part of mash to handle some accelerators. These accelerators conflict with the accelerators already registered by ash. In order to continue having AppDriver work I've allowed AcceleratorRegistrarImpl to override accelerators provided. That should not be allowed.

One overriding desire is to avoid more dependencies from ash on chrome.

I see two options:

1. Ash already defines these accelerators and in fact has delegates for portions of what appdriver is using (see ash::NewWindowDelegate). Nuke AppDriver and instead have ash connect to YYY, ask for an interface like NewWindowDelegate, and call it as appropriate. I say YYY, we could make make it exe:chrome, but configurable via a switch.

2. Assume app driver will be launched and remove the set of accelerators from the table that ash registers (when in mus).

I tend to favor 1 as it's more explicit and we have a more well defined interface for expectations of what ash uses.
 

Comment 1 by sky@chromium.org, Jul 26 2016

Blocking: 612331
I also favor 1.

Comment 3 by sadrul@chromium.org, Jul 27 2016

I think ash should handle the accelerators too.

Comment 4 by sky@chromium.org, Jul 27 2016

Summary: Replace AppDriver with explicit interface exported by ash and implemented by chrome (was: Decide future of AppDriver)
I'm updating summary to reflect what should happen.

Comment 5 by sky@chromium.org, Jul 27 2016

As part of this we'll need to implement NewWindowDelegateMus, which is the place making most (but not all) of the calls serviced by AppDriver.

Comment 6 by sky@chromium.org, Aug 24 2016

Labels: Proj-Mustash-Mash
Components: MUS
Labels: Proj-Mustash
Components: Internals>MUS
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fafcda5411844c3a723b0a5be575f77acf8d731d

commit fafcda5411844c3a723b0a5be575f77acf8d731d
Author: erg <erg@chromium.org>
Date: Wed Oct 26 18:47:42 2016

mash: Remove app_driver.

As part of actually implementing a mus version of the NewWindowDelegate,
accelerators need to be routed through ash instead of through temporary
shim code. Without AppDriver, the keyboard shortcuts are handled by
ash's NewWindowDelegate, of which implementing is the next patch.

BUG= 631836 

Review-Url: https://codereview.chromium.org/2428173002
Cr-Commit-Position: refs/heads/master@{#427761}

[modify] https://crrev.com/fafcda5411844c3a723b0a5be575f77acf8d731d/chrome/app/mash/BUILD.gn
[modify] https://crrev.com/fafcda5411844c3a723b0a5be575f77acf8d731d/chrome/browser/chromeos/chrome_interface_factory.cc
[modify] https://crrev.com/fafcda5411844c3a723b0a5be575f77acf8d731d/chrome/test/BUILD.gn
[delete] https://crrev.com/7891dc06c41d1296293d5fba3401f44c3930ada1/mash/app_driver/BUILD.gn
[delete] https://crrev.com/7891dc06c41d1296293d5fba3401f44c3930ada1/mash/app_driver/app_driver.cc
[delete] https://crrev.com/7891dc06c41d1296293d5fba3401f44c3930ada1/mash/app_driver/app_driver.h
[delete] https://crrev.com/7891dc06c41d1296293d5fba3401f44c3930ada1/mash/app_driver/main.cc
[delete] https://crrev.com/7891dc06c41d1296293d5fba3401f44c3930ada1/mash/app_driver/manifest.json
[modify] https://crrev.com/fafcda5411844c3a723b0a5be575f77acf8d731d/mash/package/BUILD.gn
[modify] https://crrev.com/fafcda5411844c3a723b0a5be575f77acf8d731d/mash/package/mash_packaged_service.cc
[add] https://crrev.com/fafcda5411844c3a723b0a5be575f77acf8d731d/mash/public/interfaces/OWNERS
[modify] https://crrev.com/fafcda5411844c3a723b0a5be575f77acf8d731d/mash/public/interfaces/launchable.mojom
[modify] https://crrev.com/fafcda5411844c3a723b0a5be575f77acf8d731d/mash/session/BUILD.gn
[modify] https://crrev.com/fafcda5411844c3a723b0a5be575f77acf8d731d/mash/session/session.cc
[modify] https://crrev.com/fafcda5411844c3a723b0a5be575f77acf8d731d/mash/session/session.h

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3404382dde0b1a5a2911d476252b6a823dca3e65

commit 3404382dde0b1a5a2911d476252b6a823dca3e65
Author: erg <erg@chromium.org>
Date: Wed Oct 26 21:58:19 2016

mash: Move directly linked NewWindowDelegate to mojom::NewWindowClient.

Previously, NewWindowDelegate was an interface that chrome implemented
and injected into ash through the shell delegate. Now chrome exports its
implementation of mojom::NewWindowClient.

BUG= 631836 

Review-Url: https://codereview.chromium.org/2434463004
Cr-Commit-Position: refs/heads/master@{#427834}

[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/BUILD.gn
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/accelerators/accelerator_controller.cc
[add] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/new_window_client_proxy.cc
[add] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/new_window_client_proxy.h
[delete] https://crrev.com/23efaf6aa7ffc0c186772321cce481f90160a9a6/ash/common/new_window_delegate.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/shell_delegate.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/test/BUILD.gn
[add] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/test/test_new_window_client.cc
[add] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/test/test_new_window_client.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/test/wm_shell_test_api.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/test/wm_shell_test_api.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/wm_shell.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/common/wm_shell.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/display/display_util.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/mus/BUILD.gn
[delete] https://crrev.com/23efaf6aa7ffc0c186772321cce481f90160a9a6/ash/mus/new_window_delegate_mus.cc
[delete] https://crrev.com/23efaf6aa7ffc0c186772321cce481f90160a9a6/ash/mus/new_window_delegate_mus.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/mus/shell_delegate_mus.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/mus/shell_delegate_mus.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/mus/test/wm_test_helper.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/public/interfaces/BUILD.gn
[add] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/public/interfaces/new_window.mojom
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/shell/shell_delegate_impl.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/shell/shell_delegate_impl.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/test/ash_test_helper.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/test/test_shell_delegate.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/ash/test/test_shell_delegate.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/app/mash/chrome_mash_content_browser_manifest_overlay.json
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/chrome_content_browser_manifest_overlay.json
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/chromeos/chrome_interface_factory.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/ui/BUILD.gn
[rename] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/ui/ash/chrome_new_window_client.cc
[add] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/ui/ash/chrome_new_window_client.h
[rename] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/ui/ash/chrome_new_window_client_browsertest.cc
[delete] https://crrev.com/23efaf6aa7ffc0c186772321cce481f90160a9a6/chrome/browser/ui/ash/chrome_new_window_delegate.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/ui/ash/chrome_shell_delegate.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/ui/ash/chrome_shell_delegate.h
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/ui/views/task_manager_view.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/ui/webui/options/chromeos/keyboard_handler.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/browser/ui/webui/settings/chromeos/device_keyboard_handler.cc
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/chrome/test/BUILD.gn
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/components/arc/BUILD.gn
[modify] https://crrev.com/3404382dde0b1a5a2911d476252b6a823dca3e65/components/arc/intent_helper/arc_intent_helper_bridge.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 26 2016

Comment 13 by e...@chromium.org, Oct 26 2016

Status: Fixed (was: Untriaged)
AppDriver is gone (along with accelerator_registrar.mojom), ash handles Ctr+T, Ctr+N, etc, and delegates to a mojo client exported by chrome, removing the dependency from chrome to ash internals.
Blockedon: 665179
Blocking: 665179
Blockedon: -665179

Comment 17 by dchan@google.com, Jan 21 2017

Labels: VerifyIn-57

Comment 18 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 19 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 20 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 22 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)
Components: -Internals>MUS Internals>Services>WindowService
Components: -MUS

Sign in to add a comment