Side Slide Gestures for Accessibility

ash/ash_touch_exploration_manager_chromeos.h

+// Copyright 2014 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.
+
+#ifndef ASH_TOUCH_EXPLORATION_MANAGER_CHROMEOS_H_
+#define ASH_TOUCH_EXPLORATION_MANAGER_CHROMEOS_H_
+
+#include "ash/root_window_controller.h"

[James Cook, 2014/07/15 18:21:57]

Do you need this #include? The forward declaration below seems sufficient.

[lisayin, 2014/07/15 21:46:44]

Done.

+#include "ash/system/tray_accessibility.h"
+#include "chromeos/audio/cras_audio_handler.h"

[James Cook, 2014/07/15 18:21:57]

Can you forward declare the class you need here?

[lisayin, 2014/07/15 21:46:44]

To clarify, do you mean forward declare CrasAudioHandler? I think it's already
forward declared.

+#include "ui/chromeos/touch_exploration_controller.h"
+

[James Cook, 2014/07/15 18:21:57]

nit: one blank line only

[lisayin, 2014/07/15 21:46:44]

Done.

+
+namespace chromeos {
+class CrasAudioHandler;
+}
+
+namespace ash {
+class RootWindowController;
+
+// Responsible for initializing TouchExplorationController when spoken
+// feedback is on.
+class CrosAccessibilityObserver

[dmazzoni, 2014/07/15 18:04:01]

This class name should match the file name.

[lisayin, 2014/07/15 18:47:43]

Done.

+    : public ash::AccessibilityObserver,
+      public ui::TouchExplorationControllerDelegate {
+ public:
+  explicit CrosAccessibilityObserver(
+      RootWindowController* root_window_controller);
+  virtual ~CrosAccessibilityObserver();
+  // Overridden from TouchExplorationControllerDelegate:

[dmazzoni, 2014/07/15 18:04:01]

Newline before this.

[James Cook, 2014/07/15 18:21:57]

nit: blank line above

Also, while this comment meets the style guide, I prefer versions where the
overridden interface name appears first, because that's the important part of
the line. For example:

// TouchExplorationControllerDelegate overrides:

[lisayin, 2014/07/15 18:47:43]

Done.

+  virtual void const PlayVolumeAdjustSound() OVERRIDE;
+  virtual void AdjustSound(float volume) OVERRIDE;
+
+ private:
+  void UpdateTouchExplorationState();
+  // Overridden from AccessibilityObserver.

[dmazzoni, 2014/07/15 18:04:01]

Separate any overridden functions from others with newlines - group the ones
that aren't overridden together.

[James Cook, 2014/07/15 18:21:57]

nit: blank line above

Also, I tend to put all overridden interfaces in the same section (public vs.
private), and keep them in the same order as the declaration above (public A,
public B has overrides in the order A B).

[lisayin, 2014/07/15 18:47:42]

Done.

+  virtual void OnAccessibilityModeChanged(
+      AccessibilityNotificationVisibility notify) OVERRIDE;
+  bool VolumeAdjustSoundEnabled();
+
+  scoped_ptr<ui::TouchExplorationController> touch_exploration_controller_;
+  RootWindowController* root_window_controller_;
+  chromeos::CrasAudioHandler* audio_handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(CrosAccessibilityObserver);
+};
+
+}  // namespace ash
+
+#endif  // ASH_TOUCH_EXPLORATION_MANAGER_CHROMEOS_H_