Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6609047:
[linux_views][Win] spoof proof redesign infobar extension with tab. (Closed)
|
Created:
9 years, 9 months ago by Sheridan Rawlins Modified:
9 years, 6 months ago CC:
chromium-reviews, Aaron Boodman, Erik does not do reviews, pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[linux_views][Win] spoof proof redesign infobar extension with tab.
This Refactor causes InfoBarViews to be layout overlapping each other.
This has the following benefits:
- InfoBarBackground can draw one gradient from top of tab to bottom
of the bar.
- InfoBarView can draw everything including its tab at the same time.
- If the image needs to be drawn into the tab area, it may be drawn
exactly one time (instead of in two chunks).
It had the following challenges:
- Because all InfoBarViews may be animating independently, all
of InfoBarContainer's children must be considered to find the
overlay amount |bar_height|.
- clipPath has a bug in skia where text is not clipped, so a
rectangle is used currently.
- InfoBarView must InvalidateLayout when animating because changing
the tab height on the lowest InfoBarView does not cause the bounds
of InfoBarContainer to change.
Note: Hit testing was not tackled in this CL. it doesn't seem to cause
a problem with underlying bookmarks because the InfoBarView doesn't
intercept events and none of its children which do overlap the toolbar
separator.
Summary of changes:
- Draw tab not arrow.
- Prepare/cache the stroke & fill paths in InfoBarView::OnBoundsChanged
- Move tab drawing iniation from BrowserView::PaintChildren to InfoBarBackground, which fills & strokes the paths.
- Overlap the infobars during layout (InfoBarContainer & BrowserViewLayout) so that InfoBarView/InfoBarBackground can draw all of the bar & tab within its bounds.
- Center the icon within the tab (horizontally & vertically).
- Animate by sliding the bar downward, then sliding the tab upwards.
BUG=74437
TEST=Browse to http://russia.ru to cause translate infobar. Kill the
flash plugin to cause another infobar.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77397
Patch Set 1 #
Total comments: 28
Patch Set 2 : nits, 1 animation, vertical_overlap, path fix #Patch Set 3 : Cast stroke width. #
Total comments: 45
Patch Set 4 : More review changes. #Patch Set 5 : more review changes. #Patch Set 6 : Offset stroke path by 0.5, 0.5. #Patch Set 7 : No slow animation. #Patch Set 8 : Pulled latest. #
Total comments: 18
Patch Set 9 : nit fixes. #
Total comments: 10
Patch Set 10 : renames, don't const_cast, ditch InfoBarView::VerticalOffset. #
Total comments: 1
Messages
Total messages: 13 (0 generated)
Peter - you are the most involved in InfoBar refactoring, and I look forward to your review. Ben - you had asked to review this change after our discussion earlier this week.
Please update your checkout to get my latest refactoring bits. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/frame/b... File chrome/browser/ui/views/frame/browser_view.cc (right): http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/frame/b... chrome/browser/ui/views/frame/browser_view.cc:1811: for (int i = 0, count = child_count(); i < count; ++i) { Nit: Don't create a separate |count| temp. child_count() is cheap, call it directly in the comparison. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/frame/b... chrome/browser/ui/views/frame/browser_view.cc:1813: if (!child) { Nit: Remove this conditional. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... File chrome/browser/ui/views/infobars/infobar_background.cc (right): http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_background.cc:56: NULL, 2, SkShader::kMirror_TileMode); Nit: Seems like kClamp_TileMode would make it clearer that you don't expect any tiling. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_background.cc:71: paint.setStrokeCap(SkPaint::kSquare_Cap); Why do this? The default (no) cap seems better. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... File chrome/browser/ui/views/infobars/infobar_container.h (right): http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_container.h:37: class Heights { I think it would be clearer to write the InfoBarContainer and InfoBarView classes without this class. In my mind, what you want is these two API functions in both classes: int vertical_overlap(); virtual gfx::Size GetPreferredSize(); // View override InfoBarView::vertical_overlap() gives the current height of the top tab (i.e. the tab's full height * the animation value). InfoBarView::GetPreferredSize() gives the current size of the infobar including the top tab. InfoBarContainer::vertical_overlap() gives the vertical_overlap() of the first child infobar. InfoBarContainer::GetPreferredSize gives a size whose height is the sum of (for all children, GetPreferredSize().height() - vertical_overlap()) plus vertical_overlap(). This does not have a radical effect on the various callers, but I think it makes things slightly clearer. For example, the BrowserViewLayout code can be slightly more direct. This should also let you get rid of InfoBarView::get_preferred_xxx_height(), I think. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... File chrome/browser/ui/views/infobars/infobar_view.cc (right): http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.cc:59: ALLOW_THIS_IN_INITIALIZER_LIST(tab_animation_(new ui::SlideAnimation(this))), Nit: 80 columns (but this should go away when you revert to one animation) http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.cc:203: Nit: No blank line http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.cc:293: // with skia. For now, just clip to the bar bounds. Did you contact Mike Reed? He may be able to just fix this for you immediately without you having to work around, or point out your error if it turns out to not be a bug. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.cc:390: stroke_path_->moveTo(SkIntToScalar(mirrored_x) + SK_ScalarHalf, These offsets all seem wrong to me. It seems like you should get rid of the 0.5s and 1s here, copy to the fill path, and offset by (0.5, 0.5), like the old code did. You may have to increase the tab height by 1 to make everything still look the same. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.cc:423: // Use the same clip path as the fill path. Nit: We shouldn't even have |clip_path_|, then. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... File chrome/browser/ui/views/infobars/infobar_view.h (right): http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.h:127: // To allow different animations, proxy needed routines for Based on my chat with Ben and you, let's just do a single-phase animation and leave the API like it was. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.h:156: SkPath* fill_path() const { return fill_path_.get(); } Nit: Put non-virtual functions below the virtual ones. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.h:212: // The target height for the bar portion of the InfoBarView. Nit: Extra space http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.h:215: // The current stroke path of the InfoBar. Nit: These comments add nothing. Just eliminate them and the blank lines between these.
Updates from Peter's review. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/frame/b... File chrome/browser/ui/views/frame/browser_view.cc (right): http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/frame/b... chrome/browser/ui/views/frame/browser_view.cc:1811: for (int i = 0, count = child_count(); i < count; ++i) { FWIW, this was copied from views/view.cc Done. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/frame/b... chrome/browser/ui/views/frame/browser_view.cc:1813: if (!child) { FWIW, this was copied from views/view.cc Done. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... File chrome/browser/ui/views/infobars/infobar_background.cc (right): http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_background.cc:56: NULL, 2, SkShader::kMirror_TileMode); Yes, this agrees with views/painter.cc Done. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_background.cc:71: paint.setStrokeCap(SkPaint::kSquare_Cap); On 2011/03/04 22:23:42, Peter Kasting wrote: > Why do this? The default (no) cap seems better. It's how the mac is implemented: http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browse... Both platforms should agree. We can change both as separate CL if desired. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... File chrome/browser/ui/views/infobars/infobar_container.h (right): http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_container.h:37: class Heights { On 2011/03/04 22:23:42, Peter Kasting wrote: > I think it would be clearer to write the InfoBarContainer and InfoBarView > classes without this class. The reason for the class was that the layout code had to walk stuff and I didn't want a bunch of routines all doing similar things for different reasons (laying out, getting overlap, getting height). Picking apart that concern lead me to a LayoutHelper private method which does all three and simplifies by recursing once when it needs to to set the |top| to the top of the toolbar separator. > This does not have a radical effect on the various callers, but I think it makes > things slightly clearer. For example, the BrowserViewLayout code can be > slightly more direct. This should also let you get rid of > InfoBarView::get_preferred_xxx_height(), I think. I still like the preferred_xxx_height methods as private methods for all of the internal calculations in InfoBarView for paths & clips. In that context, it seems more sensible to use the tab or bar height than doing calculations on the full height and the vertical_overlap. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... File chrome/browser/ui/views/infobars/infobar_view.cc (right): http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.cc:59: ALLOW_THIS_IN_INITIALIZER_LIST(tab_animation_(new ui::SlideAnimation(this))), On 2011/03/04 22:23:42, Peter Kasting wrote: > Nit: 80 columns (but this should go away when you revert to one animation) Done. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.cc:203: On 2011/03/04 22:23:42, Peter Kasting wrote: > Nit: No blank line Done. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.cc:293: // with skia. For now, just clip to the bar bounds. On 2011/03/04 22:23:42, Peter Kasting wrote: > Did you contact Mike Reed? He may be able to just fix this for you immediately > without you having to work around, or point out your error if it turns out to > not be a bug. I definitely want to file a bug for this, but it seems very hairy to find the reduction. Apparently, on linux_views, views::Label uses cairo to draw on a skia canvas, which seems ridiculous to me. I'll take a look on windows and see if it works there. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.cc:390: stroke_path_->moveTo(SkIntToScalar(mirrored_x) + SK_ScalarHalf, On 2011/03/04 22:23:42, Peter Kasting wrote: > These offsets all seem wrong to me. It seems like you should get rid of the > 0.5s and 1s here, copy to the fill path, and offset by (0.5, 0.5), like the old > code did. You may have to increase the tab height by 1 to make everything still > look the same. Ok. Instead of the SK_Scalar1, I added a kTabStrokeWidth, and also used that in the InfoBarBackground call to SkPaint::setStrokeWidth. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.cc:423: // Use the same clip path as the fill path. On 2011/03/04 22:23:42, Peter Kasting wrote: > Nit: We shouldn't even have |clip_path_|, then. Done. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... File chrome/browser/ui/views/infobars/infobar_view.h (right): http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.h:127: // To allow different animations, proxy needed routines for On 2011/03/04 22:23:42, Peter Kasting wrote: > Based on my chat with Ben and you, let's just do a single-phase animation and > leave the API like it was. Done. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.h:156: SkPath* fill_path() const { return fill_path_.get(); } On 2011/03/04 22:23:42, Peter Kasting wrote: > Nit: Put non-virtual functions below the virtual ones. Done. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.h:212: // The target height for the bar portion of the InfoBarView. On 2011/03/04 22:23:42, Peter Kasting wrote: > Nit: Extra space Done. http://codereview.chromium.org/6609047/diff/1/chrome/browser/ui/views/infobar... chrome/browser/ui/views/infobars/infobar_view.h:215: // The current stroke path of the InfoBar. On 2011/03/04 22:23:42, Peter Kasting wrote: > Nit: These comments add nothing. Just eliminate them and the blank lines > between these. Done.
small change to cast the stroke width.
http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view.cc (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.cc:1812: DCHECK(child) << "Should not have a NULL child View for index in bounds"; Nit: I would remove this DCHECK(). There are hundreds of places in code we iterate through View children, and AFAIK none DCHECK the pointers. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view_layout.cc (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view_layout.cc:368: int height = visible ? infobar_container_->GetPreferredSize().height() : 0; Nit: I don't think either of these "visible ?" conditionals should be necessary. GetProferredSize() and vertical_overlap() ought to be returning 0 in this case. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_container.cc (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:36: void InfoBarContainer::LayoutHelper(bool set_bounds, Nit: Function definition order should match declaration order. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:123: // available width of the delegate). Our preferred height is the sum Nit: Why did you rewrap these lines? They were fine before. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_container.h (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.h:46: int vertical_overlap() const; Nit: Use CamelCase because the function implementation is not a cheap inline accessor. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.h:81: void LayoutHelper(bool set_bounds, int* vertical_overlap, int* height) const; Nit: This function should not be marked const, as when |set_bounds| is true it is not logically const. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_view.cc (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:44: const int InfoBarView::kCurveDistance = 13; Nit: kCurveWidth? http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:45: const int InfoBarView::kIconWidth = 29; Nit: kMaxIconWidth? http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:48: const int InfoBarView::kTabPadding = 4; Nit: Seems like it'd be clearer to have a padding value that's the padding on one side of the icon, and use two of them elsewhere (e.g. "(kCurveDistance + kIconPadding) * 2 ..."). http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:203: // Center the icon vertically and horizontally within the tab. Nit: This is a little unclear since you're not actually centering vertically "within the tab" but within the bar + tab. I'd eliminate this comment in favor of the one I suggest just below. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:206: int full_height = target_height_ + kTabHeight; Nit: I'd stick a comment here that you're duplicating a call to OffsetY() except centered within the entire (tab + bar) height instead of just within the bar. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:207: int preferred_height = preferred_tab_height() + preferred_bar_height(); Nit: This temp is unnecessary. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:290: // with skia. For now, just clip to the bar bounds. Nit: This is on file now, might want to change the comments. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:296: DCHECK_EQ(tab_height + bar_height, height()); Nit: This DCHECK and the temps above seem unnecessary. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:368: DCHECK_EQ(tab_height + bar_height, height()); Nit: This DCHECK seems unnecessary. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:370: // Fill and stroke have different opinions about how to treat paths. Because Nit: This comment goes above the offset() call. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:381: if (tab_height) { Nit: You can get rid of all the conditionals in this function assuming you follow my directive below. Just eliminate the last block entirely and everything does the right thing automatically. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:408: stroke_path_->offset(SK_ScalarHalf, -SK_ScalarHalf); This still is not right. You should be offsetting in y by +0.5, not -0.5. This ought to eliminate kTabStrokeWidth entirely. You can just use 1 directly in InfoBarBackground. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_view.h (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.h:151: friend class InfoBarBackground; Nit: If these need to be friends, we're doing something wrong. We should be making members public instead. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.h:167: SkPath* clip_path() const { return fill_path_.get(); } Nit: Remove this function and call fill_path() directly where needed. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.h:175: int preferred_tab_height() const; Nit: Use CamelCase because neither of these is a cheap inline accessor (Animation::GetCurrentValue() is virtual). Rename "Preferred" to "Current" for both of these. Eliminate vertical_overlap() and just make CurrentTabHeight() public.
One more. I forgot to review LayoutHelper(). http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_container.cc (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:42: if (set_bounds) { This recursive method works correctly, but it has two problems: (1) When |set_bounds| is true, LayoutHelper() returns 0 in |vertical_overlap|. This doesn't matter to you now, but maybe someone will write code someday where it does. (2) It's confusing. It took me a long time to understand why using recursion here produced the right answer to the problem scenario you describe. Therefore, I suggest you do the following: (1) Make InfoBarView's functions for tab height and bar height both public. For the sake of argument let's call them GetTabHeight() and GetBarHeight(). (2) Convert this function to the following: int InfoBarContainer::GetVerticalOverlap(int* height_without_overlap) const { int height_temp; if (!height_without_overlap) height_without_overlap = &height_temp; int vertical_overlap = 0; *height_without_overlap = 0; for (int i = 0; i < child_count(); ++i) { const InfoBarView* child = static_cast<const InfoBarView*>(GetChildViewAt(i)); vertical_overlap = std::max(vertical_overlap, child->GetTabHeight() - *height_without_overlap); *height_without_overlap += child->GetBarHeight(); } return vertical_overlap; } (3) Rewrite these three functions as follows: int InfoBarContainer::VerticalOverlap() const { return GetVerticalOverlap(NULL); } gfx::Size InfoBarContainer::GetPreferredSize() { int height_without_overlap; int vertical_overlap = GetVerticalOverlap(&height_without_overlap); return gfx::Size(0, vertical_overlap + height_without_overlap); } void InfoBarContainer::Layout() { int next_child_bar_y = GetVerticalOverlap(NULL); for (int i = 0; i < child_count(); ++i) { InfoBarView* child = static_cast<InfoBarView*>(GetChildViewAt(i)); child->SetBounds(0, next_child_bar_y - child->GetTabHeight(), width(), child->GetPreferredSize().height()); next_child_bar_y = child->GetMirroredBounds().bottom(); } } I realize that to some degree this represents moving back towards the GetHeights() code you had before. I'm sorry that at first I didn't realize that the net vertical overlap of the container might not be the vertical overlap of the first child, which makes things much more complicated.
review changes. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view.cc (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.cc:1812: DCHECK(child) << "Should not have a NULL child View for index in bounds"; On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: I would remove this DCHECK(). There are hundreds of places in code we > iterate through View children, and AFAIK none DCHECK the pointers. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view_layout.cc (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view_layout.cc:368: int height = visible ? infobar_container_->GetPreferredSize().height() : 0; On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: I don't think either of these "visible ?" conditionals should be necessary. > GetProferredSize() and vertical_overlap() ought to be returning 0 in this case. return browser()->SupportsWindowFeature(Browser::FEATURE_INFOBAR) && (infobar_container_->GetPreferredSize().height() != 0); Could browser()->SupportsWindowFeature(Browser::FEATURE_INFOBAR) be false, but the InfoBarContainer have a height? Added DCHECK and removed the conditionals. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_container.cc (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:36: void InfoBarContainer::LayoutHelper(bool set_bounds, On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: Function definition order should match declaration order. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:42: if (set_bounds) { On 2011/03/07 22:23:07, Peter Kasting wrote: > This recursive method works correctly, but it has two problems: > > (1) When |set_bounds| is true, LayoutHelper() returns 0 in |vertical_overlap|. > This doesn't matter to you now, but maybe someone will write code someday where > it does. > (2) It's confusing. It took me a long time to understand why using recursion > here produced the right answer to the problem scenario you describe. > > Therefore, I suggest you do the following: > > (1) Make InfoBarView's functions for tab height and bar height both public. For > the sake of argument let's call them GetTabHeight() and GetBarHeight(). > (2) Convert this function to the following: > > int InfoBarContainer::GetVerticalOverlap(int* height_without_overlap) const { > int height_temp; > if (!height_without_overlap) > height_without_overlap = &height_temp; > int vertical_overlap = 0; > *height_without_overlap = 0; > for (int i = 0; i < child_count(); ++i) { > const InfoBarView* child = > static_cast<const InfoBarView*>(GetChildViewAt(i)); > vertical_overlap = std::max(vertical_overlap, > child->GetTabHeight() - *height_without_overlap); > *height_without_overlap += child->GetBarHeight(); > } > return vertical_overlap; > } > > (3) Rewrite these three functions as follows: > > int InfoBarContainer::VerticalOverlap() const { > return GetVerticalOverlap(NULL); > } > > gfx::Size InfoBarContainer::GetPreferredSize() { > int height_without_overlap; > int vertical_overlap = GetVerticalOverlap(&height_without_overlap); > return gfx::Size(0, vertical_overlap + height_without_overlap); > } > > void InfoBarContainer::Layout() { > int next_child_bar_y = GetVerticalOverlap(NULL); > for (int i = 0; i < child_count(); ++i) { > InfoBarView* child = static_cast<InfoBarView*>(GetChildViewAt(i)); > child->SetBounds(0, next_child_bar_y - child->GetTabHeight(), width(), > child->GetPreferredSize().height()); > next_child_bar_y = child->GetMirroredBounds().bottom(); > } > } > > I realize that to some degree this represents moving back towards the > GetHeights() code you had before. I'm sorry that at first I didn't realize that > the net vertical overlap of the container might not be the vertical overlap of > the first child, which makes things much more complicated. Slightly different but in the same spirit. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:123: // available width of the delegate). Our preferred height is the sum On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: Why did you rewrap these lines? They were fine before. After adding text, I hit M-q. Emacs' c-fill-paragraph apparently wraps more aggressively than 80 chars. http://www.emacswiki.org/emacs/FillParagraph The right margin is determined by the variable fill-column. You can set it using C-x f or M-x set-fill-column using an explicit argument. Use C-u followed by a number to specify a column. Just C-u as argument means to use the current column. The default value for fill-column is 70. This can be set to 80 in the InitFile with the command (setq-default fill-column 80) I'll fix this so it shouldn't happen again. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_container.h (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.h:46: int vertical_overlap() const; On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: Use CamelCase because the function implementation is not a cheap inline > accessor. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.h:81: void LayoutHelper(bool set_bounds, int* vertical_overlap, int* height) const; On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: This function should not be marked const, as when |set_bounds| is true it > is not logically const. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_view.cc (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:44: const int InfoBarView::kCurveDistance = 13; On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: kCurveWidth? Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:45: const int InfoBarView::kIconWidth = 29; On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: kMaxIconWidth? Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:48: const int InfoBarView::kTabPadding = 4; On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: Seems like it'd be clearer to have a padding value that's the padding on > one side of the icon, and use two of them elsewhere (e.g. "(kCurveDistance + > kIconPadding) * 2 ..."). Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:203: // Center the icon vertically and horizontally within the tab. On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: This is a little unclear since you're not actually centering vertically > "within the tab" but within the bar + tab. I'd eliminate this comment in favor > of the one I suggest just below. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:206: int full_height = target_height_ + kTabHeight; On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: I'd stick a comment here that you're duplicating a call to OffsetY() except > centered within the entire (tab + bar) height instead of just within the bar. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:207: int preferred_height = preferred_tab_height() + preferred_bar_height(); On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: This temp is unnecessary. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:290: // with skia. For now, just clip to the bar bounds. On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: This is on file now, might want to change the comments. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:296: DCHECK_EQ(tab_height + bar_height, height()); On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: This DCHECK and the temps above seem unnecessary. It was there to test assumption that animation wouldn't progress between Layout and bounds changing. Added text to explain failure case. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:368: DCHECK_EQ(tab_height + bar_height, height()); On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: This DCHECK seems unnecessary. It was there to test assumption that animation wouldn't progress between Layout and painting children. Added text to explain failure case. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:370: // Fill and stroke have different opinions about how to treat paths. Because On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: This comment goes above the offset() call. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:381: if (tab_height) { On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: You can get rid of all the conditionals in this function assuming you > follow my directive below. Just eliminate the last block entirely and > everything does the right thing automatically. without these, there is some anti aliasing artifacts when drawing a zero height tab. The conditional needs to be somewhere; either here or in InfoBarBackground to check whether the path is empty. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:408: stroke_path_->offset(SK_ScalarHalf, -SK_ScalarHalf); On 2011/03/07 20:14:30, Peter Kasting wrote: > This still is not right. You should be offsetting in y by +0.5, not -0.5. > > This ought to eliminate kTabStrokeWidth entirely. You can just use 1 directly > in InfoBarBackground. If it is offset by +.5 then it draws below the line. If it is adjusted by -1, then the fill fills over the toolbar separator. This really looks fine as is. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_view.h (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.h:151: friend class InfoBarBackground; On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: If these need to be friends, we're doing something wrong. We should be > making members public instead. Fixed by using views::View and casting to InfoBarView where needed to get its public methods http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.h:167: SkPath* clip_path() const { return fill_path_.get(); } On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: Remove this function and call fill_path() directly where needed. Done. http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.h:175: int preferred_tab_height() const; On 2011/03/07 20:14:30, Peter Kasting wrote: > Nit: Use CamelCase because neither of these is a cheap inline accessor > (Animation::GetCurrentValue() is virtual). > > Rename "Preferred" to "Current" for both of these. > > Eliminate vertical_overlap() and just make CurrentTabHeight() public. Choosing Animated instead of Preferred or Current, since that's more accurately what it is - it's called both by InfoBarContainer to get the right size for laying out, which then indirectly calls OnBoundsChanged which gets it again. Done.
Responded to Peter's concern about the stroke path needing to be offset by positive (0.5, 0.5) rather than (0.5, -0.5). Just needed to extend the fill of the tab downward by one pixel, then this works (otherwise, the stroke path also extends 1 pixel below the toolbar separator, which is not good). http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_view.cc (right): http://codereview.chromium.org/6609047/diff/5003/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:408: stroke_path_->offset(SK_ScalarHalf, -SK_ScalarHalf); On 2011/03/08 01:38:19, Sheridan Rawlins wrote: > On 2011/03/07 20:14:30, Peter Kasting wrote: > > This still is not right. You should be offsetting in y by +0.5, not -0.5. > > > > This ought to eliminate kTabStrokeWidth entirely. You can just use 1 directly > > in InfoBarBackground. > > If it is offset by +.5 then it draws below the line. If it is adjusted by -1, > then the fill fills over the toolbar separator. This really looks fine as is. Ok. Figured it out. Needed to fill the 1 pixel below the tab, and add the bar's rect if it is non-zero height.
http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_background.cc (right): http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_background.cc:54: SkShader* gradient_shader = Nit: Could also wrap like this if you want, and save a line SkShader* gradient_shader = SkGradientShader::CreateLinear( gradient_points, gradient_colors, NULL, 2, SkShader::kClamp_TileMode); http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_container.cc (right): http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:90: // of the delegate). Our preferred height is the sum of the preferred heights Nit: I'd move this second sentence to the declaration of GetVerticalOverlap (and drop the uses of "preferred"). http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:104: int overlapped_top = top - Nit: Just use "top -=" here and "top +=" below, and avoid |overlapped_top|. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:143: int minimum_top = 0; I do not find this algorithm at all comprehensible compared to the one I suggested, which is not only simpler, but winds up with one fewer total function on the InfoBarView (because we ditch VerticalOverlap() and make the tab and bar height accessors public directly). http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:155: DCHECK(total_height); Your API comments say this can be NULL. Either make that true or change the comments. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_view.cc (right): http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:285: DCHECK_EQ(tab_height + bar_height, height()) << Nit: Special-case: For << "message", put the << at the beginning of the subsequent line instead of the end of the first. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:335: AnimatedBarHeight()); Nit: This indenting is strange. I suggest wrapping before the open paren and indenting 4. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:366: stroke_path_->moveTo(SkIntToScalar(mirrored_x), Nit: Use a temp: int divider_y = tab_height - 1; Then substitute |divider_y| or |-divider_y| below. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:384: fill_path_->rLineTo(0.0, 1.0); This seems unnecessary since the addRect() call below will cover this area.
http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_background.cc (right): http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_background.cc:54: SkShader* gradient_shader = On 2011/03/08 23:21:31, Peter Kasting wrote: > Nit: Could also wrap like this if you want, and save a line > > SkShader* gradient_shader = SkGradientShader::CreateLinear( > gradient_points, gradient_colors, NULL, 2, SkShader::kClamp_TileMode); Done. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_container.cc (right): http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:90: // of the delegate). Our preferred height is the sum of the preferred heights On 2011/03/08 23:21:31, Peter Kasting wrote: > Nit: I'd move this second sentence to the declaration of GetVerticalOverlap (and > drop the uses of "preferred"). Done. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:104: int overlapped_top = top - On 2011/03/08 23:21:31, Peter Kasting wrote: > Nit: Just use "top -=" here and "top +=" below, and avoid |overlapped_top|. Done. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:143: int minimum_top = 0; On 2011/03/08 23:21:31, Peter Kasting wrote: > I do not find this algorithm at all comprehensible compared to the one I > suggested, which is not only simpler, but winds up with one fewer total function > on the InfoBarView (because we ditch VerticalOverlap() and make the tab and bar > height accessors public directly). I think we're going to have to agree to disagree here. I find it much easier to conceptualize doing the layout and getting the minimum top. I really liked the top -= and top += changes you suggested below and I think they help the readability. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_container.cc:155: DCHECK(total_height); On 2011/03/08 23:21:31, Peter Kasting wrote: > Your API comments say this can be NULL. Either make that true or change the > comments. Done. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... File chrome/browser/ui/views/infobars/infobar_view.cc (right): http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:285: DCHECK_EQ(tab_height + bar_height, height()) << On 2011/03/08 23:21:31, Peter Kasting wrote: > Nit: Special-case: For << "message", put the << at the beginning of the > subsequent line instead of the end of the first. Done. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:335: AnimatedBarHeight()); On 2011/03/08 23:21:31, Peter Kasting wrote: > Nit: This indenting is strange. I suggest wrapping before the open paren and > indenting 4. Done. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:366: stroke_path_->moveTo(SkIntToScalar(mirrored_x), On 2011/03/08 23:21:31, Peter Kasting wrote: > Nit: Use a temp: > > int divider_y = tab_height - 1; > > Then substitute |divider_y| or |-divider_y| below. Done. http://codereview.chromium.org/6609047/diff/2013/chrome/browser/ui/views/info... chrome/browser/ui/views/infobars/infobar_view.cc:384: fill_path_->rLineTo(0.0, 1.0); On 2011/03/08 23:21:31, Peter Kasting wrote: > This seems unnecessary since the addRect() call below will cover this area. It doesn't cover it. A fill without this extension does not fill over the infobar. I'll add a comment explaining this.
LGTM, let's get this in and then see if it needs to be tweaked further. http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_container.cc (right): http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container.cc:141: int minimum_top = 0; Nit: I suggest renaming this |vertical_overlap| and negating the sign. (That means changing to std::max() below.) http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container.cc:142: int top = 0; Nit: |top| is kind of vague, how about |next_child_y| or |children_bottom| or something? http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container.cc:145: View* child = const_cast<View*>(GetChildViewAt(i)); Nit: It is really a shame that GetPreferredSize() is not const, but since it isn't, make this function non-const instead of const-casting here. http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container.cc:146: gfx::Size ps = child->GetPreferredSize(); Nit: Eliminate this, just roll into its use below http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_view.h (right): http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_view.h:73: int VerticalOverlap() const { return AnimatedTabHeight(); } Nit: Just make AnimatedTabHeight() public. If you want to preserve this function you need to de-inline it, and at that point it's not obvious in the header that it does the same thing as the other, so blah. Just publicize the other API call.
Here ya go. http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_container.cc (right): http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container.cc:141: int minimum_top = 0; On 2011/03/09 00:41:45, Peter Kasting wrote: > Nit: I suggest renaming this |vertical_overlap| and negating the sign. (That > means changing to std::max() below.) Done. http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container.cc:142: int top = 0; On 2011/03/09 00:41:45, Peter Kasting wrote: > Nit: |top| is kind of vague, how about |next_child_y| or |children_bottom| or > something? Done. http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container.cc:145: View* child = const_cast<View*>(GetChildViewAt(i)); On 2011/03/09 00:41:45, Peter Kasting wrote: > Nit: It is really a shame that GetPreferredSize() is not const, but since it > isn't, make this function non-const instead of const-casting here. Done. http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_container.cc:146: gfx::Size ps = child->GetPreferredSize(); On 2011/03/09 00:41:45, Peter Kasting wrote: > Nit: Eliminate this, just roll into its use below Done. http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_view.h (right): http://codereview.chromium.org/6609047/diff/13001/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_view.h:73: int VerticalOverlap() const { return AnimatedTabHeight(); } On 2011/03/09 00:41:45, Peter Kasting wrote: > Nit: Just make AnimatedTabHeight() public. > > If you want to preserve this function you need to de-inline it, and at that > point it's not obvious in the header that it does the same thing as the other, > so blah. Just publicize the other API call. Done.
Still LGTM http://codereview.chromium.org/6609047/diff/13002/chrome/browser/ui/views/inf... File chrome/browser/ui/views/infobars/infobar_view.h (right): http://codereview.chromium.org/6609047/diff/13002/chrome/browser/ui/views/inf... chrome/browser/ui/views/infobars/infobar_view.h:74: int AnimatedTabHeight() const; Nit: Probably should comment this and AnimatedBarHeight() |
