Refactored the translate infobars.

Refactored the translate infobars.
Since some work is needed on Linux and Mac to use the new translate infobar delegate, I created a new version of the refactored classes instead of replacing them. Once Linux and Mac use the new classes, we can make the swicth.
The TranslateInfoBarDelegate now contains all states, so there is no more logic on the infobar classes.
I broke down the single infobar class into multiple ones (there is now an infobar for each state: before translate, translating, after translate, error): it makes the code simpler.
I had to fix ReplaceInfoBar on Windows as it was not working properly.

BUG=40828
TEST=Test thoroughly the translate feature.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49307

[tfarina, 2010-06-03 23:59:05]

Jay, I made some minor comments below (also I looked only yet in those header
files).

http://codereview.chromium.org/2602003/diff/1/19
File chrome/browser/views/infobars/after_translate_infobar.h (right):

http://codereview.chromium.org/2602003/diff/1/19#newcode37
chrome/browser/views/infobars/after_translate_infobar.h:37: //
TranslateInfoBarView override:
// Overridden from ...

http://codereview.chromium.org/2602003/diff/1/19#newcode41
chrome/browser/views/infobars/after_translate_infobar.h:41: //
views::ButtonListener implementation:
// Overridden from ..

http://codereview.chromium.org/2602003/diff/1/19#newcode53
chrome/browser/views/infobars/after_translate_infobar.h:53: views::Label*
label_1_;
could you make these label names less generic?

http://codereview.chromium.org/2602003/diff/1/19#newcode63
chrome/browser/views/infobars/after_translate_infobar.h:63: LanguagesMenuModel2
original_language_menu_model_;
I would use scoped_ptr for this and the models below, but I think the static
ones is ok too.

http://codereview.chromium.org/2602003/diff/1/21
File chrome/browser/views/infobars/before_translate_infobar.h (right):

http://codereview.chromium.org/2602003/diff/1/21#newcode38
chrome/browser/views/infobars/before_translate_infobar.h:38: //
views::ButtonListener implementation:
// Overridden from...

Same thing below (or change the above one to be views::View implementation:)

http://codereview.chromium.org/2602003/diff/1/21#newcode54
chrome/browser/views/infobars/before_translate_infobar.h:54: views::Label*
label_1_;
name these with less generic name?

http://codereview.chromium.org/2602003/diff/1/32
File chrome/browser/views/infobars/translate_message_infobar.h (right):

http://codereview.chromium.org/2602003/diff/1/32#newcode29
chrome/browser/views/infobars/translate_message_infobar.h:29: 
remove these blank lines.

[jeremy, 2010-06-06 11:53:00]

Thanks for doing this!

Q: Are we planning on redoing the UI for the translate feature or is the current
behavior "relatively" stable?

http://codereview.chromium.org/2602003/diff/1/7
File chrome/browser/translate/languages_menu_model2.h (right):

http://codereview.chromium.org/2602003/diff/1/7#newcode41
chrome/browser/translate/languages_menu_model2.h:41: 
nit: lots of newlines.

http://codereview.chromium.org/2602003/diff/1/10
File chrome/browser/translate/options_menu_model2.cc (right):

http://codereview.chromium.org/2602003/diff/1/10#newcode45
chrome/browser/translate/options_menu_model2.cc:45: case
IDC_TRANSLATE_OPTIONS_NEVER_TRANSLATE_LANG :
nit: no space before ':'

http://codereview.chromium.org/2602003/diff/1/12
File chrome/browser/translate/translate_infobar_delegate2.cc (right):

http://codereview.chromium.org/2602003/diff/1/12#newcode22
chrome/browser/translate/translate_infobar_delegate2.cc:22: const std::string&
original_language,
Are these language codes or language names?
Does it make sense to pass these in as string16 ?

http://codereview.chromium.org/2602003/diff/1/13
File chrome/browser/translate/translate_infobar_delegate2.h (right):

