TabContentsViewViews should be used instead of TabContentsViewGtk for TOUCH_UI

TabContentsViewViews should be used instead of TabContentsViewGtk for TOUCH_UI
builds.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71745

[sadrul, 2010-12-29 21:02:47]

LGTM (with nits).

I think there is currently an effort of avoiding the ifdef'ed approach for
TOUCH_UI and have separate files in separate locations (under ui/touch/) where
possible. See http://codereview.chromium.org/5700003/, for a brief discussion on
this. It's probably not relevant in a small file as find_bar_host_gtk.cc, but I
would wait for confirmation from Alex/sky.

http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
File chrome/browser/ui/views/find_bar_host_gtk.cc (right):

http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
chrome/browser/ui/views/find_bar_host_gtk.cc:11: #endif
I think the conditional headers go below the general headers.

[Alex Nicolaou, 2011-01-17 16:29:44]

[+sky] LGTM though the #include does need to be moved below the others.

I think for a case like this multiple files are not justified. See
what sky says.

alex

On Wednesday, December 29, 2010,  <sadrul@chromium.org> wrote:
> LGTM (with nits).
>
> I think there is currently an effort of avoiding the ifdef'ed approach for
> TOUCH_UI and have separate files in separate locations (under ui/touch/) where
> possible. See http://codereview.chromium.org/5700003/, for a brief discussion
on
> this. It's probably not relevant in a small file as find_bar_host_gtk.cc, but
I
> would wait for confirmation from Alex/sky.
>
>
>
http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
> File chrome/browser/ui/views/find_bar_host_gtk.cc (right):
>
>
http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
> chrome/browser/ui/views/find_bar_host_gtk.cc:11: #endif
> I think the conditional headers go below the general headers.
>
> http://codereview.chromium.org/6012011/
>

[varunjain, 2011-01-17 18:09:14]

Another way that Alex suggested was to add GetBounds() in the TabContentsView
interface itself so that this ifdef'ery here is not longer needed. I can go
either way on this... although, changing the interface sounds cleaner.

http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
File chrome/browser/ui/views/find_bar_host_gtk.cc (right):

http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
chrome/browser/ui/views/find_bar_host_gtk.cc:11: #endif
On 2010/12/29 21:02:47, sadrul wrote:
> I think the conditional headers go below the general headers.

Done.

[Alex Nicolaou, 2011-01-17 19:01:16]

I assumed that approach got hung up and that was the reason for the
long silence :-) ... but if it's just a question of getting to it,
that's still my preference. But one way or another let's get something
checked in.

alex

On Monday, January 17, 2011,  <varunjain@chromium.org> wrote:
> Another way that Alex suggested was to add GetBounds() in the TabContentsView
> interface itself so that this ifdef'ery here is not longer needed. I can go
> either way on this... although, changing the interface sounds cleaner.
>
>
>
http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
> File chrome/browser/ui/views/find_bar_host_gtk.cc (right):
>
>
http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
> chrome/browser/ui/views/find_bar_host_gtk.cc:11: #endif
> On 2010/12/29 21:02:47, sadrul wrote:
>
> I think the conditional headers go below the general headers.
>
>
> Done.
>
> http://codereview.chromium.org/6012011/
>

[sadrul, 2011-01-17 19:13:53]

On 2011/01/17 18:09:14, varunjain wrote:
> Another way that Alex suggested was to add GetBounds() in the TabContentsView

Nit: In keeping with RenderWidgetHostView, perhaps GetViewBounds() instead?

[varunjain, 2011-01-17 20:07:21]

On 2011/01/17 19:01:16, Alex Nicolaou wrote:
> I assumed that approach got hung up and that was the reason for the
> long silence :-) ... but if it's just a question of getting to it,
> that's still my preference. But one way or another let's get something
> checked in.

