[MacViews] Implemented text context menu

chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm

Index: chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
diff --git a/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm b/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
index 549d41b6ec2f32c6222b816402010e7e14a8a3e4..63a787abd1d7121c01fe6ca44cf165138fe87194 100644
--- a/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
+++ b/chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
@@ -183,9 +183,8 @@ RenderViewContextMenuMac::RenderViewContextMenuMac(
     const content::ContextMenuParams& params,
     NSView* parent_view)
     : RenderViewContextMenu(render_frame_host, params),
-      speech_submenu_model_(this),
-      bidi_submenu_model_(this),
-      parent_view_(parent_view) {
+      parent_view_(parent_view),
+      text_services_context_menu_(this) {
   std::unique_ptr<ToolkitDelegate> delegate(new ToolkitDelegateMac(this));
   set_toolkit_delegate(std::move(delegate));
 }
@@ -246,96 +245,36 @@ void RenderViewContextMenuMac::Show() {
 
 void RenderViewContextMenuMac::ExecuteCommand(int command_id, int event_flags) {
   switch (command_id) {

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

no need to switch here

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

Done.

-    case IDC_CONTENT_CONTEXT_LOOK_UP:
-      LookUpInDictionary();
-      break;
-
-    case IDC_CONTENT_CONTEXT_SPEECH_START_SPEAKING:
-      StartSpeaking();
-      break;
-
-    case IDC_CONTENT_CONTEXT_SPEECH_STOP_SPEAKING:
-      StopSpeaking();
-      break;
-
-    case IDC_WRITING_DIRECTION_DEFAULT:
-      // WebKit's current behavior is for this menu item to always be disabled.
-      NOTREACHED();
-      break;
-
-    case IDC_WRITING_DIRECTION_RTL:
-    case IDC_WRITING_DIRECTION_LTR: {
-      content::RenderViewHost* view_host = GetRenderViewHost();
-      blink::WebTextDirection dir = blink::WebTextDirectionLeftToRight;
-      if (command_id == IDC_WRITING_DIRECTION_RTL)
-        dir = blink::WebTextDirectionRightToLeft;
-      view_host->GetWidget()->UpdateTextDirection(dir);
-      view_host->GetWidget()->NotifyTextDirection();
-      RenderViewContextMenu::RecordUsedItem(command_id);
-      break;
-    }
-
     default:
+      if (text_services_context_menu_.IsTextServicesCommandId(command_id)) {
+        return text_services_context_menu_.ExecuteCommand(command_id,
+                                                          event_flags);
+      }
+
       RenderViewContextMenu::ExecuteCommand(command_id, event_flags);
       break;
   }
 }
 
 bool RenderViewContextMenuMac::IsCommandIdChecked(int command_id) const {
-  switch (command_id) {
-    case IDC_WRITING_DIRECTION_DEFAULT:
-      return params_.writing_direction_default &
-          blink::WebContextMenuData::CheckableMenuItemChecked;
-    case IDC_WRITING_DIRECTION_RTL:
-      return params_.writing_direction_right_to_left &
-          blink::WebContextMenuData::CheckableMenuItemChecked;
-    case IDC_WRITING_DIRECTION_LTR:
-      return params_.writing_direction_left_to_right &
-          blink::WebContextMenuData::CheckableMenuItemChecked;
+  if (text_services_context_menu_.IsTextServicesCommandId(command_id))
+    return text_services_context_menu_.IsCommandIdChecked(command_id);
 
-    default:
-      return RenderViewContextMenu::IsCommandIdChecked(command_id);
-  }
+  return RenderViewContextMenu::IsCommandIdChecked(command_id);
 }
 
 bool RenderViewContextMenuMac::IsCommandIdEnabled(int command_id) const {
-  switch (command_id) {
-    case IDC_CONTENT_CONTEXT_LOOK_UP:
-      // This is OK because the menu is not shown when it isn't
-      // appropriate.
-      return true;
-
-    case IDC_CONTENT_CONTEXT_SPEECH_START_SPEAKING:
-      // This is OK because the menu is not shown when it isn't
-      // appropriate.
-      return true;
-
-    case IDC_CONTENT_CONTEXT_SPEECH_STOP_SPEAKING: {
-      content::RenderWidgetHostView* view =
-          GetRenderViewHost()->GetWidget()->GetView();
-      return view && view->IsSpeaking();
-    }
+  if (text_services_context_menu_.IsTextServicesCommandId(command_id))

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

Is it only IDS_CONTENT_CONTEXT_LOOK_UP that comes through here? (should the
others go directly to TextServicesContextMenu since that is the delegate for the
submenus?)

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

Yes, only lookup goes through here. I'm a bit confused with what you mean with
your second statement

+    return text_services_context_menu_.IsCommandIdEnabled(command_id);
 
-    case IDC_WRITING_DIRECTION_DEFAULT:  // Provided to match OS defaults.
-      return params_.writing_direction_default &
-          blink::WebContextMenuData::CheckableMenuItemEnabled;
-    case IDC_WRITING_DIRECTION_RTL:
-      return params_.writing_direction_right_to_left &
-          blink::WebContextMenuData::CheckableMenuItemEnabled;
-    case IDC_WRITING_DIRECTION_LTR:
-      return params_.writing_direction_left_to_right &
-          blink::WebContextMenuData::CheckableMenuItemEnabled;
-
-    default:
-      return RenderViewContextMenu::IsCommandIdEnabled(command_id);
-  }
+  return RenderViewContextMenu::IsCommandIdEnabled(command_id);
 }
 
 void RenderViewContextMenuMac::AppendPlatformEditableItems() {
   // OS X provides a contextual menu to set writing direction for BiDi
   // languages.
   // This functionality is exposed as a keyboard shortcut on Windows & Linux.
-  AppendBidiSubMenu();
+  text_services_context_menu_.AppendPlatformEditableItems(&menu_model_);
 }
 
 void RenderViewContextMenuMac::InitToolkitMenu() {
@@ -357,31 +296,10 @@ void RenderViewContextMenuMac::InitToolkitMenu() {
       index += 1; // Place it below the separator.

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

I think the "it" here is IDS_CONTENT_CONTEXT_LOOK_UP which has been moved to
text_services_context_menu, so the stuff from `if (params_.link_url.is_empty())
{` should move into text_services_context_menu as well.

that is, InitToolkitMenu() should just be

void RenderViewContextMenuMac::InitToolkitMenu() {
  text_services_context_menu_.AppendToContextMenu(&menu_model_);
}

with the logic above implemented using delegate interfaces

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

I'm a bit confused. I'm pretty sure that this section is for the spellchecks and
adding new words into the dictionary.

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

The problem is that all the code between `if (params_.link_url.is_empty()) {`
and here just sets a value for |index|. That |index| was then used to insert
IDC_CONTENT_CONTEXT_LOOK_UP immediately followed by a separator. Now it just
inserts a separator at some location, but there's no link between that separator
and IDC_CONTENT_CONTEXT_LOOK_UP since that's now inserted elsewhere.

So shouldn't the InsertSeparatorAt line below (and the |index| calculation here)
happen where IDC_CONTENT_CONTEXT_LOOK_UP is inserted?

     }
 
-    base::string16 printable_selection_text = PrintableSelectionText();
-    EscapeAmpersands(&printable_selection_text);
-    menu_model_.InsertItemAt(
-        index++,
-        IDC_CONTENT_CONTEXT_LOOK_UP,
-        l10n_util::GetStringFUTF16(IDS_CONTENT_CONTEXT_LOOK_UP,
-                                   printable_selection_text));
     menu_model_.InsertSeparatorAt(index++, ui::NORMAL_SEPARATOR);
   }
 
-  content::RenderWidgetHostView* view =
-      GetRenderViewHost()->GetWidget()->GetView();
-  if (view && view->SupportsSpeech()) {
-    menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
-    speech_submenu_model_.AddItemWithStringId(
-        IDC_CONTENT_CONTEXT_SPEECH_START_SPEAKING,
-        IDS_SPEECH_START_SPEAKING_MAC);
-    speech_submenu_model_.AddItemWithStringId(
-        IDC_CONTENT_CONTEXT_SPEECH_STOP_SPEAKING,
-        IDS_SPEECH_STOP_SPEAKING_MAC);
-    menu_model_.AddSubMenu(
-        IDC_CONTENT_CONTEXT_SPEECH_MENU,
-        l10n_util::GetStringUTF16(IDS_SPEECH_MAC),
-        &speech_submenu_model_);
-  }
+  text_services_context_menu_.AppendToContextMenu(&menu_model_);
 }
 
 void RenderViewContextMenuMac::CancelToolkitMenu() {
@@ -405,37 +323,73 @@ void RenderViewContextMenuMac::UpdateToolkitMenuItem(
   [[item menu] itemChanged:item];
 }
 
-void RenderViewContextMenuMac::AppendBidiSubMenu() {
-  bidi_submenu_model_.AddCheckItem(IDC_WRITING_DIRECTION_DEFAULT,
-      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_WRITING_DIRECTION_DEFAULT));
-  bidi_submenu_model_.AddCheckItem(IDC_WRITING_DIRECTION_LTR,
-      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_WRITING_DIRECTION_LTR));
-  bidi_submenu_model_.AddCheckItem(IDC_WRITING_DIRECTION_RTL,
-      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_WRITING_DIRECTION_RTL));
-
-  menu_model_.AddSubMenu(
-      IDC_WRITING_DIRECTION_MENU,
-      l10n_util::GetStringUTF16(IDS_CONTENT_CONTEXT_WRITING_DIRECTION_MENU),
-      &bidi_submenu_model_);
+base::string16 RenderViewContextMenuMac::GetSelectedText() const {
+  return params_.selection_text;
 }
 
