[Translate] Return before the new delegate is created if needed

[Translate] Return before the new delegate is created if needed

Peter K. suggested to do this in https://chromiumcodereview.appspot.com/17368005 so we avoid leaking the new delegate.

BUG=

[Takashi Toyoshima, 2013-07-17 17:02:08]

LGTM, but can you wait for double-checking by pkasting?

https://codereview.chromium.org/19596002/diff/2001/chrome/browser/translate/t...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/19596002/diff/2001/chrome/browser/translate/t...
chrome/browser/translate/translate_infobar_delegate.cc:77:
DCHECK(!old_delegate);
I agreed this DCHECK is always correct.
But I'd like to ask pkasting double checking this before submitting this CL.

[Peter Kasting, 2013-07-17 18:06:19]

This doesn't actually fix any leaks, because the new delegate is in a scoped_ptr
and thus will be deleted properly.  (I checked, and my original comment hadn't
said "avoid leaking" either.)  However, I do think moving this block upwards is
the right thing to do.

If you really want to avoid unnecessary delegate creations, you could also add
an "if (!replace_existing_infobar) return;" inside the loop where we find the
old delegate, and then remove that clause from the conditional at the bottom of
the function.

https://codereview.chromium.org/19596002/diff/2001/chrome/browser/translate/t...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/19596002/diff/2001/chrome/browser/translate/t...
chrome/browser/translate/translate_infobar_delegate.cc:77:
DCHECK(!old_delegate);
On 2013/07/17 17:02:08, Takashi Toyoshima (chromium) wrote:
> I agreed this DCHECK is always correct.
> But I'd like to ask pkasting double checking this before submitting this CL.

I don't know whether the DCHECK here is important.  My instinct would be to
simplify this by omitting the DCHECK so you can move this block up above the
"find the old delegate" block as well.  I actually already did this in a local
change I hadn't gotten around to submitting yet.

https://codereview.chromium.org/19596002/diff/2001/chrome/browser/translate/t...
File chrome/browser/translate/translate_infobar_delegate.h (right):

https://codereview.chromium.org/19596002/diff/2001/chrome/browser/translate/t...
chrome/browser/translate/translate_infobar_delegate.h:61: // translate infobar
already present and if the infobar needs to be displayed.
Nit: Don't make this change; it's somewhat inaccurate in that this clause really
applies more broadly than where you have it, and it's vague and confusing
anyway.  People who want this level of detail can just read the function.

[Peter Kasting, 2013-07-17 18:44:23]

Heh, now that I see your replies to my previous review comments I realize I'm
giving you conflicting feedback between then and now.

I'm not opposed to updating the header comment on this function, but if you want
to do it, make sure it's clear and correct.  The comment added in this patch
isn't really either.

The DCHECK might be worth keeping.  I'm on the fence.  If you want to keep it,
maybe add a comment on it describing why it holds.

[Peter Kasting, 2013-10-08 00:15:41]

Patch set 2 seems to have been replaced by a completely unrelated patch set 3. 
Is this CL dead?  Please close if so.

[Miguel Garcia, 2013-10-08 08:48:09]

On 2013/10/08 00:15:41, Peter Kasting wrote:
> Patch set 2 seems to have been replaced by a completely unrelated patch set 3.

> Is this CL dead?  Please close if so.

I have no idea how that happened. I am closing this issue as wyou mentioned and
will start from scratch if needed. Although given that as you mention there are
no leaks it's not as urgent.