[Translate] Do not show TRANSLATING and AFTER_TRANSLATE when on a translate session

[Translate] Do not show TRANSLATING and AFTER_TRANSLATE when on a translate session

BUG=244790
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207846

[Takashi Toyoshima, 2013-06-21 12:19:53]

Thanks!
lgtm with one small nit.

https://codereview.chromium.org/17368005/diff/1/chrome/browser/translate/tran...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/17368005/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_infobar_delegate.cc:78: if (infobar_type ==
TranslateInfoBarDelegate::AFTER_TRANSLATE
In chromium style, "||" should be the end of previous line if it can not fit
single-line
e.g.,
if (foo ||
    bar)

[Miguel Garcia, 2013-06-21 13:24:59]

Thanks for the quick review!

https://codereview.chromium.org/17368005/diff/1/chrome/browser/translate/tran...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/17368005/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_infobar_delegate.cc:78: if (infobar_type ==
TranslateInfoBarDelegate::AFTER_TRANSLATE
On 2013/06/21 12:19:53, Takashi Toyoshima (chromium) wrote:
> In chromium style, "||" should be the end of previous line if it can not fit
> single-line
> e.g.,
> if (foo ||
>     bar)

Done.

[commit-bot: I haz the power, 2013-06-21 13:28:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/17368005/9003

[commit-bot: I haz the power, 2013-06-21 16:10:20]

Change committed as 207846

[Peter Kasting, 2013-06-21 17:23:59]

https://chromiumcodereview.appspot.com/17368005/diff/9003/chrome/browser/tran...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/17368005/diff/9003/chrome/browser/tran...
chrome/browser/translate/translate_infobar_delegate.cc:84: return;
(1) What about the old delegate?  Don't we need to remove it?

(2) If we're going to return like this, why not do it right after the initial
DCHECKs, instead of after we've created the new delegate?

https://chromiumcodereview.appspot.com/17368005/diff/9003/chrome/browser/tran...
File chrome/browser/translate/translate_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/17368005/diff/9003/chrome/browser/tran...
chrome/browser/translate/translate_infobar_delegate.h:61: // translate infobar
already present.
Nit: Consider updating this comment to describe your new behavior.

[Miguel Garcia, 2013-07-17 14:37:11]

I sent out https://codereview.chromium.org/19596002/ to fix your comments.

On 2013/06/21 17:23:59, Peter Kasting wrote:
>
https://chromiumcodereview.appspot.com/17368005/diff/9003/chrome/browser/tran...
> File chrome/browser/translate/translate_infobar_delegate.cc (right):
> 
>
https://chromiumcodereview.appspot.com/17368005/diff/9003/chrome/browser/tran...
> chrome/browser/translate/translate_infobar_delegate.cc:84: return;
> (1) What about the old delegate?  Don't we need to remove it?

We don't have an old delegate here since the BEFORE_TRANSLATE infobar is not
shown after a navigation. I added a DCHECK on the patch.

> 
> (2) If we're going to return like this, why not do it right after the initial
> DCHECKs, instead of after we've created the new delegate?

Great idea
> 
>
https://chromiumcodereview.appspot.com/17368005/diff/9003/chrome/browser/tran...
> File chrome/browser/translate/translate_infobar_delegate.h (right):
> 
>
https://chromiumcodereview.appspot.com/17368005/diff/9003/chrome/browser/tran...
> chrome/browser/translate/translate_infobar_delegate.h:61: // translate infobar
> already present.
> Nit: Consider updating this comment to describe your new behavior.

Great idea