Fix ui::Accelerator class hierarchy

Currently ui::Accelerator has two platform specific subclasses (ui::AcceleratorCocoa and ui::AcceleratorGtk).

The CL changes the hierarchy so that platform specific data is now stored in a ui::Accelerator::platform_accelerator_ member variable instead. If ui::Accelerator is created by cross platform code or by views code then the member variable is NULL. If it's created by Mac code then it's an instance of ui::PlatformAcceleratorCocoa. If it's create by GTK code then it's an instance of ui::PlatformAcceleratorGtk.

This fixes an issue where code would incorrectly cast a ui::Accelerator instance to a platform specific version. With this change the code can instead do a NULL check for ui::Accelerator::platform_accelerator_.

I also changed ui::Accelerator::modifiers_ so that it no longer stores cross platform modifier values. It instead store ui::EventFlags values.

BUG=160844

Review URL: https://chromiumcodereview.appspot.com/11316071

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@169615 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 250 insertions, 177 deletions

diff --git a/ui/base/accelerators/accelerator.cc b/ui/base/accelerators/accelerator.cc
index 1980225..1104b97 100644
--- a/ui/base/accelerators/accelerator.cc
+++ b/ui/base/accelerators/accelerator.cc
@@ -39,6 +39,8 @@ Accelerator::Accelerator(const Accelerator& accelerator) {
   key_code_ = accelerator.key_code_;
   type_ = accelerator.type_;
   modifiers_ = accelerator.modifiers_;
+  if (accelerator.platform_accelerator_.get())
+    platform_accelerator_ = accelerator.platform_accelerator_->CreateCopy();
 }
 
 Accelerator::~Accelerator() {
@@ -49,6 +51,10 @@ Accelerator& Accelerator::operator=(const Accelerator& accelerator) {
     key_code_ = accelerator.key_code_;
     type_ = accelerator.type_;
     modifiers_ = accelerator.modifiers_;
+    if (accelerator.platform_accelerator_.get())
+      platform_accelerator_ = accelerator.platform_accelerator_->CreateCopy();
+    else
+      platform_accelerator_.reset();
   }
   return *this;
 }
@@ -62,6 +68,12 @@ bool Accelerator::operator <(const Accelerator& rhs) const {
 }
 
 bool Accelerator::operator ==(const Accelerator& rhs) const {
+  if (platform_accelerator_.get() != rhs.platform_accelerator_.get() &&
+      ((!platform_accelerator_.get() || !rhs.platform_accelerator_.get()) ||
+       !platform_accelerator_->Equals(*rhs.platform_accelerator_))) {
+    return false;
+  }
+
   return (key_code_ == rhs.key_code_) && (type_ == rhs.type_) &&
       (modifiers_ == rhs.modifiers_);
 }
diff --git a/ui/base/accelerators/accelerator.h b/ui/base/accelerators/accelerator.h
index 3b92729..4cc4429 100644
--- a/ui/base/accelerators/accelerator.h
+++ b/ui/base/accelerators/accelerator.h
@@ -11,15 +11,19 @@
 #ifndef UI_BASE_ACCELERATORS_ACCELERATOR_H_
 #define UI_BASE_ACCELERATORS_ACCELERATOR_H_
 
+#include "base/memory/scoped_ptr.h"
 #include "base/string16.h"