-void RenderViewContextMenuMac::LookUpInDictionary() {
+void RenderViewContextMenuMac::OnSpeakRequested() {
   content::RenderWidgetHostView* view =
       GetRenderViewHost()->GetWidget()->GetView();
   if (view)
-    view->ShowDefinitionForSelection();
+    view->SpeakSelection();
 }
 
-void RenderViewContextMenuMac::StartSpeaking() {
-  content::RenderWidgetHostView* view =
-      GetRenderViewHost()->GetWidget()->GetView();
-  if (view)
-    view->SpeakSelection();
+bool RenderViewContextMenuMac::IsLookUpAvailable() const {
+  return params_.link_url.is_empty();
 }
 
-void RenderViewContextMenuMac::StopSpeaking() {
+void RenderViewContextMenuMac::LookUpInDictionary() {
   content::RenderWidgetHostView* view =
       GetRenderViewHost()->GetWidget()->GetView();
   if (view)
-    view->StopSpeaking();
+    view->ShowDefinitionForSelection();
+}
+
+bool RenderViewContextMenuMac::IsWritingDirectionEnabled(
+    ui::WritingDirection direction) const {
+  switch (direction) {
+    case ui::WritingDirection::DEFAULT:
+      return params_.writing_direction_default &
+             blink::WebContextMenuData::CheckableMenuItemEnabled;
+    case ui::WritingDirection::RTL:
+      return params_.writing_direction_right_to_left &
+             blink::WebContextMenuData::CheckableMenuItemEnabled;
+    case ui::WritingDirection::LTR:
+      return params_.writing_direction_left_to_right &
+             blink::WebContextMenuData::CheckableMenuItemEnabled;
+  }
+}
+
+bool RenderViewContextMenuMac::IsWritingDirectionChecked(
+    ui::WritingDirection direction) const {
+  switch (direction) {
+    case ui::WritingDirection::DEFAULT:
+      return params_.writing_direction_default &
+             blink::WebContextMenuData::CheckableMenuItemChecked;
+    case ui::WritingDirection::RTL:
+      return params_.writing_direction_right_to_left &
+             blink::WebContextMenuData::CheckableMenuItemChecked;
+    case ui::WritingDirection::LTR:
+      return params_.writing_direction_left_to_right &
+             blink::WebContextMenuData::CheckableMenuItemChecked;
+  }
+}
+
+void RenderViewContextMenuMac::UpdateTextDirection(
+    ui::WritingDirection direction) {
+  if (direction == ui::WritingDirection::DEFAULT)

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

nit: DCHECK_NE(ui::WritingDirection::DEFAULT, direction)

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

Done.

+    NOTREACHED();
+
+  content::RenderViewHost* view_host = GetRenderViewHost();
+  blink::WebTextDirection dir = blink::WebTextDirectionLeftToRight;
+  int command_id = IDC_WRITING_DIRECTION_LTR;
+  if (direction == ui::WritingDirection::RTL) {
+    dir = blink::WebTextDirectionRightToLeft;
+    command_id = IDC_WRITING_DIRECTION_RTL;
+  }
+
+  view_host->GetWidget()->UpdateTextDirection(dir);
+  view_host->GetWidget()->NotifyTextDirection();
+
+  RenderViewContextMenu::RecordUsedItem(command_id);
 }