Translate bubble for Mac OS X

chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm

Index: chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm
diff --git a/chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm b/chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm
index 577da21c46945240cd981e26285ec200d6bea601..93116c3bccc3bd02a15f42f8818148b1b3a0b9ae 100644
--- a/chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm
+++ b/chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm
@@ -4,15 +4,80 @@
 
 #import "chrome/browser/ui/cocoa/translate/translate_bubble_controller.h"
 
+#include "base/mac/foundation_util.h"
 #include "base/mac/scoped_nsobject.h"
+#include "base/strings/sys_string_conversions.h"
 #include "chrome/browser/translate/translate_ui_delegate.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
 #import "chrome/browser/ui/cocoa/info_bubble_view.h"
 #import "chrome/browser/ui/cocoa/info_bubble_window.h"
 #import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
+#include "chrome/browser/ui/translate/language_combobox_model.h"
 #include "chrome/browser/ui/translate/translate_bubble_model_impl.h"
-#import "ui/base/cocoa/flipped_view.h"
+#include "content/public/browser/browser_context.h"
+#include "grit/generated_resources.h"
+#include "third_party/google_toolbox_for_mac/src/AppKit/GTMUILocalizerAndLayoutTweaker.h"
+#import "ui/base/cocoa/controls/hyperlink_button_cell.h"
 #import "ui/base/cocoa/window_size_constants.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/models/combobox_model.h"
+
+class TranslateDenialComboboxModel : public ui::ComboboxModel {

[groby-ooo-7-16, 2014/03/21 01:21:42]

This seems identical to the Views combobox model - can this be shared?

[hajimehoshi, 2014/03/24 07:16:30]

They are a little different in that the first item is dummy on Mac. They are
similar anyway, hmm, let me unify them later.

[groby-ooo-7-16, 2014/03/25 18:49:04]

True, OSX needs a dummy item - but that should be handled when converting from
ComboboxModel to NSMenu. Shared models are always good. 

[hajimehoshi, 2014/03/26 11:06:05]

Sure, but I'll leave it until your question about the UX of the popup button is
resolved.

+ public:
+  enum {
+    kIndexNope = 1,
+    kIndexNeverTranslateLanguage = 2,
+    kIndexNeverTranslateSite = 3,
+  };
+
+  explicit TranslateDenialComboboxModel(
+      const base::string16& original_language_name) {
+    // Dummy menu item, which is shown on the top of a NSPopUpButton. The top
+    // text of the denial pop up menu should be IDS_TRANSLATE_BUBBLE_DENY, while
+    // it is impossible to use it here because NSPopUpButtons' addItemWithTitle
+    // removes a duplicated menu item. Instead, the title will be set later by
+    // NSMenuItem's setTitle.
+    items_.push_back(base::string16());
+
+    // Menu items in the drop down menu.
+    items_.push_back(l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_DENY));
+    items_.push_back(l10n_util::GetStringFUTF16(
+        IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_LANG,
+        original_language_name));
+    items_.push_back(l10n_util::GetStringUTF16(
+        IDS_TRANSLATE_BUBBLE_NEVER_TRANSLATE_SITE));
+  }
+  virtual ~TranslateDenialComboboxModel() {}
+
+ private:
+  // ComboboxModel:
+  virtual int GetItemCount() const OVERRIDE {
+    return items_.size();
+  }
+  virtual base::string16 GetItemAt(int index) OVERRIDE {
+    return items_[index];
+  }
+  virtual bool IsItemSeparatorAt(int index) OVERRIDE {
+    return false;
+  }
+  virtual int GetDefaultIndex() const OVERRIDE {
+    return 0;
+  }
+
+  std::vector<base::string16> items_;
+
+  DISALLOW_COPY_AND_ASSIGN(TranslateDenialComboboxModel);
+};
+
+const CGFloat kWindowWidth = 320;
+
+// Padding between the window frame and content.
+const CGFloat kFramePadding = 16;
+
+const CGFloat kRelatedControlHorizontalSpacing = -2;

[groby-ooo-7-16, 2014/03/21 01:21:42]

Why is this negative? Intuitively, I would read that as "related controls
overlap horizontally" - which probably means I read this wrong :)

[hajimehoshi, 2014/03/24 07:16:30]

This is actually negative. I put two buttons side by side on Xcode and adjust
along with the automatic horizontal space constraint. I found that (the right
button's X) - (the left button's X) - (the left button's width) was minus 2.
Maybe buttons were overlapping.

[groby-ooo-7-16, 2014/03/25 18:49:04]

That's odd. Looking at the mocks, I see no two buttons that are directly
side-by-side, though?

[hajimehoshi, 2014/03/26 11:06:05]

Yes, there is space between two buttons, while the number of this constant is
negative. I wonder if the logical width and the actual width differ...

[groby-ooo-7-16, 2014/03/27 00:39:39]

They sort-of do. The frame indicates "most" of the area for the button. The
focus ring is (IIRC) drawn outside the frame. But if you place them just using
NSWidth, you shouldn't need a constant. This is weird, but if this fixes your
layout, leave it in.

+
+const CGFloat kRelatedControlVerticalSpacing = 4;
+const CGFloat kUnrelatedControlVerticalSpacing = 20;
 
 @implementation TranslateBubbleController
 
@@ -39,6 +104,12 @@
       translateExecuted_ = YES;
     }
 
