Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
435863002:
Fix some infobar problems. (Closed)
DescriptionFix the info bar doesn't get redrawn when its frame is changed. Also fix the
height of top infobar arrow doesn't changed through animation when the status
of bookmark bar changes from show to hide/detach.
Set the view of info bar to be auto resizable and change the height of top infobar
arrow when the its frame is changed.
BUG=382141
TEST= ./browser_tests --
gtest_filter="BrowserWindowControllerTest.InfoBarTipStrechedWhenBookmarkBar
StatusChanged", manually tested.
Committed: https://crrev.com/7d9f474ac604f3c7a40afbb808e2ed61fe460442
Cr-Commit-Position: refs/heads/master@{#292317}
Patch Set 1 #Patch Set 2 : Update by adding a line #Patch Set 3 : Updated #
Total comments: 2
Patch Set 4 : Updated CL #
Total comments: 5
Patch Set 5 : Animated tip #
Total comments: 1
Patch Set 6 : Change the naming #Patch Set 7 : Add browser tests #Patch Set 8 : Add browser tests #
Total comments: 9
Patch Set 9 : Fix some naming problems #Patch Set 10 : Using static_cast #
Total comments: 2
Patch Set 11 : Add 4 spaces indent #
Messages
Total messages: 36 (0 generated)
Would you help me to review this CL, thanks!
I'm not familiar with infobar code, but this doesn't seem right to me. Is the arrow size really supposed to change when the bookmark bar comes and goes? Looking at InfoBar::RecalculateHeights(), I think it wants to scale the arrow size as the infobar animates as it slides down. But, InfoBarGradientView does not call -setNeedsDisplay: when arrowHeight is changed, causing the small arrow to be shown until the window is resized. We can add such call, but I don't understand why it needs to animate the arrow size because the slide out is happening from behind the bookmark bar or omnibox view, so the arrow is not currently visible during animation. I think what we need first is a clear definition of what the arrow size should be under what condition, and how the slide out animation is supposed to look. Can you check how it works on Windows or Linux? Avi, WDYT?
(I think rsesek did that arrow thingy and can answer questions. I think the arrow height _is_ supposed to change: The idea is that it points at the omnibox icon so that webpage ui can't spoof inforbars, and with the bookmark bar visible, the arrow must become longer so that it still reaches the omnibox (?))
a) Yes, talk to rsesek. b) The arrow is supposed to overlap the browser chrome, but isn't supposed to be stretched. AFAIK, the screenshot at https://code.google.com/p/chromium/issues/detail?id=382141#c10 shows correct behavior.
On 2014/08/05 00:00:54, Avi wrote: > a) Yes, talk to rsesek. > > b) The arrow is supposed to overlap the browser chrome, but isn't supposed to be > stretched. AFAIK, the screenshot at > https://code.google.com/p/chromium/issues/detail?id=382141#c10 shows correct > behavior. Hi, thanks for all your comments. I've investigated the behavior before I made this CL, also the code path on Windows/Linux. (1)Record what happens on Windows OS, the arrow changes according to the bookmark bar status (linux is the same behavior): https://code.google.com/p/chromium/issues/detail?id=382141#c12 https://code.google.com/p/chromium/issues/detail?id=382141#c13 (2)Andre@, the InfoBarGradientView does not call -setNeedsDisplay:, this really leads to some problems, like what #c7 shows. By set -setAutoresizingMask:, the view will be redrawn when frame is changed. (3)rsesek@ please help to clarify the right definition here.Thanks!
On 2014/08/05 06:06:19, Malcolm wrote: > On 2014/08/05 00:00:54, Avi wrote: > > a) Yes, talk to rsesek. > > > > b) The arrow is supposed to overlap the browser chrome, but isn't supposed to > be > > stretched. AFAIK, the screenshot at > > https://code.google.com/p/chromium/issues/detail?id=382141#c10 shows correct > > behavior. > > Hi, thanks for all your comments. I've investigated the behavior before I made > this CL, also the code path on Windows/Linux. > > (1)Record what happens on Windows OS, the arrow changes according to the > bookmark > bar status (linux is the same behavior): > https://code.google.com/p/chromium/issues/detail?id=382141#c12 > https://code.google.com/p/chromium/issues/detail?id=382141#c13 > > (2)Andre@, the InfoBarGradientView does not call -setNeedsDisplay:, this really > leads to some problems, like what #c7 shows. By set -setAutoresizingMask:, the > view > will be redrawn when frame is changed. > > (3)rsesek@ please help to clarify the right definition here.Thanks! Seems like there might be several different problems here. 1. Nico's original from #1. How to reproduce? 2. Mehmet's from #7. Might be related to missing setNeedsDisplay call. 3. Malcolm's from #10, where the arrow does not change size when the bookmark bar comes and goes. Looks like Mac never had code to handle this? If we are going to fix, I think we should open a separate bug for this.
On 2014/08/05 18:40:16, Andre wrote: > On 2014/08/05 06:06:19, Malcolm wrote: > > On 2014/08/05 00:00:54, Avi wrote: > > > a) Yes, talk to rsesek. > > > > > > b) The arrow is supposed to overlap the browser chrome, but isn't supposed > to > > be > > > stretched. AFAIK, the screenshot at > > > https://code.google.com/p/chromium/issues/detail?id=382141#c10 shows correct > > > behavior. > > > > Hi, thanks for all your comments. I've investigated the behavior before I made > > this CL, also the code path on Windows/Linux. > > > > (1)Record what happens on Windows OS, the arrow changes according to the > > bookmark > > bar status (linux is the same behavior): > > https://code.google.com/p/chromium/issues/detail?id=382141#c12 > > https://code.google.com/p/chromium/issues/detail?id=382141#c13 > > > > (2)Andre@, the InfoBarGradientView does not call -setNeedsDisplay:, this > really > > leads to some problems, like what #c7 shows. By set -setAutoresizingMask:, the > > view > > will be redrawn when frame is changed. > > > > (3)rsesek@ please help to clarify the right definition here.Thanks! > > Seems like there might be several different problems here. > 1. Nico's original from #1. How to reproduce? > 2. Mehmet's from #7. Might be related to missing setNeedsDisplay call. > 3. Malcolm's from #10, where the arrow does not change size when the bookmark > bar comes and goes. Looks like Mac never had code to handle this? If we are > going to fix, I think we should open a separate bug for this. I added a comment, pls see https://code.google.com/p/chromium/issues/detail?id=382141#c16
On 2014/08/05 18:40:16, Andre wrote: > On 2014/08/05 06:06:19, Malcolm wrote: > > On 2014/08/05 00:00:54, Avi wrote: > > > a) Yes, talk to rsesek. > > > > > > b) The arrow is supposed to overlap the browser chrome, but isn't supposed > to > > be > > > stretched. AFAIK, the screenshot at > > > https://code.google.com/p/chromium/issues/detail?id=382141#c10 shows correct > > > behavior. > > > > Hi, thanks for all your comments. I've investigated the behavior before I made > > this CL, also the code path on Windows/Linux. > > > > (1)Record what happens on Windows OS, the arrow changes according to the > > bookmark > > bar status (linux is the same behavior): > > https://code.google.com/p/chromium/issues/detail?id=382141#c12 > > https://code.google.com/p/chromium/issues/detail?id=382141#c13 > > > > (2)Andre@, the InfoBarGradientView does not call -setNeedsDisplay:, this > really > > leads to some problems, like what #c7 shows. By set -setAutoresizingMask:, the > > view > > will be redrawn when frame is changed. > > > > (3)rsesek@ please help to clarify the right definition here.Thanks! > > Seems like there might be several different problems here. > 1. Nico's original from #1. How to reproduce? I believe this was a result from CoreAnimation layers being enabled. ccameron@ would know more. > 2. Mehmet's from #7. Might be related to missing setNeedsDisplay call. Not sure how to reproduce this. > 3. Malcolm's from #10, where the arrow does not change size when the bookmark > bar comes and goes. Looks like Mac never had code to handle this? If we are > going to fix, I think we should open a separate bug for this. Yes, Mac never had the variable-height anti-spoof infobar tip. This turned out to be incredibly difficult to do because of the way we handle layout in the BrowserWindowController. See https://code.google.com/p/chromium/issues/detail?id=76381 for the history.
On 2014/08/06 17:17:52, rsesek wrote: > On 2014/08/05 18:40:16, Andre wrote: > > On 2014/08/05 06:06:19, Malcolm wrote: > > > On 2014/08/05 00:00:54, Avi wrote: > > > > a) Yes, talk to rsesek. > > > > > > > > b) The arrow is supposed to overlap the browser chrome, but isn't supposed > > to > > > be > > > > stretched. AFAIK, the screenshot at > > > > https://code.google.com/p/chromium/issues/detail?id=382141#c10 shows > correct > > > > behavior. > > > > > > Hi, thanks for all your comments. I've investigated the behavior before I > made > > > this CL, also the code path on Windows/Linux. > > > > > > (1)Record what happens on Windows OS, the arrow changes according to the > > > bookmark > > > bar status (linux is the same behavior): > > > https://code.google.com/p/chromium/issues/detail?id=382141#c12 > > > https://code.google.com/p/chromium/issues/detail?id=382141#c13 > > > > > > (2)Andre@, the InfoBarGradientView does not call -setNeedsDisplay:, this > > really > > > leads to some problems, like what #c7 shows. By set -setAutoresizingMask:, > the > > > view > > > will be redrawn when frame is changed. > > > > > > (3)rsesek@ please help to clarify the right definition here.Thanks! > > > > Seems like there might be several different problems here. > > 1. Nico's original from #1. How to reproduce? > > I believe this was a result from CoreAnimation layers being enabled. ccameron@ > would know more. > > > 2. Mehmet's from #7. Might be related to missing setNeedsDisplay call. > > Not sure how to reproduce this. > > > 3. Malcolm's from #10, where the arrow does not change size when the bookmark > > bar comes and goes. Looks like Mac never had code to handle this? If we are > > going to fix, I think we should open a separate bug for this. > > Yes, Mac never had the variable-height anti-spoof infobar tip. This turned out > to be incredibly difficult to do because of the way we handle layout in the > BrowserWindowController. See > https://code.google.com/p/chromium/issues/detail?id=76381 for the history. Thanks for your reply, rsesek. The tip has such a long history, a little amazing :) 1. You can find (attached video)： steps to repro #1 @https://code.google.com/p/chromium/issues/detail?id=382141#c16 steps to repro #7 @https://code.google.com/p/chromium/issues/detail?id=382141#c17 I noticed that the infobar tip becomes normal immediately when you resize the window in both of the cases, so maybe it's not caused by CoreAnimation. 2. The variable-height tip issue: I reset the arrow height before/after the bookmark bar animation happened/finished in the delegate, by doing this a layout process will be triggered. Would you have a look at the CL if you have time, I'll appreciate that. Finally, at least we can use -setNeedsDisplay: to fix some of the them, thanks!
Which of the three issues does this fix? It's not clear from the CL description (which you should also wrap to 80 columns). Does it fix (1), (2), (3), or some combination (or all) of them? https://codereview.chromium.org/435863002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/infobars/infobar_container_controller.h (right): https://codereview.chromium.org/435863002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/infobars/infobar_container_controller.h:96: - (void)setMaxTopArrowHeight:(int)height; Use NSInteger, for consistency with -defaultArrowHeight. https://codereview.chromium.org/435863002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/infobars/infobar_container_controller.mm (right): https://codereview.chromium.org/435863002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/infobars/infobar_container_controller.mm:168: return infobars::InfoBar::kDefaultArrowTargetHeight; Why not just use this constant instead of calling this method?
I've updated the CL, please review, thanks! https://codereview.chromium.org/435863002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right): https://codereview.chromium.org/435863002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1395: toState:currentState_]; We should guarantee everything is done before notify the delegate. https://codereview.chromium.org/435863002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/infobars/infobar_container_controller.mm (right): https://codereview.chromium.org/435863002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/infobars/infobar_container_controller.mm:168: return infobars::InfoBar::kDefaultArrowTargetHeight; I prefer not to expose the infobars namespace out of the InfoBarController, will break encapsulation. https://codereview.chromium.org/435863002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/infobars/infobar_controller.mm (right): https://codereview.chromium.org/435863002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/infobars/infobar_controller.mm:72: [infoBarView_ setAutoresizingMask:NSViewWidthSizable | NSViewHeightSizable]; Using -setAutoresizingMask: to let the view be redrawn when frame changes.
So this change is making the infobar tip draw up to the lock icon if the bookmark bar is visible? Does it animate when doing that (it doesn't look like it does, it looks like this change will just jump the frame)? https://codereview.chromium.org/435863002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/infobars/infobar_container_controller.mm (right): https://codereview.chromium.org/435863002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/infobars/infobar_container_controller.mm:168: return infobars::InfoBar::kDefaultArrowTargetHeight; On 2014/08/08 10:51:51, Malcolm wrote: > I prefer not to expose the infobars namespace out of the InfoBarController, > will break encapsulation. Why do you think it breaks encapsulation? By having this be a method instead of just using a constant, it requires understanding that this always returns a constant value, rather than a variable one.
Yes, the tip will draw up to the lock icon when bookmark bar is shown and this process doesn't have animation, the same on Windows. Because the animation of bookmark persists very shortly, the effect is acceptable. https://codereview.chromium.org/435863002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/infobars/infobar_container_controller.mm (right): https://codereview.chromium.org/435863002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/infobars/infobar_container_controller.mm:168: return infobars::InfoBar::kDefaultArrowTargetHeight; Sounds reasonable. From my point of view, InfoBarContainerController should have all necessary interfaces. If including infobar_container_controller.h AND infobar_cocoa.h at the same time, I'll feel a little confused, because InfoBarCocoa is not a controller/view.
On 2014/08/08 15:54:25, Malcolm wrote: > Yes, the tip will draw up to the lock icon when bookmark bar is shown and this > process doesn't have animation, the same on Windows. Because the animation of > bookmark persists very shortly, the effect is acceptable. That's not right. Windows animates this process, it doesn't jump directly.
On 2014/08/08 17:21:27, Peter Kasting wrote: > On 2014/08/08 15:54:25, Malcolm wrote: > > Yes, the tip will draw up to the lock icon when bookmark bar is shown and this > > process doesn't have animation, the same on Windows. Because the animation of > > bookmark persists very shortly, the effect is acceptable. > > That's not right. Windows animates this process, it doesn't jump directly. Sorry, I made a mistake here, window does the change through animation. I've update the CL, hope it can meet our requirement, thanks!
Is it possible to write a regression test for this? https://codereview.chromium.org/435863002/diff/80001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_controller_private.h (right): https://codereview.chromium.org/435863002/diff/80001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller_private.h:157: - (NSInteger)getMaxTopInfoBarArrowHeight; naming: -infoBarMaxTopArrowHeight.
On 2014/08/12 20:51:28, rsesek wrote: > Is it possible to write a regression test for this? > > https://codereview.chromium.org/435863002/diff/80001/chrome/browser/ui/cocoa/... > File chrome/browser/ui/cocoa/browser_window_controller_private.h (right): > > https://codereview.chromium.org/435863002/diff/80001/chrome/browser/ui/cocoa/... > chrome/browser/ui/cocoa/browser_window_controller_private.h:157: - > (NSInteger)getMaxTopInfoBarArrowHeight; > naming: -infoBarMaxTopArrowHeight. Hi Rsesek, I've changed the naming problem and tried to write a unit test for it. But I didn't find a perfect way to test this interface, because the InfoBarContainerController depends on the infobar component, which most of the logic process is done inside. I have little experience on the unit test, do you have any useful suggestion? Thanks!
On 2014/08/18 08:42:21, Malcolm wrote: > On 2014/08/12 20:51:28, rsesek wrote: > > Is it possible to write a regression test for this? > > > > > https://codereview.chromium.org/435863002/diff/80001/chrome/browser/ui/cocoa/... > > File chrome/browser/ui/cocoa/browser_window_controller_private.h (right): > > > > > https://codereview.chromium.org/435863002/diff/80001/chrome/browser/ui/cocoa/... > > chrome/browser/ui/cocoa/browser_window_controller_private.h:157: - > > (NSInteger)getMaxTopInfoBarArrowHeight; > > naming: -infoBarMaxTopArrowHeight. > > Hi Rsesek, > I've changed the naming problem and tried to write a unit test for it. But I > didn't find a perfect way to test this interface, > because the InfoBarContainerController depends on the infobar component, which > most of the logic process is done inside. > I have little experience on the unit test, do you have any useful suggestion? > Thanks! Sorry for not responding yesterday, I had a lot on my plate. I think you'll want to test this using the browser_tests framework. Take a look at this example test, which is also testing for the infobar tip: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... I think you'll want a similar test, but one that verifies that the frame rectangle for the infobar container and the infobar view have enough height and are positioned correctly on the Y axis to reach the page info button.
On 2014/08/19 14:13:50, rsesek wrote: > On 2014/08/18 08:42:21, Malcolm wrote: > > On 2014/08/12 20:51:28, rsesek wrote: > > > Is it possible to write a regression test for this? > > > > > > > > > https://codereview.chromium.org/435863002/diff/80001/chrome/browser/ui/cocoa/... > > > File chrome/browser/ui/cocoa/browser_window_controller_private.h (right): > > > > > > > > > https://codereview.chromium.org/435863002/diff/80001/chrome/browser/ui/cocoa/... > > > chrome/browser/ui/cocoa/browser_window_controller_private.h:157: - > > > (NSInteger)getMaxTopInfoBarArrowHeight; > > > naming: -infoBarMaxTopArrowHeight. > > > > Hi Rsesek, > > I've changed the naming problem and tried to write a unit test for it. But I > > didn't find a perfect way to test this interface, > > because the InfoBarContainerController depends on the infobar component, > which > > most of the logic process is done inside. > > I have little experience on the unit test, do you have any useful suggestion? > > Thanks! > > Sorry for not responding yesterday, I had a lot on my plate. > > I think you'll want to test this using the browser_tests framework. Take a look > at this example test, which is also testing for the infobar tip: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/.... > I think you'll want a similar test, but one that verifies that the frame > rectangle for the infobar container and the infobar view have enough height and > are positioned correctly on the Y axis to reach the page info button. Yes, that is what I want to know, thanks! I'll reference the existing test, then try to write one for the CL.
It's a little long since last update, please review it, thanks!
Really nice test! Just some nit comment, and then this is good. I ran it through the trybots for you, and things look solid. https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:81: const_cast<const InfoBarCocoa*>(infoBarController.infobar)->animation(); Why do you need this const cast? https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:203: CGFloat overlappingTipHeight = naming: overlapping_tip_height https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:205: LocationBarViewMac* locationBarView = [controller() locationBarBridge]; naming: location_bar_view https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:206: NSPoint iconBottom = locationBarView->GetPageInfoBubblePoint(); naming: icon_bottom https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:208: NSPoint infoBarTop = NSMakePoint(0, naming: info_bar_top https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:449: NSInteger maxTipHeight = infobars::InfoBar::kMaximumArrowTargetHeight + naming: In C++ functions (which tests are), use under_score naming, e.g. max_tip_height.
Change the naming and thanks for your help! https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:81: const_cast<const InfoBarCocoa*>(infoBarController.infobar)->animation(); On 2014/08/26 16:26:18, rsesek wrote: > Why do you need this const cast? Class InfoBar has two methods named "animation()", one is public, the other is protected. Here, const cast it in order to call the public&const "animation()".
https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:81: const_cast<const InfoBarCocoa*>(infoBarController.infobar)->animation(); On 2014/08/27 06:29:21, Malcolm wrote: > On 2014/08/26 16:26:18, rsesek wrote: > > Why do you need this const cast? > > Class InfoBar has two methods named "animation()", one is public, the other is > protected. Here, const cast it in order to call the public&const "animation()". If you're adding const, use static_cast rather than const_cast. Just use const_cast for removing const; static_cast is safer in general.
Update CL, please review it, thanks! https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/435863002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:81: const_cast<const InfoBarCocoa*>(infoBarController.infobar)->animation(); On 2014/08/27 06:59:51, Peter Kasting wrote: > On 2014/08/27 06:29:21, Malcolm wrote: > > On 2014/08/26 16:26:18, rsesek wrote: > > > Why do you need this const cast? > > > > Class InfoBar has two methods named "animation()", one is public, the other is > > protected. Here, const cast it in order to call the public&const > "animation()". > > If you're adding const, use static_cast rather than const_cast. Just use > const_cast for removing const; static_cast is safer in general. Oh, should be static_cast here, very careful, thanks!
LGTM! https://codereview.chromium.org/435863002/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/435863002/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:82: infoBarController.infobar)->animation(); nit: indent +4 more spaces
https://codereview.chromium.org/435863002/diff/180001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm (right): https://codereview.chromium.org/435863002/diff/180001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm:82: infoBarController.infobar)->animation(); On 2014/08/27 14:59:45, rsesek wrote: > nit: indent +4 more spaces Done. thanks!
The CQ bit was checked by malcolm.2.wang@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/malcolm.2.wang@gmail.com/435863002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by malcolm.2.wang@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/malcolm.2.wang@gmail.com/435863002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as d8523f389b55111a224686a36f2bb71f001031fd
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/7d9f474ac604f3c7a40afbb808e2ed61fe460442 Cr-Commit-Position: refs/heads/master@{#292317} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
