[Mac] Fix keyword editor-related crashes

[Mac] Fix keyword editor-related crashes

* Set all outlets in KeywordEditorCocoaController to nil.
* Adjust the edit buttons after adding a new keyword.

BUG=23350, 22545
TEST=Preferences-->Manage. Add a search engine. Click "Make Default", delete it ("-)". Crash.

[Scott Hess - ex-Googler, 2009-10-08 20:36:04]

I was trying to check out what was happening - but when I try to add a keyword,
DCHECKs fire and exceptions are raised.  I think you're maybe chasing a red
herring.  Note that -insertPointer:atIndex: must be passed an index < count,
which is not the case when I add a search engine.

My earlier comment about using a vector<scoped_nsobject<NSImage> > is maybe
becoming more pertinent.  Most everyone around here will have full comfort with
vector<>.

http://codereview.chromium.org/262028/diff/1/2
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/262028/diff/1/2#newcode135
Line 135: makeDefaultButton_ = nil;
Why are you making this change?

The change I was encouraging earlier was observert_.reset().

[Scott Hess - ex-Googler, 2009-10-08 20:37:58]

On 2009/10/08 20:36:04, shess wrote:
> I was trying to check out what was happening - but when I try to add a
keyword,
> DCHECKs fire and exceptions are raised.  I think you're maybe chasing a red
> herring.  Note that -insertPointer:atIndex: must be passed an index < count,
> which is not the case when I add a search engine.

Hmm, note that when NSExceptions are thrown, it wreaks havoc.  Your object's
setup will mismatch the Chrome setup because of the longjmp().  NSExceptions are
bad.

[Robert Sesek, 2009-10-09 00:49:15]

On 2009/10/08 20:36:04, shess wrote:
> I was trying to check out what was happening - but when I try to add a
keyword,
> DCHECKs fire and exceptions are raised.  I think you're maybe chasing a red
> herring.  Note that -insertPointer:atIndex: must be passed an index < count,
> which is not the case when I add a search engine.
> 

Fixed. |-setCount:| before inserting, now. Alternative is to switch this to a
standard NSArray and not lazily cache the icons.

> My earlier comment about using a vector<scoped_nsobject<NSImage> > is maybe
> becoming more pertinent.  Most everyone around here will have full comfort
with
> vector<>.

We cant v.insert() a scoped_nsobject<> because its DISALLOW_COPY_AND_ASSIGN, so
that is out :(.

http://codereview.chromium.org/262028/diff/1/2
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/262028/diff/1/2#newcode135
Line 135: makeDefaultButton_ = nil;
On 2009/10/08 20:36:04, shess wrote:
> Why are you making this change?
> 
> The change I was encouraging earlier was observert_.reset().

I read http://code.google.com/p/chromium/issues/detail?id=22545 wrong. Changed.

[Scott Hess - ex-Googler, 2009-10-09 02:20:43]

http://codereview.chromium.org/262028/diff/5002/5003
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/262028/diff/5002/5003#newcode74
Line 74: [iconImages_ setCount:count + length];
This will add length items to iconImages_ and the loop will add length items to
iconImages_.

One option would be capture the count before, [iconImages addPointer:NULL]
before, and then [iconImages_ setCount:count + length] after the loop to trim it
back.  Make sure you leave a comment about having to work around
-insertPointer:atIndex:.

http://codereview.chromium.org/262028/diff/5002/5003#newcode133
Line 133: [tableView_ setTarget:nil];
If you're going to clear target, clear dataSource, too.  That said, I'm not as
worried about tableView_ retaining leftover references, though, because we're
the window controller for its window, and straight Cocoa code should have all
the ref-counting right in the first place.  Where I worry about is the interface
objects between Cocoa and Chrome (to observer_.reset(), so that if something
delayed comes in from the table view, we don't call out to released C++ objects.

[pink (ping after 24hrs), 2009-10-09 14:50:38]

http://codereview.chromium.org/262028/diff/5002/5003
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/262028/diff/5002/5003#newcode74
Line 74: [iconImages_ setCount:count + length];
i don't follow why you need to do this.

[Scott Hess - ex-Googler, 2009-10-09 14:54:16]

http://codereview.chromium.org/262028/diff/5002/5003
File chrome/browser/cocoa/keyword_editor_cocoa_controller.mm (right):

http://codereview.chromium.org/262028/diff/5002/5003#newcode74
Line 74: [iconImages_ setCount:count + length];
On 2009/10/09 14:50:38, pink wrote:
> i don't follow why you need to do this.

-[NSPointerArray insertPointer:atIndex:] requires the index to be strictly less
than the count.  If you try to insert _at_ the count, it throws an exception. 
Though documented, I would call that a bug in NSPointerArray.

http://codereview.chromium.org/262028/diff/5002/5003#newcode174
Line 174: - (void)modelChanged {
To help catch out-of-sync issues (which is what happened due to the exception),
it might not hurt to have this (or other appropriate central location) do
DCHECK_EQ(row_count(), [iconImages_ count]), because at this point they should
always be identical.

[Scott Hess - ex-Googler, 2009-10-19 16:55:48]

I saw a rainbow this morning.  It was SO PRETTY!  It reminded me that we should
get this checked in.  I think the last point was around adding-versus-inserting
at the end of NSPointerArray?

[Robert Sesek, 2009-10-20 00:24:07]

My MBP was irreparably damaged last weekend, and the HD didn't survive :'(.
It's replacement is coming from Apple and I'm hoping it will be here by EOW.
I'll re-send out the CL when I have a computer again.

rsesek / @chromium.org

On Mon, Oct 19, 2009 at 12:55 PM, <shess@chromium.org> wrote:

> I saw a rainbow this morning.  It was SO PRETTY!  It reminded me that we
> should
> get this checked in.  I think the last point was around
> adding-versus-inserting
> at the end of NSPointerArray?
>
> http://codereview.chromium.org/262028
>

[Scott Hess - ex-Googler, 2009-10-21 17:45:03]

Robert,

Bummer - would you mind terribly if we invoke the "hit by a bus" clause?  Which
is to say have someone else (I guess that would be me) patch it into their
client and have you review it before they check in?  Would be nice if we could
get this in for next week's dev channel.

Thanks,
scott


On 2009/10/20 00:24:07, rsesek wrote:
> My MBP was irreparably damaged last weekend, and the HD didn't survive :'(.
> It's replacement is coming from Apple and I'm hoping it will be here by EOW.
> I'll re-send out the CL when I have a computer again.
> 
> rsesek / @chromium.org
> 
> On Mon, Oct 19, 2009 at 12:55 PM, <mailto:shess@chromium.org> wrote:
> 
> > I saw a rainbow this morning.  It was SO PRETTY!  It reminded me that we
> > should
> > get this checked in.  I think the last point was around
> > adding-versus-inserting
> > at the end of NSPointerArray?
> >
> > http://codereview.chromium.org/262028
> >
>

[Robert Sesek, 2009-10-21 23:20:19]

Scott, that sounds fine to me (and my MBP certainly looks like it was hit by
a bus). I agree this is would be better to have fixed sooner rather than
later. Thanks for taking this over,

rsesek / @chromium.org

On Wed, Oct 21, 2009 at 1:45 PM, <shess@chromium.org> wrote:

> Robert,
>
> Bummer - would you mind terribly if we invoke the "hit by a bus" clause?
>  Which
> is to say have someone else (I guess that would be me) patch it into their
> client and have you review it before they check in?  Would be nice if we
> could
> get this in for next week's dev channel.
>
> Thanks,
> scott
>
>
> On 2009/10/20 00:24:07, rsesek wrote:
>
>> My MBP was irreparably damaged last weekend, and the HD didn't survive
>> :'(.
>> It's replacement is coming from Apple and I'm hoping it will be here by
>> EOW.
>> I'll re-send out the CL when I have a computer again.
>>
>
>  rsesek / @chromium.org
>>
>
>  On Mon, Oct 19, 2009 at 12:55 PM, <mailto:shess@chromium.org> wrote:
>>
>
>  > I saw a rainbow this morning.  It was SO PRETTY!  It reminded me that we
>> > should
>> > get this checked in.  I think the last point was around
>> > adding-versus-inserting
>> > at the end of NSPointerArray?
>> >
>> > http://codereview.chromium.org/262028
>> >
>>
>
>
>
>
> http://codereview.chromium.org/262028
>

[Scott Hess - ex-Googler, 2009-10-22 22:09:22]

On 2009/10/21 23:20:19, rsesek wrote:
> Scott, that sounds fine to me (and my MBP certainly looks like it was hit by
> a bus). I agree this is would be better to have fixed sooner rather than
> later. Thanks for taking this over,

Moving to http://codereview.chromium.org/319006