[Mac] Expose AutocompleteEditViewMac to Cocoa code.

[Mac] Expose AutocompleteEditViewMac to Cocoa code.

This reduces the number of forwarding/delegate shenanigans we have to
go through.

[Scott Hess - ex-Googler, 2009-09-18 18:39:53]

Finally, am moderately happy with this.  Figured I should stop here, though.

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

On 2009/09/18 18:39:53, shess wrote:
> Finally, am moderately happy with this.  Figured I should stop here, though.

So it looks like we're just making a formal delegate interface and passing that
through to the AutocompleteTextField, rather than relying on the chain of
informal delegates.  This sounds ok to me, but I'll think about it a bit more...

[Scott Hess - ex-Googler, 2009-09-18 20:45:37]

Basically.  I didn't want to pass the direct pointer, because as
things are currently put together over in autocomplete/, it would make
the cocoa/autocomplete* stuff impossible to unit-test (*).  It does
bind everything more tightly together, but I think the loose
Objective-C binding was a lot of what made it painful to add new
call-throughs due to all the selector testing.

I was really excited when I finally figured out what to _name_ the damned t=
hing.

-scott

(*) Unless I made it so that you could have a subclass Mock on
AutocompleteEditViewMac.  I think that would be substantially more
annoying than this :-).

On Fri, Sep 18, 2009 at 1:08 PM,  <rohitrao@chromium.org> wrote:
> On 2009/09/18 18:39:53, shess wrote:
>>
>> Finally, am moderately happy with this. =A0Figured I should stop here,
>> though.
>
> So it looks like we're just making a formal delegate interface and passin=
g
> that
> through to the AutocompleteTextField, rather than relying on the chain of
> informal delegates. =A0This sounds ok to me, but I'll think about it a bi=
t
> more...
>
> http://codereview.chromium.org/211030
>

[rohitrao (ping after 24h), 2009-09-21 14:33:49]

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

http://codereview.chromium.org/211030/diff/1/4#newcode36
Line 36: // Called when the user to pastes into the field.
Typo: extra "to"

http://codereview.chromium.org/211030/diff/1/8
File chrome/browser/cocoa/autocomplete_text_field_editor_unittest.mm (right):

http://codereview.chromium.org/211030/diff/1/8#newcode53
Line 53: id e = [field_.get() currentEditor];
Call this "editor," not "e."

http://codereview.chromium.org/211030/diff/1/8#newcode54
Line 54: EXPECT_TRUE(e != nil);
Is it ok to put EXPECTs into a test fixture constructor?  I don't think I've
ever tried that before.

What happens if you put in an EXPECT_TRUE(false).  Does the test fail in the
intended way?

http://codereview.chromium.org/211030/diff/1/8#newcode59
Line 59: // The removeFromSuperview call is needed to prevent crashes in later
tests.
I think this means we're screwing something up?  Can you add a TODO to
investigate?  We really shouldn't have to call removeFromSuperview manually.

http://codereview.chromium.org/211030/diff/1/12
File chrome/chrome.gyp (right):

http://codereview.chromium.org/211030/diff/1/12#newcode4058
Line 4058: 'browser/cocoa/autocomplete_text_field_unittest_helper.mm',
For some reason files like browser_test_helper.h and cocoa_test_helper.h are
defined in the browser lib, not in the unit_tests target.

[Scott Hess - ex-Googler, 2009-09-21 17:29:05]

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

http://codereview.chromium.org/211030/diff/1/4#newcode36
Line 36: // Called when the user to pastes into the field.
On 2009/09/21 14:33:49, rohitrao wrote:
> Typo: extra "to"

Done.

http://codereview.chromium.org/211030/diff/1/8
File chrome/browser/cocoa/autocomplete_text_field_editor_unittest.mm (right):

http://codereview.chromium.org/211030/diff/1/8#newcode53
Line 53: id e = [field_.get() currentEditor];
On 2009/09/21 14:33:49, rohitrao wrote:
> Call this "editor," not "e."

Done.  Had done some refactoring from a place where I had e _and_ editor :-).

http://codereview.chromium.org/211030/diff/1/8#newcode54
Line 54: EXPECT_TRUE(e != nil);
On 2009/09/21 14:33:49, rohitrao wrote:
> Is it ok to put EXPECTs into a test fixture constructor?  I don't think I've
> ever tried that before.
> 
> What happens if you put in an EXPECT_TRUE(false).  Does the test fail in the
> intended way?

Good point.  It does fail in the intended way, but reading the gtest docs makes
me think that it's maybe not the best place for those tests to be.  Moved them
to SetUp().

http://codereview.chromium.org/211030/diff/1/8#newcode59
Line 59: // The removeFromSuperview call is needed to prevent crashes in later
tests.
On 2009/09/21 14:33:49, rohitrao wrote:
> I think this means we're screwing something up?  Can you add a TODO to
> investigate?  We really shouldn't have to call removeFromSuperview manually.

Done.  I copied that from your comment in autocomplete_text_field_unittest.mm
:-).

http://codereview.chromium.org/211030/diff/1/12
File chrome/chrome.gyp (right):

http://codereview.chromium.org/211030/diff/1/12#newcode4058
Line 4058: 'browser/cocoa/autocomplete_text_field_unittest_helper.mm',
On 2009/09/21 14:33:49, rohitrao wrote:
> For some reason files like browser_test_helper.h and cocoa_test_helper.h are
> defined in the browser lib, not in the unit_tests target.

Which is probably why CocoaTestHelperWindow class exists in the Chromium
Framework shlib we ship.  Probably pretty benign, but shouldn't be necessary. 
browser_test_helper.h is just an interface file so it shouldn't put anything
into the shlib (unless someone really is referring to it).

Seems reasonable to leave give breakpad_mac_stubs.mm and keystone_glue.m.  But
I'll bring it up on chromium-dev.

[rohitrao (ping after 24h), 2009-09-21 18:00:36]

LGTM

What is the plan for the new delegate methods I added for paste and go?  Should 
they be observerized too?

http://codereview.chromium.org/211030/diff/1/8
File chrome/browser/cocoa/autocomplete_text_field_editor_unittest.mm (right):

http://codereview.chromium.org/211030/diff/1/8#newcode59
Line 59: // The removeFromSuperview call is needed to prevent crashes in later
tests.
> Done.  I copied that from your comment in autocomplete_text_field_unittest.mm
> :-).

Ooops :)

[Scott Hess - ex-Googler, 2009-09-21 18:06:11]

I've got another CL queued up to handle them.  I wanted to keep this
one small so that there wouldn't be a bunch of second-order
contention...

-scott


On Mon, Sep 21, 2009 at 11:00 AM,  <rohitrao@chromium.org> wrote:
> LGTM
>
> What is the plan for the new delegate methods I added for paste and go?
> =A0Should
> they be observerized too?
>
>
> http://codereview.chromium.org/211030/diff/1/8
> File chrome/browser/cocoa/autocomplete_text_field_editor_unittest.mm
> (right):
>
> http://codereview.chromium.org/211030/diff/1/8#newcode59
> Line 59: // The removeFromSuperview call is needed to prevent crashes in
> later tests.
>>
>> Done. =A0I copied that from your comment in
>
> autocomplete_text_field_unittest.mm
>>
>> :-).
>
> Ooops :)
>
> http://codereview.chromium.org/211030
>