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

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

BUG=135106
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156406

[dominich, 2012-09-05 00:01:08]

dhollowa: ui/cocoa
estade: ui/gtk
pkasting: ui/views
cpu: chrome/app

[cpu_(ooo_6.6-7.5), 2012-09-05 01:11:30]

/app changes lgtm

[Evan Stade, 2012-09-05 21:07:16]

gtk lgtm with nits

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:147: }
blank line and function docs

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:149: string16
stock_label(UTF8ToUTF16(label));
why do you convert to utf16 before comparison?

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1605: const string16& text =
toolbar_model()->GetText(false);
nit: I'd declare these in the order they're used.

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1610:
data.WriteToClipboard(NULL);
why don't you pass the profile?

[dominich, 2012-09-05 21:21:39]

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:147: }
On 2012/09/05 21:07:17, Evan Stade wrote:
> blank line and function docs

Done.

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:149: string16
stock_label(UTF8ToUTF16(label));
On 2012/09/05 21:07:17, Evan Stade wrote:
> why do you convert to utf16 before comparison?

It's what the original code was doing. It seemed strange to me as UTF8 string
compare should work as expected.

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1605: const string16& text =
toolbar_model()->GetText(false);
On 2012/09/05 21:07:17, Evan Stade wrote:
> nit: I'd declare these in the order they're used.

Done.

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1610:
data.WriteToClipboard(NULL);
On 2012/09/05 21:07:17, Evan Stade wrote:
> why don't you pass the profile?

the HandleCopyOrCutClipboard method below passes NULL.

[dhollowa, 2012-09-05 21:22:34]

LGTM w/ nits.

http://codereview.chromium.org/10915069/diff/4034/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

http://codereview.chromium.org/10915069/diff/4034/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:102: if
(observer && observer->CanCopy())
nit: No need to handle DCHECK failure.  Prefer: if (observer->CanCopy())

http://codereview.chromium.org/10915069/diff/4034/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:223: if
(observer && observer->ShouldAddCopyURL()) {
nit: No need to handle DCHECK failure.  Prefer: if
(observer->ShouldAddCopyURL())

http://codereview.chromium.org/10915069/diff/4034/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:235: 
nit: remove extra line.

http://codereview.chromium.org/10915069/diff/4034/chrome/browser/ui/cocoa/omn...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/10915069/diff/4034/chrome/browser/ui/cocoa/omn...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:894: // TODO(dominich): Move
to OmniboxView base class?
Seems like now is a good time for this, and consolidate with Win implementation.

[dominich, 2012-09-05 21:30:54]

cpu: Any idea why android is failing thinking that generated_resources.grd has a
third party license in it?

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:102: if
(observer && observer->CanCopy())
On 2012/09/05 21:22:34, dhollowa wrote:
> nit: No need to handle DCHECK failure.  Prefer: if (observer->CanCopy())

Done.

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:223: if
(observer && observer->ShouldAddCopyURL()) {
On 2012/09/05 21:22:34, dhollowa wrote:
> nit: No need to handle DCHECK failure.  Prefer: if
> (observer->ShouldAddCopyURL())

Done.

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:235: 
On 2012/09/05 21:22:34, dhollowa wrote:
> nit: remove extra line.

Done.

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/c...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:894: // TODO(dominich): Move
to OmniboxView base class?
On 2012/09/05 21:22:34, dhollowa wrote:
> Seems like now is a good time for this, and consolidate with Win
implementation.

Sadly it's not that simple. ShouldAddCopyURL is actually called on the
AutocompleteTextFieldObserver by the AutocompleteTextField, not on the
OmniboxView. I could put it on OmniboxView but that would mean making the
AutocompleteTextFieldObserver inherit from OmniboxView directly which is a big
change to the class layout.

[Evan Stade, 2012-09-05 22:11:50]

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:149: string16
stock_label(UTF8ToUTF16(label));
On 2012/09/05 21:21:39, dominich wrote:
> On 2012/09/05 21:07:17, Evan Stade wrote:
> > why do you convert to utf16 before comparison?
> 
> It's what the original code was doing. It seemed strange to me as UTF8 string
> compare should work as expected.

well then let's change it.

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1610:
data.WriteToClipboard(NULL);
On 2012/09/05 21:21:39, dominich wrote:
> On 2012/09/05 21:07:17, Evan Stade wrote:
> > why don't you pass the profile?
> 
> the HandleCopyOrCutClipboard method below passes NULL.

why does that one pass NULL?

[dhollowa, 2012-09-05 22:13:42]

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/c...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:894: // TODO(dominich): Move
to OmniboxView base class?
On 2012/09/05 21:30:54, dominich wrote:
> On 2012/09/05 21:22:34, dhollowa wrote:
> > Seems like now is a good time for this, and consolidate with Win
> implementation.
> 
> Sadly it's not that simple. ShouldAddCopyURL is actually called on the
> AutocompleteTextFieldObserver by the AutocompleteTextField, not on the
> OmniboxView. I could put it on OmniboxView but that would mean making the
> AutocompleteTextFieldObserver inherit from OmniboxView directly which is a big
> change to the class layout.

Right.  Ok.

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

http://codereview.chromium.org/10915069/diff/2006/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10915069/diff/2006/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:804: if (copy_position >=
0)
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?

http://codereview.chromium.org/10915069/diff/2006/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:838: return;
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);

