[Mac] Implement Paste and Go/Search.

[Mac] Implement Paste and Go/Search.

BUG=http://crbug.com/10937
BUG=http://crbug.com/13021
TEST=Omnibox context menu should contain paste and go/search.

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

[rohitrao (ping after 24h), 2009-09-03 17:41:01]

Scott,
  Can I get a thumbs up/down on the concept (not a full review, yet).  Paste and
go is apparently handled by the model, so I have to plumb a lot of info down
through two levels.  It feels a little awful, so let me know if there is a
better way :)

[Scott Hess - ex-Googler, 2009-09-03 18:03:23]

I think the concept is reasonable, given what we're working with.

The Windows implementation has seemed to be moving more stuff into
views/autocomplete/*.  I originally broke things down with the glue between
Chrome and Cocoa in autocomplete/autocomplete_edit_view_mac.* (mixture of C++
and Objective-C), and the additional functionality for Cocoa in
cocoa/autocomplete_text* (Objective-C only).  Maybe it's time to bite the bullet
and have AutocompleteText* provide a C++ abstract base class that
AutocompleteEditViewMac can implement.  At least then we just have one level of
vtable instead of multiple levels of objc_msgSend().  WDYT?

http://codereview.chromium.org/192008/diff/1/4
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/192008/diff/1/4#newcode47
Line 47: if ([delegate
respondsToSelector:@selector(control:textShouldPasteAndGo:)]) {
Wrong selector.

[Peter Kasting, 2009-09-03 18:06:37]

On Thu, Sep 3, 2009 at 11:03 AM, <shess@chromium.org> wrote:

> The Windows implementation has seemed to be moving more stuff into
> views/autocomplete/*.


Eh??  The only thing there is Ben's popup implementation.  We're not moving
anything more into there nor have we been.


> http://codereview.chromium.org/192008
>

[Scott Hess - ex-Googler, 2009-09-03 18:17:47]

Sorry - initially when I looked around browser/autocomplete/, it
seemed like I was finding everything I needed to look at on other
platforms in that directory, but over time it has seemed like I've
needed to look in browser/views/autocomplete/ for some stuff.  Maybe
I'm not remembering reality with high fidelity.

Anyhow, feel free to comment on the gist of my point about shifting
more stuff of the Cocoa->Chrome plumbing to
browser/cocoa/autocomplete*.  Right now there's a lot of Cocoa-style
delegation stuff, where browser/autocomplete/* implements an
Objective-C delegate to catch things and bridge them over, while
browser/cocoa/autocomplete* implements additional methods to delegate.
 Cocoa-style delegation is a lot more useful for objects which can be
repurposed for many uses, though, so in this case it's starting to
look like pointless overhead rather than useful encapsulation.

Maybe I should just spend an afternoon doing a prototype and see what
it looks like.

-scott


On Thu, Sep 3, 2009 at 11:06 AM, Peter Kasting<pkasting@chromium.org> wrote=
:
> On Thu, Sep 3, 2009 at 11:03 AM, <shess@chromium.org> wrote:
>>
>> The Windows implementation has seemed to be moving more stuff into
>> views/autocomplete/*.
>
> Eh?? =A0The only thing there is Ben's popup implementation. =A0We're not =
moving
> anything more into there nor have we been.
>
>>
>> http://codereview.chromium.org/192008
>
>

[Scott Hess - ex-Googler, 2009-09-04 00:08:20]

On 2009/09/03 17:41:01, rohitrao wrote:
> Scott,
>   Can I get a thumbs up/down on the concept (not a full review, yet).  Paste
and
> go is apparently handled by the model, so I have to plumb a lot of info down
> through two levels.  It feels a little awful, so let me know if there is a
> better way :)

Rohit,

I'm thinking maybe I should wire up the direct connection from
cocoa/autocomplete* stuff earlier rather than later, because it would sure be
easier to do it now rather than wait until two or three more calls get plumbed
through.  Possibly could post a CL tomorrow.  WDYT?

-scott

[rohitrao (ping after 24h), 2009-09-04 00:25:09]

Agreed.  Looking forward to seeing what you come up with.

On Thu, Sep 3, 2009 at 8:08 PM, <shess@chromium.org> wrote:
> On 2009/09/03 17:41:01, rohitrao wrote:
>>
>> Scott,
>> =A0 Can I get a thumbs up/down on the concept (not a full review, yet).
>
> Paste and
>>
>> go is apparently handled by the model, so I have to plumb a lot of
>
> info down
>>
>> through two levels. =A0It feels a little awful, so let me know if there
>
> is a
>>
>> better way :)
>
> Rohit,
>
> I'm thinking maybe I should wire up the direct connection from
> cocoa/autocomplete* stuff earlier rather than later, because it would
> sure be easier to do it now rather than wait until two or three more
> calls get plumbed through. =A0Possibly could post a CL tomorrow. =A0WDYT?
>
> -scott
>
>
> http://codereview.chromium.org/192008
>

[Scott Hess - ex-Googler, 2009-09-09 23:45:24]

On 2009/09/04 00:25:09, rohitrao wrote:
> Agreed.  Looking forward to seeing what you come up with.

Rohit - suggest you go forward with your CL.  The field/cell/editor trio is
challenging me somewhat.  Getting one step in is easy, I'm going to have to
revamp the unit tests to accommodate the change (the existing field-editor unit
test doesn't text it in the field's context, and that's going to have to
change).

> http://codereview.chromium.org/192008

[Scott Hess - ex-Googler, 2009-09-15 18:18:42]

Rohit - I see that you uploaded stuff, is it ready for review?

[rohitrao (ping after 24h), 2009-09-15 18:20:37]

On 2009/09/15 18:18:42, shess wrote:
> Rohit - I see that you uploaded stuff, is it ready for review?

Ready for review.  Same basic idea as before, but slightly cleaned up.

[Scott Hess - ex-Googler, 2009-09-15 21:03:00]

http://codereview.chromium.org/192008/diff/3002/4002
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/192008/diff/3002/4002#newcode582
Line 582: int AutocompleteEditViewMac::GetPasteActionString() {
Header-file comment might imply a DCHECK(CanPasteAndGo()) here.

http://codereview.chromium.org/192008/diff/3002/4002#newcode618
Line 618: if
(model_->CanPasteAndGo(GetClipboardText(g_browser_process->clipboard())))
if (CanPasteAndGo())?

http://codereview.chromium.org/192008/diff/3002/4004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/192008/diff/3002/4004#newcode47
Line 47: if ([delegate
respondsToSelector:@selector(control:textShouldPasteAndGo:)]) {
Wrong selector.

http://codereview.chromium.org/192008/diff/3002/4007
File chrome/browser/cocoa/autocomplete_text_field_editor.h (right):

http://codereview.chromium.org/192008/diff/3002/4007#newcode27
Line 27: - (int)textPasteActionString:(NSText*)fieldEditor;
How about:
- (NSString*)textPasteActionString:*;
then disable if nil.  Alternately, define an invalid int and disable on that.

http://codereview.chromium.org/192008/diff/3002/4006
File chrome/browser/cocoa/autocomplete_text_field_editor.mm (right):

http://codereview.chromium.org/192008/diff/3002/4006#newcode51
Line 51: - (void)pasteandgo:sender {
-pasteAndGo:?

[rohitrao (ping after 24h), 2009-09-16 16:19:05]

http://codereview.chromium.org/192008/diff/3002/4002
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/192008/diff/3002/4002#newcode582
Line 582: int AutocompleteEditViewMac::GetPasteActionString() {
On 2009/09/15 21:03:02, shess wrote:
> Header-file comment might imply a DCHECK(CanPasteAndGo()) here.

Done.

http://codereview.chromium.org/192008/diff/3002/4002#newcode618
Line 618: if
(model_->CanPasteAndGo(GetClipboardText(g_browser_process->clipboard())))
On 2009/09/15 21:03:02, shess wrote:
> if (CanPasteAndGo())?

Especially sad since I wrote that function myself.  Done.

http://codereview.chromium.org/192008/diff/3002/4004
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/192008/diff/3002/4004#newcode47
Line 47: if ([delegate
respondsToSelector:@selector(control:textShouldPasteAndGo:)]) {
On 2009/09/15 21:03:02, shess wrote:
> Wrong selector.

Good catch!  I guess this worked by happy coincidence, since objects either have
all three selectors or none of them.

http://codereview.chromium.org/192008/diff/3002/4007
File chrome/browser/cocoa/autocomplete_text_field_editor.h (right):

http://codereview.chromium.org/192008/diff/3002/4007#newcode27
Line 27: - (int)textPasteActionString:(NSText*)fieldEditor;
On 2009/09/15 21:03:02, shess wrote:
> How about:
> - (NSString*)textPasteActionString:*;
> then disable if nil.  Alternately, define an invalid int and disable on that.

So for this to make sense, I would have to return the NSString* from
AutocompleteEditViewMac.  How does the latest diff look?  Is it ok for a C++
class to return an autoreleased NSString?

http://codereview.chromium.org/192008/diff/3002/4006
File chrome/browser/cocoa/autocomplete_text_field_editor.mm (right):

http://codereview.chromium.org/192008/diff/3002/4006#newcode51
Line 51: - (void)pasteandgo:sender {
On 2009/09/15 21:03:02, shess wrote:
> -pasteAndGo:?

No no, it's all one big word, you just have to say it real fast =)

Done.

[Scott Hess - ex-Googler, 2009-09-16 22:31:00]

I think we got our signals crossed on this.  I meant that you could skip wiring
the "shouldPasteAndGo" stuff down through Objective-C.  I've commented on what I
meant, here.

I'm not insistent on this change, but, unfortunately, I think you either need to
go forward and clean it up, or roll back to what you had before.  Now it's sort
of mixed up, I think.

http://codereview.chromium.org/192008/diff/5003/7001
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/192008/diff/5003/7001#newcode587
Line 587: NSString* AutocompleteEditViewMac::GetPasteActionString() {
This can still return an int, like before.

http://codereview.chromium.org/192008/diff/5003/7001#newcode861
Line 861: - (BOOL)control:(NSControl*)control
textShouldPasteAndGo:(NSText*)fieldEditor {
Get rid of this entirely.

http://codereview.chromium.org/192008/diff/5003/7001#newcode866
Line 866: textPasteActionString:(NSText*)fieldEditor {
If !CanPasteAndGo(), return nil.
Else return l10n_util::GetNSStringWithFixup(GetPasteActionString()).

Though calling that int a string may not be kosher.

http://codereview.chromium.org/192008/diff/5003/7004
File chrome/browser/cocoa/autocomplete_text_field.h (right):

http://codereview.chromium.org/192008/diff/5003/7004#newcode36
Line 36: - (BOOL)control:(NSControl*)control
textShouldPasteAndGo:(NSText*)fieldEditor;
Get rid of this entirely, and first comment line below.

http://codereview.chromium.org/192008/diff/5003/7003
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/192008/diff/5003/7003#newcode34
Line 34: }
Get rid of this entirely.

http://codereview.chromium.org/192008/diff/5003/7006
File chrome/browser/cocoa/autocomplete_text_field_editor.h (right):

http://codereview.chromium.org/192008/diff/5003/7006#newcode26
Line 26: - (BOOL)textShouldPasteAndGo:(NSText*)fieldEditor;
Get rid of this entirely.

http://codereview.chromium.org/192008/diff/5003/7005
File chrome/browser/cocoa/autocomplete_text_field_editor.mm (right):

http://codereview.chromium.org/192008/diff/5003/7005#newcode86
Line 86: NSString* label = [delegate textPasteActionString:self];
if ([delegate rTS:@sel(textPasteActionString:)]) {
  NSString* label = [delegate tPAS:self];
  if (label) {
    ...;
  }
}

No DCHECK because nil is valid.

[rohitrao (ping after 24h), 2009-09-17 14:42:41]

Changes made.

[Scott Hess - ex-Googler, 2009-09-18 18:48:58]

LGTM, with a couple commenting nits.  Ignore them if you like, since I'll be
doing a couple passes to refactor things in this area next week anyhow :-).

http://codereview.chromium.org/192008/diff/12001/12003
File chrome/browser/autocomplete/autocomplete_edit_view_mac.h (right):

http://codereview.chromium.org/192008/diff/12001/12003#newcode125
Line 125: int GetPasteActionStringId();
Can't be returning nil, here.

http://codereview.chromium.org/192008/diff/12001/12003#newcode129
Line 129: void OnPasteAndGo();
Cleanup the sense of the comment.  They aren't attempting, as you have no return
value for success, they are simply doing it.

http://codereview.chromium.org/192008/diff/12001/12002
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/192008/diff/12001/12002#newcode9
Line 9: #include "app/resource_bundle.h"
"gfx" < "l10n", "gfx" < "res"

http://codereview.chromium.org/192008/diff/12001/12006
File chrome/browser/cocoa/autocomplete_text_field_editor.mm (right):

http://codereview.chromium.org/192008/diff/12001/12006#newcode68
Line 68: - (NSMenu*)menuForEvent:(NSEvent*)event {
Do you think this justifies a TODO(rohitrao): around determining if we should
cache the resulting NSMenu?  I'm mixed on such things, since this happens in
response to a user click, it doesn't seem essential.  But we should at least
verify that this is being called when expected, not when random other stuff
happens (like is there any other way to provoke a context menu, say some
keypress that might call this more often than we expect it to?).

[rohitrao (ping after 24h), 2009-09-18 19:17:12]

http://codereview.chromium.org/192008/diff/12001/12003
File chrome/browser/autocomplete/autocomplete_edit_view_mac.h (right):

http://codereview.chromium.org/192008/diff/12001/12003#newcode125
Line 125: int GetPasteActionStringId();
On 2009/09/18 18:48:58, shess wrote:
> Can't be returning nil, here.

Done.

http://codereview.chromium.org/192008/diff/12001/12003#newcode129
Line 129: void OnPasteAndGo();
On 2009/09/18 18:48:58, shess wrote:
> Cleanup the sense of the comment.  They aren't attempting, as you have no
return
> value for success, they are simply doing it.

Done.

http://codereview.chromium.org/192008/diff/12001/12002
File chrome/browser/autocomplete/autocomplete_edit_view_mac.mm (right):

http://codereview.chromium.org/192008/diff/12001/12002#newcode9
Line 9: #include "app/resource_bundle.h"
On 2009/09/18 18:48:58, shess wrote:
> "gfx" < "l10n", "gfx" < "res"

I usually sort within a directory first, then go to subdirectories.  Is there a
clear rule on this?  I'll check the style guide, but changed for now.

http://codereview.chromium.org/192008/diff/12001/12006
File chrome/browser/cocoa/autocomplete_text_field_editor.mm (right):

http://codereview.chromium.org/192008/diff/12001/12006#newcode68
Line 68: - (NSMenu*)menuForEvent:(NSEvent*)event {
On 2009/09/18 18:48:58, shess wrote:
> Do you think this justifies a TODO(rohitrao): around determining if we should
> cache the resulting NSMenu?  I'm mixed on such things, since this happens in
> response to a user click, it doesn't seem essential.  But we should at least
> verify that this is being called when expected, not when random other stuff
> happens (like is there any other way to provoke a context menu, say some
> keypress that might call this more often than we expect it to?).

I'm leaving the TODO out for now.  Because this menu changes depending on what's
in the clipboard, we can't simply fetch the menu once and save it forever
(otherwise we'd just override defaultMenu).  We could mostly build the menu and
then simply change the text of the last item, but I'm not sure how much that
saves.

http://codereview.chromium.org/192008/diff/12001/12006#newcode84
Line 84: NSString* label = [delegate textPasteActionString:self];
Added:
    // TODO(rohitrao): If the clipboard is empty, should we show a greyed-out
    // "Paste and Go" or nothing at all?

http://codereview.chromium.org/192008/diff/15001/15003
File chrome/browser/autocomplete/autocomplete_edit_view_mac.h (right):

http://codereview.chromium.org/192008/diff/15001/15003#newcode126
Line 126: // Called when the user initiates a "pastes and go" or "paste and
search" into
Typo fixed but didn't bother to reupload.

[Scott Hess - ex-Googler, 2009-09-18 19:32:45]

LGTM!