+    beforeTranslateView_.reset([self createViewBeforeTranslate]);
+    translatingView_.reset([self createViewTranslating]);
+    afterTranslateView_.reset([self createViewAfterTranslate]);
+    errorView_.reset([self createViewError]);
+    advancedView_.reset([self createViewAdvanced]);
+
     [self performLayout];
   }
   return self;
@@ -46,6 +117,23 @@
 
 @synthesize webContents = webContents_;
 
+- (NSView*)currentView {

[groby-ooo-7-16, 2014/03/21 01:21:42]

I'm wondering if it would be nicer to simply have a ScopedPtrHashMap of
ViewState->NSView.

[hajimehoshi, 2014/03/24 07:16:30]

It seems that ScopedPtrHashMap can't be used for NSView bacause NSView is not
C++ class. I could use std::map, but the member variable *View_ would remain and
the ownerships would remain on them.

[groby-ooo-7-16, 2014/03/25 18:49:04]

You are of course right, sorry. I should have said NSDictionary - I have no idea
what I was thinking about when I suggested otherwise.

The dictionary then has ownership of all the views, and the switch becomes a
simple valueForKey. (I'm not a fan of large switch statements)

[hajimehoshi, 2014/03/26 11:06:05]

Done.

+  switch (model_->GetViewState()) {
+    case TranslateBubbleModel::VIEW_STATE_BEFORE_TRANSLATE:
+      return beforeTranslateView_;
+    case TranslateBubbleModel::VIEW_STATE_TRANSLATING:
+      return translatingView_;
+    case TranslateBubbleModel::VIEW_STATE_AFTER_TRANSLATE:
+      return afterTranslateView_;
+    case TranslateBubbleModel::VIEW_STATE_ERROR:
+      return errorView_;
+    case TranslateBubbleModel::VIEW_STATE_ADVANCED:
+      return advancedView_;
+  }
+  NOTREACHED();
+  return nil;
+}
+
 - (const TranslateBubbleModel*)model {
   return model_.get();
 }
@@ -71,8 +159,544 @@
 }
 
 - (void)performLayout {
-  // TODO(hajimehoshi): Now this shows just an empty bubble. Implement this.
-  [[self window] display];
+  [[[self window] contentView] setSubviews:@[ [self currentView] ]];
+
+  CGFloat height = NSHeight([[self currentView] frame]) +
+      2 * kFramePadding + info_bubble::kBubbleArrowHeight;
+
+  NSRect contentViewFrame = [[[self window] contentView] frame];
+  contentViewFrame.size.height = height;
+  [[[self window] contentView] setFrame:contentViewFrame];

[groby-ooo-7-16, 2014/03/21 01:21:42]

If you don't need to animate the window, you probably want [[self window]
setContentSize:contentViewFrame.size];

This will automatically adjust the window's size, so you only need to comput
windowFrame.origin, and call [[self window] setFrameOrigin:windowFrameOrigin];

If you DO want to animate, you want to compute the window's frame size via
-frameRectForContentRect: - this takes potential window decorations into
accoumt.

[hajimehoshi, 2014/03/24 07:16:30]

Done (animation is needed, so I tried to use frameRectForContentRect.)

+
+  NSRect windowFrame = [[self window] frame];
+  windowFrame.origin.y += NSHeight(windowFrame) - height;
+  windowFrame.size.width = kWindowWidth;
+  windowFrame.size.height = height;
+  [[self window] setFrame:windowFrame
+                  display:YES
+                  animate:[[self window] isVisible]];

[groby-ooo-7-16, 2014/03/21 01:21:42]

I assume you want the window to just pop up if it wasn't visible yet, no
animation?

[hajimehoshi, 2014/03/24 07:16:30]

Right. It needs animation when changing the view. (ex: clicking the 'options'
link to show the advanced view.)