http://codereview.chromium.org/10915069/diff/2006/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:926:
ui::ScopedClipboardWriter scw(cb, ui::Clipboard::BUFFER_STANDARD);
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.

http://codereview.chromium.org/10915069/diff/2006/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/10915069/diff/2006/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1018: bool
OmniboxViewWin::ShouldAddCopyURL() const {
Nit: I'm fine with inlining this into IsCommandIdEnabled() and making each
switch case there be linebroken in a more typical manner.

http://codereview.chromium.org/10915069/diff/2006/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1032:
scw.WriteHyperlink(net::EscapeForHTML(text), url.spec());
Don't call WriteHyperlink (see https://chromiumcodereview.appspot.com/10827330
).

To ensure these stay in sync, you might want to factor the end of OnCopy() out
to its own function, and call it from both there and here.  (Something like
"DoCopy(text, url, write_url);" and have that function create the
ScopedClipboardWriter.  Then this function would become a single statement.)

[dominich, 2012-09-05 23:35:33]

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:149: string16
stock_label(UTF8ToUTF16(label));
On 2012/09/05 22:11:50, Evan Stade wrote:
> On 2012/09/05 21:21:39, dominich wrote:
> > On 2012/09/05 21:07:17, Evan Stade wrote:
> > > why do you convert to utf16 before comparison?
> > 
> > It's what the original code was doing. It seemed strange to me as UTF8
string
> > compare should work as expected.
> 
> well then let's change it.

Done.

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1610:
data.WriteToClipboard(NULL);
On 2012/09/05 22:11:50, Evan Stade wrote:
> On 2012/09/05 21:21:39, dominich wrote:
> > On 2012/09/05 21:07:17, Evan Stade wrote:
> > > why don't you pass the profile?
> > 
> > the HandleCopyOrCutClipboard method below passes NULL.
> 
> why does that one pass NULL?

we don't want to write the profile path to the bookmark pickle.

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:804: if (copy_position >=
0)
On 2012/09/05 22:32:58, Peter Kasting wrote:
> 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?

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.

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:838: return;
On 2012/09/05 22:32:58, Peter Kasting wrote:
> 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);

Done.

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:926:
ui::ScopedClipboardWriter scw(cb, ui::Clipboard::BUFFER_STANDARD);
On 2012/09/05 22:32:58, Peter Kasting wrote:
> 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.

