Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831009:
Change the GetSelectedTab method of the NativeTabbedPaneView to return the tab contents. (Closed)
|
Created:
8 years, 4 months ago by markusheintz_ Modified:
8 years, 4 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix the method GetSelectedTab of the class NativeTabbedPaneView to return the tab contents of the currently selected tab.
Change the unittest to cover the NativeTabbedPaneViews and the NativeTabbedPaneWin.
BUG=139159
TEST=ui\views\control\tabbed_pane\tabbe_pane_view_unittest.cc
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149158
Patch Set 1 #
Total comments: 8
Patch Set 2 : " #
Total comments: 17
Patch Set 3 : Address comments (msw). #Patch Set 4 : Fix nits #Patch Set 5 : Address comments (msw): Remove WidgetDelegateImpl. #
Total comments: 4
Patch Set 6 : Address comments (msw). #
Total comments: 2
Patch Set 7 : Address comments(msw). #
Messages
Total messages: 13 (0 generated)
One option instead of making a separate test file for NTPW, is instead making the tests pass for NTPW and NTPV on OS_WIN && !USE_AURA, looping over each test twice with set_use_native_win_control(true/false). Generalize or use preprocessor blocks for platform/impl-specific differences as needed if you pursue this route. I prefer that over duplicating code, but we plan to deprecate NTPW soon, so having a separate temporary test file in the interim seems okay too. Either way, thank for fixing this up! https://chromiumcodereview.appspot.com/10831009/diff/1/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc (right): https://chromiumcodereview.appspot.com/10831009/diff/1/ui/views/controls/tabb... ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:362: //return tab_strip_->selected_tab(); nit: WIP, I know, sorry :p https://chromiumcodereview.appspot.com/10831009/diff/1/ui/views/controls/tabb... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://chromiumcodereview.appspot.com/10831009/diff/1/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:77: // The |tabbed_pane_| has no border. Therefor it should be as wide as the nit: "therefore" here and at 87 https://chromiumcodereview.appspot.com/10831009/diff/1/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:79: EXPECT_EQ(pref.width(), 20); EXPECT_GE would cover both impls then, right? https://chromiumcodereview.appspot.com/10831009/diff/1/ui/views/controls/tabb... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:89: EXPECT_EQ(bounds.width(), 100); EXPECT_LE would cover both impls then, right?
I'm totally with you to avaid duplicating code. Even if the duplicated code is
only meant to exist temporary. ("There is nothing more permanent than a
temporary solution" ;-) )
Patchset 1 of this CL was only a hack :(.
So here we go. (I'm not sure I understood the "looping over the tests" part of
your comment).
http://codereview.chromium.org/10831009/diff/1/ui/views/controls/tabbed_pane/...
File ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc (right):
http://codereview.chromium.org/10831009/diff/1/ui/views/controls/tabbed_pane/...
ui/views/controls/tabbed_pane/native_tabbed_pane_views.cc:362: //return
tab_strip_->selected_tab();
On 2012/07/25 20:19:27, msw wrote:
> nit: WIP, I know, sorry :p
:) done.
http://codereview.chromium.org/10831009/diff/1/ui/views/controls/tabbed_pane/...
File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right):
http://codereview.chromium.org/10831009/diff/1/ui/views/controls/tabbed_pane/...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:77: // The |tabbed_pane_|
has no border. Therefor it should be as wide as the
On 2012/07/25 20:19:27, msw wrote:
> nit: "therefore" here and at 87
Done.
http://codereview.chromium.org/10831009/diff/1/ui/views/controls/tabbed_pane/...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:79:
EXPECT_EQ(pref.width(), 20);
On 2012/07/25 20:19:27, msw wrote:
> EXPECT_GE would cover both impls then, right?
Correct. Done.
http://codereview.chromium.org/10831009/diff/1/ui/views/controls/tabbed_pane/...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:89:
EXPECT_EQ(bounds.width(), 100);
On 2012/07/25 20:19:27, msw wrote:
> EXPECT_LE would cover both impls then, right?
Correct. Done.
You can ignore my suggestion about looping, I like how you've (temporarily) made the tests members of the test class called twice on native win, but please call that out with a TODO. Thanks for fixing this up! http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:34: class WidgetDelegateImpl : public WidgetDelegate { None of this Widget and contents view wrangling should be necessary. The default WidgetDelegate::GetContentsView impl supplies a new View owned by the Widget, which you can use via widget->GetContentsView()->AddChildView(tabbed_pane_). If I'm wrong, please add a comment describing the purpose of this class. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:62: TabbedPane* tabbed_pane_; // Owned by |tabbed_pane_container_| nit: append a period. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:65: TabbedPane* tabbed_pane_win_; // Owned by the |window_|s |WidgetDelegate|. nit: "|window_|'s" http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:70: virtual void SetUp() OVERRIDE { Move this definition out-of-line. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:73: // In order to proper initialize the |TabbedPane| it must be added to a nit: "properly" and "|TabbedPane|, " http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:74: // parent view (see the ViewHierarchyChanged method of the |TabbedPane|. nit: close the parentheses. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:80: window_ = Widget::CreateWindowWithBounds( The Widget should be safe to use regardless of platform. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:93: scoped_ptr<View> tabbed_pane_container_; See the comment above about why this isn't necessary. Remove the scoped_ptr.h include along with this. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:173: #if defined(OS_WIN) && !defined(USE_AURA) Please add a comment here (or maybe somewhere else?) that the test class function TabbedPaneTest::TestSizeAndLayout should be restored to a normal test once NTPW in deprecated. Ditto for AddRemove.
http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:34: class WidgetDelegateImpl : public WidgetDelegate { On 2012/07/26 19:29:30, msw wrote: > None of this Widget and contents view wrangling should be necessary. The default > WidgetDelegate::GetContentsView impl supplies a new View owned by the Widget, > which you can use via widget->GetContentsView()->AddChildView(tabbed_pane_). > > If I'm wrong, please add a comment describing the purpose of this class. The default implementation of WidgetDelegate is an abstract class. The following methods are pure virtual: virtual views::Widget* GetWidget() OVERRIDE; virtual const views::Widget* GetWidget() const OVERRIDE; BTW: If I add both tabbed panes to the same view then I get the following warning when I run the test: "[6356:6924:0726/231848:95945840:ERROR:window_impl.cc(55)] Failed to unregister class Chrome_WidgetWin_0. Error = 1412" I didn't yet figure out if this is an issue or not. Therefor I use two different views. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:62: TabbedPane* tabbed_pane_; // Owned by |tabbed_pane_container_| On 2012/07/26 19:29:30, msw wrote: > nit: append a period. Done. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:65: TabbedPane* tabbed_pane_win_; // Owned by the |window_|s |WidgetDelegate|. On 2012/07/26 19:29:30, msw wrote: > nit: "|window_|'s" Done. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:70: virtual void SetUp() OVERRIDE { On 2012/07/26 19:29:30, msw wrote: > Move this definition out-of-line. Done. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:73: // In order to proper initialize the |TabbedPane| it must be added to a On 2012/07/26 19:29:30, msw wrote: > nit: "properly" and "|TabbedPane|, " Done. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:74: // parent view (see the ViewHierarchyChanged method of the |TabbedPane|. On 2012/07/26 19:29:30, msw wrote: > nit: close the parentheses. Done. http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:173: #if defined(OS_WIN) && !defined(USE_AURA) On 2012/07/26 19:29:30, msw wrote: > Please add a comment here (or maybe somewhere else?) that the test class > function TabbedPaneTest::TestSizeAndLayout should be restored to a normal test > once NTPW in deprecated. Ditto for AddRemove. Done.
http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:34: class WidgetDelegateImpl : public WidgetDelegate { Try following the pattern in ui/views/view_unittest.cc where using void Widget::Init(const InitParams& params) uses DefaultWidgetDelegate, or I suppose you can try using DefaultWidgetDelegate directly. Hmm, I'm not sure why adding the second TabbedPane view causes that error... Debug it or try a quick and dirty fix like adding container view(s) to the GetContentsView. That said, using the Widget in just the NTPW case seems fine, since it's on the chopping block; but try to at least remove this WidgetDelegateImpl.
I guess this is what you had in mind. On 2012/07/26 22:19:05, msw wrote: > http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... > File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): > > http://codereview.chromium.org/10831009/diff/3001/ui/views/controls/tabbed_pa... > ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:34: class > WidgetDelegateImpl : public WidgetDelegate { > Try following the pattern in ui/views/view_unittest.cc where using void > Widget::Init(const InitParams& params) uses DefaultWidgetDelegate, or I suppose > you can try using DefaultWidgetDelegate directly. > > Hmm, I'm not sure why adding the second TabbedPane view causes that error... > Debug it or try a quick and dirty fix like adding container view(s) to the > GetContentsView. That said, using the Widget in just the NTPW case seems fine, > since it's on the chopping block; but try to at least remove this > WidgetDelegateImpl.
Nice! That's much cleaner and to the point! Thanks for going the extra kilometer :) https://chromiumcodereview.appspot.com/10831009/diff/12001/ui/views/controls/... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://chromiumcodereview.appspot.com/10831009/diff/12001/ui/views/controls/... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:83: // The |tabbed_pane_| has no border. Therefore it should be as wide as the Sorry for changing my advice, but this comment should probably be merged with the semi-contradictory one above. Alternatively, (though I'm not sure this is really beneficial) you could split the check and comments up by platform to check GT and EQUAL... Then we could just delete the NTPW case lalter... Whichever option you choose, do the same below. https://chromiumcodereview.appspot.com/10831009/diff/12001/ui/views/controls/... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:146: // Tests that TabbedPane::GetPreferredSize() and TabbedPane::Layout(). nit: remove "that" or extrapolate.
Cool. I like your attention to detail : "kilometer" :-). It's no extra effort at all. The code's so much better without the delegate impl. https://chromiumcodereview.appspot.com/10831009/diff/12001/ui/views/controls/... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://chromiumcodereview.appspot.com/10831009/diff/12001/ui/views/controls/... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:83: // The |tabbed_pane_| has no border. Therefore it should be as wide as the On 2012/07/27 18:09:18, msw wrote: > Sorry for changing my advice, but this comment should probably be merged with > the semi-contradictory one above. > > Alternatively, (though I'm not sure this is really beneficial) you could split > the check and comments up by platform to check GT and EQUAL... Then we could > just delete the NTPW case lalter... > > Whichever option you choose, do the same below. Uups I merged the two comments. It's not possible (without further chagnes) to use "if-defs" in this method as this method is used to test both the NTPW and NTPV on Windows. I could add a bool flag that is set to true when we use the native win control. But then we would start adding more code to cover special cases. https://chromiumcodereview.appspot.com/10831009/diff/12001/ui/views/controls/... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:146: // Tests that TabbedPane::GetPreferredSize() and TabbedPane::Layout(). On 2012/07/27 18:09:18, msw wrote: > nit: remove "that" or extrapolate. Done.
LGTM with a nit; thanks! https://chromiumcodereview.appspot.com/10831009/diff/2003/ui/views/controls/t... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://chromiumcodereview.appspot.com/10831009/diff/2003/ui/views/controls/t... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:57: tabbed_pane_ = new TabbedPane(); nit: move this down with the AddChildView call.
Thanks a lot for the review! Adding Ben for owners approval. https://chromiumcodereview.appspot.com/10831009/diff/2003/ui/views/controls/t... File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right): https://chromiumcodereview.appspot.com/10831009/diff/2003/ui/views/controls/t... ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:57: tabbed_pane_ = new TabbedPane(); On 2012/07/30 15:23:01, msw wrote: > nit: move this down with the AddChildView call. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10831009/3004
Change committed as 149158 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
