A NativeViewHostViews class for embedding views inside NativeHostView instances.

A NativeHostViewViews class for embedding views inside NativeHostView instances.

BUG=none
TEST=manually on touchui build

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

[Alex Nicolaou, 2010-12-10 05:33:43]

The NativeViewHostViews class we discussed last week.

[Alex Nicolaou, 2010-12-10 05:35:58]

The NativeViewHostViews class we discussed that week, with typos in your email
and class name corrected <blush>

alex

[Ben Goodger (Google), 2010-12-10 15:42:57]

http://codereview.chromium.org/5785001/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/5785001/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents.cc:118: #include
"chrome/browser/ui/views/tab_contents/tab_contents_view_views.h"
Don't add this here - it's a layering violation.

Instead, cast tab_contents->view() in UI-land.

[Alex Nicolaou, 2010-12-10 18:05:23]

Do you have a specific file in which you want to see the cast?

alex

On Friday, December 10, 2010,  <ben@chromium.org> wrote:
>
>
http://codereview.chromium.org/5785001/diff/1/chrome/browser/tab_contents/tab...
> File chrome/browser/tab_contents/tab_contents.cc (right):
>
>
http://codereview.chromium.org/5785001/diff/1/chrome/browser/tab_contents/tab...
> chrome/browser/tab_contents/tab_contents.cc:118: #include
> "chrome/browser/ui/views/tab_contents/tab_contents_view_views.h"
> Don't add this here - it's a layering violation.
>
> Instead, cast tab_contents->view() in UI-land.
>
> http://codereview.chromium.org/5785001/
>

[Ben Goodger (Google), 2010-12-10 18:15:17]

Someplace other than tab_contents.*, which is not supposed to know about
ui/views really. :-)

Ideally, closest to where you attach this in the DOMView.

-Ben

On Fri, Dec 10, 2010 at 10:05 AM, Alex Nicolaou <anicolao@chromium.org>wrote:

> Do you have a specific file in which you want to see the cast?
>
> alex
>
> On Friday, December 10, 2010,  <ben@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/5785001/diff/1/chrome/browser/tab_contents/tab...
> > File chrome/browser/tab_contents/tab_contents.cc (right):
> >
> >
>
http://codereview.chromium.org/5785001/diff/1/chrome/browser/tab_contents/tab...
> > chrome/browser/tab_contents/tab_contents.cc:118: #include
> > "chrome/browser/ui/views/tab_contents/tab_contents_view_views.h"
> > Don't add this here - it's a layering violation.
> >
> > Instead, cast tab_contents->view() in UI-land.
> >
> > http://codereview.chromium.org/5785001/
> >
>

[Alex Nicolaou, 2010-12-15 18:28:28]

Fixed (hopefully) to be the way you wanted - PTAL

[Ben Goodger (Google), 2010-12-15 18:32:28]

http://codereview.chromium.org/5785001/diff/8001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/5785001/diff/8001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.h:61: namespace views {
This is no longer needed.

http://codereview.chromium.org/5785001/diff/8001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.h:459: TabContentsView*
GetTabContentsView();
TabContentsView* view() const { return view_.get(); }

http://codereview.chromium.org/5785001/diff/8001/chrome/browser/ui/views/dom_...
File chrome/browser/ui/views/dom_view.cc (right):

http://codereview.chromium.org/5785001/diff/8001/chrome/browser/ui/views/dom_...
chrome/browser/ui/views/dom_view.cc:77: Detach();
Looks like you might actually not need this method other than for symmetry.
Instead... just add a comment to AttachTabContents is only needed because the 2
different types of attachable view.

[Alex Nicolaou, 2010-12-16 22:34:48]

Thanks for the lightning fast review! Here's the next round - think I got
everything, please take another look.

alex

http://codereview.chromium.org/5785001/diff/8001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/5785001/diff/8001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.h:61: namespace views {
On 2010/12/15 18:32:28, Ben Goodger wrote:
> This is no longer needed.

Done.

http://codereview.chromium.org/5785001/diff/8001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_contents.h:459: TabContentsView*
GetTabContentsView();
On 2010/12/15 18:32:28, Ben Goodger wrote:
> TabContentsView* view() const { return view_.get(); }

Oh <duh> I didn't see this was already in the file. I'd used a longer name to
try to avoid creating confusion about this method (as you'd think view would
return a View but it doesn't) but since the other entry point is already here I
deleted this one.

http://codereview.chromium.org/5785001/diff/8001/chrome/browser/ui/views/dom_...
File chrome/browser/ui/views/dom_view.cc (right):

http://codereview.chromium.org/5785001/diff/8001/chrome/browser/ui/views/dom_...
chrome/browser/ui/views/dom_view.cc:77: Detach();
On 2010/12/15 18:32:28, Ben Goodger wrote:
> Looks like you might actually not need this method other than for symmetry.
> Instead... just add a comment to AttachTabContents is only needed because the
2
> different types of attachable view.

I prefer the symmetry but done.

[Ben Goodger (Google), 2010-12-16 22:54:12]

OK LGTM

On Thu, Dec 16, 2010 at 2:34 PM, <anicolao@chromium.org> wrote:

> Thanks for the lightning fast review! Here's the next round - think I got
> everything, please take another look.
>
> alex
>
>
>
>
>
http://codereview.chromium.org/5785001/diff/8001/chrome/browser/tab_contents/...
> File chrome/browser/tab_contents/tab_contents.h (right):
>
>
>
http://codereview.chromium.org/5785001/diff/8001/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/tab_contents.h:61: namespace views {
> On 2010/12/15 18:32:28, Ben Goodger wrote:
>
>> This is no longer needed.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/5785001/diff/8001/chrome/browser/tab_contents/...
> chrome/browser/tab_contents/tab_contents.h:459: TabContentsView*
> GetTabContentsView();
> On 2010/12/15 18:32:28, Ben Goodger wrote:
>
>> TabContentsView* view() const { return view_.get(); }
>>
>
> Oh <duh> I didn't see this was already in the file. I'd used a longer
> name to try to avoid creating confusion about this method (as you'd
> think view would return a View but it doesn't) but since the other entry
> point is already here I deleted this one.
>
>
>
>
http://codereview.chromium.org/5785001/diff/8001/chrome/browser/ui/views/dom_...
> File chrome/browser/ui/views/dom_view.cc (right):
>
>
>
http://codereview.chromium.org/5785001/diff/8001/chrome/browser/ui/views/dom_...
> chrome/browser/ui/views/dom_view.cc:77: Detach();
> On 2010/12/15 18:32:28, Ben Goodger wrote:
>
>> Looks like you might actually not need this method other than for
>>
> symmetry.
>
>> Instead... just add a comment to AttachTabContents is only needed
>>
> because the 2
>
>> different types of attachable view.
>>
>
> I prefer the symmetry but done.
>
>
> http://codereview.chromium.org/5785001/
>