Done.

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1018: bool
OmniboxViewWin::ShouldAddCopyURL() const {
On 2012/09/05 22:32:58, Peter Kasting wrote:
> Nit: I'm fine with inlining this into IsCommandIdEnabled() and making each
> switch case there be linebroken in a more typical manner.

Done.

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1032:
scw.WriteHyperlink(net::EscapeForHTML(text), url.spec());
On 2012/09/05 22:32:58, Peter Kasting wrote:
> Don't call WriteHyperlink (see https://chromiumcodereview.appspot.com/10827330
> ).
> 
> To ensure these stay in sync, you might want to factor the end of OnCopy() out
> to its own function, and call it from both there and here.  (Something like
> "DoCopy(text, url, write_url);" and have that function create the
> ScopedClipboardWriter.  Then this function would become a single statement.)

Done.

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

Comments below apply to all platforms, assuming the relevant menu items are
guaranteed to be present at all times on those platforms.

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:801: if
(toolbar_model()->WouldReplaceSearchURLWithSearchTerms() &&
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.)

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:804: if (copy_position >=
0)
On 2012/09/05 23:35:34, dominich wrote:
> On 2012/09/05 22:32:58, Peter Kasting wrote:
> > 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?
> 
> 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.

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.

[Evan Stade, 2012-09-06 01:35:03]

still lgtm

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/4034/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1610:
data.WriteToClipboard(NULL);
On 2012/09/05 23:35:34, dominich wrote:
> On 2012/09/05 22:11:50, Evan Stade wrote:
> > On 2012/09/05 21:21:39, dominich wrote:
> > > On 2012/09/05 21:07:17, Evan Stade wrote:
> > > > why don't you pass the profile?
> > > 
> > > the HandleCopyOrCutClipboard method below passes NULL.
> > 
> > why does that one pass NULL?
> 
> we don't want to write the profile path to the bookmark pickle.

can you add documentation to BookmarkNodeData::WriteToClipboard since it's not
at all obvious what |profile| is there for? (it seems to only exist for drag and
drop?)

https://chromiumcodereview.appspot.com/10915069/diff/6040/chrome/browser/ui/g...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/6040/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:152: std::string
stock_label(label);
you don't need this variable. compare directly to label.

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

adding sky for bookmarks

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:801: if
(toolbar_model()->WouldReplaceSearchURLWithSearchTerms() &&
On 2012/09/06 00:01:46, Peter Kasting wrote:
> 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.)

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.

https://chromiumcodereview.appspot.com/10915069/diff/2006/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:804: if (copy_position >=
0)
On 2012/09/06 00:01:46, Peter Kasting wrote:
> On 2012/09/05 23:35:34, dominich wrote:
> > On 2012/09/05 22:32:58, Peter Kasting wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

Done.

https://chromiumcodereview.appspot.com/10915069/diff/6040/chrome/browser/ui/g...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10915069/diff/6040/chrome/browser/ui/g...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:152: std::string
stock_label(label);
On 2012/09/06 01:35:03, Evan Stade wrote:
> you don't need this variable. compare directly to label.

Done.

[sky, 2012-09-06 20:12:42]

LGTM

[Peter Kasting, 2012-09-06 20:37:12]

On 2012/09/06 17:50:25, dominich wrote:
> 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.

Why is this particular difference "not unexpected"?  I don't see any obvious
reason why this should differ.

I suppose I'm OK with fixing this as a followup (I would have preferred it here)
but unless there's some rationale I'm missing I really would like to see this
made consistent, and you kind of get elected since you're touching this :/.

[dominich, 2012-09-06 21:10:48]

On 2012/09/06 20:37:12, Peter Kasting wrote:
> On 2012/09/06 17:50:25, dominich wrote:
> > 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.
> 
> Why is this particular difference "not unexpected"?  I don't see any obvious
> reason why this should differ.

it seems that the cocoa UI already has different rules to other platforms. paste
and go, for instance, is already treated differently to the other platforms.

> 
> I suppose I'm OK with fixing this as a followup (I would have preferred it
here)
> but unless there's some rationale I'm missing I really would like to see this
> made consistent, and you kind of get elected since you're touching this :/.