http://codereview.chromium.org/2602003/diff/1/13#newcode68
chrome/browser/translate/translate_infobar_delegate2.h:68: // Returns whether
the infobar should use a fadding effect of the background
nti: fading

http://codereview.chromium.org/2602003/diff/1/13#newcode73
chrome/browser/translate/translate_infobar_delegate2.h:73: bool
FadeBackgroundColorEnabled();
IMHO the colors involved should be defined in a platform-agnostic manner.

http://codereview.chromium.org/2602003/diff/1/18
File chrome/browser/views/infobars/after_translate_infobar.cc (right):

http://codereview.chromium.org/2602003/diff/1/18#newcode37
chrome/browser/views/infobars/after_translate_infobar.cc:37: string16(),
string16(), &offsets);
Can you put this logic into a base class or function since we're going to need
identical code on all platforms.

Maybe something like:
ParseTranslateString(int id, std::vector<std::string16>* strings, bool*
is_flipped);

http://codereview.chromium.org/2602003/diff/1/35
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/2602003/diff/1/35#newcode640
chrome/renderer/render_widget.cc:640: LOG(ERROR) << "** DID FOCUS " << this << "
**";
debug code?

http://codereview.chromium.org/2602003/diff/1/35#newcode651
chrome/renderer/render_widget.cc:651: LOG(ERROR) << "** DID BLUR " << this << "
**";
debug code?

[kuan, 2010-06-07 19:48:53]

LGTM.

http://codereview.chromium.org/2602003/diff/1/5
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/2602003/diff/1/5#newcode38
chrome/browser/tab_contents/render_view_context_menu.cc:38: #include
"chrome/browser/translate/translate_manager2.h"
do u need #if defined(OS_WIN) here?

http://codereview.chromium.org/2602003/diff/1/12
File chrome/browser/translate/translate_infobar_delegate2.cc (right):

http://codereview.chromium.org/2602003/diff/1/12#newcode60
chrome/browser/translate/translate_infobar_delegate2.cc:60: if (language_code ==
target_language)
should there be an "else" here?

http://codereview.chromium.org/2602003/diff/1/13
File chrome/browser/translate/translate_infobar_delegate2.h (right):

http://codereview.chromium.org/2602003/diff/1/13#newcode70
chrome/browser/translate/translate_infobar_delegate2.h:70: // If this is an
error infobar (if IsError() returns true), it should fades
should fades -> should fade

http://codereview.chromium.org/2602003/diff/1/13#newcode110
chrome/browser/translate/translate_infobar_delegate2.h:110: // Convenience
methods that returns the displayable language name for
either change methods to method or returns to return

http://codereview.chromium.org/2602003/diff/1/13#newcode147
chrome/browser/translate/translate_infobar_delegate2.h:147: // The error that
occurred when tryimg to translate (NONE if no error).
trying

http://codereview.chromium.org/2602003/diff/1/14
File chrome/browser/translate/translate_infobar_view.h (right):

http://codereview.chromium.org/2602003/diff/1/14#newcode5
chrome/browser/translate/translate_infobar_view.h:5: #ifndef
CHROME_BROWSER_TRANSLATE_TRANSLATE_INFOBARS_VIEW_H_
INFOBARS -> INFOBAR

http://codereview.chromium.org/2602003/diff/1/18
File chrome/browser/views/infobars/after_translate_infobar.cc (right):

