(Views only): Allow choosing between NativeTabbedPaneView and NativeTabbedPaneWin on Windows.

(Views only): Allow switching between NativeTabbedPaneView and NativeTabbedPaneWin on Windows.


BUG=138059
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148617

[markusheintz_, 2012-07-20 16:49:20]

Hey Michael could you please take a look at this CL.
It only lacks some comments, but I would love to get some feedback first before
I add them.

[msw, 2012-07-20 23:50:50]

Very awesome! Thanks!

https://chromiumcodereview.appspot.com/10797046/diff/8/chrome/browser/ui/view...
File chrome/browser/ui/views/collected_cookies_views.cc (right):

https://chromiumcodereview.appspot.com/10797046/diff/8/chrome/browser/ui/view...
chrome/browser/ui/views/collected_cookies_views.cc:48: // pane is not used
anymore.
optional nit: include bug # in comment; capitalize Windows.

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane.cc:17: // pane is not used anymore.
ditto optional nit: include bug # and capitalize Windows.

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane.cc:30:
set_use_native_win_control(false);
Set this in the initializer list if it's not terribly difficult.

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane.cc:157: NativeTabbedPaneWrapper*
NativeTabbedPaneWrapper::CreateNativeWrapper(
Make this a static member on TabbedPane now and it'll be more at home in this
file. I should have probably done the same thing for
NativeTextfieldWrapper::CreateWrapper(), so I guess this is optional :)

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
File ui/views/controls/tabbed_pane/tabbed_pane.h (right):

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane.h:66: bool use_native_win_control() {
return use_native_win_control_; }
Move these up above the View override, potentially just below set_listener().

[markusheintz_, 2012-07-23 12:22:06]

https://chromiumcodereview.appspot.com/10797046/diff/8/chrome/browser/ui/view...
File chrome/browser/ui/views/collected_cookies_views.cc (right):

https://chromiumcodereview.appspot.com/10797046/diff/8/chrome/browser/ui/view...
chrome/browser/ui/views/collected_cookies_views.cc:48: // pane is not used
anymore.
On 2012/07/20 23:50:51, msw wrote:
> optional nit: include bug # in comment; capitalize Windows.

Done.

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane.cc:17: // pane is not used anymore.
On 2012/07/20 23:50:51, msw wrote:
> ditto optional nit: include bug # and capitalize Windows.

Done.

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane.cc:30:
set_use_native_win_control(false);
On 2012/07/20 23:50:51, msw wrote:
> Set this in the initializer list if it's not terribly difficult.

Done. The coding style looks a bit strange but it works :).

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane.cc:157: NativeTabbedPaneWrapper*
NativeTabbedPaneWrapper::CreateNativeWrapper(
True. This static method feels a bit alien in this file. 

Here are some more thoughts of mine before I fix this:

Maybe I'm wrong here but I guess the method CreateNativeWrapper is only meant to
be used internally by |TabbedPane|. So I wonder weather it should be really
added to |TabbedPane| as static method. 

It feels more like having a tabbed_pane_wrapper.cc file that contains this
method. But then again, adding a .cc file just because of a single static method
seems like an overkill. 

We could also move this method to an anonymous namespace this file and delete it
from the TabbedPaneWrapperInterface. This way we don't expose this method to the
rest of the word. 


On 2012/07/20 23:50:51, msw wrote:
> Make this a static member on TabbedPane now and it'll be more at home in this
> file. I should have probably done the same thing for
> NativeTextfieldWrapper::CreateWrapper(), so I guess this is optional :)

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
File ui/views/controls/tabbed_pane/tabbed_pane.h (right):

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane.h:66: bool use_native_win_control() {
return use_native_win_control_; }
On 2012/07/20 23:50:51, msw wrote:
> Move these up above the View override, potentially just below set_listener().

Done.

[msw, 2012-07-23 22:55:43]

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
File ui/views/controls/tabbed_pane/tabbed_pane.cc (right):