+#include "ui/base/accelerators/platform_accelerator.h"
 #include "ui/base/events/event_constants.h"
 #include "ui/base/keycodes/keyboard_codes.h"
 #include "ui/base/ui_export.h"
 
 namespace ui {
 
-// This is a cross-platform base class for accelerator keys used in menus. It is
-// meant to be subclassed for concrete toolkit implementations.
+class PlatformAccelerator;
+
+// This is a cross-platform class for accelerator keys used in menus.
+// |platform_accelerator| should be used to store platform specific data.
 class UI_EXPORT Accelerator {
  public:
   Accelerator();
@@ -29,8 +33,8 @@ class UI_EXPORT Accelerator {
 
   Accelerator& operator=(const Accelerator& accelerator);
 
-  // We define the < operator so that the KeyboardShortcut can be used as a key
-  // in a std::map.
+  // Define the < operator so that the KeyboardShortcut can be used as a key in
+  // a std::map.
   bool operator <(const Accelerator& rhs) const;
 
   bool operator ==(const Accelerator& rhs) const;
@@ -54,6 +58,16 @@ class UI_EXPORT Accelerator {
   // Returns a string with the localized shortcut if any.
   string16 GetShortcutText() const;
 
+  void set_platform_accelerator(scoped_ptr<PlatformAccelerator> p) {
+    platform_accelerator_ = p.Pass();
+  }
+
+  // This class keeps ownership of the returned object.
+  const PlatformAccelerator* platform_accelerator() const {
+    return platform_accelerator_.get();
+  }
+
+
  protected:
   // The keycode (VK_...).
   KeyboardCode key_code_;
@@ -61,8 +75,11 @@ class UI_EXPORT Accelerator {
   // The event type (usually ui::ET_KEY_PRESSED).
   EventType type_;
 
-  // The state of the Shift/Ctrl/Alt keys (platform-dependent).
+  // The state of the Shift/Ctrl/Alt keys.
   int modifiers_;
+
+  // Stores platform specific data. May be NULL.
+  scoped_ptr<PlatformAccelerator> platform_accelerator_;
 };
 
 // An interface that classes that want to register for keyboard accelerators
diff --git a/ui/base/accelerators/accelerator_cocoa.h b/ui/base/accelerators/accelerator_cocoa.h
deleted file mode 100644
index 4c0736e..0000000
--- a/ui/base/accelerators/accelerator_cocoa.h
+++ /dev/null
@@ -1,43 +0,0 @@
-// Copyright (c) 2011 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 UI_BASE_ACCELERATORS_ACCELERATOR_COCOA_H_
-#define UI_BASE_ACCELERATORS_ACCELERATOR_COCOA_H_
-
-#include <Foundation/Foundation.h>
-
-#include "base/memory/scoped_nsobject.h"
-#include "ui/base/accelerators/accelerator.h"
-#include "ui/base/ui_export.h"
-
-namespace ui {
-
-// This is a subclass of the cross-platform Accelerator, but with more direct
-// support for Cocoa key equivalents. Note that the typical use case for this
-// class is to initialize it with a string literal, which is why it sends
-// |-copy| to the |key_code| paramater in the constructor.
-class UI_EXPORT AcceleratorCocoa : public Accelerator {
- public:
-  AcceleratorCocoa();
-  AcceleratorCocoa(NSString* key_code, NSUInteger mask);
-  AcceleratorCocoa(const AcceleratorCocoa& accelerator);
-  virtual ~AcceleratorCocoa();
-
-  AcceleratorCocoa& operator=(const AcceleratorCocoa& accelerator);
-
-  bool operator==(const AcceleratorCocoa& rhs) const;
-  bool operator!=(const AcceleratorCocoa& rhs) const;
-
-  NSString* characters() const {
-    return characters_.get();
-  }
-
- private:
-  // String of characters for the key equivalent.
-  scoped_nsobject<NSString> characters_;
-};
-
-}  // namespace ui
-
-#endif  // UI_BASE_ACCELERATORS_ACCELERATOR_COCOA_H_
diff --git a/ui/base/accelerators/accelerator_cocoa.mm b/ui/base/accelerators/accelerator_cocoa.mm
deleted file mode 100644
index f88290c..0000000
--- a/ui/base/accelerators/accelerator_cocoa.mm
+++ /dev/null
@@ -1,41 +0,0 @@
-// Copyright (c) 2011 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 "ui/base/accelerators/accelerator_cocoa.h"
-
-namespace ui {
-
-AcceleratorCocoa::AcceleratorCocoa() : Accelerator() {}
-
-AcceleratorCocoa::AcceleratorCocoa(NSString* key_code, NSUInteger mask)
-    : Accelerator(ui::VKEY_UNKNOWN, mask),
-      characters_([key_code copy]) {
-}
-
-AcceleratorCocoa::AcceleratorCocoa(const AcceleratorCocoa& accelerator)
-    : Accelerator(accelerator) {
-  characters_.reset([accelerator.characters_ copy]);
-}
-
-AcceleratorCocoa::~AcceleratorCocoa() {}
-
-AcceleratorCocoa& AcceleratorCocoa::operator=(
-    const AcceleratorCocoa& accelerator) {
-  if (this != &accelerator) {
-    *static_cast<Accelerator*>(this) = accelerator;
-    characters_.reset([accelerator.characters_ copy]);
-  }
-  return *this;
-}
-
-bool AcceleratorCocoa::operator==(const AcceleratorCocoa& rhs) const {
-  return [characters_ isEqualToString:rhs.characters_.get()] &&
-  (modifiers_ == rhs.modifiers_);
-}
-
-bool AcceleratorCocoa::operator!=(const AcceleratorCocoa& rhs) const {
-  return !(*this == rhs);
-}
-
-}  // namespace ui
diff --git a/ui/base/accelerators/accelerator_gtk.cc b/ui/base/accelerators/accelerator_gtk.cc
deleted file mode 100644
index 5bac48c..0000000
--- a/ui/base/accelerators/accelerator_gtk.cc
+++ /dev/null
@@ -1,46 +0,0 @@
-// Copyright (c) 2012 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 "ui/base/accelerators/accelerator_gtk.h"
-
-#include "ui/base/keycodes/keyboard_code_conversion_gtk.h"
-#include "ui/base/keycodes/keyboard_codes_posix.h"
-
-namespace ui {
-
-AcceleratorGtk::AcceleratorGtk() : gdk_key_code_(0) {
-}
-
-AcceleratorGtk::AcceleratorGtk(guint key_code, GdkModifierType modifier_type)
-    : Accelerator(WindowsKeyCodeForGdkKeyCode(key_code), modifier_type),
-      gdk_key_code_(key_code) {
-}
-
-AcceleratorGtk::AcceleratorGtk(KeyboardCode key_code,
-                               bool shift_pressed,
-                               bool ctrl_pressed,
-                               bool alt_pressed)
-    : Accelerator(key_code, 0),
-      gdk_key_code_(0) {
-  if (shift_pressed)
-    modifiers_ |= GDK_SHIFT_MASK;
-  if (ctrl_pressed)
-    modifiers_ |= GDK_CONTROL_MASK;
-  if (alt_pressed)
-    modifiers_ |= GDK_MOD1_MASK;
-}
-
-AcceleratorGtk::~AcceleratorGtk() {
-}
-
-guint AcceleratorGtk::GetGdkKeyCode() const {
-  if (gdk_key_code_ > 0)
-    return gdk_key_code_;
-
-  // The second parameter is false because accelerator keys are expressed in
-  // terms of the non-shift-modified key.
-  return GdkKeyCodeForWindowsKeyCode(key_code_, false);
-}
-
-}  // namespace ui
diff --git a/ui/base/accelerators/accelerator_gtk.h b/ui/base/accelerators/accelerator_gtk.h
deleted file mode 100644
index 3673d24..0000000
--- a/ui/base/accelerators/accelerator_gtk.h
+++ /dev/null
@@ -1,42 +0,0 @@
-// Copyright (c) 2012 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 UI_BASE_ACCELERATORS_ACCELERATOR_GTK_H_
-#define UI_BASE_ACCELERATORS_ACCELERATOR_GTK_H_
-
-#include <gdk/gdk.h>
-
-#include "ui/base/accelerators/accelerator.h"
-#include "ui/base/ui_export.h"
-
-namespace ui {
-
-class UI_EXPORT AcceleratorGtk : public Accelerator {
- public:
-  AcceleratorGtk();
-
-  AcceleratorGtk(guint key_code, GdkModifierType modifier_type);
-
-  AcceleratorGtk(KeyboardCode key_code,
-                 bool shift_pressed,
-                 bool ctrl_pressed,
-                 bool alt_pressed);
-
-  virtual ~AcceleratorGtk();
-
-  guint GetGdkKeyCode() const;
-
-  GdkModifierType gdk_modifier_type() const {
-    return static_cast<GdkModifierType>(modifiers_);
-  }
-
- private:
-  guint gdk_key_code_;
-
-  // Copy and assign is allowed.
-};
-
-}  // namespace ui
-
-#endif  // UI_BASE_ACCELERATORS_ACCELERATOR_GTK_H_
diff --git a/ui/base/accelerators/platform_accelerator.h b/ui/base/accelerators/platform_accelerator.h
new file mode 100644
index 0000000..1b46003
--- /dev/null
+++ b/ui/base/accelerators/platform_accelerator.h
@@ -0,0 +1,24 @@
+// Copyright (c) 2012 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 UI_BASE_ACCELERATORS_PLATFORM_ACCELERATOR_H_
+#define UI_BASE_ACCELERATORS_PLATFORM_ACCELERATOR_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "ui/base/ui_export.h"
+
+namespace ui {
+
+// Abstract base class for platform specific accelerator keys.
+class UI_EXPORT PlatformAccelerator {
+ public:
+  virtual ~PlatformAccelerator() {}
+
+  virtual scoped_ptr<PlatformAccelerator> CreateCopy() const = 0;
+  virtual bool Equals(const PlatformAccelerator& rhs) const = 0;
+};
+
+}  // namespace ui
+
+#endif  // UI_BASE_ACCELERATORS_PLATFORM_ACCELERATOR_H_
diff --git a/ui/base/accelerators/platform_accelerator_cocoa.h b/ui/base/accelerators/platform_accelerator_cocoa.h
new file mode 100644
index 0000000..5ac8261
--- /dev/null
+++ b/ui/base/accelerators/platform_accelerator_cocoa.h
@@ -0,0 +1,39 @@
+// Copyright (c) 2011 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 UI_BASE_ACCELERATORS_PLATFORM_ACCELERATOR_COCOA_H_
+#define UI_BASE_ACCELERATORS_PLATFORM_ACCELERATOR_COCOA_H_
+
+#include <Foundation/Foundation.h>
+
+#include "base/memory/scoped_nsobject.h"
+#include "ui/base/accelerators/platform_accelerator.h"
+
+namespace ui {
+
+// This is a Mac specific class for specifing accelerator keys.
+class UI_EXPORT PlatformAcceleratorCocoa : public PlatformAccelerator {
+ public:
+  PlatformAcceleratorCocoa();
+  PlatformAcceleratorCocoa(NSString* key_code, NSUInteger modifier_mask);
+  virtual ~PlatformAcceleratorCocoa();
+
+  // PlatformAccelerator:
+  virtual scoped_ptr<PlatformAccelerator> CreateCopy() const OVERRIDE;
+  virtual bool Equals(const PlatformAccelerator& rhs) const OVERRIDE;
+
+  NSString* characters() const { return characters_.get(); }
+  NSUInteger modifier_mask() const { return modifier_mask_; }
+
+ private:
+  // String of characters for the key equivalent.
+  scoped_nsobject<NSString> characters_;
+  NSUInteger modifier_mask_;
+
+  DISALLOW_COPY_AND_ASSIGN(PlatformAcceleratorCocoa);
+};
+
+}  // namespace ui
+
+#endif  // UI_BASE_ACCELERATORS_PLATFORM_ACCELERATOR_COCOA_H_
diff --git a/ui/base/accelerators/platform_accelerator_cocoa.mm b/ui/base/accelerators/platform_accelerator_cocoa.mm
new file mode 100644
index 0000000..cffc458
--- /dev/null
+++ b/ui/base/accelerators/platform_accelerator_cocoa.mm
@@ -0,0 +1,37 @@
+// Copyright (c) 2011 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.
+
+#import "ui/base/accelerators/platform_accelerator_cocoa.h"
+
+#include "base/memory/scoped_ptr.h"
+
+namespace ui {
+
+PlatformAcceleratorCocoa::PlatformAcceleratorCocoa() : modifier_mask_(0) {
+}
+
+PlatformAcceleratorCocoa::PlatformAcceleratorCocoa(NSString* key_code,
+                                                   NSUInteger modifier_mask)
+    : characters_([key_code copy]),
+      modifier_mask_(modifier_mask) {
+}
+
+PlatformAcceleratorCocoa::~PlatformAcceleratorCocoa() {
+}
+
+scoped_ptr<PlatformAccelerator> PlatformAcceleratorCocoa::CreateCopy() const {
+  scoped_ptr<PlatformAcceleratorCocoa> copy(new PlatformAcceleratorCocoa);
+  copy->characters_.reset([characters_ copy]);
+  copy->modifier_mask_ = modifier_mask_;
+  return scoped_ptr<PlatformAccelerator>(copy.release());
+}
+
+bool PlatformAcceleratorCocoa::Equals(const PlatformAccelerator& rhs) const {
+  const PlatformAcceleratorCocoa& rhs_cocoa =
+      static_cast<const PlatformAcceleratorCocoa&>(rhs);
+  return [characters_ isEqualToString:rhs_cocoa.characters_] &&
+         modifier_mask_ == rhs_cocoa.modifier_mask_;
+}
+
+}  // namespace ui
diff --git a/ui/base/accelerators/platform_accelerator_gtk.cc b/ui/base/accelerators/platform_accelerator_gtk.cc
new file mode 100644
index 0000000..cae259b
--- /dev/null
+++ b/ui/base/accelerators/platform_accelerator_gtk.cc
@@ -0,0 +1,68 @@
+// Copyright (c) 2012 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 "ui/base/accelerators/platform_accelerator_gtk.h"
+
+#include "ui/base/events/event_conversion_gtk.h"
+#include "ui/base/keycodes/keyboard_code_conversion_gtk.h"
+
+namespace ui {
+
+PlatformAcceleratorGtk::PlatformAcceleratorGtk()
+    : gdk_key_code_(0),
+      gdk_modifier_(static_cast<GdkModifierType>(0)) {
+}
+
+PlatformAcceleratorGtk::PlatformAcceleratorGtk(guint gdk_key_code,
+                                               GdkModifierType gdk_modifier)
+    : gdk_key_code_(gdk_key_code),
+      gdk_modifier_(gdk_modifier) {
+}
+
+PlatformAcceleratorGtk::~PlatformAcceleratorGtk() {
+}
+
+scoped_ptr<PlatformAccelerator> PlatformAcceleratorGtk::CreateCopy() const {
+  scoped_ptr<PlatformAcceleratorGtk> copy(new PlatformAcceleratorGtk);
+  copy->gdk_key_code_ = gdk_key_code_;
+  copy->gdk_modifier_ = gdk_modifier_;
+  return scoped_ptr<PlatformAccelerator>(copy.release());
+}
+
+bool PlatformAcceleratorGtk::Equals(const PlatformAccelerator& rhs) const {
+  const PlatformAcceleratorGtk& rhs_gtk =
+      static_cast<const PlatformAcceleratorGtk&>(rhs);
+  return gdk_key_code_ == rhs_gtk.gdk_key_code_ &&
+         gdk_modifier_ == rhs_gtk.gdk_modifier_;
+}
+
+Accelerator AcceleratorForGdkKeyCodeAndModifier(guint gdk_key_code,
+                                                GdkModifierType gdk_modifier) {
+  ui::Accelerator accelerator(ui::WindowsKeyCodeForGdkKeyCode(gdk_key_code),
+                              ui::GdkModifierToEventFlag(gdk_modifier));
+  scoped_ptr<PlatformAccelerator> platform_accelerator(
+      new PlatformAcceleratorGtk(gdk_key_code, gdk_modifier));
+  accelerator.set_platform_accelerator(platform_accelerator.Pass());
+  return accelerator;
+}
+
+guint GetGdkKeyCodeForAccelerator(const Accelerator& accelerator) {
+  if (accelerator.platform_accelerator()) {
+    return static_cast<const PlatformAcceleratorGtk*>(
+        accelerator.platform_accelerator())->gdk_key_code();
+  }
+  // The second parameter is false because accelerator keys are expressed in
+  // terms of the non-shift-modified key.
+  return GdkKeyCodeForWindowsKeyCode(accelerator.key_code(), false);
+}
+
+GdkModifierType GetGdkModifierForAccelerator(const Accelerator& accelerator) {
+  if (accelerator.platform_accelerator()) {
+    return static_cast<const PlatformAcceleratorGtk*>(
+        accelerator.platform_accelerator())->gdk_modifier();
+  }
+  return EventFlagToGdkModifier(accelerator.modifiers());
+}
+
+}  // namespace ui
diff --git a/ui/base/accelerators/platform_accelerator_gtk.h b/ui/base/accelerators/platform_accelerator_gtk.h
new file mode 100644
index 0000000..87900dd
--- /dev/null
+++ b/ui/base/accelerators/platform_accelerator_gtk.h
@@ -0,0 +1,48 @@
+// Copyright (c) 2012 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 UI_BASE_ACCELERATORS_PLATFORM_ACCELERATOR_GTK_H_
+#define UI_BASE_ACCELERATORS_PLATFORM_ACCELERATOR_GTK_H_
+
+#include <gdk/gdk.h>
+
+#include "base/compiler_specific.h"
+#include "ui/base/accelerators/accelerator.h"
+#include "ui/base/accelerators/platform_accelerator.h"
+
+namespace ui {
+
+class Accelerator;
+
+// This is a GTK specific class for specifing accelerator keys.
+class UI_EXPORT PlatformAcceleratorGtk : public PlatformAccelerator {
+ public:
+  PlatformAcceleratorGtk();
+  PlatformAcceleratorGtk(guint gdk_key_code, GdkModifierType gdk_modifier);
+  virtual ~PlatformAcceleratorGtk();
+
+  // PlatformAccelerator:
+  virtual scoped_ptr<PlatformAccelerator> CreateCopy() const OVERRIDE;
+  virtual bool Equals(const PlatformAccelerator& rhs) const OVERRIDE;
+
+  guint gdk_key_code() const { return gdk_key_code_; }
+  GdkModifierType gdk_modifier() const { return gdk_modifier_; }
+
+ private:
+  guint gdk_key_code_;
+  GdkModifierType gdk_modifier_;
+
+  DISALLOW_COPY_AND_ASSIGN(PlatformAcceleratorGtk);
+};
+
+UI_EXPORT Accelerator AcceleratorForGdkKeyCodeAndModifier(
+    guint gdk_key_code,
+    GdkModifierType gdk_modifier);
+UI_EXPORT guint GetGdkKeyCodeForAccelerator(const Accelerator& accelerator);
+UI_EXPORT GdkModifierType GetGdkModifierForAccelerator(
+    const Accelerator& accelerator);
+
+}  // namespace ui
+
+#endif  // UI_BASE_ACCELERATORS_PLATFORM_ACCELERATOR_GTK_H_