Translate Infobars for OS X.

Translate Infobars for OS X.

Infobars.xib changes - Connect and fix class for close button since the translate infobar needs to know where it is to position the "Options" menu to it's left.

BUG=34466
TEST=Translate infobars should continue to workon Windows.

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

[jeremy, 2010-02-15 13:16:16]

rohit,tvl: This is a messy preliminary version of the CL.  I'm mainly looking
for some high level feedback and validation of the direction I'm going in. 

This CL contains a bunch of files, the only interesting ones are
chrome/browser/cocoa/translate_infobar.{mm,h}

Questions:
* Does the code for building and resizing the UI elements look OK to you?
* Does the general approach look valid?

Notable things missing from this CL:
* Messy - this is just a checkpoint, so I'm still moving things around.
* unit tests - TBD
* The translation infobar can transition between a "before" and "after"
translation state in which we swap in new UI elements, the code to do this isn't
fully implemented.

[TVL, 2010-02-16 14:46:26]

http://codereview.chromium.org/594056/diff/1/7
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/594056/diff/1/7#newcode813
chrome/browser/browser_main.cc:813: // TODO(jcampan): enable on non Windows
platforms when the info-bars are
is there a bug we can reference here?

http://codereview.chromium.org/594056/diff/1/10
File chrome/browser/cocoa/translate_infobar.h (right):