http://codereview.chromium.org/2602003/diff/1/18#newcode143
chrome/browser/views/infobars/after_translate_infobar.cc:143: const gfx::Point&
pt) {
extra space before "const"

http://codereview.chromium.org/2602003/diff/1/19
File chrome/browser/views/infobars/after_translate_infobar.h (right):

http://codereview.chromium.org/2602003/diff/1/19#newcode49
chrome/browser/views/infobars/after_translate_infobar.h:49: // Sets the text of
the original uor target language menu buttons to reflect
uor -> or

http://codereview.chromium.org/2602003/diff/1/20
File chrome/browser/views/infobars/before_translate_infobar.cc (right):

http://codereview.chromium.org/2602003/diff/1/20#newcode85
chrome/browser/views/infobars/before_translate_infobar.cc:85: 
remove empty line (like the others)?

http://codereview.chromium.org/2602003/diff/1/23
File chrome/browser/views/infobars/infobar_button_border.h (right):

http://codereview.chromium.org/2602003/diff/1/23#newcode20
chrome/browser/views/infobars/infobar_button_border.h:20: class
InfoBarButtonBorder : public views::TextButtonBorder {
i remember that back then when i got to the final stage of drawing the button
border, i realize there was no need to inherit from TextButtonBorder anymore,
but didn't get the chance to change it.  i cld be wrong, but it's up to u if u
want to look into that.

http://codereview.chromium.org/2602003/diff/1/28
File chrome/browser/views/infobars/translate_infobar_base.h (right):

http://codereview.chromium.org/2602003/diff/1/28#newcode64
chrome/browser/views/infobars/translate_infobar_base.h:64: // Returns the
background that shoudl be displayed when not animating.
should

[Jay Civelli, 2010-06-07 21:30:43]

All comments addressed.

Jeremy, to answer your question, the UI is going to be mostly the same in 6.0.
We are going to add an extra menu in options (to report bad language detection)
and possibly make the "don't translate this language" more prominent, but other
than that it should stay the same.

http://codereview.chromium.org/2602003/diff/1/5
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/2602003/diff/1/5#newcode38
chrome/browser/tab_contents/render_view_context_menu.cc:38: #include
"chrome/browser/translate/translate_manager2.h"
On 2010/06/07 19:48:54, kuan wrote:
> do u need #if defined(OS_WIN) here?

Good point. Added.

http://codereview.chromium.org/2602003/diff/1/7
File chrome/browser/translate/languages_menu_model2.h (right):

http://codereview.chromium.org/2602003/diff/1/7#newcode41
chrome/browser/translate/languages_menu_model2.h:41: 
On 2010/06/06 11:53:00, jeremy wrote:
> nit: lots of newlines.

Done.

http://codereview.chromium.org/2602003/diff/1/10
File chrome/browser/translate/options_menu_model2.cc (right):

http://codereview.chromium.org/2602003/diff/1/10#newcode45
chrome/browser/translate/options_menu_model2.cc:45: case
IDC_TRANSLATE_OPTIONS_NEVER_TRANSLATE_LANG :
On 2010/06/06 11:53:00, jeremy wrote:
> nit: no space before ':'

Done.

http://codereview.chromium.org/2602003/diff/1/12
File chrome/browser/translate/translate_infobar_delegate2.cc (right):

http://codereview.chromium.org/2602003/diff/1/12#newcode22
chrome/browser/translate/translate_infobar_delegate2.cc:22: const std::string&
original_language,
On 2010/06/06 11:53:00, jeremy wrote:
> Are these language codes or language names?
> Does it make sense to pass these in as string16 ?
Yes, they are language code (I update the comment in the .h file to explain
that).
Language codes are ASCII, so there is no need to us string16.

http://codereview.chromium.org/2602003/diff/1/12#newcode60
chrome/browser/translate/translate_infobar_delegate2.cc:60: if (language_code ==
target_language)
On 2010/06/07 19:48:54, kuan wrote:
> should there be an "else" here?

Done.

http://codereview.chromium.org/2602003/diff/1/13
File chrome/browser/translate/translate_infobar_delegate2.h (right):

http://codereview.chromium.org/2602003/diff/1/13#newcode68
chrome/browser/translate/translate_infobar_delegate2.h:68: // Returns whether
the infobar should use a fadding effect of the background
On 2010/06/06 11:53:00, jeremy wrote:
> nti: fading

Done.

http://codereview.chromium.org/2602003/diff/1/13#newcode70
chrome/browser/translate/translate_infobar_delegate2.h:70: // If this is an
error infobar (if IsError() returns true), it should fades
On 2010/06/07 19:48:54, kuan wrote:
> should fades -> should fade

Done.

http://codereview.chromium.org/2602003/diff/1/13#newcode73
chrome/browser/translate/translate_infobar_delegate2.h:73: bool
FadeBackgroundColorEnabled();
On 2010/06/06 11:53:00, jeremy wrote:
> IMHO the colors involved should be defined in a platform-agnostic manner.
The colors are defined in some of the views infobar base classes, so I am not
convinced they should be reported by the model.
However I agree that the logic on what animation to use should be in the model
rather than the infobar, so I changed this method to do that.

http://codereview.chromium.org/2602003/diff/1/13#newcode110
chrome/browser/translate/translate_infobar_delegate2.h:110: // Convenience
methods that returns the displayable language name for
On 2010/06/07 19:48:54, kuan wrote:
> either change methods to method or returns to return

Done.

http://codereview.chromium.org/2602003/diff/1/13#newcode147
chrome/browser/translate/translate_infobar_delegate2.h:147: // The error that
occurred when tryimg to translate (NONE if no error).
On 2010/06/07 19:48:54, kuan wrote:
> trying

Done.

http://codereview.chromium.org/2602003/diff/1/14
File chrome/browser/translate/translate_infobar_view.h (right):

http://codereview.chromium.org/2602003/diff/1/14#newcode5
chrome/browser/translate/translate_infobar_view.h:5: #ifndef
CHROME_BROWSER_TRANSLATE_TRANSLATE_INFOBARS_VIEW_H_
On 2010/06/07 19:48:54, kuan wrote:
> INFOBARS -> INFOBAR

Done.

http://codereview.chromium.org/2602003/diff/1/18
File chrome/browser/views/infobars/after_translate_infobar.cc (right):

http://codereview.chromium.org/2602003/diff/1/18#newcode37
chrome/browser/views/infobars/after_translate_infobar.cc:37: string16(),
string16(), &offsets);
On 2010/06/06 11:53:00, jeremy wrote:
> Can you put this logic into a base class or function since we're going to need
> identical code on all platforms.
> 
> Maybe something like:
> ParseTranslateString(int id, std::vector<std::string16>* strings, bool*
> is_flipped);
I added a method to TranslateInfobarDelegate2 to get the strings for the after
translate infobar.
It did not seem necessary to make it more generic and use it for the before
translate infobar since it that case there are just 2 strings.

http://codereview.chromium.org/2602003/diff/1/18#newcode143
chrome/browser/views/infobars/after_translate_infobar.cc:143: const gfx::Point&
pt) {
On 2010/06/07 19:48:54, kuan wrote:
> extra space before "const"

Done.

http://codereview.chromium.org/2602003/diff/1/19
File chrome/browser/views/infobars/after_translate_infobar.h (right):

http://codereview.chromium.org/2602003/diff/1/19#newcode37
chrome/browser/views/infobars/after_translate_infobar.h:37: //
TranslateInfoBarView override:
On 2010/06/03 23:59:05, tfarina wrote:
> // Overridden from ...

Done.

http://codereview.chromium.org/2602003/diff/1/19#newcode41
chrome/browser/views/infobars/after_translate_infobar.h:41: //
views::ButtonListener implementation:
On 2010/06/03 23:59:05, tfarina wrote:
> // Overridden from ..

Done.

http://codereview.chromium.org/2602003/diff/1/19#newcode49
chrome/browser/views/infobars/after_translate_infobar.h:49: // Sets the text of
the original uor target language menu buttons to reflect
On 2010/06/07 19:48:54, kuan wrote:
> uor -> or

Done.

http://codereview.chromium.org/2602003/diff/1/19#newcode53
chrome/browser/views/infobars/after_translate_infobar.h:53: views::Label*
label_1_;
On 2010/06/03 23:59:05, tfarina wrote:
> could you make these label names less generic?
I could not come up with a meaningful name, so I added a comment to explain what
those labels are.

http://codereview.chromium.org/2602003/diff/1/19#newcode63
chrome/browser/views/infobars/after_translate_infobar.h:63: LanguagesMenuModel2
original_language_menu_model_;
On 2010/06/03 23:59:05, tfarina wrote:
> I would use scoped_ptr for this and the models below, but I think the static
> ones is ok too.
I usually prefer to make the object part of the class rather than use a
scoped_ptr. The reason I am using a scoped_ptr for Menu2 is because the header
of that class says they should not be created on the stack.

http://codereview.chromium.org/2602003/diff/1/20
File chrome/browser/views/infobars/before_translate_infobar.cc (right):

http://codereview.chromium.org/2602003/diff/1/20#newcode85
chrome/browser/views/infobars/before_translate_infobar.cc:85: 
On 2010/06/07 19:48:54, kuan wrote:
> remove empty line (like the others)?

Done.

http://codereview.chromium.org/2602003/diff/1/21
File chrome/browser/views/infobars/before_translate_infobar.h (right):

http://codereview.chromium.org/2602003/diff/1/21#newcode38
chrome/browser/views/infobars/before_translate_infobar.h:38: //
views::ButtonListener implementation:
On 2010/06/03 23:59:05, tfarina wrote:
> // Overridden from...
> 
> Same thing below (or change the above one to be views::View implementation:)

Done.

http://codereview.chromium.org/2602003/diff/1/21#newcode54
chrome/browser/views/infobars/before_translate_infobar.h:54: views::Label*
label_1_;
On 2010/06/03 23:59:05, tfarina wrote:
> name these with less generic name?

Added a comment explaining what they are.

http://codereview.chromium.org/2602003/diff/1/23
File chrome/browser/views/infobars/infobar_button_border.h (right):

http://codereview.chromium.org/2602003/diff/1/23#newcode20
chrome/browser/views/infobars/infobar_button_border.h:20: class
InfoBarButtonBorder : public views::TextButtonBorder {
On 2010/06/07 19:48:54, kuan wrote:
> i remember that back then when i got to the final stage of drawing the button
> border, i realize there was no need to inherit from TextButtonBorder anymore,
> but didn't get the chance to change it.  i cld be wrong, but it's up to u if u
> want to look into that.

Done.

http://codereview.chromium.org/2602003/diff/1/28
File chrome/browser/views/infobars/translate_infobar_base.h (right):

http://codereview.chromium.org/2602003/diff/1/28#newcode64
chrome/browser/views/infobars/translate_infobar_base.h:64: // Returns the
background that shoudl be displayed when not animating.
On 2010/06/07 19:48:54, kuan wrote:
> should

Done.

http://codereview.chromium.org/2602003/diff/1/32
File chrome/browser/views/infobars/translate_message_infobar.h (right):

http://codereview.chromium.org/2602003/diff/1/32#newcode29
chrome/browser/views/infobars/translate_message_infobar.h:29: 
On 2010/06/03 23:59:05, tfarina wrote:
> remove these blank lines.

Done.

http://codereview.chromium.org/2602003/diff/1/35
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/2602003/diff/1/35#newcode640
chrome/renderer/render_widget.cc:640: LOG(ERROR) << "** DID FOCUS " << this << "
**";
On 2010/06/06 11:53:00, jeremy wrote:
> debug code?

Removed.

http://codereview.chromium.org/2602003/diff/1/35#newcode651
chrome/renderer/render_widget.cc:651: LOG(ERROR) << "** DID BLUR " << this << "
**";
On 2010/06/06 11:53:00, jeremy wrote:
> debug code?

Removed.

[jeremy, 2010-06-08 07:40:26]

LGTM

http://codereview.chromium.org/2602003/diff/1/12
File chrome/browser/translate/translate_infobar_delegate2.cc (right):

http://codereview.chromium.org/2602003/diff/1/12#newcode22
chrome/browser/translate/translate_infobar_delegate2.cc:22: const std::string&
original_language,
Can you mention that these are ASCII encoded in the comment too or if your
feeling really adventurous suffix the variable names with _ascii :) Though I'm
not sure if there's precedence for doing that...