Keep track of when file dialogs are destroyed.

Keep track of when file dialogs are destroyed.

BUG=none
TEST=Open two windows, open a file dialog in one, quit the browser in other. There should not be any GTK assertion errors.

[Lei Zhang, 2009-10-31 05:31:44]

More GTK objects getting destroyed twice...

[Evan Stade, 2009-11-02 18:03:29]

http://codereview.chromium.org/339096/diff/5001/5002
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/339096/diff/5001/5002#newcode175
Line 175: skip_file_dialog_destroyed_ = true;
is this necessary, or just an optimization?

[Lei Zhang, 2009-11-02 18:31:19]

http://codereview.chromium.org/339096/diff/5001/5002
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/339096/diff/5001/5002#newcode175
Line 175: skip_file_dialog_destroyed_ = true;
On 2009/11/02 18:03:29, Evan Stade wrote:
> is this necessary, or just an optimization?

It's necessary. In SelectFileDialogImpl::FileDialogDestroyed, |parent| is null
by the time we get to ~SelectFileDialogImpl.

[Evan Stade, 2009-11-02 18:35:25]

http://codereview.chromium.org/339096/diff/5001/5002
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/339096/diff/5001/5002#newcode175
Line 175: skip_file_dialog_destroyed_ = true;
On 2009/11/02 18:31:20, Lei Zhang wrote:
> On 2009/11/02 18:03:29, Evan Stade wrote:
> > is this necessary, or just an optimization?
> 
> It's necessary. In SelectFileDialogImpl::FileDialogDestroyed, |parent| is null
> by the time we get to ~SelectFileDialogImpl.

ah. So you have found a case where the DCHECK legitimately fails. I think it
would be ok to replace the DCHECK with an early return then, and a comment about
when it can be NULL, instead of adding |skip_file_dialog_detroyed_|.

[Lei Zhang, 2009-11-03 00:56:51]

http://codereview.chromium.org/339096/diff/5001/5002
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/339096/diff/5001/5002#newcode175
Line 175: skip_file_dialog_destroyed_ = true;
On 2009/11/02 18:35:25, Evan Stade wrote:
> On 2009/11/02 18:31:20, Lei Zhang wrote:
> > On 2009/11/02 18:03:29, Evan Stade wrote:
> > > is this necessary, or just an optimization?
> > 
> > It's necessary. In SelectFileDialogImpl::FileDialogDestroyed, |parent| is
null
> > by the time we get to ~SelectFileDialogImpl.
> 
> ah. So you have found a case where the DCHECK legitimately fails. I think it
> would be ok to replace the DCHECK with an early return then, and a comment
about
> when it can be NULL, instead of adding |skip_file_dialog_detroyed_|.

Done.

http://codereview.chromium.org/339096/diff/10001/10002#newcode169
Line 169: last_saved_path_ = new FilePath();
calling dialogs_.erase() in the middle of a for look didn't seem to work very
well. Changed it to a for loop that always look at dialogs_.begin().

http://codereview.chromium.org/339096/diff/10001/10002#newcode455
Line 455: params_map_.erase(iter);
We needed to do this first to avoid double frees.

[Evan Stade, 2009-11-03 20:46:21]

lg, nits

http://codereview.chromium.org/339096/diff/10001/10002
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/339096/diff/10001/10002#newcode172
Line 172: iter = dialogs_.begin();
can get rid of the |iter| variable altogether?

http://codereview.chromium.org/339096/diff/10001/10002#newcode458
Line 458: if (!parent)
can you put a comment since it's not obvious why parent could be null

[Lei Zhang, 2009-11-03 22:44:50]

http://codereview.chromium.org/339096/diff/10001/10002
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/339096/diff/10001/10002#newcode172
Line 172: iter = dialogs_.begin();
On 2009/11/03 20:46:21, Evan Stade wrote:
> can get rid of the |iter| variable altogether?

Done.

http://codereview.chromium.org/339096/diff/10001/10002#newcode458
Line 458: if (!parent)
On 2009/11/03 20:46:21, Evan Stade wrote:
> can you put a comment since it's not obvious why parent could be null

Done.