the approach got hung up on my inefficiency :(
I did the required changes... PTAL

> 
> alex
> 
> On Monday, January 17, 2011,  <mailto:varunjain@chromium.org> wrote:
> > Another way that Alex suggested was to add GetBounds() in the
TabContentsView
> > interface itself so that this ifdef'ery here is not longer needed. I can go
> > either way on this... although, changing the interface sounds cleaner.
> >
> >
> >
>
http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
> > File chrome/browser/ui/views/find_bar_host_gtk.cc (right):
> >
> >
>
http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
> > chrome/browser/ui/views/find_bar_host_gtk.cc:11: #endif
> > On 2010/12/29 21:02:47, sadrul wrote:
> >
> > I think the conditional headers go below the general headers.
> >
> >
> > Done.
> >
> > http://codereview.chromium.org/6012011/
> >

[Alex Nicolaou, 2011-01-17 20:41:19]

LGTM

On Monday, January 17, 2011,  <varunjain@chromium.org> wrote:
> On 2011/01/17 19:01:16, Alex Nicolaou wrote:
>
> I assumed that approach got hung up and that was the reason for the
> long silence :-) ... but if it's just a question of getting to it,
> that's still my preference. But one way or another let's get something
> checked in.
>
>
> the approach got hung up on my inefficiency :(
> I did the required changes... PTAL
>
>
>
> alex
>
>
>
> On Monday, January 17, 2011,  <mailto:varunjain@chromium.org> wrote:
>> Another way that Alex suggested was to add GetBounds() in the
>
> TabContentsView
>
>> interface itself so that this ifdef'ery here is not longer needed. I can go
>> either way on this... although, changing the interface sounds cleaner.
>>
>>
>>
>
>
>
http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
>
>> File chrome/browser/ui/views/find_bar_host_gtk.cc (right):
>>
>>
>
>
>
http://codereview.chromium.org/6012011/diff/1/chrome/browser/ui/views/find_ba...
>
>> chrome/browser/ui/views/find_bar_host_gtk.cc:11: #endif
>> On 2010/12/29 21:02:47, sadrul wrote:
>>
>> I think the conditional headers go below the general headers.
>>
>>
>> Done.
>>
>> http://codereview.chromium.org/6012011/
>>
>
>
>
>
> http://codereview.chromium.org/6012011/
>

[sky, 2011-01-18 17:19:48]

LGTM with the following changes.

  -Scott

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_contents/tab_contents_view_gtk.h (right):

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/tab_contents_view_gtk.h:66: virtual void
GetViewBounds(gfx::Rect* out) const {
Don't inline virtual methods.

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_contents/tab_contents_view_views.h (right):

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/tab_contents_view_views.h:62: virtual void
GetViewBounds(gfx::Rect* out) const {
Don't inline virtual methods.

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_contents/tab_contents_view_win.h (right):

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/tab_contents_view_win.h:60: virtual void
GetViewBounds(gfx::Rect* out) const {
ditto.

[varunjain, 2011-01-18 20:25:32]

Also included the new method in mac implementation. PTAL

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_contents/tab_contents_view_gtk.h (right):

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/tab_contents_view_gtk.h:66: virtual void
GetViewBounds(gfx::Rect* out) const {
On 2011/01/18 17:19:48, sky wrote:
> Don't inline virtual methods.

Done.

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_contents/tab_contents_view_views.h (right):

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/tab_contents_view_views.h:62: virtual void
GetViewBounds(gfx::Rect* out) const {
On 2011/01/18 17:19:48, sky wrote:
> Don't inline virtual methods.

Done.

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_contents/tab_contents_view_win.h (right):

http://codereview.chromium.org/6012011/diff/16002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_contents/tab_contents_view_win.h:60: virtual void
GetViewBounds(gfx::Rect* out) const {
On 2011/01/18 17:19:48, sky wrote:
> ditto.

Done.

[sky, 2011-01-19 01:43:38]

LGTM

http://codereview.chromium.org/6012011/diff/9002/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/6012011/diff/9002/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_view_mac.mm:326: NOTIMPLEMENTED();
Add a comment that this isn't used on mac.

[varunjain, 2011-01-19 01:50:44]

http://codereview.chromium.org/6012011/diff/9002/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents_view_mac.mm (right):

http://codereview.chromium.org/6012011/diff/9002/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents_view_mac.mm:326: NOTIMPLEMENTED();
On 2011/01/19 01:43:38, sky wrote:
> Add a comment that this isn't used on mac.

Done.