http://codereview.chromium.org/594056/diff/1/10#newcode15
chrome/browser/cocoa/translate_infobar.h:15: class TranslateInfoBarMenuModel :
public menus::SimpleMenuModel::Delegate {
comment for why this is needed?

http://codereview.chromium.org/594056/diff/1/10#newcode37
chrome/browser/cocoa/translate_infobar.h:37: class
TranslateInfoBarNotificationObserverBridge :
don't we put most of the bridges within the impl files and not in headers?

http://codereview.chromium.org/594056/diff/1/11
File chrome/browser/cocoa/translate_infobar.mm (right):

http://codereview.chromium.org/594056/diff/1/11#newcode29
chrome/browser/cocoa/translate_infobar.mm:29: void moveControl(NSView* anchor,
NSView* toMove, int spacing, bool after) {
C++ naming, so MoveControl (and the others below)

http://codereview.chromium.org/594056/diff/1/11#newcode73
chrome/browser/cocoa/translate_infobar.mm:73: // false -This is an "After
Translate" infobar.
space after -?

http://codereview.chromium.org/594056/diff/1/11#newcode74
chrome/browser/cocoa/translate_infobar.mm:74: - (bool)beforeTranslateState;
is this the same type of info as TranslateInfoBarDelegate::TranslateState?  Why
is that an enum and this a bool?

http://codereview.chromium.org/594056/diff/1/11#newcode87
chrome/browser/cocoa/translate_infobar.mm:87: -
(void)sourceLanguageModified:(int)newLanguageIdx;
NSInteger?  (what is the objc type, code it clean so 64bit is easier)

http://codereview.chromium.org/594056/diff/1/11#newcode91
chrome/browser/cocoa/translate_infobar.mm:91: - (void)languageModified;
how is this different from the above pair?

http://codereview.chromium.org/594056/diff/1/11#newcode97
chrome/browser/cocoa/translate_infobar.mm:97: #pragma mark
TranslateInfoBarController Class
blank line?

http://codereview.chromium.org/594056/diff/1/11#newcode170
chrome/browser/cocoa/translate_infobar.mm:170: NSLog(@"Transitioning to new
state");
don't forget these

http://codereview.chromium.org/594056/diff/1/11#newcode221
chrome/browser/cocoa/translate_infobar.mm:221: NSString* optionsLabel =
do we need to worry about windows accelerators, ellipsis, etc in here?  ie
WithFixup calls.

http://codereview.chromium.org/594056/diff/1/11#newcode254
chrome/browser/cocoa/translate_infobar.mm:254: // TODO(jeremy): Move to [- init]
should these get a bug or are you gonna do all this before sending it out for
checkin?

http://codereview.chromium.org/594056/diff/1/11#newcode306
chrome/browser/cocoa/translate_infobar.mm:306: [okButton_
setTitle:l10n_util::GetNSString(IDS_TRANSLATE_INFOBAR_ACCEPT)];
WithFixup?

http://codereview.chromium.org/594056/diff/1/11#newcode364
chrome/browser/cocoa/translate_infobar.mm:364: if ([item
respondsToSelector:@selector(tag)]) {
DCHECK that you don't get any items that don't?

[jeremy, 2010-02-24 18:27:52]

http://codereview.chromium.org/594056/diff/7015/7022
File chrome/browser/renderer_host/translation_service.cc (right):

http://codereview.chromium.org/594056/diff/7015/7022#newcode13
chrome/browser/renderer_host/translation_service.cc:13: #if 1 ||
defined(GOOGLE_CHROME_BUILD)
Please ignore this, I won't commit this change.

[rohitrao (ping after 24h), 2010-02-24 21:52:23]

http://codereview.chromium.org/594056/diff/7015/7017
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/594056/diff/7015/7017#newcode829
chrome/browser/browser_main.cc:829: // TODO(jcampan): enable on non Windows
platforms when the info-bars are
s/non Windows platforms/Linux/

http://codereview.chromium.org/594056/diff/7015/7020
File chrome/browser/cocoa/translate_infobar.h (right):

http://codereview.chromium.org/594056/diff/7015/7020#newcode13
chrome/browser/cocoa/translate_infobar.h:13: @class TranslateInfoBarController;
Do you need to forward-declare this if you're going to define it a few lines
later?

http://codereview.chromium.org/594056/diff/7015/7020#newcode29
chrome/browser/cocoa/translate_infobar.h:29: NSTextField* label2_;
Maybe add a comment that label_ is defined in the base class, in case anyone
else wondered as I did? :)

http://codereview.chromium.org/594056/diff/7015/7021
File chrome/browser/cocoa/translate_infobar.mm (right):

http://codereview.chromium.org/594056/diff/7015/7021#newcode31
chrome/browser/cocoa/translate_infobar.mm:31: // Move control |toMove| |spacing|
pixels before/after the |anchor| view.
I had some trouble parsing this sentence.  Maybe something like:

Move the |toMove| view |spacing| pixels...

http://codereview.chromium.org/594056/diff/7015/7021#newcode38
chrome/browser/cocoa/translate_infobar.mm:38: // it doesn't hurt to be forward
lookign.
Typo: lookign

http://codereview.chromium.org/594056/diff/7015/7021#newcode39
chrome/browser/cocoa/translate_infobar.mm:39: bool toRight = after;
Why is this a local variable?  Can you just remove this line and rename the
|after| argument to |placeOnRight|?

http://codereview.chromium.org/594056/diff/7015/7021#newcode51
chrome/browser/cocoa/translate_infobar.mm:51: // Vertically center |toMove| in
it's container.
s/it's/its/

http://codereview.chromium.org/594056/diff/7015/7021#newcode56
chrome/browser/cocoa/translate_infobar.mm:56: viewFrame.origin.y =
(NSHeight(superViewFrame) - NSHeight(viewFrame))/2.0;
What if the difference is odd?  Do we have to worry about drawing across pixels
and getting blurry text?

http://codereview.chromium.org/594056/diff/7015/7021#newcode80
chrome/browser/cocoa/translate_infobar.mm:80: if (checked) {
No braces if only one line.

http://codereview.chromium.org/594056/diff/7015/7021#newcode191
chrome/browser/cocoa/translate_infobar.mm:191: new
LanguagesMenuModel(menu_model_.get(), [self delegate], true));
Can you add a /*original_language=*/ comment to this call and the next?

http://codereview.chromium.org/594056/diff/7015/7021#newcode202
chrome/browser/cocoa/translate_infobar.mm:202: [label2_ release];
Can these all be scoped_nsobjects?

http://codereview.chromium.org/594056/diff/7015/7023
File chrome/browser/translate/translate_infobars_delegates.cc (left):

http://codereview.chromium.org/594056/diff/7015/7023#oldcode217
chrome/browser/translate/translate_infobars_delegates.cc:217: InfoBar*
TranslateInfoBarDelegate::CreateInfoBar() {
Why is this going away?  What about the other non-views, non-mac platforms?

[jeremy, 2010-02-24 22:54:09]

OK, all fixed...

http://codereview.chromium.org/594056/diff/7015/7017
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/594056/diff/7015/7017#newcode829
chrome/browser/browser_main.cc:829: // TODO(jcampan): enable on non Windows
platforms when the info-bars are
And I even added a bug number :) , douze points!

http://codereview.chromium.org/594056/diff/7015/7020
File chrome/browser/cocoa/translate_infobar.h (right):

http://codereview.chromium.org/594056/diff/7015/7020#newcode13
chrome/browser/cocoa/translate_infobar.h:13: @class TranslateInfoBarController;
On 2010/02/24 21:52:24, rohitrao wrote:
> Do you need to forward-declare this if you're going to define it a few lines
> later?

Done - leftover crud :/

http://codereview.chromium.org/594056/diff/7015/7020#newcode29
chrome/browser/cocoa/translate_infobar.h:29: NSTextField* label2_;
On 2010/02/24 21:52:24, rohitrao wrote:
> Maybe add a comment that label_ is defined in the base class, in case anyone
> else wondered as I did? :)

Done.

http://codereview.chromium.org/594056/diff/7015/7021
File chrome/browser/cocoa/translate_infobar.mm (right):

http://codereview.chromium.org/594056/diff/7015/7021#newcode31
chrome/browser/cocoa/translate_infobar.mm:31: // Move control |toMove| |spacing|
pixels before/after the |anchor| view.
On 2010/02/24 21:52:24, rohitrao wrote:
> I had some trouble parsing this sentence.  Maybe something like:
> 
> Move the |toMove| view |spacing| pixels...

Done.

http://codereview.chromium.org/594056/diff/7015/7021#newcode38
chrome/browser/cocoa/translate_infobar.mm:38: // it doesn't hurt to be forward
lookign.
On 2010/02/24 21:52:24, rohitrao wrote:
> Typo: lookign

Done.

http://codereview.chromium.org/594056/diff/7015/7021#newcode39
chrome/browser/cocoa/translate_infobar.mm:39: bool toRight = after;
Keeping as is per f2f conversation.

http://codereview.chromium.org/594056/diff/7015/7021#newcode51
chrome/browser/cocoa/translate_infobar.mm:51: // Vertically center |toMove| in
it's container.
On 2010/02/24 21:52:24, rohitrao wrote:
> s/it's/its/

Done.

http://codereview.chromium.org/594056/diff/7015/7021#newcode56
chrome/browser/cocoa/translate_infobar.mm:56: viewFrame.origin.y =
(NSHeight(superViewFrame) - NSHeight(viewFrame))/2.0;
Dunno, but rounded.

http://codereview.chromium.org/594056/diff/7015/7021#newcode80
chrome/browser/cocoa/translate_infobar.mm:80: if (checked) {
On 2010/02/24 21:52:24, rohitrao wrote:
> No braces if only one line.

Done.

http://codereview.chromium.org/594056/diff/7015/7021#newcode202
chrome/browser/cocoa/translate_infobar.mm:202: [label2_ release];
On 2010/02/24 21:52:24, rohitrao wrote:
> Can these all be scoped_nsobjects?

Done.

http://codereview.chromium.org/594056/diff/7015/7023
File chrome/browser/translate/translate_infobars_delegates.cc (left):

http://codereview.chromium.org/594056/diff/7015/7023#oldcode217
chrome/browser/translate/translate_infobars_delegates.cc:217: InfoBar*
TranslateInfoBarDelegate::CreateInfoBar() {
I don't think this was needed in the first place, if the try bots complain I'll
add it back.

[rohitrao (ping after 24h), 2010-02-24 23:38:53]

LGTM

Per discussion, the CreateInfoBar() needs to go back in on Linux and any other
platforms where the trybots complain =)

[jcampan, 2010-02-24 23:51:45]

I look at the non mac specific code, LGTM.

Don't forget not to check-in translation_service.cc.