[MacViews] Implemented text context menu

ui/base/cocoa/text_services_context_menu.mm

Index: ui/base/cocoa/text_services_context_menu.mm
diff --git a/ui/base/cocoa/text_services_context_menu.mm b/ui/base/cocoa/text_services_context_menu.mm
new file mode 100644
index 0000000000000000000000000000000000000000..d8af2ada5b3bad0baa2efc6938e1609d0dabe878
--- /dev/null
+++ b/ui/base/cocoa/text_services_context_menu.mm
@@ -0,0 +1,176 @@
+// Copyright 2016 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/cocoa/text_services_context_menu.h"
+
+#include <utility>
+
+#import <Cocoa/Cocoa.h>

[tapted, 2016/12/13 05:11:23]

We might not need this now that we're not calling private methods on NSApp.

Which I think means that this file doesn't need to be ObjectiveC -- we can make
it a .cc file instead.

[spqchan, 2016/12/15 23:29:01]

Done.

+
+#include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/strings/grit/ui_strings.h"
+
+namespace ui {
+
+SpeechChannel TextServicesContextMenu::speechChannel;

[tapted, 2016/12/13 05:11:23]

i.e.

namespace {

SpeechChannel g_speech_channel;

/* other stuff */

}  // namespace

[spqchan, 2016/12/15 23:29:01]

Done.

+
+//////////////////////////////////////////////////////////////////
+// TextServicesContextMenu, public:
+
+TextServicesContextMenu::TextServicesContextMenu(Delegate* delegate)
+    : speech_submenu_model_(this),
+      bidi_submenu_model_(this),
+      delegate_(delegate) {
+  DCHECK(delegate);
+}
+
+void TextServicesContextMenu::SpeakText(const base::string16& text) {
+  if (IsSpeaking())
+    StopSpeaking();
+
+  NewSpeechChannel(NULL, &speechChannel);

[tapted, 2016/12/13 05:11:23]

NULL -> nullptr. Same below.

[spqchan, 2016/12/15 23:29:01]

Done.

+  SpeakCFString(speechChannel, SysUTF16ToCFStringRef(text), NULL);
+}
+
+void TextServicesContextMenu::StopSpeaking() {
+  StopSpeechAt(speechChannel, kImmediate);
+  DisposeSpeechChannel(speechChannel);
+}
+
+bool TextServicesContextMenu::IsSpeaking() {
+  return SpeechBusy();
+}
+
+void TextServicesContextMenu::AppendToContextMenu(ui::SimpleMenuModel* model) {

[tapted, 2016/12/13 05:11:23]

nit: no ui::

[spqchan, 2016/12/15 23:29:01]

Done.

+  base::string16 printable_selection_text = delegate_->GetSelectedText();
+  if (printable_selection_text.empty())
+    return;
+
+  if (delegate_->IsLookUpAvailable()) {
+    model->InsertItemAt(0, IDS_CONTENT_CONTEXT_LOOK_UP,

[tapted, 2016/12/13 05:11:23]

I guess this `0` was previously the calculated index - do we need to calculate
it here to avoid a regression for the render view context menu?

[spqchan, 2016/12/15 23:29:01]

Yes, we need it so that the LookUp item actually appears at the top of the menu.

+                        l10n_util::GetStringFUTF16(IDS_CONTENT_CONTEXT_LOOK_UP,
+                                                   printable_selection_text));
+  }
+
+  model->AddSeparator(ui::NORMAL_SEPARATOR);

[tapted, 2016/12/13 05:11:23]

nit: no ui::

[spqchan, 2016/12/15 23:29:01]

Done.

+  speech_submenu_model_.AddItemWithStringId(IDS_SPEECH_START_SPEAKING_MAC,

[tapted, 2016/12/13 05:11:23]

Can we do these `AddItemWithStringId` calls on the submenu in the constructor
instead? I think they're always the same -- no need to recreate each time.

[spqchan, 2016/12/15 23:29:01]

Done

+                                            IDS_SPEECH_START_SPEAKING_MAC);
+  speech_submenu_model_.AddItemWithStringId(IDS_SPEECH_STOP_SPEAKING_MAC,
+                                            IDS_SPEECH_STOP_SPEAKING_MAC);
+  model->AddSubMenu(IDS_SPEECH_MAC, l10n_util::GetStringUTF16(IDS_SPEECH_MAC),
+                    &speech_submenu_model_);
+}
+
+void TextServicesContextMenu::AppendPlatformEditableItems(
+    ui::SimpleMenuModel* model) {

[tapted, 2016/12/13 05:11:23]

nit: no ui::

[spqchan, 2016/12/15 23:29:01]

Done.

+  bidi_submenu_model_.AddCheckItem(
+      IDS_CONTENT_CONTEXT_WRITING_DIRECTION_DEFAULT,

[tapted, 2016/12/13 05:11:23]

Same here - I think these calls can be made in the constructor, and we just add
the submenu here.

[spqchan, 2016/12/15 23:29:01]

Done.

+      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_WRITING_DIRECTION_DEFAULT));
+  bidi_submenu_model_.AddCheckItem(
+      IDS_CONTENT_CONTEXT_WRITING_DIRECTION_LTR,
+      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_WRITING_DIRECTION_LTR));
+  bidi_submenu_model_.AddCheckItem(
+      IDS_CONTENT_CONTEXT_WRITING_DIRECTION_RTL,
+      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_WRITING_DIRECTION_RTL));
+
+  model->AddSubMenu(
+      IDS_CONTENT_CONTEXT_WRITING_DIRECTION_MENU,
+      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_WRITING_DIRECTION_MENU),
+      &bidi_submenu_model_);
+}
+
+bool TextServicesContextMenu::IsTextServicesCommandId(int command_id) const {
+  switch (command_id) {
+    case IDS_SPEECH_START_SPEAKING_MAC:
+    case IDS_SPEECH_STOP_SPEAKING_MAC:
+    case IDS_CONTENT_CONTEXT_LOOK_UP:

[tapted, 2016/12/13 05:11:23]

For the others it might be OK, but do we risk a command_id conflict using
IDS_CONTENT_CONTEXT_LOOK_UP here? That is, can we can guarantee that the
renderer context menu doesn't decide to use a command_id equal to whatever value
IDS_CONTENT_CONTEXT_LOOK_UP takes from the generated header?

[spqchan, 2016/12/15 23:29:01]

That's true. AFAIK. there's no conflict now, but I'm not sure how to prevent
this  I'm not really sure how to do it without using chrome_command_ids. 

I can include chrome_command_ids.h and use the IDC values, or copy the values to
text_services_menu and add a comment about that.

[tapted, 2016/12/16 07:01:19]

So one way to avoid it could be to use MenuModel rather than SimpleMenuModel.
MenuModel uses the index in the menu rather than the command_id to refer to
items. However, I think that will add extra complexity. RenderViewContextMenu is
pretty heavy on the command_id thing.

I think this does need a proper solution. I think for now we should pass the
command_id to use for "lookup" to the method that adds that item.

+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_DEFAULT:
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_LTR:
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_RTL:
+      return true;
+  }
+
+  return false;
+}
+
+//////////////////////////////////////////////////////////////////
+// TextServicesContextMenu::SimpleMenuModel::Delegate:
+
+void TextServicesContextMenu::ExecuteCommand(int command_id, int event_flags) {
+  switch (command_id) {
+    case IDS_SPEECH_START_SPEAKING_MAC:
+      delegate_->OnSpeakRequested();
+      break;
+    case IDS_SPEECH_STOP_SPEAKING_MAC:
+      StopSpeaking();
+      break;
+    case IDS_CONTENT_CONTEXT_LOOK_UP:
+      delegate_->LookUpInDictionary();
+      break;
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_DEFAULT:
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_LTR:
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_RTL:
+      delegate_->UpdateTextDirection(
+          GetWritingDirectionFromCommandId(command_id));
+      break;
+  }

[tapted, 2016/12/13 05:11:23]

default: NOTREACHED()?

[spqchan, 2016/12/15 23:29:01]

Done.

+}
+
+bool TextServicesContextMenu::IsCommandIdChecked(int command_id) const {
+  switch (command_id) {
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_DEFAULT:
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_LTR:
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_RTL:
+      return delegate_->IsWritingDirectionChecked(
+          GetWritingDirectionFromCommandId(command_id));
+  }
+  return false;
+}
+
+bool TextServicesContextMenu::IsCommandIdEnabled(int command_id) const {
+  switch (command_id) {
+    case IDS_SPEECH_START_SPEAKING_MAC:
+    case IDS_CONTENT_CONTEXT_LOOK_UP:
+      // This is OK because the menu is not shown when it isn't
+      // appropriate.
+      return true;
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_DEFAULT:
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_LTR:
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_RTL:
+      return delegate_->IsWritingDirectionEnabled(
+          GetWritingDirectionFromCommandId(command_id));
+    case IDS_SPEECH_STOP_SPEAKING_MAC:
+      return IsSpeaking();
+  }
+
+  return false;

[tapted, 2016/12/13 05:11:23]

NOTREACHED(); before this?

[spqchan, 2016/12/15 23:29:01]

Done.

+}
+
+bool TextServicesContextMenu::GetAcceleratorForCommandId(
+    int command_id,
+    ui::Accelerator* accelerator) const {
+  return false;

[tapted, 2016/12/13 05:11:23]

this matches the default implementation, so I think this method override can be
removed

[spqchan, 2016/12/15 23:29:01]

Done.

+}
+
+//////////////////////////////////////////////////////////////////
+// TextServicesContextMenu, private:
+
+WritingDirection TextServicesContextMenu::GetWritingDirectionFromCommandId(
+    int command_id) const {
+  switch (command_id) {
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_DEFAULT:
+      return WritingDirection::DEFAULT;
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_LTR:
+      return WritingDirection::LTR;
+    case IDS_CONTENT_CONTEXT_WRITING_DIRECTION_RTL:
+      return WritingDirection::RTL;
+    default:
+      NOTREACHED();
+      return WritingDirection::DEFAULT;
+  }
+}
+
+}  // namespace ui