Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8265005:
first run bubble using the views/bubble api. (Closed)
|
Created:
9 years, 2 months ago by alicet1 Modified:
9 years, 1 month ago CC:
chromium-reviews, Emmanuel Saint-loubert-Bié, jennyz Visibility:
Public. |
Descriptionfirst run bubble using the views/bubble api.
BUG=98323, 51704
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110154
Patch Set 1 #
Total comments: 1
Patch Set 2 : more refactoring #Patch Set 3 : minor update #Patch Set 4 : update #Patch Set 5 : update #Patch Set 6 : fix defines. #
Total comments: 36
Patch Set 7 : add layout manager. #Patch Set 8 : updated per commets #Patch Set 9 : updated per comments. #Patch Set 10 : update #Patch Set 11 : fix defines #Patch Set 12 : fix defines. #
Total comments: 48
Patch Set 13 : update. #
Total comments: 28
Patch Set 14 : update, add test, remove large and oem flavor of bubble. #Patch Set 15 : update. #
Total comments: 8
Patch Set 16 : updates #Patch Set 17 : update #Patch Set 18 : update #
Total comments: 42
Patch Set 19 : updates. #Patch Set 20 : update #Patch Set 21 : fix bubble offset #Patch Set 22 : update to windows code #Patch Set 23 : update #Patch Set 24 : update #Patch Set 25 : update #Patch Set 26 : update #Patch Set 27 : merge #Patch Set 28 : update includes. #Patch Set 29 : update comments #Patch Set 30 : update #
Total comments: 36
Patch Set 31 : update #Patch Set 32 : update #Patch Set 33 : style #
Total comments: 22
Patch Set 34 : update #Patch Set 35 : update #Patch Set 36 : update chrome_tests.gypi #Messages
Total messages: 34 (0 generated)
Depending on how quickly I'll be able to land the views bubble change, you might want to base your branches off of a local copy of my change... that should facilitate development in the mean time. Let me know if you want help doing that. http://codereview.chromium.org/8265005/diff/1/chrome/browser/ui/views/first_r... File chrome/browser/ui/views/first_run_bubble_view_views.h (right): http://codereview.chromium.org/8265005/diff/1/chrome/browser/ui/views/first_r... chrome/browser/ui/views/first_run_bubble_view_views.h:49: virtual views::Widget* GetWidget() OVERRIDE; You shouldn't need to override GetWidget/GetContentsView, etc. Nor should you need to store a parent Widget*. See my reviews views/examples/bubble_example.cc for, well, an example :)
I'll move to the new api that you have under review in a bit. In the mean time, more clean up about the files inclusions and compiled chrome with this under use_aura=1. will add tests also. thanx, alice
updated with the new bubbles changes. thanx, alice
Some initial feedback. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... File chrome/browser/ui/views/first_run_bubble.cc (left): http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:30: leave this line there. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:515: view->BubbleShown(); Do you need to still call BubbleShown? http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:9: #include "chrome/browser/ui/views/bubble/bubble.h" // for kBackgroundColor really? this seems wrong, let's not mix bubbles. Assume the next step is deleting those files; would we move kBackgroundColor here or into the new views bubble delegate header as the default GetColor value, or as a seperate constant? http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:31: #include "views/widget/native_widget_win.h" Can we avoid this platform specific include altogether? http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:52: HWND GetLogicalBubbleOwner(HWND bubble_hwnd) { You should eliminate this function (along with EnableParent) in this bubble upgrade. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:518: AddChildView(view); You might need to use a FillLayout to make the child view bounds match this view's bounds. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:549: return preferred_size_; preferred_size_ and this override are unnecessary as far as I can tell; remove them. The preferred size calculations should work just fine as long as you add FirstRunBubbleViewBase view to this view in Init(). http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:551: views::View* FirstRunBubble::GetInitiallyFocusedView() { insert a blank line between these function definitions. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:555: #if !defined(USE_AURA) You've got an OS_WIN block above for GetLogicalBubbleOwner, but !USE_AURA here, this should be OS_WIN if it can't be eliminated altogether. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:556: void FirstRunBubble::EnableParent() { This just seems wrong altogether... Can you safely eliminate this and GetLogicalBubbleOwner, perhaps using Widget/FocusManager API instead? http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:601: #if !defined(USE_AURA) This seems like it won't work in any case... I think we need to add some BubbleClosing API to the new bubble (I have the same roadblock in CriticalNotificationBubbleView) http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... File chrome/browser/ui/views/first_run_bubble.h (right): http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.h:40: virtual void Init(); Please try to match the order and visibility (Init is protected) of these OVERRIDEs. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.h:42: // View override: Add a blank line before new "* overrides:" blocks. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.h:58: virtual void BubbleClosing(Bubble* bubble, bool closed_by_escape) OVERRIDE; What is the Bubble type here? We should be careful not to mix bubbles... http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.h:63: const gfx::Rect position_relative_to_; I thought you liked 'anchor' :) http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.h:67: // pointer into content view for initially focused view. Not owned. Capitalize 'p'; perhaps BubbleDelegateView should override GetInitiallyFocusedView to return itself? If there are any focus issues to sort out with the bubble/delegate, it would be better to solve them on the base class. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/loca... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/loca... chrome/browser/ui/views/location_bar/location_bar_view.cc:64: #if defined(OS_WIN) || defined(USE_AURA) What's blocking this for cross-platform use? If we just don't want it on Chrome OS, then this should just be !defined(OS_CHROMEOS) http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/loca... chrome/browser/ui/views/location_bar/location_bar_view.cc:1024: #if defined(OS_WIN) || (defined(USE_AURA) && !defined(OS_CHROMEOS)) If we just don't want it on Chrome OS, then this should just be !defined(OS_CHROMEOS)
http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... File chrome/browser/ui/views/first_run_bubble.cc (left): http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:30: On 2011/10/18 18:39:23, msw wrote: > leave this line there. Done. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:515: view->BubbleShown(); On 2011/10/18 18:39:23, msw wrote: > Do you need to still call BubbleShown? re-added. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:9: #include "chrome/browser/ui/views/bubble/bubble.h" // for kBackgroundColor On 2011/10/18 18:39:23, msw wrote: > really? this seems wrong, let's not mix bubbles. Assume the next step is > deleting those files; would we move kBackgroundColor here or into the new views > bubble delegate header as the default GetColor value, or as a seperate constant? I think you're handling this one already, so I'll leave it here. The constant should move from old bubbles to views/bubbles. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:31: #include "views/widget/native_widget_win.h" On 2011/10/18 18:39:23, msw wrote: > Can we avoid this platform specific include altogether? I'll remove it when I manage to remove the two OS_WIN blocks below, not entirely sure I can remove the EnableParent part. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:52: HWND GetLogicalBubbleOwner(HWND bubble_hwnd) { On 2011/10/18 18:39:23, msw wrote: > You should eliminate this function (along with EnableParent) in this bubble > upgrade. removed this. EnableParent is another one I'm not sure yet. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:518: AddChildView(view); On 2011/10/18 18:39:23, msw wrote: > You might need to use a FillLayout to make the child view bounds match this > view's bounds. Done. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:549: return preferred_size_; On 2011/10/18 18:39:23, msw wrote: > preferred_size_ and this override are unnecessary as far as I can tell; remove > them. The preferred size calculations should work just fine as long as you add > FirstRunBubbleViewBase view to this view in Init(). Done. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:551: views::View* FirstRunBubble::GetInitiallyFocusedView() { On 2011/10/18 18:39:23, msw wrote: > insert a blank line between these function definitions. removed http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:555: #if !defined(USE_AURA) On 2011/10/18 18:39:23, msw wrote: > You've got an OS_WIN block above for GetLogicalBubbleOwner, but !USE_AURA here, > this should be OS_WIN if it can't be eliminated altogether. Done. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:556: void FirstRunBubble::EnableParent() { On 2011/10/18 18:39:23, msw wrote: > This just seems wrong altogether... Can you safely eliminate this and > GetLogicalBubbleOwner, perhaps using Widget/FocusManager API instead? Got rid of the GetLogicalBubbleOwner, not sure if I can get rid of this method all together. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.cc:601: #if !defined(USE_AURA) On 2011/10/18 18:39:23, msw wrote: > This seems like it won't work in any case... > I think we need to add some BubbleClosing API to the new bubble (I have the same > roadblock in CriticalNotificationBubbleView) updated. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... File chrome/browser/ui/views/first_run_bubble.h (right): http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.h:40: virtual void Init(); On 2011/10/18 18:39:23, msw wrote: > Please try to match the order and visibility (Init is protected) of these > OVERRIDEs. Done. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.h:42: // View override: On 2011/10/18 18:39:23, msw wrote: > Add a blank line before new "* overrides:" blocks. Done. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.h:58: virtual void BubbleClosing(Bubble* bubble, bool closed_by_escape) OVERRIDE; On 2011/10/18 18:39:23, msw wrote: > What is the Bubble type here? > We should be careful not to mix bubbles... you mean, when the bubble close, do we care how it closed here? All the 3 bubbles in this file don't want close by escape behavior. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.h:63: const gfx::Rect position_relative_to_; On 2011/10/18 18:39:23, msw wrote: > I thought you liked 'anchor' :) I do, but the caller (location_bar) called it position_relative_to, so I left it here. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/firs... chrome/browser/ui/views/first_run_bubble.h:67: // pointer into content view for initially focused view. Not owned. On 2011/10/18 18:39:23, msw wrote: > Capitalize 'p'; perhaps BubbleDelegateView should override > GetInitiallyFocusedView to return itself? If there are any focus issues to sort > out with the bubble/delegate, it would be better to solve them on the base > class. Done. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/loca... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/loca... chrome/browser/ui/views/location_bar/location_bar_view.cc:64: #if defined(OS_WIN) || defined(USE_AURA) On 2011/10/18 18:39:23, msw wrote: > What's blocking this for cross-platform use? > If we just don't want it on Chrome OS, then this should just be > !defined(OS_CHROMEOS) I think this has to do with FirstRunBubble depends on NativeWidgetWin, still does for OS_WIN until we get rid of those bits. I changed the defines a bit. http://codereview.chromium.org/8265005/diff/9006/chrome/browser/ui/views/loca... chrome/browser/ui/views/location_bar/location_bar_view.cc:1024: #if defined(OS_WIN) || (defined(USE_AURA) && !defined(OS_CHROMEOS)) On 2011/10/18 18:39:23, msw wrote: > If we just don't want it on Chrome OS, then this should just be > !defined(OS_CHROMEOS) Done.
http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:23: #include "views/bubble/bubble_view.h" remove to merge with the latest bubble changes. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:53: class FirstRunBubbleViewBase : public views::View, This should be merged with FirstRunBubble (it's exactly what BubbleDelegateView subclasses are for). Unfortunately, that *might* be complicated by all the various FirstRunBubbleViews. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:59: virtual void BubbleShown() = 0; remove extra space http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:67: // Override from FirstRunBubbleViewBase: Add a blank line above this comment. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:81: FirstRunBubble* bubble_window_; bubble_window_ and profile_ are use on all three FirstRunBubbleViewBase subclasses. How about moving them there and adding protected accessors if they're needed by the bubble subclasses? http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:144: GetFocusManager()->RemoveFocusChangeListener(this); This belongs in ~FirstRunBubbleViewBase. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:151: GetWidget()->client_view()->AsBubbleView()->StartFade(false); You can just call StartFade on a BubbleDelegateView subclass with the latest changes. This should be made a BubbleDelegateView subclass via FirstRunBubbleViewBase, but if you don't do that, I guess you can static_cast<BubbleDelegateView>(GetWidget->widget_delegate())->StartFade. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:165: keep_button_->RequestFocus(); Couldn't this be accomplished by overriding GetInitiallyFocusedView? With that, this function may not be needed after all. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:241: private: add a blank line above private. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:315: GetFocusManager()->RemoveFocusChangeListener(this); Ditto, this belongs in ~FirstRunBubbleViewBase. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:322: GetWidget()->client_view()->AsBubbleView()->StartFade(/*fade_in=*/false); Ditto on BubbleDelegateView::StartFade comment, also remove the "/*fade_in=*/" http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:373: // stored in a variable because it's a hack and should go away. cry :( even if this comment is [still] true, switching by aero glass isn't equivalent to switching on xp/vista, although it might be what's actually intended by this hack. It'd be great if you could remove this and ensure that's okay. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:394: // Override from FirstRunBubbleViewBase: Can you go through and make sure all the override comments in these files are consistent? "override/overriden from"/[x]/"overrides"... http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:448: GetFocusManager()->RemoveFocusChangeListener(this); Ditto, this belongs in ~FirstRunBubbleViewBase. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:506: widget->client_view()->AsBubbleView()->StartFade(/*fade_in=*/true); Ditto on BubbleDelegateView::StartFade comment, also remove the "/*fade_in=*/" http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:527: parent_->GetFocusManager()->AddFocusChangeListener(view); I think you can just call GetFocusManager on this view. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:537: : profile_(profile), You can use the new BubbleDelegateView ctor here to set the anchor and arrow, and then you don't need to override GetAnchorPoint http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:598: parent_->GetRootView()->SetEnabled(true); I don't think this is right... perhaps try the FocusManager? http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.h (right): http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:15: namespace views { You shouldn't need these forward declarations, since bubble_delegate.h already includes/forward declares them. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:60: const gfx::Rect position_relative_to_; merge and you won't need to store position_relative_to_ nor arrow_location_, these can be passed directly to the new BubbleDelegateView ctor http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:64: #if defined(OS_WIN) || !defined(OS_CHROMEOS) Shouldn't this just be: "#if !defined(OS_CHROMEOS)"? http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1024: #if defined(OS_WIN) || !defined(OS_CHROMEOS) Ditto, shouldn't this just be: "#if !defined(OS_CHROMEOS)"? http://codereview.chromium.org/8265005/diff/18006/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/8265005/diff/18006/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:5074: ['include', '^browser/ui/views/first_run_bubble.cc'], I don't think this explicit include is required, but I'm not sure... http://codereview.chromium.org/8265005/diff/18006/views/bubble/bubble_delegat... File views/bubble/bubble_delegate.cc (right): http://codereview.chromium.org/8265005/diff/18006/views/bubble/bubble_delegat... views/bubble/bubble_delegate.cc:50: return this; already done, will land soon :) http://codereview.chromium.org/8265005/diff/18006/views/bubble/bubble_delegate.h File views/bubble/bubble_delegate.h (right): http://codereview.chromium.org/8265005/diff/18006/views/bubble/bubble_delegat... views/bubble/bubble_delegate.h:30: virtual View* GetInitiallyFocusedView() OVERRIDE; already done, will land soon :)
I think we should move the bubble background stuff in bubble.h in another CL. seems like most bubble would want their background that is the current default. thanx, alice http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:23: #include "views/bubble/bubble_view.h" On 2011/10/20 19:17:50, msw wrote: > remove to merge with the latest bubble changes. Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:53: class FirstRunBubbleViewBase : public views::View, On 2011/10/20 19:17:50, msw wrote: > This should be merged with FirstRunBubble (it's exactly what BubbleDelegateView > subclasses are for). Unfortunately, that *might* be complicated by all the > various FirstRunBubbleViews. I think I'll leave this like this, the three views are slightly different, and laying them out seems clearer. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:59: virtual void BubbleShown() = 0; On 2011/10/20 19:17:50, msw wrote: > remove extra space Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:67: // Override from FirstRunBubbleViewBase: On 2011/10/20 19:17:50, msw wrote: > Add a blank line above this comment. Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:81: FirstRunBubble* bubble_window_; On 2011/10/20 19:17:50, msw wrote: > bubble_window_ and profile_ are use on all three FirstRunBubbleViewBase > subclasses. How about moving them there and adding protected accessors if > they're needed by the bubble subclasses? Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:144: GetFocusManager()->RemoveFocusChangeListener(this); On 2011/10/20 19:17:50, msw wrote: > This belongs in ~FirstRunBubbleViewBase. Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:151: GetWidget()->client_view()->AsBubbleView()->StartFade(false); On 2011/10/20 19:17:50, msw wrote: > You can just call StartFade on a BubbleDelegateView subclass with the latest > changes. This should be made a BubbleDelegateView subclass via > FirstRunBubbleViewBase, but if you don't do that, I guess you can > static_cast<BubbleDelegateView>(GetWidget->widget_delegate())->StartFade. Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:165: keep_button_->RequestFocus(); On 2011/10/20 19:17:50, msw wrote: > Couldn't this be accomplished by overriding GetInitiallyFocusedView? > With that, this function may not be needed after all. if this is a delegate, then it would have GetInitiallyFocusedView. Since we kept the original implementation of separate views, so this method stays. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:241: private: On 2011/10/20 19:17:50, msw wrote: > add a blank line above private. Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:315: GetFocusManager()->RemoveFocusChangeListener(this); On 2011/10/20 19:17:50, msw wrote: > Ditto, this belongs in ~FirstRunBubbleViewBase. Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:322: GetWidget()->client_view()->AsBubbleView()->StartFade(/*fade_in=*/false); On 2011/10/20 19:17:50, msw wrote: > Ditto on BubbleDelegateView::StartFade comment, also remove the "/*fade_in=*/" Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:373: // stored in a variable because it's a hack and should go away. On 2011/10/20 19:17:50, msw wrote: > cry :( even if this comment is [still] true, switching by aero glass isn't > equivalent to switching on xp/vista, although it might be what's actually > intended by this hack. It'd be great if you could remove this and ensure that's > okay. I can probably believe this is still true. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:394: // Override from FirstRunBubbleViewBase: On 2011/10/20 19:17:50, msw wrote: > Can you go through and make sure all the override comments in these files are > consistent? "override/overriden from"/[x]/"overrides"... Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:448: GetFocusManager()->RemoveFocusChangeListener(this); On 2011/10/20 19:17:50, msw wrote: > Ditto, this belongs in ~FirstRunBubbleViewBase. Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:506: widget->client_view()->AsBubbleView()->StartFade(/*fade_in=*/true); On 2011/10/20 19:17:50, msw wrote: > Ditto on BubbleDelegateView::StartFade comment, also remove the "/*fade_in=*/" Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:527: parent_->GetFocusManager()->AddFocusChangeListener(view); On 2011/10/20 19:17:50, msw wrote: > I think you can just call GetFocusManager on this view. Init() is called before we have a widget for this view, that will mean a GetWidget() call inside GetFocusManager() will fail and not return a proper focus manager. parent_ seems safer. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:537: : profile_(profile), On 2011/10/20 19:17:50, msw wrote: > You can use the new BubbleDelegateView ctor here to set the anchor and arrow, > and then you don't need to override GetAnchorPoint Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:598: parent_->GetRootView()->SetEnabled(true); On 2011/10/20 19:17:50, msw wrote: > I don't think this is right... perhaps try the FocusManager? I'm not sure why we need to explicitly enable/set focus for the parent, assuming this bubble is already closing? http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.h (right): http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:15: namespace views { On 2011/10/20 19:17:50, msw wrote: > You shouldn't need these forward declarations, since bubble_delegate.h already > includes/forward declares them. Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:60: const gfx::Rect position_relative_to_; On 2011/10/20 19:17:50, msw wrote: > merge and you won't need to store position_relative_to_ nor arrow_location_, > these can be passed directly to the new BubbleDelegateView ctor Done. http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/18006/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:64: #if defined(OS_WIN) || !defined(OS_CHROMEOS) On 2011/10/20 19:17:50, msw wrote: > Shouldn't this just be: "#if !defined(OS_CHROMEOS)"? Done. http://codereview.chromium.org/8265005/diff/18006/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/8265005/diff/18006/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:5074: ['include', '^browser/ui/views/first_run_bubble.cc'], On 2011/10/20 19:17:50, msw wrote: > I don't think this explicit include is required, but I'm not sure... I tried removing it, but the generated make file omitted the file, so I added it back. http://codereview.chromium.org/8265005/diff/18006/views/bubble/bubble_delegate.h File views/bubble/bubble_delegate.h (right): http://codereview.chromium.org/8265005/diff/18006/views/bubble/bubble_delegat... views/bubble/bubble_delegate.h:30: virtual View* GetInitiallyFocusedView() OVERRIDE; On 2011/10/20 19:17:50, msw wrote: > already done, will land soon :) Done.
Sorry this fell off my radar earlier this week. Please add a second reviewer, especially for the gyp changes. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (left): http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:71: Replace this blank line. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:72: Profile* profile) Fix this indenting. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:85: void BubbleShown(); Please use the OVERRIDE keyword for this and other overrides. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:120: label1_->SetBackgroundColor(SK_ColorWHITE); Define a local constant as white to use instead of Bubble::kBackgroundColor here and elsewhere, so fixing this later isn't as troublesome. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:162: if (change_button_ == sender) { Code after StartFade is technically in a race condition with the bubble closing and this view being deleted, right? I wonder if this should be executed before starting the fade, or be made robust to the destruction of |this|. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:164: UserMetricsAction("FirstRunBubbleView_ChangeButton")); Fix this indenting. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:168: browser->OpenSearchEngineOptionsDialog(); Single line if doesn't require braces. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:171: GetWidget()->Close(); Why is this closing the bubble mid-fade? http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:225: void FirstRunBubbleView::FocusWillChange(View* focused_before, This is the only view that actually watches focus changes, can you move the focus listener add/remove to just this class and scrap the other empty overrides and base class involvement? http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:319: RequestFocus(); This probably isn't necessary. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:406: const views::Event& event) { } I think inline virtual overrides break clang. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:594: GetFocusManager()->FocusNativeView(parent_->GetNativeView()); Can you check if this is necessary? I suspect it isn't. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1024: #if defined(OS_WIN) || !defined(OS_CHROMEOS) Again, shouldn't this just be "#if !defined(OS_CHROMEOS)"?
http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:120: label1_->SetBackgroundColor(SK_ColorWHITE); On 2011/10/29 02:55:11, msw wrote: > Define a local constant as white to use instead of Bubble::kBackgroundColor here > and elsewhere, so fixing this later isn't as troublesome. Actually, I suspect all of these label backgrounds are unnecessary, you should try removing them and let the bubble's background show. Otherwise, if possible, you should use the delegate's GetColor() instead of a constant.
changed this a bit, unified the three bubble flavors. adding miranda for the review also. thanx, alice http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (left): http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:71: On 2011/10/29 02:55:11, msw wrote: > Replace this blank line. Done. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:72: Profile* profile) On 2011/10/29 02:55:11, msw wrote: > Fix this indenting. Done. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:85: void BubbleShown(); On 2011/10/29 02:55:11, msw wrote: > Please use the OVERRIDE keyword for this and other overrides. removed. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:120: label1_->SetBackgroundColor(SK_ColorWHITE); On 2011/10/30 21:02:15, msw wrote: > On 2011/10/29 02:55:11, msw wrote: > > Define a local constant as white to use instead of Bubble::kBackgroundColor > here > > and elsewhere, so fixing this later isn't as troublesome. > > Actually, I suspect all of these label backgrounds are unnecessary, you should > try removing them and let the bubble's background show. Otherwise, if possible, > you should use the delegate's GetColor() instead of a constant. removed. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:162: if (change_button_ == sender) { On 2011/10/29 02:55:11, msw wrote: > Code after StartFade is technically in a race condition with the bubble closing > and this view being deleted, right? I wonder if this should be executed before > starting the fade, or be made robust to the destruction of |this|. this is removed as part of removing this bubble -- the large version of this bubble is no longer shown. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:164: UserMetricsAction("FirstRunBubbleView_ChangeButton")); On 2011/10/29 02:55:11, msw wrote: > Fix this indenting. Done. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:168: browser->OpenSearchEngineOptionsDialog(); On 2011/10/29 02:55:11, msw wrote: > Single line if doesn't require braces. removed. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:171: GetWidget()->Close(); On 2011/10/29 02:55:11, msw wrote: > Why is this closing the bubble mid-fade? I changed it to close only. I'm not sure why a button press should be a fade instead of a direct close. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:225: void FirstRunBubbleView::FocusWillChange(View* focused_before, On 2011/10/29 02:55:11, msw wrote: > This is the only view that actually watches focus changes, can you move the > focus listener add/remove to just this class and scrap the other empty overrides > and base class involvement? removed other classes. we talked about this -- I think when there is an explicit focus change, bubble should close. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:319: RequestFocus(); On 2011/10/29 02:55:11, msw wrote: > This probably isn't necessary. removed. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:406: const views::Event& event) { } On 2011/10/29 02:55:11, msw wrote: > I think inline virtual overrides break clang. removed. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:594: GetFocusManager()->FocusNativeView(parent_->GetNativeView()); On 2011/10/29 02:55:11, msw wrote: > Can you check if this is necessary? I suspect it isn't. as in, when bubble widget is closing, parent should automatically get the focus? I'm actually not sure if L550 - 590 is needed. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1024: #if defined(OS_WIN) || !defined(OS_CHROMEOS) On 2011/10/29 02:55:11, msw wrote: > Again, shouldn't this just be "#if !defined(OS_CHROMEOS)"? dropped.
LGTM with nits. http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/24001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:594: GetFocusManager()->FocusNativeView(parent_->GetNativeView()); On 2011/11/02 06:51:25, alicet1 wrote: > On 2011/10/29 02:55:11, msw wrote: > > Can you check if this is necessary? I suspect it isn't. > > as in, when bubble widget is closing, parent should automatically get the focus? > I'm actually not sure if L550 - 590 is needed. I remember there being bizarre focus problems that we had to work around when the first run bubble was first introduced years ago; if these are no longer a problem, it would be great if we could get rid of some of this code. http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:53: void FocusWillChange(View* focused_before, View* focused_now); should also have OVERRIDE. http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:192: // TODO(beng): this only works in custom-frame mode, not glass-frame mode. Is this TODO still valid, or did it refer to the GetLogicalBubbleOwner and GetWidgetForNativeView lines? http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1035: // int location_y = kVerticalEdgeThickness; remove commented-out code. http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1037: // const int kYOffset = -(kVerticalEdgeThickness * 2); same as above.
adding jenny, who is working on first run. thanx, alice
thanks miranda. moved up 3 px per glen on the new bubble look (minimal bubble text + cross on top right.) still need test on old and new windows. mike can you look at the focus change, whether your change will replace that? adding ben to see if I still need the windows specific enable parent code. thanx, alice http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:53: void FocusWillChange(View* focused_before, View* focused_now); On 2011/11/02 15:36:14, Miranda Callahan wrote: > should also have OVERRIDE. oops. sorry, it's an implementation of this interface, not an override. http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:192: // TODO(beng): this only works in custom-frame mode, not glass-frame mode. On 2011/11/02 15:36:14, Miranda Callahan wrote: > Is this TODO still valid, or did it refer to the GetLogicalBubbleOwner and > GetWidgetForNativeView lines? ah right, this is left there so that I remember to test on glass. I should update the comment in L185 instead and remove this. http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1035: // int location_y = kVerticalEdgeThickness; On 2011/11/02 15:36:14, Miranda Callahan wrote: > remove commented-out code. Done. http://codereview.chromium.org/8265005/diff/33001/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1037: // const int kYOffset = -(kVerticalEdgeThickness * 2); On 2011/11/02 15:36:14, Miranda Callahan wrote: > same as above. Done.
lgtm http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:234: GetFocusManager()->FocusNativeView(parent_->GetNativeView()); Not your fault, just an observation: the comment here and the actions taken as a result aren't obvious.
Unfortunatley, the three seperate first run bubbles are still implemented in GTK at chrome/browser/ui/gtk/first_run_bubble.h|cc. I think Mac is okay (from looking at the code), but we should make sure the platforms are consistent (at least file a bug to follow up on GTK and remove unused string resources, etc.). http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (left): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:113: label1_ = new views::Label(l10n_util::GetStringUTF16(IDS_FR_BUBBLE_TITLE)); Can you remove any string resources that are now unused? (or file a bug to remove them once GTK is fixed?) http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:36: class FirstRunBubbleView : public views::View, It's very awesome that we've consolidated the first run bubbles to a single bubble; can you combine the FirstRunBubbleView and FirstRunBubble? It shouldn't be difficult. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:59: views::Label* label3_; label3_ is no longer used, please remove it. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:86: l10n_util::GetStringUTF16(IDS_FR_BUBBLE_SUBTEXT)); This should fit on the line above. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:128: UserMetricsAction("FirstRunOEMBubbleView_Clicked")); We should make sure we're recording the right metric here, updating the metrics for the GTK bubble, etc. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:137: void FirstRunBubbleView::FocusWillChange(View* focused_before, This is unfortunately wrong, but luckily you can remove this code altogether, since I'm working on closing all bubbles when they're deactivated in http://codereview.chromium.org/8384020/. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:190: // TODO(alicet): do I still need this block. Must check on glass-frame mode. It looks like this code is meant to prevent the bubble from closing due to accidental deactivation (not by the user, but by some windowing problem). Can you find out if the bubble is actually trying to prevent the user from dismissing it within 3 seconds? that seems unlikely and probably bad. If that's not the case, then I'm hoping that this code will not be needed with the new bubble and the close-on-deactivation change I'm working on now. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:234: GetFocusManager()->FocusNativeView(parent_->GetNativeView()); This also shouldn't be necessary, or if it is, it's probably needed for every bubble; I hope I can address this in the close-on-deactivation change. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble_unittest.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:6: #include "base/string_util.h" I don't think string_util.h is needed. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:7: #include "base/utf_string_conversions.h" I don't think utf_string_conversions.h is needed. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:14: #include "views/view.h" I don't think view.h is needed. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:23: void SetUp() { SetUp and TearDown are protected virtual method overrides, they should be defined outside the declaration, kept protected, and marked as OVERRIDESs with the appropriate comment. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:26: TemplateURLService* turl_model = Wow is all this really needed for a TestingProfile? Miranda probably knows the simplest test/mock profile we can use for this test. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:35: MessageLoopForUI message_loop_; These data members should be made private. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:39: void RunPendingMessages() { It seems like this function's body could be put directly in TearDown. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:40: #if defined(USE_AURA) Hopefully Ben or someone more familiar with aura testing can comment on this, but it seems to do the right thing. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1031: int location_height = std::max(height() - (kVerticalEdgeThickness * 2), 0); Shouldn't the vertical position be in terms of the location_icon_view_ y position and height? http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1032: const int kXOffset = ResourceBundle::GetSharedInstance().GetBitmapNamed( Shouldn't this be simply getting the middle of the location_icon_view_? What if the icon is scaled inside the view? So instead of kXOffset, you should just use location_icon_view_.width(). If I'm wrong, you've got an extra space after the '='. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1034: gfx::Point origin(location_icon_view_->bounds().x() + kXOffset, you can just call x() on the view. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1035: location_height - 1); Why '- 1'?
http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:190: // TODO(alicet): do I still need this block. Must check on glass-frame mode. On 2011/11/03 20:25:38, msw wrote: > It looks like this code is meant to prevent the bubble from closing due to > accidental deactivation (not by the user, but by some windowing problem). Can > you find out if the bubble is actually trying to prevent the user from > dismissing it within 3 seconds? that seems unlikely and probably bad. If that's > not the case, then I'm hoping that this code will not be needed with the new > bubble and the close-on-deactivation change I'm working on now. Yes, that's indeed what's happening; on Windows, there are (were) painful focus-grabbing issues that needed to be worked around. If you can get rid of this hack, it would be great. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble_unittest.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:26: TemplateURLService* turl_model = On 2011/11/03 20:25:38, msw wrote: > Wow is all this really needed for a TestingProfile? > Miranda probably knows the simplest test/mock profile we can use for this test. I don't think you have to do all this -- would it make sense to just use a TestingProfile, and mock in Google for the DefaultSearchProvider?
http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:190: // TODO(alicet): do I still need this block. Must check on glass-frame mode. On 2011/11/03 20:59:39, Miranda Callahan wrote: > On 2011/11/03 20:25:38, msw wrote: > > It looks like this code is meant to prevent the bubble from closing due to > > accidental deactivation (not by the user, but by some windowing problem). Can > > you find out if the bubble is actually trying to prevent the user from > > dismissing it within 3 seconds? that seems unlikely and probably bad. If > that's > > not the case, then I'm hoping that this code will not be needed with the new > > bubble and the close-on-deactivation change I'm working on now. > > Yes, that's indeed what's happening; on Windows, there are (were) painful > focus-grabbing issues that needed to be worked around. If you can get rid of > this hack, it would be great. Please see this CL, and accompanying comments for some background: http://codereview.chromium.org/3141015
http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:190: // TODO(alicet): do I still need this block. Must check on glass-frame mode. Okay, if that's the case and we still have this mysterious(?) activation problem (I'm still not quite sure what's causing activation loss on the bubble), then we should be able to initially set the new close_on_deactivate to false for the bubble on create, then start a three second timer which will set that value to true when it fires.
addressed some of the changes, not all yet. working on the test again, and the pixel setting. thanx, alice http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (left): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:113: label1_ = new views::Label(l10n_util::GetStringUTF16(IDS_FR_BUBBLE_TITLE)); On 2011/11/03 20:25:38, msw wrote: > Can you remove any string resources that are now unused? (or file a bug to > remove them once GTK is fixed?) I'll file a bug to clean when cleaning the gtk case. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:36: class FirstRunBubbleView : public views::View, On 2011/11/03 20:25:38, msw wrote: > It's very awesome that we've consolidated the first run bubbles to a single > bubble; can you combine the FirstRunBubbleView and FirstRunBubble? It shouldn't > be difficult. giving it a try. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:59: views::Label* label3_; On 2011/11/03 20:25:38, msw wrote: > label3_ is no longer used, please remove it. Done. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:86: l10n_util::GetStringUTF16(IDS_FR_BUBBLE_SUBTEXT)); On 2011/11/03 20:25:38, msw wrote: > This should fit on the line above. Done. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:128: UserMetricsAction("FirstRunOEMBubbleView_Clicked")); On 2011/11/03 20:25:38, msw wrote: > We should make sure we're recording the right metric here, updating the metrics > for the GTK bubble, etc. Interesting, I think this may be the only case we need the bubble type again. The metric was only updated for oem case, since minimal bubble never had a button. in gtk, there was never an oem bubble (it always falls back to minimal), so this metric would not have been updated there. http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/gtk/first_r... http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:137: void FirstRunBubbleView::FocusWillChange(View* focused_before, On 2011/11/03 20:25:38, msw wrote: > This is unfortunately wrong, but luckily you can remove this code altogether, > since I'm working on closing all bubbles when they're deactivated in > http://codereview.chromium.org/8384020/. Done. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:190: // TODO(alicet): do I still need this block. Must check on glass-frame mode. On 2011/11/03 21:46:24, msw wrote: > Okay, if that's the case and we still have this mysterious(?) activation problem > (I'm still not quite sure what's causing activation loss on the bubble), then we > should be able to initially set the new close_on_deactivate to false for the > bubble on create, then start a three second timer which will set that value to > true when it fires. ok, I'll work on that. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:234: GetFocusManager()->FocusNativeView(parent_->GetNativeView()); On 2011/11/03 20:25:38, msw wrote: > This also shouldn't be necessary, or if it is, it's probably needed for every > bubble; I hope I can address this in the close-on-deactivation change. removed. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble_unittest.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:6: #include "base/string_util.h" On 2011/11/03 20:25:38, msw wrote: > I don't think string_util.h is needed. Done. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:7: #include "base/utf_string_conversions.h" On 2011/11/03 20:25:38, msw wrote: > I don't think utf_string_conversions.h is needed. Done. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:14: #include "views/view.h" On 2011/11/03 20:25:38, msw wrote: > I don't think view.h is needed. Done. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:23: void SetUp() { On 2011/11/03 20:25:38, msw wrote: > SetUp and TearDown are protected virtual method overrides, they should be > defined outside the declaration, kept protected, and marked as OVERRIDESs with > the appropriate comment. Done. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:26: TemplateURLService* turl_model = On 2011/11/03 20:59:39, Miranda Callahan wrote: > On 2011/11/03 20:25:38, msw wrote: > > Wow is all this really needed for a TestingProfile? > > Miranda probably knows the simplest test/mock profile we can use for this > test. > > I don't think you have to do all this -- would it make sense to just use a > TestingProfile, and mock in Google for the DefaultSearchProvider? > I tried that route first, and I think it worked out to about the same amount of code, yeah I need to set the search provider, so that requires a template url and model somewhere. I can try the mocks again to see if it looks better. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:35: MessageLoopForUI message_loop_; On 2011/11/03 20:25:38, msw wrote: > These data members should be made private. Done. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:39: void RunPendingMessages() { On 2011/11/03 20:25:38, msw wrote: > It seems like this function's body could be put directly in TearDown. Done. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:40: #if defined(USE_AURA) On 2011/11/03 20:25:38, msw wrote: > Hopefully Ben or someone more familiar with aura testing can comment on this, > but it seems to do the right thing. yeah, this is from views_test_base.h, on how aura desktop should do the message dispatching. I've made this a subclass of that instead, that is how the views unittests can get the message loop working properly for aura. the downside is I'll drag in the views_test_base/delegate into unit_tests, whenever views lib is included in the setting. http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/37002/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1031: int location_height = std::max(height() - (kVerticalEdgeThickness * 2), 0); On 2011/11/03 20:25:38, msw wrote: > Shouldn't the vertical position be in terms of the location_icon_view_ y > position and height? I remembered trying that and the bubble was positioned still too low, hence you see the odd x and y offset here. I'll give it a try again.
I tested the code on windows 7 and xp with the activation code removed, and the bubble seems to be still showing fine in both cases, with close_on_deactivate set. removing that code in the change. also update the insets and offsets of the bubble. thanx, alice
updated, and as discussed, PTAL. thanx, alice
As we discussed, please re-base on codereview.chromium.org/8265005. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:25: #include "views/layout/fill_layout.h" fill_layout.h isn't needed. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:49: SetLayoutManager(new views::FillLayout()); Remove this. This view is really using the GridLayout from InitContents. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:50: InitContents(); Merge Init and InitContents http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:104: // TODO(alicet): fix this after padding change goes in. Which padding change? Please fix now or be more specific. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.h (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:17: class Label; This is no longer needed when you remove label1_ and label2_. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:18: class ImageButton; This is no longer needed when you remove close_button_. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:36: virtual void ButtonPressed(views::Button* sender, Order this below the ctor and dtor, same for impls. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:51: views::Widget* parent_; This can be replaced with anchor_view()->GetWidget(); http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:55: views::Label* label1_; label1_ is only ever used in Init; remove it as a class member. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:56: views::Label* label2_; label2_ is only ever used in Init; remove it as a class member. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:57: views::ImageButton* close_button_; close_button_ is only ever used in Init; remove it as a class member. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble_unittest.cc (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:43: default_t_url_ = new TemplateURL(); Outdent these three lines by two spaces.
LGTM with nits. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:55: if (bubble_type_ == FirstRun::OEM_BUBBLE) use { }, as if clause is more than one line. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:64: #if !defined(OS_CHROMEOS) This shouldn't be included for OS_MAC, either. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1001: // to the height the bitmap instead. s/height the/height of the/ http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1015: FirstRunBubble::Show(browser_->profile(), GetWidget(), if you're going to have the args each on one line (which I like), please move GetWidget() down as well. http://codereview.chromium.org/8265005/diff/64018/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/8265005/diff/64018/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:4723: ['include', '^browser/ui/views/first_run_bubble.cc'], nit: alphabetize
http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:64: #if !defined(OS_CHROMEOS) On 2011/11/11 15:26:24, Miranda Callahan wrote: > This shouldn't be included for OS_MAC, either. I don't think we typically need to explicitly exclude OS_MAC from views files, do we?
http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:64: #if !defined(OS_CHROMEOS) On 2011/11/11 19:05:25, msw wrote: > On 2011/11/11 15:26:24, Miranda Callahan wrote: > > This shouldn't be included for OS_MAC, either. > > I don't think we typically need to explicitly exclude OS_MAC from views files, > do we? Oh, good point -- we're already in /views, so we don't need to bother.
merged in msw's changes to bubble delegate. thanx, alice http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:25: #include "views/layout/fill_layout.h" On 2011/11/11 02:30:21, msw wrote: > fill_layout.h isn't needed. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:49: SetLayoutManager(new views::FillLayout()); On 2011/11/11 02:30:21, msw wrote: > Remove this. This view is really using the GridLayout from InitContents. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:50: InitContents(); On 2011/11/11 02:30:21, msw wrote: > Merge Init and InitContents Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:55: if (bubble_type_ == FirstRun::OEM_BUBBLE) On 2011/11/11 15:26:24, Miranda Callahan wrote: > use { }, as if clause is more than one line. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:104: // TODO(alicet): fix this after padding change goes in. On 2011/11/11 02:30:21, msw wrote: > Which padding change? Please fix now or be more specific. actually, that went in, the padding looks right on linux and win now. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.h (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:17: class Label; On 2011/11/11 02:30:21, msw wrote: > This is no longer needed when you remove label1_ and label2_. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:18: class ImageButton; On 2011/11/11 02:30:21, msw wrote: > This is no longer needed when you remove close_button_. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:36: virtual void ButtonPressed(views::Button* sender, On 2011/11/11 02:30:21, msw wrote: > Order this below the ctor and dtor, same for impls. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:51: views::Widget* parent_; On 2011/11/11 02:30:21, msw wrote: > This can be replaced with anchor_view()->GetWidget(); removed http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:55: views::Label* label1_; On 2011/11/11 02:30:21, msw wrote: > label1_ is only ever used in Init; remove it as a class member. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:56: views::Label* label2_; On 2011/11/11 02:30:21, msw wrote: > label2_ is only ever used in Init; remove it as a class member. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:57: views::ImageButton* close_button_; On 2011/11/11 02:30:21, msw wrote: > close_button_ is only ever used in Init; remove it as a class member. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble_unittest.cc (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:43: default_t_url_ = new TemplateURL(); On 2011/11/11 02:30:21, msw wrote: > Outdent these three lines by two spaces. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... File chrome/browser/ui/views/location_bar/location_bar_view.cc (right): http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:64: #if !defined(OS_CHROMEOS) On 2011/11/11 15:26:24, Miranda Callahan wrote: > This shouldn't be included for OS_MAC, either. I think the mac branch is under http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/locat... and not using this file so leave OS_MAC out here is fine. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1001: // to the height the bitmap instead. On 2011/11/11 15:26:24, Miranda Callahan wrote: > s/height the/height of the/ Done. http://codereview.chromium.org/8265005/diff/64018/chrome/browser/ui/views/loc... chrome/browser/ui/views/location_bar/location_bar_view.cc:1015: FirstRunBubble::Show(browser_->profile(), GetWidget(), On 2011/11/11 15:26:24, Miranda Callahan wrote: > if you're going to have the args each on one line (which I like), please move > GetWidget() down as well. Done. http://codereview.chromium.org/8265005/diff/64018/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/8265005/diff/64018/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:4723: ['include', '^browser/ui/views/first_run_bubble.cc'], On 2011/11/11 15:26:24, Miranda Callahan wrote: > nit: alphabetize Done.
I'd appreciate it if someone else could review the gyp changes. Everything else LGTM with nits and minor fixes. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:69: rb.GetBitmapNamed(IDR_CLOSE_BAR)); outdent this line by one space. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:71: rb.GetBitmapNamed(IDR_CLOSE_BAR_H)); outdent this line by one space. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:73: rb.GetBitmapNamed(IDR_CLOSE_BAR_P)); outdent this line by one space. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:97: layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing); You can remove this padding row, and merge this extra padding with kLayoutBottomInset. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.h (right): http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:9: #include "base/basictypes.h" nit: Can you check if this include is still needed? http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:19: static FirstRunBubble* Show(Profile* profile, This overloads BubbleDelegateView::Show(), which isn't allowed by our style guidelines. Can you rename one or the other? I renamed a similar static on BookmarkBubbleView to ShowBubble(). http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:26: virtual void Init() OVERRIDE; nit: order Init after GetAnchorPoint, like BubbleDelegateView. GetAnchorPoint is declared public, so this one probably should be too. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble_unittest.cc (right): http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:12: #include "views/view.h" including view.h may not be necessary. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:45: turl_model->SetDefaultSearchProvider(default_t_url_); It seems like you should be able to SetDefaultSearchProvider, and then GetDefaultSearchEngineName will just return an empty string. Perhaps you don't even have to SetDefaultSearchProvider at all, and even more of this tangential code can go away. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:52: anchor, You should be able to just send in NULL here and remove the |anchor| local. http://codereview.chromium.org/8265005/diff/70003/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/8265005/diff/70003/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:1966: '../views/test/test_views_delegate.cc', alphabetize '../views' after '../third_party'.
will get it in, thanks. thanx, alice http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.cc (right): http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:69: rb.GetBitmapNamed(IDR_CLOSE_BAR)); On 2011/11/12 03:35:55, msw wrote: > outdent this line by one space. Done. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:71: rb.GetBitmapNamed(IDR_CLOSE_BAR_H)); On 2011/11/12 03:35:55, msw wrote: > outdent this line by one space. Done. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:73: rb.GetBitmapNamed(IDR_CLOSE_BAR_P)); On 2011/11/12 03:35:55, msw wrote: > outdent this line by one space. Done. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.cc:97: layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing); On 2011/11/12 03:35:55, msw wrote: > You can remove this padding row, and merge this extra padding with > kLayoutBottomInset. Done. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble.h (right): http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:9: #include "base/basictypes.h" On 2011/11/12 03:35:55, msw wrote: > nit: Can you check if this include is still needed? Done. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:19: static FirstRunBubble* Show(Profile* profile, On 2011/11/12 03:35:55, msw wrote: > This overloads BubbleDelegateView::Show(), which isn't allowed by our style > guidelines. Can you rename one or the other? I renamed a similar static on > BookmarkBubbleView to ShowBubble(). Done. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble.h:26: virtual void Init() OVERRIDE; On 2011/11/12 03:35:55, msw wrote: > nit: order Init after GetAnchorPoint, like BubbleDelegateView. > GetAnchorPoint is declared public, so this one probably should be too. Done. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... File chrome/browser/ui/views/first_run_bubble_unittest.cc (right): http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:12: #include "views/view.h" On 2011/11/12 03:35:55, msw wrote: > including view.h may not be necessary. Done. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:45: turl_model->SetDefaultSearchProvider(default_t_url_); On 2011/11/12 03:35:55, msw wrote: > It seems like you should be able to SetDefaultSearchProvider, and then > GetDefaultSearchEngineName will just return an empty string. Perhaps you don't > even have to SetDefaultSearchProvider at all, and even more of this tangential > code can go away. ah not quite, you'll need at least one valid template url service. updated the code to get rid of more. http://codereview.chromium.org/8265005/diff/70003/chrome/browser/ui/views/fir... chrome/browser/ui/views/first_run_bubble_unittest.cc:52: anchor, On 2011/11/12 03:35:55, msw wrote: > You should be able to just send in NULL here and remove the |anchor| local. Done. http://codereview.chromium.org/8265005/diff/70003/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/8265005/diff/70003/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:1966: '../views/test/test_views_delegate.cc', On 2011/11/12 03:35:55, msw wrote: > alphabetize '../views' after '../third_party'. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alicet@chromium.org/8265005/57010
Can't apply patch for file chrome/chrome_tests.gypi. While running patch -p1 --forward --force; patching file chrome/chrome_tests.gypi Hunk #1 FAILED at 1851. Hunk #2 succeeded at 1982 (offset 2 lines). Hunk #3 succeeded at 2188 (offset 2 lines). 1 out of 3 hunks FAILED -- saving rejects to file chrome/chrome_tests.gypi.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alicet@chromium.org/8265005/57010
Can't apply patch for file chrome/chrome_tests.gypi. While running patch -p1 --forward --force; patching file chrome/chrome_tests.gypi Hunk #1 FAILED at 1851. Hunk #2 succeeded at 1982 (offset 2 lines). Hunk #3 succeeded at 2188 (offset 2 lines). 1 out of 3 hunks FAILED -- saving rejects to file chrome/chrome_tests.gypi.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alicet@chromium.org/8265005/77002
Change committed as 110154 |