+}
+
+- (NSView*)createViewBeforeTranslate {
+  CGFloat contentWidth = kWindowWidth - 2 * kFramePadding;

[groby-ooo-7-16, 2014/03/21 01:21:42]

That seems to always be the contentWidth - maybe just add 
CGFloat kContentWidth = kWindowWidth - 2 * kFramePadding; 

to the constants?

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+  NSRect contentFrame = NSMakeRect(
+      kFramePadding,
+      kFramePadding,
+      contentWidth,
+      1);

[groby-ooo-7-16, 2014/03/21 01:21:42]

It's OK to use 0 for the height - only NSWindow doesn't like zero width or
height.

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+  NSView* view = [[NSView alloc] initWithFrame:contentFrame];
+
+  NSString* message =
+      l10n_util::GetNSStringWithFixup(IDS_TRANSLATE_BUBBLE_BEFORE_TRANSLATE);
+  NSTextField* textLabel = [self addText:message
+                                withSize:[NSFont smallSystemFontSize]
+                                    bold:NO
+                                  toView:view];
+  message = l10n_util::GetNSStringWithFixup(IDS_TRANSLATE_BUBBLE_ADVANCED);
+  NSButton* advancedLinkButton =
+      [self addLinkButtonWithText:message
+                           target:self
+                           action:@selector(handleAdvancedLinkButtonPressed)
+                           toView:view];
+
+  NSString* title =
+      l10n_util::GetNSStringWithFixup(IDS_TRANSLATE_BUBBLE_ACCEPT);
+  NSButton* translateButton =
+      [self addButton:title
+               target:self
+               action:@selector(handleTranslateButtonPressed)
+               toView:view];
+
+  base::string16 originalLanguageName =
+      model_->GetLanguageNameAt(model_->GetOriginalLanguageIndex());
+  translateDenialComboboxModel_.reset(
+      new TranslateDenialComboboxModel(originalLanguageName));
+  SEL action = @selector(handleDenialPopUpButtonSelectedItemChanged:);
+  NSPopUpButton* denyPopUpButton =
+      [self addPopUpButton:translateDenialComboboxModel_.get()
+                    target:self
+                    action:action
+                    toView:view];
+  [denyPopUpButton setPullsDown:YES];
+  title = base::SysUTF16ToNSString(
+      l10n_util::GetStringUTF16(IDS_TRANSLATE_BUBBLE_DENY));
+  [[denyPopUpButton itemAtIndex:0] setTitle:title];
+
+  // Adjust width for the first item.

[groby-ooo-7-16, 2014/03/21 01:21:42]

A shorter version would be to just copy the popup, remove all but the first
item, sizeToFit, and copy over the size.

I.e.:
scoped_nsobject <NSPopUpButton> helper([denyPopUpButton copy]);
[helper removeAllItems];
[helper addItemWithTitle:[[denyPopUpButton itemAtIndex:0] title]];
[helper sizeToFit];
[denyPopUpButton setFrameSize:[helper frame].size];

(I can't believe we don't have a helper function that does that... Odd!)

[hajimehoshi, 2014/03/24 07:16:30]

Oh, cool! However, it seems that NSButton is not copyable...

[groby-ooo-7-16, 2014/03/25 18:49:04]

Sigh. I forgot, sorry. The cell is copyable - but that might not be enough since
you change control attributes. 

Well, in this case, how about:

scoped_nsobject<NSMenu> originalMenu([[denyPopUpButton menu] copy]);
[denyPopUpButton removeAllItems];
[denyPopUpButton addItemWithTitle:[[originalMenu itemAtIndex:0] title]];
[denyPopUpButton sizeToFit];
[denyPopUpButton setMenu:menu];

NSMenu is copyable, so this should work. (I'd really like to avoid all the for
loops, extracting the titles, etc.)

[hajimehoshi, 2014/03/26 11:06:05]

Done.

+  std::vector<NSString*> titles;
+  for (int i = 1; i < [denyPopUpButton numberOfItems]; i++) {

[groby-ooo-7-16, 2014/03/21 01:21:42]

If the above solution works, you don't need this, but as general info, it's
easier to iterate over Objective C arrays this way:

for (NSMenuItem* item in [denyPopupButton itemArray]) {
...

[hajimehoshi, 2014/03/24 07:16:30]

Done leaving the above as it is.

+    NSMenuItem* item = [denyPopUpButton itemAtIndex:i];
+    titles.push_back([item title]);
+    [item setTitle:@""];
+  }
+  [denyPopUpButton sizeToFit];
+  for (int i = 1; i < [denyPopUpButton numberOfItems]; i++) {
+    NSMenuItem* item = [denyPopUpButton itemAtIndex:i];
+    [item setTitle:titles[i-1]];
+  }
+
+  // Layout
+  CGFloat yPos = 0;
+
+  [translateButton setFrameOrigin:NSMakePoint(
+      contentWidth - NSWidth([translateButton frame]), yPos)];
+
+  NSRect denyPopUpButtonFrame = [denyPopUpButton frame];
+  CGFloat diffY = [[denyPopUpButton cell]
+                    titleRectForBounds:[denyPopUpButton bounds]].origin.y;

[groby-ooo-7-16, 2014/03/21 01:21:42]

Usually, we don't base layout computations on cell size, since the size of
control decorations could conceivable change across SDKs. Can you change the
computation to base this on the button only?

Or maybe I don't quite understand the computation - where is the button supposed
to be? It seems "almost at the bottom, with a small offset"?

[hajimehoshi, 2014/03/24 07:16:30]

Since NSPopUpButton and NSButtond didn't align vertically, I needed to adjust Y
of the NSPopUpButton. It looks like that NSPopUp is put a little below than
normal NSButtons.  I referred some similar cases:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[groby-ooo-7-16, 2014/03/25 18:49:04]

Ah, you're adjusting the baseline of the texts to match? Thank you for the
explanation - that does make sense.

+  [denyPopUpButton setFrameOrigin:NSMakePoint(
+      NSMinX([translateButton frame]) - denyPopUpButtonFrame.size.width
+      - kRelatedControlHorizontalSpacing,
+      yPos + diffY)];
+
+  yPos += NSHeight([translateButton frame]);

[groby-ooo-7-16, 2014/03/21 01:21:42]

You can roll these two into one line :)

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+  yPos += kUnrelatedControlVerticalSpacing;
+
+  [textLabel setFrameOrigin:NSMakePoint(0, yPos)];
+  [advancedLinkButton setFrameOrigin:NSMakePoint(

[groby-ooo-7-16, 2014/03/21 01:21:42]

Are you sure you don't want any horizontal spacing between the label and the
button?

And tiny nit: In this case it's identical, but you probably want NSMaxX, not
NSWidth. It is a little bit cleaner in case the label is ever not at origin.x ==
0./

[hajimehoshi, 2014/03/24 07:16:30]

Yes because the link seems like a text and I think it is natural to put texts
side by side without space.
Done.

[groby-ooo-7-16, 2014/03/25 18:49:04]

I'm OK with this, but out of curiosity: From the mocks, it seems there's a space
in between. But I assume they're screenshots you made using your code? 

[hajimehoshi, 2014/03/26 11:06:05]

Your assuming is right. I don't put any space, but there seems space between the
texts. I'm not familiar with the coordinate system on Mac OS X, but maybe the
same thing is happaning to the buttons we're discussing about above.

[groby-ooo-7-16, 2014/03/27 00:39:39]

This is strange, but that doesn't impact the CL :)

As a side note: If you are ever curious about what frames are used and where
they are - are you familiar with F-Script? It's an excellent tool for looking
where the actual frames are.
(http://www.chromium.org/developers/f-script-anywhere).

[hajimehoshi, 2014/03/27 10:37:28]

I didn't know that. Thank you for the information!

+      NSWidth([textLabel frame]), yPos)];
+
+  yPos += NSHeight([textLabel frame]);

[groby-ooo-7-16, 2014/03/21 01:21:42]

It's probably shorter to skip everything until the return, and replace with:

[view setFrameSize:NSMakeSize(contentWidth, NSMaxY([textLabel frame]);

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+
+  contentFrame = [view frame];
+  contentFrame.size.height = yPos;
+  [view setFrame:contentFrame];
+
+  return view;
+}
+
+- (NSView*)createViewTranslating {
+  CGFloat contentWidth = kWindowWidth - 2 * kFramePadding;
+  NSRect contentFrame = NSMakeRect(
+      kFramePadding,
+      kFramePadding,
+      contentWidth,
+      1);
+  NSView* view = [[NSView alloc] initWithFrame:contentFrame];
+
+  NSString* message =
+      l10n_util::GetNSStringWithFixup(IDS_TRANSLATE_BUBBLE_TRANSLATING);
+  NSTextField* textLabel = [self addText:message
+                                withSize:[NSFont smallSystemFontSize]
+                                    bold:NO
+                                  toView:view];
+  NSString* title =
+      l10n_util::GetNSStringWithFixup(IDS_TRANSLATE_BUBBLE_REVERT);
+  NSButton* showOriginalButton =
+      [self addButton:title
+               target:self
+               action:@selector(handleShowOriginalButtonPressed)
+               toView:view];
+  [showOriginalButton setEnabled:NO];
+
+  // Layout
+  CGFloat yPos = 0;
+
+  [showOriginalButton setFrameOrigin:NSMakePoint(
+      contentWidth - NSWidth([showOriginalButton frame]), yPos)];
+
+  yPos += NSHeight([showOriginalButton frame]);
+  yPos += kUnrelatedControlVerticalSpacing;
+
+  [textLabel setFrameOrigin:NSMakePoint(0, yPos)];
+
+  yPos += NSHeight([textLabel frame]);

[groby-ooo-7-16, 2014/03/21 01:21:42]

it seems most of your controls are only vertically stacked, with fixed spacing -
if you want to, maybe it might be worth the effort to change
l10n_util::VerticallyReflowGroup to take a spacing parameter.

(No need to, though. And even if you want to do it, I'd suggest a follow up CL)

[hajimehoshi, 2014/03/24 07:16:30]

Thanks. I'll do this if I have time. I think other tasks like adding tests have
higher priority.

[groby-ooo-7-16, 2014/03/25 18:49:04]

That is fine. 

+
+  contentFrame = [view frame];

[groby-ooo-7-16, 2014/03/21 01:21:42]

See comment about setFrameSize: above. 

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+  contentFrame.size.height = yPos;
+  [view setFrame:contentFrame];
+
+  return view;
+}
+
+- (NSView*)createViewAfterTranslate {
+  CGFloat contentWidth = kWindowWidth - 2 * kFramePadding;

[groby-ooo-7-16, 2014/03/21 01:21:42]

As above, maybe have a file-wide kContentWidth. In general, many of the
suggestions from createViewBeforeTranslate can probably be applied to all the
createViewXYZ functions.

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+  NSRect contentFrame = NSMakeRect(
+      kFramePadding,
+      kFramePadding,
+      contentWidth,
+      1);
+  NSView* view = [[NSView alloc] initWithFrame:contentFrame];
+
+  NSString* message =
+      l10n_util::GetNSStringWithFixup(IDS_TRANSLATE_BUBBLE_TRANSLATED);
+  NSTextField* textLabel = [self addText:message
+                                withSize:[NSFont smallSystemFontSize]
+                                    bold:NO
+                                  toView:view];
+  message = l10n_util::GetNSStringWithFixup(IDS_TRANSLATE_BUBBLE_ADVANCED);
+  NSButton* advancedLinkButton =
+      [self addLinkButtonWithText:message
+                           target:self
+                           action:@selector(handleAdvancedLinkButtonPressed)
+                           toView:view];
+  NSString* title =
+      l10n_util::GetNSStringWithFixup(IDS_TRANSLATE_BUBBLE_REVERT);
+  NSButton* showOriginalButton =
+      [self addButton:title
+               target:self
+               action:@selector(handleShowOriginalButtonPressed)
+               toView:view];
+
+  // Layout
+  CGFloat yPos = 0;
+
+  [showOriginalButton setFrameOrigin:NSMakePoint(
+      contentWidth - NSWidth([showOriginalButton frame]), yPos)];
+
+  yPos += NSHeight([showOriginalButton frame]);
+  yPos += kUnrelatedControlVerticalSpacing;
+
+  [textLabel setFrameOrigin:NSMakePoint(0, yPos)];
+  [advancedLinkButton setFrameOrigin:NSMakePoint(
+      NSWidth([textLabel frame]), yPos)];
+
+  yPos += NSHeight([textLabel frame]);
+
+  contentFrame = [view frame];
+  contentFrame.size.height = yPos;
+  [view setFrame:contentFrame];
+
+  return view;
+}
+
+- (NSView*)createViewError {
+  CGFloat contentWidth = kWindowWidth - 2 * kFramePadding;
+  NSRect contentFrame = NSMakeRect(
+      kFramePadding,
+      kFramePadding,
+      contentWidth,
+      1);
+  NSView* view = [[NSView alloc] initWithFrame:contentFrame];
+
+  // TODO(hajimehoshi): Implement this.
+
+  return view;
+}
+
+- (NSView*)createViewAdvanced {
+  CGFloat contentWidth = kWindowWidth - 2 * kFramePadding;
+  NSRect contentFrame = NSMakeRect(
+      kFramePadding,
+      kFramePadding,
+      contentWidth,
+      1);
+  NSView* view = [[NSView alloc] initWithFrame:contentFrame];
+
+  NSString* title = l10n_util::GetNSStringWithFixup(
+      IDS_TRANSLATE_BUBBLE_PAGE_LANGUAGE);
+  NSTextField* sourceLanguageLabel = [self addText:title
+                                      withSize:[NSFont smallSystemFontSize]
+                                          bold:NO
+                                        toView:view];
+  title = l10n_util::GetNSStringWithFixup(
+      IDS_TRANSLATE_BUBBLE_TRANSLATION_LANGUAGE);
+  NSTextField* targetLanguageLabel = [self addText:title
+                                          withSize:[NSFont smallSystemFontSize]
+                                              bold:NO
+                                            toView:view];
+
+  // combobox
+  int sourceDefaultIndex = model_->GetOriginalLanguageIndex();
+  int targetDefaultIndex = model_->GetTargetLanguageIndex();
+  sourceLanguageComboboxModel_.reset(
+      new LanguageComboboxModel(sourceDefaultIndex, model_.get()));
+  targetLanguageComboboxModel_.reset(
+      new LanguageComboboxModel(targetDefaultIndex, model_.get()));
+  SEL action = @selector(handleSourceLanguagePopUpButtonSelectedItemChanged:);
+  NSPopUpButton* sourcePopUpButton =
+      [self addPopUpButton:sourceLanguageComboboxModel_.get()
+                    target:self
+                    action:action
+                    toView:view];
+  action = @selector(handleTargetLanguagePopUpButtonSelectedItemChanged:);
+  NSPopUpButton* targetPopUpButton =
+      [self addPopUpButton:targetLanguageComboboxModel_.get()
+                    target:self
+                    action:action
+                    toView:view];
+
+  // 'Always translate' checkbox
+  BOOL isIncognitoWindow = webContents_ ?
+      webContents_->GetBrowserContext()->IsOffTheRecord() : NO;
+  if (!isIncognitoWindow) {
+    NSString* title =
+        l10n_util::GetNSStringWithFixup(IDS_TRANSLATE_BUBBLE_ALWAYS);
+    alwaysTranslateCheckbox_ = [self addCheckbox:title
+                                          toView:view];
+  }
+
+  // Buttons
+  advancedDoneButton_ =
+      [self addButton:l10n_util::GetNSStringWithFixup(IDS_DONE)
+               target:self
+               action:@selector(handleDoneButtonPressed)
+               toView:view];
+  advancedCancelButton_ =
+      [self addButton:l10n_util::GetNSStringWithFixup(IDS_CANCEL)
+               target:self
+               action:@selector(handleCancelButtonPressed)
+               toView:view];
+
+  // Layout
+  CGFloat textLabelWidth = NSWidth([sourceLanguageLabel frame]);
+  if (textLabelWidth < NSWidth([targetLanguageLabel frame]))
+    textLabelWidth = NSWidth([targetLanguageLabel frame]);
+
+  CGFloat yPos = 0;
+
+  [advancedDoneButton_ setFrameOrigin:NSMakePoint(0, yPos)];
+  [advancedCancelButton_ setFrameOrigin:NSMakePoint(0, yPos)];
+
+  yPos += NSHeight([advancedDoneButton_ frame]);
+  yPos += kUnrelatedControlVerticalSpacing;
+
+  if (alwaysTranslateCheckbox_) {
+    [alwaysTranslateCheckbox_ setFrameOrigin:NSMakePoint(textLabelWidth, yPos)];
+
+    yPos += NSHeight([alwaysTranslateCheckbox_ frame]);
+    yPos += kRelatedControlVerticalSpacing;
+  }
+
+  CGFloat diffY = [[sourcePopUpButton cell]

[groby-ooo-7-16, 2014/03/21 01:21:42]

As above, I'm not sure why you need the cell's titleRect to place items. Would
you mind explaining the layout?

[hajimehoshi, 2014/03/24 07:16:30]

Same reason as above.

+                   titleRectForBounds:[sourcePopUpButton bounds]].origin.y;
+
+  [targetLanguageLabel setFrameOrigin:NSMakePoint(
+      textLabelWidth - NSWidth([targetLanguageLabel frame]), yPos + diffY)];
+
+  NSRect frame = [targetPopUpButton frame];
+  frame.origin.x = textLabelWidth;

[groby-ooo-7-16, 2014/03/21 01:21:42]

Please use frame.origin = NSMakePoint(textLabelWidth, yPos) instead of
individual member assignments.

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+  frame.origin.y = yPos;
+  frame.size.width = (kWindowWidth - 2 * kFramePadding) - textLabelWidth;
+  [targetPopUpButton setFrame:frame];
+
+  yPos += NSHeight([targetPopUpButton frame]);
+  yPos += kRelatedControlVerticalSpacing;
+
+  [sourceLanguageLabel setFrameOrigin:NSMakePoint(
+      textLabelWidth - NSWidth([sourceLanguageLabel frame]), yPos + diffY)];
+
+  frame = [sourcePopUpButton frame];
+  frame.origin.x = textLabelWidth;

[groby-ooo-7-16, 2014/03/21 01:21:42]

NSMakePoint, please.

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+  frame.origin.y = yPos;
+  frame.size.width = (kWindowWidth - 2 * kFramePadding) - textLabelWidth;

[groby-ooo-7-16, 2014/03/21 01:21:42]

You can probably just use NSWidth([targetPopUpButton frame]) - you want them to
be the same size, correct?

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+  [sourcePopUpButton setFrame:frame];
+
+  yPos += NSHeight([sourcePopUpButton frame]);
+
+  contentFrame = [view frame];
+  contentFrame.size.height = yPos;
+  [view setFrame:contentFrame];
+
+  [self updateAdvancedView];
+
+  return view;
+}
+
+- (void)updateAdvancedView {
+  if (alwaysTranslateCheckbox_) {

[groby-ooo-7-16, 2014/03/21 01:21:42]

No need to if-check - you can send messages to nil, that's perfectly OK.

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+    NSInteger state = model_->ShouldAlwaysTranslate() ? NSOnState : NSOffState;
+    [alwaysTranslateCheckbox_ setState:state];
+  }
+
+  NSString* title;
+  if (model_->IsPageTranslatedInCurrentLanguages())
+    title = l10n_util::GetNSStringWithFixup(IDS_DONE);
+  else
+    title = l10n_util::GetNSStringWithFixup(IDS_TRANSLATE_BUBBLE_ACCEPT);
+  [advancedDoneButton_ setTitle:title];
+  [advancedDoneButton_ sizeToFit];
+
+  NSRect frame = [advancedDoneButton_ frame];
+  frame.origin.x = (kWindowWidth - 2 * kFramePadding) - frame.size.width;
+  [advancedDoneButton_ setFrame:frame];

[groby-ooo-7-16, 2014/03/21 01:21:42]

setFrameOrigin, please, since you don't change size.

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+
+  frame = [advancedCancelButton_ frame];
+  frame.origin.x = NSMinX([advancedDoneButton_ frame]) - frame.size.width
+      - kRelatedControlHorizontalSpacing;
+  [advancedCancelButton_ setFrame:frame];

[groby-ooo-7-16, 2014/03/21 01:21:42]

setFrameOrigin, please. And NSWidth(frame) instead of frame.size.width - if
possible, do not refer to struct members of NSRect by name.

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+}
+
+// Create a new text field and add it to the given array of subviews.
+// The array will retain a reference to the object.
+- (NSTextField*)addText:(NSString*)text
+               withSize:(CGFloat)fontSize

[groby-ooo-7-16, 2014/03/21 01:21:42]

You seem to call this always with [NSFont smallSystemFontSize] - can you get rid
of that parameter?

Same thing for bold: - it seems to be always NO

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+                   bold:(BOOL)bold
+                 toView:(NSView*)view {
+  CGFloat width = NSWidth([view frame]);
+  NSRect frame = NSMakeRect(0, 0, width, 100);
+  base::scoped_nsobject<NSTextField> textField(
+      [[NSTextField alloc] initWithFrame:frame]);

[groby-ooo-7-16, 2014/03/21 01:21:42]

Since you call -sizeToFit at the end anyways, you could just use
-initWithFrame:NSZeroRect

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+  [textField setEditable:NO];
+  [textField setSelectable:YES];
+  [textField setDrawsBackground:NO];
+  [textField setBezeled:NO];
+  [textField setStringValue:text];
+  NSFont* font = bold ? [NSFont boldSystemFontOfSize:fontSize]
+                      : [NSFont systemFontOfSize:fontSize];
+  [textField setFont:font];
+
+  frame = [textField frame];
+  frame.size.height +=
+     [GTMUILocalizerAndLayoutTweaker sizeToFitFixedWidthTextField:
+         textField];

[groby-ooo-7-16, 2014/03/21 01:21:42]

Since you call sizeToFit later, do you need to do the frame size adjustment
here?

Also, if you do need this adjustment and remove the later sizeToFit, no need to
do the -setFrame: call. -sizeToFitFixedWidthTextField: already adjusts the frame
for |textField|.

[hajimehoshi, 2014/03/24 07:16:30]

Oh, calling GTM.. wasn't needed... Done.

+  [textField setFrame:frame];
+
+  [textField setAutoresizingMask:NSViewWidthSizable];
+  [view addSubview:textField.get()];
+
+  [textField sizeToFit];
+  return textField.get();
+}
+
+- (NSButton*)addLinkButtonWithText:(NSString*)text
+                            target:(id)target

[groby-ooo-7-16, 2014/03/21 01:21:42]

Since |target| is always self, can you remove that parameter?

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+                            action:(SEL)action
+                            toView:(NSView*)view {
+  base::scoped_nsobject<NSButton> button(
+      [[NSButton alloc] initWithFrame:NSZeroRect]);
+  base::scoped_nsobject<HyperlinkButtonCell> cell(
+      [[HyperlinkButtonCell alloc] initTextCell:text]);
+  [cell setControlSize:NSSmallControlSize];
+  [button setCell:cell.get()];

[groby-ooo-7-16, 2014/03/21 01:21:42]

No need to set the cell - you can just do [HyperLinkButtonCell
buttonWithString:text] to get a button with the proper cell, and an already set
title.

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+
+  [button setButtonType:NSMomentaryPushInButton];
+  [button setBezelStyle:NSRegularSquareBezelStyle];
+  [button sizeToFit];
+  if (target != nil && action != nil) {

[groby-ooo-7-16, 2014/03/21 01:21:42]

You can just set target and action - it's OK for them to be nil. (A nil target
means it tries to find a responder along the first responder chain, a nil action
just means nothing happens, independent of target)

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+    [button setTarget:target];
+    [button setAction:action];
+  }
+
+  [view addSubview:button.get()];
+
+  return button.get();
+}
+
+- (NSButton*)addButton:(NSString*)title
+                target:(id)target

[groby-ooo-7-16, 2014/03/21 01:21:42]

See above

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+                action:(SEL)action
+                toView:(NSView*)view {
+  base::scoped_nsobject<NSButton> button(
+      [[NSButton alloc] initWithFrame:NSZeroRect]);
+  [button setFont:[NSFont systemFontOfSize:[NSFont smallSystemFontSize]]];
+  [button setTitle:title];
+  [button setBezelStyle:NSRoundedBezelStyle];
+  [[button cell] setControlSize:NSSmallControlSize];
+  [button sizeToFit];
+  if (target != nil && action != nil) {

[groby-ooo-7-16, 2014/03/21 01:21:42]

See above

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+    [button setTarget:target];
+    [button setAction:action];
+  }
+
+  [view addSubview:button.get()];
+
+  return button.get();
+}
+
+- (NSButton*)addCheckbox:(NSString*)title
+                  toView:(NSView*)view {
+  base::scoped_nsobject<NSButton> button(
+      [[NSButton alloc] initWithFrame:NSZeroRect]);
+  [button setFont:[NSFont systemFontOfSize:[NSFont smallSystemFontSize]]];
+  [button setTitle:title];
+  [[button cell] setControlSize:NSSmallControlSize];
+  [button setButtonType:NSSwitchButton];
+  [button sizeToFit];
+
+  [view addSubview:button.get()];
+
+  return button.get();
+}
+
+- (NSPopUpButton*)addPopUpButton:(ui::ComboboxModel*)model
+                          target:(id)target
+                          action:(SEL)action
+                          toView:(NSView*)view {
+  base::scoped_nsobject<NSPopUpButton> button(
+      [[NSPopUpButton alloc] initWithFrame:NSZeroRect pullsDown:NO]);
+  [button setFont:[NSFont systemFontOfSize:[NSFont smallSystemFontSize]]];
+  [button setBordered:YES];
+  [[button cell] setControlSize:NSSmallControlSize];
+  if (target != nil && action != nil) {

[groby-ooo-7-16, 2014/03/21 01:21:42]

See above

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+    [button setTarget:target];
+    [button setAction:action];
+  }
+
+  for (int i = 0; i < model->GetItemCount(); ++i)
+    [button addItemWithTitle:base::SysUTF16ToNSString(model->GetItemAt(i))];
+  [button selectItemAtIndex:model->GetDefaultIndex()];
+
+  [button sizeToFit];
+
+  [view addSubview:button.get()];
+
+  return button.get();
+}
+
+- (void)handleTranslateButtonPressed {
+  translateExecuted_ = YES;
+  model_->Translate();
+}
+
+- (void)handleNopeButtonPressed {
+  [self close];
+}
+
+- (void)handleDoneButtonPressed {
+  if (alwaysTranslateCheckbox_) {
+    model_->SetAlwaysTranslate(
+        [alwaysTranslateCheckbox_ state] == NSOnState);
+  }
+  if (model_->IsPageTranslatedInCurrentLanguages()) {
+    model_->GoBackFromAdvanced();
+    [self performLayout];
+  } else {
+    translateExecuted_ = true;
+    model_->Translate();
+    [self switchView:TranslateBubbleModel::VIEW_STATE_TRANSLATING];
+  }
+}
+
+- (void)handleCancelButtonPressed {
+  model_->GoBackFromAdvanced();
+  [self performLayout];
+}
+
+- (void)handleShowOriginalButtonPressed {
+  model_->RevertTranslation();
+  [self close];
+}
+
+- (void)handleAdvancedLinkButtonPressed {
+  [self switchView:TranslateBubbleModel::VIEW_STATE_ADVANCED];
+}
+
+- (void)handleDenialPopUpButtonSelectedItemChanged:(id)sender {
+  NSPopUpButton* button = base::mac::ObjCCastStrict<NSPopUpButton>(sender);
+  switch ([button indexOfSelectedItem]) {

[groby-ooo-7-16, 2014/03/21 01:21:42]

You can directly set a target/action on the MenuItem, that would avoid the
switch here.

If you don't want to do that, I recommend not switching on the index, but
instead storing kIndexXYZ as a tag on the item. (I.e [item setTag:kIndexXYZ]
when you create, and switch ([[button selectedItem] tag]) here.

[hajimehoshi, 2014/03/24 07:16:30]

Done.

+    case TranslateDenialComboboxModel::kIndexNope:
+      break;
+    case TranslateDenialComboboxModel::kIndexNeverTranslateLanguage:
+      model_->SetNeverTranslateLanguage(true);
+      break;
+    case TranslateDenialComboboxModel::kIndexNeverTranslateSite:
+      model_->SetNeverTranslateSite(true);
+      break;
+  }
+  [self close];
+}
+
+- (void)handleSourceLanguagePopUpButtonSelectedItemChanged:(id)sender {
+  NSPopUpButton* button = base::mac::ObjCCastStrict<NSPopUpButton>(sender);
+  model_->UpdateOriginalLanguageIndex([button indexOfSelectedItem]);
+  [self updateAdvancedView];
+}
+
+- (void)handleTargetLanguagePopUpButtonSelectedItemChanged:(id)sender {
+  NSPopUpButton* button = base::mac::ObjCCastStrict<NSPopUpButton>(sender);
+  model_->UpdateTargetLanguageIndex([button indexOfSelectedItem]);
+  [self updateAdvancedView];
 }
 
 @end