Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
151283006:
Mac OS X: Show the Translate icon on Omnibox (Closed)
|
Created:
6 years, 10 months ago by hajimehoshi Modified:
6 years, 9 months ago CC:
chromium-reviews, kenjibaheux Base URL:
https://chromium.googlesource.com/chromium/src.git@issue-307352-translate-bubble-2 Visibility:
Public. |
DescriptionMac OS X: Show the Translate icon on Omnibox
This patch is the first step to implement the Translate bubble UX to
replace the current infobar. This patch fixes the Ominbox to show the
translate icon when a foreign page is loaded. This icon offers the
information to users if the page is translable or not, or if the page is
already translated or not. The icon turns blue when the page is
translated. As for screenshots, please see crbug/307352#c17.
By clicking the icon, the Translte bubble is shown. For now, the
Translate bubble is dummy and empty. I'll implement this and upload
another CL later.
Now this bubble is behind the flag --enable-translate-new-ux on Mac.
This bubble UX is already implemented and enabled by default on Aura
like Windows and Chrome OS.
BUG=307352
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256228
Patch Set 1 : . #
Total comments: 22
Patch Set 2 : Nico's review #
Total comments: 24
Patch Set 3 : (rebasing) #Patch Set 4 : groby's review #
Total comments: 11
Patch Set 5 : groby's review #
Total comments: 20
Patch Set 6 : groby and rsesek's review #Patch Set 7 : (rebasing) #
Total comments: 1
Patch Set 8 : (rebasing) #Patch Set 9 : Use kAnimateNone #Messages
Total messages: 43 (0 generated)
Can you take a look? Thank you in advance.
+kenjibaheux (CC)
This looks like it's mostly going in the right direction, good work! Please add a test for the translate bubble controller. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_cocoa.mm:287: [controller_ setTranslateIconToggled:is_lit ? YES : NO]; I think you can omit ` ? YES : NO` https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_controller.h (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller.h:242: - (void)setTranslateIconToggled:(BOOL)on; setTranslateIconVisible? setCurrentPageIsTranslated? https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/location_bar/translate_decoration.h (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/location_bar/translate_decoration.h:20: void SetToggled(bool on); SetOn or something. "Toggled" isn't something where it's clear what it being true or false means. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:447: locationBarView_->SetTranslateIconToggled(on ? true : false); No need for ` ? true : false` here either. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.h (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:18: @interface TranslateBubbleController : BaseBubbleController { Needs a class comment https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:23: // Whether the translation is acutually executed. typo "acutually" https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:33: errorType:(TranslateErrors::Type)errorType; `git cl format` produces + (void) showBubbleWithBrowserWindowController:(BrowserWindowController *)controller webContents:(content::WebContents *)webContents viewState: (TranslateBubbleModel::ViewState)viewState errorType:(TranslateErrors::Type)errorType; which looks ever so slightly nicer https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:18: const CGFloat kWindowWidth = 320; "const" has implicit internal linkage, no need for an unnamed namespace https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:26: + (void)showBubbleWithBrowserWindowController:(BrowserWindowController*)controller 80 cols https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:46: // FIXME(hajimehoshi): Set the initial languages correctly. fixme https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:64: webContents_ = webContents; You can't write to ivars before the `self = …` has run. An ivar access is implicitly the same as `self->webContents_ = …`, and if the super init returns a different self you shouldn't write to the old self.
Thank you! Added a unit test file, but I've not implemented this yet. Is it OK to implement this at the same time when implementing TranslateBubbleController? Now TranslateBubbleController is virtually empty. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_cocoa.mm:287: [controller_ setTranslateIconToggled:is_lit ? YES : NO]; On 2014/02/05 06:07:18, Nico wrote: > I think you can omit ` ? YES : NO` Done. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_controller.h (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller.h:242: - (void)setTranslateIconToggled:(BOOL)on; On 2014/02/05 06:07:18, Nico wrote: > setTranslateIconVisible? setCurrentPageIsTranslated? Done. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/location_bar/translate_decoration.h (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/location_bar/translate_decoration.h:20: void SetToggled(bool on); On 2014/02/05 06:07:18, Nico wrote: > SetOn or something. "Toggled" isn't something where it's clear what it being > true or false means. Done. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:447: locationBarView_->SetTranslateIconToggled(on ? true : false); On 2014/02/05 06:07:18, Nico wrote: > No need for ` ? true : false` here either. Done. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.h (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:18: @interface TranslateBubbleController : BaseBubbleController { On 2014/02/05 06:07:18, Nico wrote: > Needs a class comment Done. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:23: // Whether the translation is acutually executed. On 2014/02/05 06:07:18, Nico wrote: > typo "acutually" Done. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:33: errorType:(TranslateErrors::Type)errorType; On 2014/02/05 06:07:18, Nico wrote: > `git cl format` produces > > + (void) > showBubbleWithBrowserWindowController:(BrowserWindowController *)controller > webContents:(content::WebContents *)webContents > viewState: > (TranslateBubbleModel::ViewState)viewState > errorType:(TranslateErrors::Type)errorType; > > > which looks ever so slightly nicer Done. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:18: const CGFloat kWindowWidth = 320; On 2014/02/05 06:07:18, Nico wrote: > "const" has implicit internal linkage, no need for an unnamed namespace Done. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:26: + (void)showBubbleWithBrowserWindowController:(BrowserWindowController*)controller On 2014/02/05 06:07:18, Nico wrote: > 80 cols Done. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:46: // FIXME(hajimehoshi): Set the initial languages correctly. On 2014/02/05 06:07:18, Nico wrote: > fixme Lower cases? Done. https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:64: webContents_ = webContents; On 2014/02/05 06:07:18, Nico wrote: > You can't write to ivars before the `self = …` has run. An ivar access is > implicitly the same as `self->webContents_ = …`, and if the super init returns a > different self you shouldn't write to the old self. Done.
On 2014/02/05 11:08:02, hajimehoshi wrote: > Thank you! Added a unit test file, but I've not implemented this yet. Is it OK > to implement this at the same time when implementing TranslateBubbleController? > Now TranslateBubbleController is virtually empty. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/browser_window_cocoa.mm:287: [controller_ > setTranslateIconToggled:is_lit ? YES : NO]; > On 2014/02/05 06:07:18, Nico wrote: > > I think you can omit ` ? YES : NO` > > Done. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/browser_window_controller.h (right): > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/browser_window_controller.h:242: - > (void)setTranslateIconToggled:(BOOL)on; > On 2014/02/05 06:07:18, Nico wrote: > > setTranslateIconVisible? setCurrentPageIsTranslated? > > Done. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/location_bar/translate_decoration.h (right): > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/location_bar/translate_decoration.h:20: void > SetToggled(bool on); > On 2014/02/05 06:07:18, Nico wrote: > > SetOn or something. "Toggled" isn't something where it's clear what it being > > true or false means. > > Done. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm (right): > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:447: > locationBarView_->SetTranslateIconToggled(on ? true : false); > On 2014/02/05 06:07:18, Nico wrote: > > No need for ` ? true : false` here either. > > Done. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/translate/translate_bubble_controller.h (right): > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:18: @interface > TranslateBubbleController : BaseBubbleController { > On 2014/02/05 06:07:18, Nico wrote: > > Needs a class comment > > Done. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:23: // Whether > the translation is acutually executed. > On 2014/02/05 06:07:18, Nico wrote: > > typo "acutually" > > Done. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:33: > errorType:(TranslateErrors::Type)errorType; > On 2014/02/05 06:07:18, Nico wrote: > > `git cl format` produces > > > > + (void) > > showBubbleWithBrowserWindowController:(BrowserWindowController > *)controller > > webContents:(content::WebContents *)webContents > > viewState: > > (TranslateBubbleModel::ViewState)viewState > > errorType:(TranslateErrors::Type)errorType; > > > > > > which looks ever so slightly nicer > > Done. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:18: const > CGFloat kWindowWidth = 320; > On 2014/02/05 06:07:18, Nico wrote: > > "const" has implicit internal linkage, no need for an unnamed namespace > > Done. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:26: + > (void)showBubbleWithBrowserWindowController:(BrowserWindowController*)controller > On 2014/02/05 06:07:18, Nico wrote: > > 80 cols > > Done. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:46: // > FIXME(hajimehoshi): Set the initial languages correctly. > On 2014/02/05 06:07:18, Nico wrote: > > fixme > > Lower cases? Done. > > https://codereview.chromium.org/151283006/diff/60001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:64: > webContents_ = webContents; > On 2014/02/05 06:07:18, Nico wrote: > > You can't write to ivars before the `self = …` has run. An ivar access is > > implicitly the same as `self->webContents_ = …`, and if the super init returns > a > > different self you shouldn't write to the old self. > > Done. ping
groby kindly agreed to review this.
Filling in for Nico since he's OOO
At a first glance, this is doing well. But please add at least a test that displays the bubble, so we can immediately catch possible memory issues. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/DEPS (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/DEPS:2: "+components/translate/core/common", Not needed, since that's already stated in chrome/browser/DEPS https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:323: void LocationBarViewMac::SetTranslateIconToggled(bool on) { personal nit: SetTranslateIconLit, so it matches the "SetLit" call. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.h (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:27: // Whether the translation is actually executed. "isTranslated" might be a better name? https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:20: static TranslateBubbleController* translateBubbleController_ = NULL; Is there any way for an object to hold this controller, as opposed to a global? E.g. the ZoomBubbleController is stored on the ZoomDecoration. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:23: showBubbleWithBrowserWindowController:(BrowserWindowController*)controller Please keep return type and method name on same line. In general, I don't think you need to pass in the BWC - "showForParentWindow" should be enough. (And then -initForParentWindow) https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:31: if (translateBubbleController_->webContents_ == webContents && I'm curious - can translateBubbleController ever have different webContents? Shouldn't the bubble close whenever you navigate or switch tabs? https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:64: NSRect contentRect = NSMakeRect(0, 0, kWindowWidth, 100); Use ui::kWindowSizeDeterminedLater, and then set the size in performLayout https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:86: NSView* contentView = [[FlippedView alloc] initWithFrame:contentFrame]; Unless you need the FlippedView, I would skip this. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:93: [self retain]; The class is already retained through the alloc call. No need to add an extra retain. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:105: BrowserWindowController* controller = [self.parentWindow windowController]; nit:Since the rest of the file doesn't use dot notation, I'd prefer [self parentWindow] https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm:31: TEST_F(TranslateBubbleControllerTest, Show) { Please do add this test. See e.g. new_credit_card_bubble_cocoa_unittest.mm or zoom_bubble_controller_unittest.mm
Sorry for my late response. Thank you! https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/DEPS (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/DEPS:2: "+components/translate/core/common", On 2014/02/11 00:43:55, groby wrote: > Not needed, since that's already stated in chrome/browser/DEPS Done. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:323: void LocationBarViewMac::SetTranslateIconToggled(bool on) { On 2014/02/11 00:43:55, groby wrote: > personal nit: SetTranslateIconLit, so it matches the "SetLit" call. Done. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.h (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.h:27: // Whether the translation is actually executed. On 2014/02/11 00:43:55, groby wrote: > "isTranslated" might be a better name? It is intended to be true even when the page is translated and revert this once. I'll fix the comment. This is used to count a UMA whether the user declined the Translate bubble or not. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:20: static TranslateBubbleController* translateBubbleController_ = NULL; On 2014/02/11 00:43:55, groby wrote: > Is there any way for an object to hold this controller, as opposed to a global? > > E.g. the ZoomBubbleController is stored on the ZoomDecoration. Hmm, like StarDecoration, I implemented to show the bubble via the command |command_updater_|. It is because there are multiple paths to show the bubble. Of course one is clicking the icon and another is, for example, clicking the context menu to translate the page. Actually, I've already implemented it in this way for Translate bubble on Aura, so it'd be quite different from Aura implementation. Should TranslateDecoration own the TranslateBubble? https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:23: showBubbleWithBrowserWindowController:(BrowserWindowController*)controller On 2014/02/11 00:43:55, groby wrote: > Please keep return type and method name on same line. > > In general, I don't think you need to pass in the BWC - "showForParentWindow" > should be enough. (And then -initForParentWindow) Done. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:31: if (translateBubbleController_->webContents_ == webContents && On 2014/02/11 00:43:55, groby wrote: > I'm curious - can translateBubbleController ever have different webContents? > Shouldn't the bubble close whenever you navigate or switch tabs? No, translatebubbleController can have only one webContents. However, even though a bubble is shown, showBubbleWithBrowserWindowController can be called when the automatic translation is enabled. In this case, the current bubble should be closed. This 'if' clause is true when the user opens a page, shows the Translate bubble by clicking the icon and switches the advanced menu BEFORE the automatic translation starts. In this case, the webContents is not changed while showBubbleWithBrowserWindowController is called twice. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:64: NSRect contentRect = NSMakeRect(0, 0, kWindowWidth, 100); On 2014/02/11 00:43:55, groby wrote: > Use ui::kWindowSizeDeterminedLater, and then set the size in performLayout Done. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:86: NSView* contentView = [[FlippedView alloc] initWithFrame:contentFrame]; On 2014/02/11 00:43:55, groby wrote: > Unless you need the FlippedView, I would skip this. Removed this once. Done. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:93: [self retain]; On 2014/02/11 00:43:55, groby wrote: > The class is already retained through the alloc call. No need to add an extra > retain. Done. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:105: BrowserWindowController* controller = [self.parentWindow windowController]; On 2014/02/11 00:43:55, groby wrote: > nit:Since the rest of the file doesn't use dot notation, I'd prefer [self > parentWindow] Done. https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm:31: TEST_F(TranslateBubbleControllerTest, Show) { On 2014/02/11 00:43:55, groby wrote: > Please do add this test. See e.g. new_credit_card_bubble_cocoa_unittest.mm or > zoom_bubble_controller_unittest.mm Done.
Thank you for all the fixes - this looks great. Do you think it makes sense to move bookmarkBubbleController_ to the BrowserWindowController? https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:20: static TranslateBubbleController* translateBubbleController_ = NULL; On 2014/02/25 09:31:07, hajimehoshi wrote: > On 2014/02/11 00:43:55, groby wrote: > > Is there any way for an object to hold this controller, as opposed to a > global? > > > > E.g. the ZoomBubbleController is stored on the ZoomDecoration. > > Hmm, like StarDecoration, I implemented to show the bubble via the command > |command_updater_|. It is because there are multiple paths to show the bubble. > Of course one is clicking the icon and another is, for example, clicking the > context menu to translate the page. > > Actually, I've already implemented it in this way for Translate bubble on Aura, > so it'd be quite different from Aura implementation. > > Should TranslateDecoration own the TranslateBubble? I think you should stick with the CommandUpdater solution. I don't think this should be a global object, though. If you follow what the Bookmark bubble is doing, bookmarkBubbleController_ is stored on the BrowserWindowController. Maybe that's a better place? https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/translate_decoration.h (right): https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/translate_decoration.h:15: class TranslateDecoration : public ImageDecoration { Please add a class level comment. https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:80: translateExecuted_ = NO; No need to do this - Cocoa member vars are always initialized to 0 (or NO/nil) https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:91: - (void)windowWillClose:(NSNotification*)notification { If you move the BubbleController onto BrowserWindowController, you should handle windowWillClose there. https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm:57: base::MessageLoopForUI::current()->RunUntilIdle(); You probably want chrome::testing::NSRunLoopRunAllPending();
Thank you! https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/160001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:20: static TranslateBubbleController* translateBubbleController_ = NULL; On 2014/02/26 03:13:58, groby wrote: > On 2014/02/25 09:31:07, hajimehoshi wrote: > > On 2014/02/11 00:43:55, groby wrote: > > > Is there any way for an object to hold this controller, as opposed to a > > global? > > > > > > E.g. the ZoomBubbleController is stored on the ZoomDecoration. > > > > Hmm, like StarDecoration, I implemented to show the bubble via the command > > |command_updater_|. It is because there are multiple paths to show the bubble. > > Of course one is clicking the icon and another is, for example, clicking the > > context menu to translate the page. > > > > Actually, I've already implemented it in this way for Translate bubble on > Aura, > > so it'd be quite different from Aura implementation. > > > > Should TranslateDecoration own the TranslateBubble? > > > I think you should stick with the CommandUpdater solution. I don't think this > should be a global object, though. If you follow what the Bookmark bubble is > doing, bookmarkBubbleController_ is stored on the BrowserWindowController. Maybe > that's a better place? > Thanks. I agree to move this. Done. https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/translate_decoration.h (right): https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/translate_decoration.h:15: class TranslateDecoration : public ImageDecoration { On 2014/02/26 03:13:58, groby wrote: > Please add a class level comment. Done. https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:25: if (translateBubbleController_) { I remembered that this logic was very similar to TranslateBubbleView::ShowBubble's. I add the comment for now, and I'm going to refactor this asap. https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:80: translateExecuted_ = NO; On 2014/02/26 03:13:58, groby wrote: > No need to do this - Cocoa member vars are always initialized to 0 (or NO/nil) Done. https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:91: - (void)windowWillClose:(NSNotification*)notification { On 2014/02/26 03:13:58, groby wrote: > If you move the BubbleController onto BrowserWindowController, you should handle > windowWillClose there. Done. https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm:57: base::MessageLoopForUI::current()->RunUntilIdle(); On 2014/02/26 03:13:58, groby wrote: > You probably want chrome::testing::NSRunLoopRunAllPending(); Hmm, NSRunLoopRunAllPending couldn't work for this test (the following EXPECT_FALSE fails.)
LGTM w/ a few tiny nits. Thank you! https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm:57: base::MessageLoopForUI::current()->RunUntilIdle(); On 2014/02/26 09:18:18, hajimehoshi wrote: > On 2014/02/26 03:13:58, groby wrote: > > You probably want chrome::testing::NSRunLoopRunAllPending(); > > Hmm, NSRunLoopRunAllPending couldn't work for this test (the following > EXPECT_FALSE fails.) Odd. I can't see anything that would require PostTask's to complete. Would you mind filing a bug (assign to me)? https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/translate_decoration.mm (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/translate_decoration.mm:22: is_lit_ = on; You can probably get rid of the is_lit_ member - it's used nowhere else. (Unless you plan to use it in later CL's of course) https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:19: - (const content::WebContents*)webContents { Since this property only needs a getter that returns the contents of an ivar unmodified, you could write this instead as @synthesize webContents = webContents_;
A few drive-by nits, too. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:1713: // fixme(hajimehoshi): The similar logic exists at TranslateBubbleView:: TODO not fixme https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:1733: // fixme(hajimehoshi): Set the initial languages correctly. TODO https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:109: NSPoint GetTranslateBubblePoint() const; Comment please. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/translate_decoration.mm (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/translate_decoration.mm:32: bool TranslateDecoration::AcceptsMousePress() { return true; } Put the body on its own line in .cc files. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:19: - (const content::WebContents*)webContents { Getters come after -init https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:72: // fixme(hajimehoshi): Implement this. TODO https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:76: // fixme(hajimehoshi): Now this shows just an empty bubble. Implement this. TODO https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm:31: virtual void TearDown() OVERRIDE { Don't need to OVERRIDE if there is no work to be done.
Thank you! https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151283006/diff/290001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm:57: base::MessageLoopForUI::current()->RunUntilIdle(); On 2014/02/27 19:19:02, groby wrote: > On 2014/02/26 09:18:18, hajimehoshi wrote: > > On 2014/02/26 03:13:58, groby wrote: > > > You probably want chrome::testing::NSRunLoopRunAllPending(); > > > > Hmm, NSRunLoopRunAllPending couldn't work for this test (the following > > EXPECT_FALSE fails.) > > Odd. I can't see anything that would require PostTask's to complete. Would you > mind filing a bug (assign to me)? Sure. I'll comment about this. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller.mm (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:1713: // fixme(hajimehoshi): The similar logic exists at TranslateBubbleView:: On 2014/02/27 20:45:01, rsesek wrote: > TODO not fixme Done. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller.mm:1733: // fixme(hajimehoshi): Set the initial languages correctly. On 2014/02/27 20:45:01, rsesek wrote: > TODO Done. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:109: NSPoint GetTranslateBubblePoint() const; On 2014/02/27 20:45:01, rsesek wrote: > Comment please. Done. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/translate_decoration.mm (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/translate_decoration.mm:22: is_lit_ = on; On 2014/02/27 19:19:02, groby wrote: > You can probably get rid of the is_lit_ member - it's used nowhere else. (Unless > you plan to use it in later CL's of course) Done (Removed. Thanks!) https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/translate_decoration.mm:32: bool TranslateDecoration::AcceptsMousePress() { return true; } On 2014/02/27 20:45:01, rsesek wrote: > Put the body on its own line in .cc files. Done. (You meant .mm files?) https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:19: - (const content::WebContents*)webContents { On 2014/02/27 20:45:01, rsesek wrote: > Getters come after -init Done. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:19: - (const content::WebContents*)webContents { On 2014/02/27 19:19:02, groby wrote: > Since this property only needs a getter that returns the contents of an ivar > unmodified, you could write this instead as > > @synthesize webContents = webContents_; Done. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:72: // fixme(hajimehoshi): Implement this. On 2014/02/27 20:45:01, rsesek wrote: > TODO Done. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:76: // fixme(hajimehoshi): Now this shows just an empty bubble. Implement this. On 2014/02/27 20:45:01, rsesek wrote: > TODO Done. https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm (right): https://codereview.chromium.org/151283006/diff/310001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm:31: virtual void TearDown() OVERRIDE { On 2014/02/27 20:45:01, rsesek wrote: > Don't need to OVERRIDE if there is no work to be done. Done.
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/151283006/350001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/151283006/350001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
On 2014/02/28 11:32:11, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: mac_chromium_rel Hmm, I couldn't understand what was happening on the build bot while the tests passed on my local machine... This may be flaky. Could you give me some advice?
It looks like the window is not invoking its windowWillClose notifications, but why is not clear to me. I'm reluctant to call it a flake, since the test has been retried three times. I've kicked of another mac_chromium_rel to see if it reliably fails. As soon as I'm back in the office, I'll try the patch on my machine to see what's happening... https://codereview.chromium.org/151283006/diff/350001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm (right): https://codereview.chromium.org/151283006/diff/350001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/translate/translate_bubble_controller.mm:19: - (id)initWithParentWindow:(BrowserWindowController*)controller You might want to pass an actual window here :)
A quick update from me - I'm still up in MTV, and will return to my desk tomorrow. Unfortunately, I don't have an opportunity to check out the problem before that. Thank you for your patience!
On 2014/03/04 19:52:39, groby wrote: > A quick update from me - I'm still up in MTV, and will return to my desk > tomorrow. Unfortunately, I don't have an opportunity to check out the problem > before that. > > Thank you for your patience! No problem. Thank you!
This is exceedingly odd. 1) It passes fine on my local machine 2) It works fine with NSRunLoopRunAllPending on my local machine nico, rsesek: Any idea what's going on here? (Just out of curiosity, I'm running a tryjob with NSRunLoopRunAllPending at https://codereview.chromium.org/183853036)
On 2014/03/06 04:37:58, groby wrote: > This is exceedingly odd. > > 1) It passes fine on my local machine > 2) It works fine with NSRunLoopRunAllPending on my local machine > > nico, rsesek: Any idea what's going on here? > > (Just out of curiosity, I'm running a tryjob with NSRunLoopRunAllPending at > https://codereview.chromium.org/183853036) This sounds like a difference between the way the bots run the tests and the way they're run locally. What if you invoke the test suite using the same command line as the bots?
On 2014/03/06 18:15:13, rsesek wrote: > On 2014/03/06 04:37:58, groby wrote: > > This is exceedingly odd. > > > > 1) It passes fine on my local machine > > 2) It works fine with NSRunLoopRunAllPending on my local machine > > > > nico, rsesek: Any idea what's going on here? > > > > (Just out of curiosity, I'm running a tryjob with NSRunLoopRunAllPending at > > https://codereview.chromium.org/183853036) > > This sounds like a difference between the way the bots run the tests and the way > they're run locally. What if you invoke the test suite using the same command > line as the bots? Done that, and they work just fine. I'm also slightly concerned that NSRunLoopRunAllPending exposes different behavior on dev machines. I'd assume there's a subtle messaging issue, but beats me if I can spot it... Currently running the test on repeat, to see if that exposes something.
Nope. 10,000 local runs, all fine. So I assume it's a trybot setup thing.
10.6 sdk? On Thu, Mar 6, 2014 at 4:25 PM, <groby@chromium.org> wrote: > Nope. 10,000 local runs, all fine. So I assume it's a trybot setup thing. > > https://codereview.chromium.org/151283006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/07 00:25:25, Nico wrote: > 10.6 sdk? > > > On Thu, Mar 6, 2014 at 4:25 PM, <mailto:groby@chromium.org> wrote: > > > Nope. 10,000 local runs, all fine. So I assume it's a trybot setup thing. > > > > https://codereview.chromium.org/151283006/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Thanks. If so, can we update the build bots, or should we keep using them to support the old versions?
> I'm also slightly concerned that
> NSRunLoopRunAllPending exposes different behavior on dev machines.
On my local machine, NSRunLoopRunAllPendings still doesn't work...
Here is the command and the result:
hajimehoshi-macpro:src hajimehoshi$ ./out/Debug/unit_tests
--gtest_filter=TranslateBubbleControllerTest.*
objc[61584]: Class MockCrApp is implemented in both
/Users/hajimehoshi/chromium/src/out/Debug/./libblink_web.dylib and
/Users/hajimehoshi/chromium/src/./out/Debug/unit_tests. One of the two will be
used. Which one is undefined.
IMPORTANT DEBUGGING NOTE: batches of tests are run inside their
own process. For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with
--single-process-tests.
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
objc[61585]: Class MockCrApp is implemented in both
/Users/hajimehoshi/chromium/src/out/Debug/./libblink_web.dylib and
/Users/hajimehoshi/chromium/src/./out/Debug/unit_tests. One of the two will be
used. Which one is undefined.
Note: Google Test filter = TranslateBubbleControllerTest.ShowAndClose
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from TranslateBubbleControllerTest
[ RUN ] TranslateBubbleControllerTest.ShowAndClose
../../chrome/browser/ui/cocoa/translate/translate_bubble_controller_unittest.mm:66:
Failure
Value of: bubble
Actual: true
Expected: false
[ FAILED ] TranslateBubbleControllerTest.ShowAndClose (380 ms)
[----------] 1 test from TranslateBubbleControllerTest (382 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (382 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] TranslateBubbleControllerTest.ShowAndClose
1 FAILED TEST
YOU HAVE 40 DISABLED TESTS
[0310/173435:ERROR:kill_posix.cc(190)] Unable to terminate process group 61585:
No such process
[1/1] TranslateBubbleControllerTest.ShowAndClose (380 ms)
1 test failed:
TranslateBubbleControllerTest.ShowAndClose
Tests took 0 seconds.
We can't update the bots, since we need to support 10.6 FWIW, I'm building against the 10.8 SDK. (Run "build/mac/find_sdk.py 10.6" to find out your SDK version) If you still have 10.6 installed, you might be building against 10.6 too, which would give us an additional clue...
Arrrg. I'm terribly sorry I didn't remember this earlier: the bubble window
animates. Which means it doesn't close immediately, but after the animation
completes. And so this test depends on timing, making it flaky.
You can disable that by adding
WithNoAnimation at_all;
as a member var declaration in the unit test. If you prefer to explicitly
disable that in the test, instead do:
InfoBubbleWindow* window = (InfoBubbleWindow*)[bubble window];
[window setAllowedAnimations:info_bubble::kAnimateNone];
> FWIW, I'm building against the 10.8 SDK. (Run "build/mac/find_sdk.py 10.6" to > find out your SDK version) ./build/mac/find_sdk.py 10.6 showed 10.7 (not 10.6), and that of 10.8 showed 10.8. I'm not sure what they meant. Am I using 10.8? > Arrrg. I'm terribly sorry I didn't remember this earlier: the bubble window > animates. Which means it doesn't close immediately, but after the animation > completes. And so this test depends on timing, making it flaky. > > You can disable that by adding > > WithNoAnimation at_all; > > as a member var declaration in the unit test. If you prefer to explicitly > disable that in the test, instead do: > > InfoBubbleWindow* window = (InfoBubbleWindow*)[bubble window]; > [window setAllowedAnimations:info_bubble::kAnimateNone]; kAnimateNone worked fine while WithNoAnimation didn't work... Anyway, I'll commit with kAnimateNone this time. Thanks a lot!
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/151283006/390001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by hajimehoshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/151283006/390001
Message was sent while issue was closed.
Change committed as 256228 |
