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

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

 // 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 "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
 
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
@@ skipping 780 matching lines @@
     data->SetURL(url, selected_text);
 }
 
 void OmniboxViewViews::UpdateContextMenu(ui::SimpleMenuModel* menu_contents) {
   // Minor note: We use IDC_ for command id here while the underlying textfield
   // is using IDS_ for all its command ids. This is because views cannot depend
   // on IDC_ for now.
   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(
         paste_position + 1, IDS_PASTE_AND_GO, IDS_PASTE_AND_GO);
 }
 
 bool OmniboxViewViews::IsCommandIdEnabled(int command_id) const {
   return (command_id == IDS_PASTE_AND_GO) ?
       model()->CanPasteAndGo(GetClipboardText()) :
       command_updater()->IsCommandEnabled(command_id);
 }
 
 bool OmniboxViewViews::IsItemForCommandIdDynamic(int command_id) const {
   return command_id == IDS_PASTE_AND_GO;
 }
 
 string16 OmniboxViewViews::GetLabelForCommandId(int command_id) const {
   if (command_id == IDS_PASTE_AND_GO) {
     return l10n_util::GetStringUTF16(
         model()->IsPasteAndSearch(GetClipboardText()) ?
         IDS_PASTE_AND_SEARCH : IDS_PASTE_AND_GO);
   }
 
   return string16();
 }
 
 void OmniboxViewViews::ExecuteCommand(int command_id) {
   if (command_id == IDS_PASTE_AND_GO) {
     model()->PasteAndGo(GetClipboardText());
     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);
 }
 
 #if defined(OS_CHROMEOS)
 void OmniboxViewViews::CandidateWindowOpened(
       chromeos::input_method::InputMethodManager* manager) {
   ime_candidate_window_open_ = true;
   CloseOmniboxPopup();
 }
 
@@ skipping 55 matching lines @@
                                                const ui::Range& range) {
   if (text != GetText())
     textfield_->SetText(text);
   textfield_->SelectRange(range);
 }
 
 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());
+}