Do not use ash in chromeos::EventRewriter.

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/chrome_browser_main_chromeos.h"
 
 #include <stddef.h>
 
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "ash/shell.h"
+#include "ash/sticky_keys/sticky_keys_controller.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/linux_util.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 775 matching lines @@
     data_promo_notification_.reset(new DataPromoNotification());
 
     // TODO(mash): Support EventRewriterController; see crbug.com/647781
     keyboard_event_rewriters_.reset(new EventRewriterController());
     keyboard_event_rewriters_->AddEventRewriter(
         std::unique_ptr<ui::EventRewriter>(new KeyboardDrivenEventRewriter()));
     keyboard_event_rewriters_->AddEventRewriter(
         std::unique_ptr<ui::EventRewriter>(new SpokenFeedbackEventRewriter()));
     keyboard_event_rewriters_->AddEventRewriter(
         std::unique_ptr<ui::EventRewriter>(new EventRewriter(
             ash::Shell::GetInstance()->sticky_keys_controller())));

[xiyuan, 2017/03/09 18:49:29]

The CL changes StickyKeysController to be an ui::EventRewriter.
chromeos::EventRewriter is also derived from ui::EventRewriter. Having an
EventRewriter to run inside another EventRewriter seems a bit strange.

Why do we need to make StickyKeysController an ui::EventRewriter? To pretend
that chromeos::EventRewriter does not depend on ash?

[Peng, 2017/03/09 18:58:45]

https://codereview.chromium.org/2724913002/ - In this CL, we will move
chromeos::EventRewriter to //ui/chromeos/events, so it can be used by
//services/ui. To do it, chromeos::EventRewriter cannot depend on ash.
That's reason why we need this CL.

[xiyuan, 2017/03/09 19:26:50]

I understand the goal. My point is changing StickyKeysController to be an
ui::EventRewriter feels a bit strange. We would then have an EventRewriter
running inside another EventRewriter. It would be confusing to read. Reader like
me would then ask why don't we chain them up in EventSource like all other
EventRewriters.

I would recommend to extract an interface, say StickyKeysControllerInterface to
contain what chromeos::EventRewriter needs. The interface can sit in //chromeos.
chromeos::EventRewriter takes this interface instead of concrete impl. And
ash::StickyKeysController is an impl of this interface. This way, we breaks up
the deps on ash and I think it would make more sense than using ui::EventWriter.

[xiyuan, 2017/03/09 20:23:00]

Per offline discussion, using a StickyKeysController interface is considered and
seems not very useful.

For the record, see
https://codereview.chromium.org/2724913002/diff/160001/ui/chromeos/events/eve...

     keyboard_event_rewriters_->Init();
   }
 
   // In classic ash must occur after ash::WmShell is initialized. Triggers a
   // fetch of the initial CrosSettings DeviceRebootOnShutdown policy.
   shutdown_policy_forwarder_ = base::MakeUnique<ShutdownPolicyForwarder>();
 
   ChromeBrowserMainPartsLinux::PostBrowserStart();
 }
 
@@ skipping 137 matching lines @@
 
   ChromeBrowserMainPartsLinux::PostDestroyThreads();
 
   // Destroy DeviceSettingsService after g_browser_process.
   DeviceSettingsService::Shutdown();
 
   chromeos::ShutdownCloseTracking();
 }
 
 }  //  namespace chromeos