https://chromiumcodereview.appspot.com/10797046/diff/8/ui/views/controls/tabb...
ui/views/controls/tabbed_pane/tabbed_pane.cc:157: NativeTabbedPaneWrapper*
NativeTabbedPaneWrapper::CreateNativeWrapper(
> We could also move this method to an anonymous namespace this file and delete
it
> from the TabbedPaneWrapperInterface. This way we don't expose this method to
the
> rest of the word. 

This sgtm, since it's really an internal detail of the TabbedPane control, but
feel free to pursue another option if it makes more sense to you. TabbedPane /
NativeTabbedPaneWrapper / NativeTabPaneViews should probably all be merged into
one class once we deprecate NTPW.

[markusheintz_, 2012-07-24 16:48:32]

@Ben: Please review this CL. This CL is the first step to replace
NativeTabbedPaneView with NativeTabbedPaneWin.

[markusheintz_, 2012-07-24 16:48:33]

@Ben: Please review this CL. This CL is the first step to replace
NativeTabbedPaneView with NativeTabbedPaneWin.

[msw, 2012-07-24 16:57:57]

LGTM with one more nit.

https://chromiumcodereview.appspot.com/10797046/diff/13001/ui/views/controls/...
File ui/views/controls/tabbed_pane/native_tabbed_pane_wrapper.h (right):

https://chromiumcodereview.appspot.com/10797046/diff/13001/ui/views/controls/...
ui/views/controls/tabbed_pane/native_tabbed_pane_wrapper.h:17: class TabbedPane;
nit: I don't think this forward decl is needed anymore.

[markusheintz_, 2012-07-24 17:03:17]

Thanks a lot for the review Michael.

https://chromiumcodereview.appspot.com/10797046/diff/13001/ui/views/controls/...
File ui/views/controls/tabbed_pane/native_tabbed_pane_wrapper.h (right):

https://chromiumcodereview.appspot.com/10797046/diff/13001/ui/views/controls/...
ui/views/controls/tabbed_pane/native_tabbed_pane_wrapper.h:17: class TabbedPane;
On 2012/07/24 16:57:57, msw wrote:
> nit: I don't think this forward decl is needed anymore.

Done.

[Ben Goodger (Google), 2012-07-25 15:55:37]

lgtm

[markusheintz_, 2012-07-25 18:31:13]

On 2012/07/25 15:55:37, Ben Goodger (Google) wrote:
> lgtm

Sorry for the post LGTM change but I just noticed that the unittest
TabbedPaneTest relies on using the NativeTabbedPaneWin on windows. I fixed this.

I also think I found a bug in NativeTabbedPaneViews which is not covered by any
tests. I will fix the bug and add a test that covers NativeTabbedPaneView in CL
(WIP) https://chromiumcodereview.appspot.com/10831009/.

I'll also file a separate issue for the bug.

If you don't object I'll commit this CL when the trybot are green.

[Ben Goodger (Google), 2012-07-25 19:30:21]

ok

[msw, 2012-07-25 20:03:53]

LGTM with option of enabling tests on more platforms now or later...

https://chromiumcodereview.appspot.com/10797046/diff/22002/ui/views/controls/...
File ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc (right):

https://chromiumcodereview.appspot.com/10797046/diff/22002/ui/views/controls/...
ui/views/controls/tabbed_pane/tabbed_pane_unittest.cc:43:
tabbed_pane_->set_use_native_win_control(true);
Right now, ui/views/views.gyp excludes this file for USE_AURA. I suppose this is
OK for now, but we should really get these tests running and fixed on CrOS and
Win Aura with the NTPV. As an interim measure, we could still use NTPW here via
OS_WIN && !USE_AURA.

[msw, 2012-07-25 20:06:28]

NVM, this is fine. I see you're already fixing up the tests in:
https://chromiumcodereview.appspot.com/10831009/ thanks!

[commit-bot: I haz the power, 2012-07-26 13:20:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10797046/23012

[commit-bot: I haz the power, 2012-07-26 18:21:34]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-07-26 18:39:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10797046/23012