I would like to be consistent, but I'm wary of conflating the changes I'm making
with deeper changes to how the context menu works on a platform I'm not entirely
familiar with.

[Peter Kasting, 2012-09-06 21:28:02]

On 2012/09/06 21:10:48, dominich wrote:
> On 2012/09/06 20:37:12, Peter Kasting wrote:
> > On 2012/09/06 17:50:25, dominich wrote:
> > > 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.
> > 
> > Why is this particular difference "not unexpected"?  I don't see any obvious
> > reason why this should differ.
> 
> it seems that the cocoa UI already has different rules to other platforms.
paste
> and go, for instance, is already treated differently to the other platforms.

You're assuming that's a correct difference.  I assume that it's wrong unless
COPY and PASTE are themselves omitted when they're not possible, because
otherwise, there's no HIG rule one could write that would justify this.  So the
question is whether those core items are also omitted when not valid.

[dominich, 2012-09-06 21:56:23]

On 2012/09/06 21:28:02, Peter Kasting wrote:
> On 2012/09/06 21:10:48, dominich wrote:
> > On 2012/09/06 20:37:12, Peter Kasting wrote:
> > > On 2012/09/06 17:50:25, dominich wrote:
> > > > 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.
> > > 
> > > Why is this particular difference "not unexpected"?  I don't see any
obvious
> > > reason why this should differ.
> > 
> > it seems that the cocoa UI already has different rules to other platforms.
> paste
> > and go, for instance, is already treated differently to the other platforms.
> 
> You're assuming that's a correct difference.  I assume that it's wrong unless
> COPY and PASTE are themselves omitted when they're not possible, because
> otherwise, there's no HIG rule one could write that would justify this.  So
the
> question is whether those core items are also omitted when not valid.

so i tested, and COPY and PASTE are always there but grey out. And
PASTE_AND_GO/SEARCH always seems to be there as long as something is in the
clipboard. In fact there's a todo in the code to consider greying it out when
the clipboard is empty.

I'm not sure where the disabling is hooked up for cocoa. I have a bad feeling
it's in the nib stuff, of which i know nothing.

[dominich, 2012-09-11 19:53:11]

PTAL.

dhollowa and pkasting, in particular. I've added support to cocoa for always
showing Paste-and-Go/Search and Copy URL, but disabling them if they shouldn't
be active.

[dhollowa, 2012-09-11 20:25:34]

