Add Copy URL option to Omnibox context menu when URL is replaced by Instant Extended.

chrome/browser/ui/views/omnibox/omnibox_view_views.cc

Index: chrome/browser/ui/views/omnibox/omnibox_view_views.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
index de46181bb1a875f1735245f9c239c11cbf1cfbd6..4c80c1bcb54f94602cc9923108919b7c028dbf57 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
@@ -798,6 +798,14 @@ void OmniboxViewViews::UpdateContextMenu(ui::SimpleMenuModel* menu_contents) {
   menu_contents->AddItemWithStringId(IDC_EDIT_SEARCH_ENGINES,
       IDS_EDIT_SEARCH_ENGINES);
 
+  if (toolbar_model()->WouldReplaceSearchURLWithSearchTerms() &&

[Peter Kasting, 2012/09/06 00:01:46]

So here you've elected to only insert the command sometimes, as opposed to
always inserting the command and then only enabling it sometimes, which is what
we do for PASTE_AND_GO.

It seems like perhaps we should be consistent.  Normally, I'd be a fan of
removing invalid commands instead of disabling them; however, given that COPY
and PASTE are always present and sometimes disabled, it seems slightly better to
me that COPY_URL and PASTE_AND_GO follow suit.  WDYT?

(Note that I'd still suggest not adding COPY_URL at all when the whole browser
is in the mode where we never ever replace URLs with search terms because then
we'd just have a permanently-disabled menu item which is weird.)

[dominich, 2012/09/06 17:50:25]

Done.

Except on cocoa, which already has the difference that it doesn't add paste and
go unless it's necessary. I don't want to change that behaviour in this CL and I
think OSX acting differently is not entirely unexpected.

+      !model()->user_input_in_progress()) {
+    int copy_position = menu_contents->GetIndexOfCommandId(IDS_APP_COPY);
+    if (copy_position >= 0)

[Peter Kasting, 2012/09/05 22:32:58]

Nit: {} (also please fix the conditional just below while you're here)

Should this be DCHECK_GE(copy_position, 0) instead?  Can we ever not have COPY?

[dominich, 2012/09/05 23:35:34]

I don't know of any cases, but I also don't know of any cases where we don't
have PASTE ,but that's checked on multiple platforms.

I don't think it's an expensive enough check to warrant worrying about it but if
you insist i'll change it.

[Peter Kasting, 2012/09/06 00:01:46]

You hit on the reason I'm worried -- it's not the cost of the change, it's that
it implies this can fail, just like the checks for paste imply those can fail
too.  And if that's not true, then we're misleading the reader, with unknown
consequences down the road.

At least for OmniboxViewViews, both COPY and PASTE are guaranteed to be present
at all times, so both these conditionals should be replaced with DCHECKs.

[dominich, 2012/09/06 17:50:25]

Done.

+      menu_contents->InsertItemWithStringIdAt(
+          copy_position + 1, IDC_COPY_URL, IDS_COPY_URL);
+  }
+
   int paste_position = menu_contents->GetIndexOfCommandId(IDS_APP_PASTE);
   if (paste_position >= 0)
     menu_contents->InsertItemWithStringIdAt(
@@ -830,6 +838,11 @@ void OmniboxViewViews::ExecuteCommand(int command_id) {
     return;

[Peter Kasting, 2012/09/05 22:32:58]

Nit: Shorter:

  if (command_id == IDS_PASTE_AND_GO)
    model()->PasteAndGo(GetClipboardText());
  else if (command_id == IDS_COPY_URL)
    CopyURL();
  else
    command_updater()->ExecuteCommand(command_id);

[dominich, 2012/09/05 23:35:34]

Done.

   }
 
+  if (command_id == IDS_COPY_URL) {
+    CopyURL();
+    return;
+  }
+
   command_updater()->ExecuteCommand(command_id);
 }
 
@@ -905,3 +918,13 @@ string16 OmniboxViewViews::GetSelectedText() const {
   // TODO(oshima): Support instant, IME.
   return textfield_->GetSelectedText();
 }
+
+void OmniboxViewViews::CopyURL() {
+  const string16& text = toolbar_model()->GetText(false);
+  const GURL& url = toolbar_model()->GetURL();
+  ui::Clipboard* cb = views::ViewsDelegate::views_delegate->GetClipboard();
+  ui::ScopedClipboardWriter scw(cb, ui::Clipboard::BUFFER_STANDARD);

[Peter Kasting, 2012/09/05 22:32:58]

I suggest factoring out the end of OnAfterCutOrCopy() and calling it from both
there and here.  See comments in omnibox_view_win.cc.

This will mean that the fix to BookmarkNodeData I've asked for on bug 976 will
automatically apply to this function as well once it lands.

[dominich, 2012/09/05 23:35:34]

Done.

+  scw.WriteText(text);
+  scw.WriteBookmark(text, url.spec());
+  scw.WriteHyperlink(net::EscapeForHTML(text), url.spec());
+}