LGTM w/ nits.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:221:
AutocompleteTextFieldObserver* observer = [self observer];
|observer| is not used here except in DCHECK.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:251: if
([search_engine_label length]) {
nit: No need to test for same thing as DCHECK.

[Peter Kasting, 2012-09-11 21:52:59]

LGTM

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:251: if
([search_engine_label length]) {
Nit: Also, be consistent in this function about where you DCHECK versus use a
conditional versus neither.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:491: //
greyed-out "Paste and Go" or nothing at all?
Nit: Shouldn't this TODO go away?

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/om...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:894: // TODO(dominich): Move
to OmniboxView base class?
Yes (since you use logic identical to this a number of places).  I don't know if
I'd call it "ShouldAddCopyURL()", though, since we now always add that -- maybe
"ShouldEnableCopyURL()".

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/gtk/omni...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1312: // Detect the Paste and
Copy menu items by searching for the ones that uses
Nit: uses -> use

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1313: // the stock labels
(i.e. gtk-paste and gtk-copy).
Nit: This comment seems a bit out of place given that we already handled copy
above.  Also we don't actually refer to "gtk-paste" (etc.) here, we use
GTK_STOCK_PASTE instead.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1611: BookmarkNodeData data;
Nit: Would be nice to factor out this chunk from here and
HandleCopyOrCutClipboard, thus turning this into a one-line function.

Also do we need to do the stop-propagating-signal stuff from that other
function?

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:780:
data.ReadFromTuple(url, text);
Nit: If DoCopy() takes 4 args (what used to be here as |selected_text|,
|write_url|, |url|, and |text|), you can move the BookmarkNodeData creation and
ReadFromTuple() call there and simplify both callsites.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:822: } else if (command_id
== IDS_COPY_URL) {
Nit: No else after return.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1364: 
Nit: Blank line unnecessary

[cpu_(ooo_6.6-7.5), 2012-09-12 01:24:50]

grd change lgtm

[dominich, 2012-09-12 15:23:09]

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:221:
AutocompleteTextFieldObserver* observer = [self observer];
On 2012/09/11 20:25:34, dhollowa wrote:
> |observer| is not used here except in DCHECK.

Done.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:251: if
([search_engine_label length]) {
On 2012/09/11 20:25:34, dhollowa wrote:
> nit: No need to test for same thing as DCHECK.

Done.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:251: if
([search_engine_label length]) {
On 2012/09/11 21:52:59, Peter Kasting wrote:
> Nit: Also, be consistent in this function about where you DCHECK versus use a
> conditional versus neither.

Done.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:491: //
greyed-out "Paste and Go" or nothing at all?
On 2012/09/11 21:52:59, Peter Kasting wrote:
> Nit: Shouldn't this TODO go away?
No. I'm not checking the contents of the clipboard.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/om...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/cocoa/om...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:894: // TODO(dominich): Move
to OmniboxView base class?
On 2012/09/11 21:52:59, Peter Kasting wrote:
> Yes (since you use logic identical to this a number of places).  I don't know
if
> I'd call it "ShouldAddCopyURL()", though, since we now always add that --
maybe
> "ShouldEnableCopyURL()".

I explained in an earlier revision that it's not a trivial change.
ShouldAddCopyURL (which I should rename) is called on the Observer, which is not
the OmniboxView base class. It's another example of the more complex cocoa class
hierarchy not playing nicely with the other platforms.

It's possible to put this in the OmniboxView base and then call it from an
AutocompleteTextFieldObserver virtual but that doesn't really gain much. 

I'll have a crack in the next CL.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/gtk/omni...
File chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc (right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1312: // Detect the Paste and
Copy menu items by searching for the ones that uses
On 2012/09/11 21:52:59, Peter Kasting wrote:
> Nit: uses -> use

Done.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1313: // the stock labels
(i.e. gtk-paste and gtk-copy).
On 2012/09/11 21:52:59, Peter Kasting wrote:
> Nit: This comment seems a bit out of place given that we already handled copy
> above.  Also we don't actually refer to "gtk-paste" (etc.) here, we use
> GTK_STOCK_PASTE instead.

Done.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/gtk/omni...
chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc:1611: BookmarkNodeData data;
On 2012/09/11 21:52:59, Peter Kasting wrote:
> Nit: Would be nice to factor out this chunk from here and
> HandleCopyOrCutClipboard, thus turning this into a one-line function.
> 
> Also do we need to do the stop-propagating-signal stuff from that other
> function?

No - it's an activate signal on the menu item rather than a copy-clipboard
signal on the text view. I'm looking at the edit search engines menu item as it
is similar.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:780:
data.ReadFromTuple(url, text);
On 2012/09/11 21:52:59, Peter Kasting wrote:
> Nit: If DoCopy() takes 4 args (what used to be here as |selected_text|,
> |write_url|, |url|, and |text|), you can move the BookmarkNodeData creation
and
> ReadFromTuple() call there and simplify both callsites.

Done.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:822: } else if (command_id
== IDS_COPY_URL) {
On 2012/09/11 21:52:59, Peter Kasting wrote:
> Nit: No else after return.

Done.

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/10915069/diff/21027/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1364: 
On 2012/09/11 21:52:59, Peter Kasting wrote:
> Nit: Blank line unnecessary

Done.

[commit-bot: I haz the power, 2012-09-12 16:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/10915069/10085

[commit-bot: I haz the power, 2012-09-12 17:25:52]

Try job failure for 10915069-10085 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-09-12 20:45:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/10915069/10090

[commit-bot: I haz the power, 2012-09-12 22:55:52]

Change committed as 156406