Replace TabContentsViewGtk with TabContentsViewViews as part of the ongoing

Replace TabContentsViewGtk with TabContentsViewViews as part of the ongoing 
effort to eliminate GTK for TOUCH_UI.

BUG=none
TEST=manually tested

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

[Alex Nicolaou, 2010-11-03 05:40:03]

Another CL in the style of RenderViewHostViewGtk -> RenderViewHostViewViews for
touch. Oshima, Scott reviewed the last one in detail so it'd be great if you
could be the primary reviewer for this one. I'll wait for both LGTMs before
committing.

thanks
alex

[brettw, 2010-11-03 17:03:10]

Drive by

http://codereview.chromium.org/4319003/diff/1/11
File gfx/canvas.h (right):

http://codereview.chromium.org/4319003/diff/1/11#newcode104
gfx/canvas.h:104: #if defined(TOUCH_UI)
I wouldn't bother with ifdefs for functions in common classes just because
you're the only one that happens to use it. You can just stub it out in
CanvasDirect2D like the DrawRectInt with a Xfer mode is.

[Alex Nicolaou, 2010-11-03 17:05:20]

Ah, I'd actually asked sky about this off-list. I am not wedded one
way or the other, but the reason I did it is because it seemed to me
that the only reason we have a canvas.h is to limit the amount of skia
we're using and make it clear what the interface is - so I put the #if
in to keep it as narrow as possible. If you still think that's not the
best, I can remove the #if.

alex

On Wednesday, November 3, 2010,  <brettw@chromium.org> wrote:
> Drive by
>
>
> http://codereview.chromium.org/4319003/diff/1/11
> File gfx/canvas.h (right):
>
> http://codereview.chromium.org/4319003/diff/1/11#newcode104
> gfx/canvas.h:104: #if defined(TOUCH_UI)
> I wouldn't bother with ifdefs for functions in common classes just
> because you're the only one that happens to use it. You can just stub it
> out in CanvasDirect2D like the DrawRectInt with a Xfer mode is.
>
> http://codereview.chromium.org/4319003/show
>

[Alex Nicolaou, 2010-11-03 17:08:28]

[And to be 100% clear, sky didn't respond on this specific point]

On Wednesday, November 3, 2010, Alex Nicolaou <anicolao@chromium.org> wrote:
> Ah, I'd actually asked sky about this off-list. I am not wedded one
> way or the other, but the reason I did it is because it seemed to me
> that the only reason we have a canvas.h is to limit the amount of skia
> we're using and make it clear what the interface is - so I put the #if
> in to keep it as narrow as possible. If you still think that's not the
> best, I can remove the #if.
>
> alex
>
> On Wednesday, November 3, 2010,  <brettw@chromium.org> wrote:
>> Drive by
>>
>>
>> http://codereview.chromium.org/4319003/diff/1/11
>> File gfx/canvas.h (right):
>>
>> http://codereview.chromium.org/4319003/diff/1/11#newcode104
>> gfx/canvas.h:104: #if defined(TOUCH_UI)
>> I wouldn't bother with ifdefs for functions in common classes just
>> because you're the only one that happens to use it. You can just stub it
>> out in CanvasDirect2D like the DrawRectInt with a Xfer mode is.
>>
>> http://codereview.chromium.org/4319003/show
>>
>

[brettw, 2010-11-03 17:11:26]

On Wed, Nov 3, 2010 at 10:04 AM, Alex Nicolaou <anicolao@chromium.org> wrote:
> Ah, I'd actually asked sky about this off-list. I am not wedded one
> way or the other, but the reason I did it is because it seemed to me
> that the only reason we have a canvas.h is to limit the amount of skia
> we're using and make it clear what the interface is - so I put the #if
> in to keep it as narrow as possible. If you still think that's not the
> best, I can remove the #if.

I think for this type of thing you should either use your own code and
just write to Skia directly, or have a function on Canvas for
everybody to use. Obviously if you're doing something very specific to
your use case, it would be better to just write the Skia in your own
code. I was originally going to suggest you do this in this case, but
then I saw that DrawRect already has a version that takes a transfer
mode and it seemed like having consistency with the fill functions
seemed like a good idea.

Brett

[Alex Nicolaou, 2010-11-03 17:18:13]

I'll make it open to everyone then.

alex

On Wednesday, November 3, 2010, Brett Wilson <brettw@chromium.org> wrote:
> On Wed, Nov 3, 2010 at 10:04 AM, Alex Nicolaou <anicolao@chromium.org> wrote:
>> Ah, I'd actually asked sky about this off-list. I am not wedded one
>> way or the other, but the reason I did it is because it seemed to me
>> that the only reason we have a canvas.h is to limit the amount of skia
>> we're using and make it clear what the interface is - so I put the #if
>> in to keep it as narrow as possible. If you still think that's not the
>> best, I can remove the #if.
>
> I think for this type of thing you should either use your own code and
> just write to Skia directly, or have a function on Canvas for
> everybody to use. Obviously if you're doing something very specific to
> your use case, it would be better to just write the Skia in your own
> code. I was originally going to suggest you do this in this case, but
> then I saw that DrawRect already has a version that takes a transfer
> mode and it seemed like having consistency with the fill functions
> seemed like a good idea.
>
> Brett
>

[oshima, 2010-11-03 18:01:44]

I wish we have better suffix for touchui rather than views. we have have
views/win, views/gtk and now views/views,
and its getting confusing....

http://codereview.chromium.org/4319003/diff/1/3
File chrome/browser/gtk/constrained_window_gtk.h (right):

http://codereview.chromium.org/4319003/diff/1/3#newcode1
chrome/browser/gtk/constrained_window_gtk.h:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
2010

http://codereview.chromium.org/4319003/diff/1/5
File chrome/browser/renderer_host/backing_store_x.h (right):

http://codereview.chromium.org/4319003/diff/1/5#newcode40
chrome/browser/renderer_host/backing_store_x.h:40: void XShowRect(int originx,
int originy, const gfx::Rect& damage,
any reason this is not gfx::Point?

http://codereview.chromium.org/4319003/diff/1/6
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/4319003/diff/1/6#newcode135
chrome/browser/renderer_host/render_widget_host_view_views.cc:135:
SetBounds(gfx::Rect(x(), y(), width, height));
or gfx::Rect(origin(), requested_size_);
up to you .

http://codereview.chromium.org/4319003/diff/1/6#newcode358
chrome/browser/renderer_host/render_widget_host_view_views.cc:358: } while (v);
ConvertPonitToWidget?

http://codereview.chromium.org/4319003/diff/1/7
File chrome/browser/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/4319003/diff/1/7#newcode54
chrome/browser/views/tab_contents/tab_contents_container.cc:54: // not?


This only clears background upwards, which means
that other children of the parent may still have background.
Will this work?

http://codereview.chromium.org/4319003/diff/1/7#newcode60
chrome/browser/views/tab_contents/tab_contents_container.cc:60: #endif
do you want to keep this? you also set v= this then v = contents->view(), which
makes the 1st line no-op.

http://codereview.chromium.org/4319003/diff/1/7#newcode64
chrome/browser/views/tab_contents/tab_contents_container.cc:64:
//v->SetVisible(true);
nuke dead code.

http://codereview.chromium.org/4319003/diff/1/7#newcode66
chrome/browser/views/tab_contents/tab_contents_container.cc:66: AddChildView(v);
will this view get removed?

http://codereview.chromium.org/4319003/diff/1/7#newcode67
chrome/browser/views/tab_contents/tab_contents_container.cc:67: }
can this condition (v->GetParent() == this) happen?
if yes, please add comment how/when as this is not intuitive.

http://codereview.chromium.org/4319003/diff/1/8
File chrome/browser/views/tab_contents/tab_contents_view_views.cc (right):

http://codereview.chromium.org/4319003/diff/1/8#newcode33
chrome/browser/views/tab_contents/tab_contents_view_views.cc:33: 
remove extra empty line

http://codereview.chromium.org/4319003/diff/1/8#newcode75
chrome/browser/views/tab_contents/tab_contents_view_views.cc:75:
SetBounds(gfx::Rect(x(), y(), initial_size.width(), initial_size.height()));
gfx::Rect(orogin(), initial_size)

http://codereview.chromium.org/4319003/diff/1/8#newcode90
chrome/browser/views/tab_contents/tab_contents_view_views.cc:90: // If we were
showing sad tab, remove it now.
doesn't look like it's removing?

http://codereview.chromium.org/4319003/diff/1/8#newcode125
chrome/browser/views/tab_contents/tab_contents_view_views.cc:125: //*out =
gfx::Rect(0, 0, 512, 1000);
debug code?

http://codereview.chromium.org/4319003/diff/1/8#newcode131
chrome/browser/views/tab_contents/tab_contents_view_views.cc:131: const
gfx::Point& image_offset) {
indent

http://codereview.chromium.org/4319003/diff/1/8#newcode145
chrome/browser/views/tab_contents/tab_contents_view_views.cc:145: 
remove extra line

http://codereview.chromium.org/4319003/diff/1/8#newcode223
chrome/browser/views/tab_contents/tab_contents_view_views.cc:223:
NOTIMPLEMENTED(); // not even clear a drag cursor will make sense for touch
2 spaces between ; and //

http://codereview.chromium.org/4319003/diff/1/9
File chrome/browser/views/tab_contents/tab_contents_view_views.h (right):

http://codereview.chromium.org/4319003/diff/1/9#newcode23
chrome/browser/views/tab_contents/tab_contents_view_views.h:23: namespace gfx {
blank lines before/after namespace

http://codereview.chromium.org/4319003/diff/1/9#newcode32
chrome/browser/views/tab_contents/tab_contents_view_views.h:32: public
views::View {
indent

http://codereview.chromium.org/4319003/diff/1/9#newcode46
chrome/browser/views/tab_contents/tab_contents_view_views.h:46: 
remove blank line and no need for -------

http://codereview.chromium.org/4319003/diff/1/9#newcode62
chrome/browser/views/tab_contents/tab_contents_view_views.h:62: virtual void
Paint(gfx::Canvas* canvas);
// views::View implementation ?

[Alex Nicolaou, 2010-11-05 04:33:07]

http://codereview.chromium.org/4319003/diff/1/5
File chrome/browser/renderer_host/backing_store_x.h (right):

http://codereview.chromium.org/4319003/diff/1/5#newcode40
chrome/browser/renderer_host/backing_store_x.h:40: void XShowRect(int originx,
int originy, const gfx::Rect& damage,
On 2010/11/03 18:01:44, oshima wrote:
> any reason this is not gfx::Point?

Done.

http://codereview.chromium.org/4319003/diff/1/6
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/4319003/diff/1/6#newcode135
chrome/browser/renderer_host/render_widget_host_view_views.cc:135:
SetBounds(gfx::Rect(x(), y(), width, height));
On 2010/11/03 18:01:44, oshima wrote:
> or gfx::Rect(origin(), requested_size_);
> up to you .

I don't actually care but this line matches another patch on the same point so I
didn't change it to avoid a merge conflict.

http://codereview.chromium.org/4319003/diff/1/6#newcode358
chrome/browser/renderer_host/render_widget_host_view_views.cc:358: } while (v);
On 2010/11/03 18:01:44, oshima wrote:
> ConvertPonitToWidget?

Done, also looks like it fixes a failure to handle potential RTL layouts.

http://codereview.chromium.org/4319003/diff/1/7
File chrome/browser/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/4319003/diff/1/7#newcode54
chrome/browser/views/tab_contents/tab_contents_container.cc:54: // not?
On 2010/11/03 18:01:44, oshima wrote:
> 
> 
> This only clears background upwards, which means
> that other children of the parent may still have background.
> Will this work?

It does, because we don't have any siblings. But I think the small gain in
efficiency here is not really worth it, I will throw out this code.

http://codereview.chromium.org/4319003/diff/1/7#newcode60
chrome/browser/views/tab_contents/tab_contents_container.cc:60: #endif
On 2010/11/03 18:01:44, oshima wrote:
> do you want to keep this? you also set v= this then v = contents->view(),
which
> makes the 1st line no-op.

The v=this is necessary only for the #if 0 code so it'll disappear in the next
diff.

http://codereview.chromium.org/4319003/diff/1/7#newcode64
chrome/browser/views/tab_contents/tab_contents_container.cc:64:
//v->SetVisible(true);
On 2010/11/03 18:01:44, oshima wrote:
> nuke dead code.

Done.

http://codereview.chromium.org/4319003/diff/1/7#newcode66
chrome/browser/views/tab_contents/tab_contents_container.cc:66: AddChildView(v);
On 2010/11/03 18:01:44, oshima wrote:
> will this view get removed?

Yes it removes itself when destroyed.

http://codereview.chromium.org/4319003/diff/1/7#newcode67
chrome/browser/views/tab_contents/tab_contents_container.cc:67: }
On 2010/11/03 18:01:44, oshima wrote:
> can this condition (v->GetParent() == this) happen?
> if yes, please add comment how/when as this is not intuitive.

Yes. Since changes with NULL are ignored, every time a tab is selected we wind
up in this case. Based on the comment above about why we ignore NULL it's not
clear that the caller is getting their intended effect (see
BrowserView::ProcessTabSelected) but this guard is necessary.

Added a comment.

http://codereview.chromium.org/4319003/diff/1/8
File chrome/browser/views/tab_contents/tab_contents_view_views.cc (right):

http://codereview.chromium.org/4319003/diff/1/8#newcode33
chrome/browser/views/tab_contents/tab_contents_view_views.cc:33: 
On 2010/11/03 18:01:44, oshima wrote:
> remove extra empty line

Done.

http://codereview.chromium.org/4319003/diff/1/8#newcode75
chrome/browser/views/tab_contents/tab_contents_view_views.cc:75:
SetBounds(gfx::Rect(x(), y(), initial_size.width(), initial_size.height()));
On 2010/11/03 18:01:44, oshima wrote:
> gfx::Rect(orogin(), initial_size)

Done.

http://codereview.chromium.org/4319003/diff/1/8#newcode90
chrome/browser/views/tab_contents/tab_contents_view_views.cc:90: // If we were
showing sad tab, remove it now.
On 2010/11/03 18:01:44, oshima wrote:
> doesn't look like it's removing?

Done.

http://codereview.chromium.org/4319003/diff/1/8#newcode125
chrome/browser/views/tab_contents/tab_contents_view_views.cc:125: //*out =
gfx::Rect(0, 0, 512, 1000);
On 2010/11/03 18:01:44, oshima wrote:
> debug code?

Removed. how embarrassing :(

http://codereview.chromium.org/4319003/diff/1/8#newcode131
chrome/browser/views/tab_contents/tab_contents_view_views.cc:131: const
gfx::Point& image_offset) {
On 2010/11/03 18:01:44, oshima wrote:
> indent
> 

Done.

http://codereview.chromium.org/4319003/diff/1/8#newcode145
chrome/browser/views/tab_contents/tab_contents_view_views.cc:145: 
On 2010/11/03 18:01:44, oshima wrote:
> remove extra line

Done.

http://codereview.chromium.org/4319003/diff/1/8#newcode223
chrome/browser/views/tab_contents/tab_contents_view_views.cc:223:
NOTIMPLEMENTED(); // not even clear a drag cursor will make sense for touch
On 2010/11/03 18:01:44, oshima wrote:
> 2 spaces between ; and //

Done.

http://codereview.chromium.org/4319003/diff/1/9
File chrome/browser/views/tab_contents/tab_contents_view_views.h (right):

http://codereview.chromium.org/4319003/diff/1/9#newcode23
chrome/browser/views/tab_contents/tab_contents_view_views.h:23: namespace gfx {
On 2010/11/03 18:01:44, oshima wrote:
> blank lines before/after namespace

Done.

http://codereview.chromium.org/4319003/diff/1/9#newcode32
chrome/browser/views/tab_contents/tab_contents_view_views.h:32: public
views::View {
On 2010/11/03 18:01:44, oshima wrote:
> indent

unwrapped instead

http://codereview.chromium.org/4319003/diff/1/9#newcode46
chrome/browser/views/tab_contents/tab_contents_view_views.h:46: 
On 2010/11/03 18:01:44, oshima wrote:
> remove blank line and no need for -------

Done.

http://codereview.chromium.org/4319003/diff/1/9#newcode62
chrome/browser/views/tab_contents/tab_contents_view_views.h:62: virtual void
Paint(gfx::Canvas* canvas);
On 2010/11/03 18:01:44, oshima wrote:
> // views::View implementation ?

Added. Actually I'd just as soon remove all these comments but when in rome...

http://codereview.chromium.org/4319003/diff/1/11
File gfx/canvas.h (right):

http://codereview.chromium.org/4319003/diff/1/11#newcode104
gfx/canvas.h:104: #if defined(TOUCH_UI)
On 2010/11/03 17:03:10, brettw wrote:
> I wouldn't bother with ifdefs for functions in common classes just because
> you're the only one that happens to use it. You can just stub it out in
> CanvasDirect2D like the DrawRectInt with a Xfer mode is.

Done.

[oshima, 2010-11-08 20:59:38]

Scott, could you please see my comment/question about
NativeTabContentsContainer?

http://codereview.chromium.org/4319003/diff/1/7
File chrome/browser/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/4319003/diff/1/7#newcode66
chrome/browser/views/tab_contents/tab_contents_container.cc:66: AddChildView(v);
On 2010/11/05 04:33:08, Alex Nicolaou wrote:
> On 2010/11/03 18:01:44, oshima wrote:
> > will this view get removed?
> 
> Yes it removes itself when destroyed. 

My question was that this change removes "detachContents", which removes the
existing tabcontents, which made me wonder when the existing tab contents
will be removed before new one is added, which is not obvious from this change.

This leads to another question whether or not this can be achieved via
NativeTabContentsContainer. This class already has a mechanism to deal with
difference in platforms, so may be we should use it? (it may require some
refactoring
of gtk code though)

Scott, what is your opinion?

http://codereview.chromium.org/4319003/diff/18001/19004
File chrome/browser/renderer_host/backing_store_x.h (right):

http://codereview.chromium.org/4319003/diff/18001/19004#newcode17
chrome/browser/renderer_host/backing_store_x.h:17: }
   // namespace gfx

http://codereview.chromium.org/4319003/diff/18001/19006
File chrome/browser/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/4319003/diff/18001/19006#newcode47
chrome/browser/views/tab_contents/tab_contents_container.cc:47: views::View *v =
(TabContentsViewViews*)contents->view();
static_cast

http://codereview.chromium.org/4319003/diff/18001/19006#newcode54
chrome/browser/views/tab_contents/tab_contents_container.cc:54: Layout();
do you need to set layout manager everytime?
Also you may not need to re-layout when re-adding?

http://codereview.chromium.org/4319003/diff/18001/19007
File chrome/browser/views/tab_contents/tab_contents_view_views.cc (right):

http://codereview.chromium.org/4319003/diff/18001/19007#newcode35
chrome/browser/views/tab_contents/tab_contents_view_views.cc:35: #endif
do you really this? If there is good reason, then should this be in header
rather than .cc?

http://codereview.chromium.org/4319003/diff/18001/19008
File chrome/browser/views/tab_contents/tab_contents_view_views.h (right):

http://codereview.chromium.org/4319003/diff/18001/19008#newcode26
chrome/browser/views/tab_contents/tab_contents_view_views.h:26: }
  // namespace gfx

http://codereview.chromium.org/4319003/diff/18001/19008#newcode30
chrome/browser/views/tab_contents/tab_contents_view_views.h:30: }
  // namespace views

http://codereview.chromium.org/4319003/diff/18001/19008#newcode99
chrome/browser/views/tab_contents/tab_contents_view_views.h:99: //
---------------------------------------------------------------------------
nuke this

http://codereview.chromium.org/4319003/diff/18001/19008#newcode115
chrome/browser/views/tab_contents/tab_contents_view_views.h:115: // TODO
scoped_ptr<TabContentsDragSource> drag_source_;
Use standard format for TODO.

http://codereview.chromium.org/4319003/diff/18001/19008#newcode122
chrome/browser/views/tab_contents/tab_contents_view_views.h:122: // TODO
scoped_ptr<WebDragDestGtk> drag_dest_;
same here.

[brettw, 2010-11-08 21:44:06]

Few style nits, I don't have any other comments so don't wait for an LGTM from
me to check in.

http://codereview.chromium.org/4319003/diff/18001/19007
File chrome/browser/views/tab_contents/tab_contents_view_views.cc (right):

http://codereview.chromium.org/4319003/diff/18001/19007#newcode221
chrome/browser/views/tab_contents/tab_contents_view_views.cc:221:
NOTIMPLEMENTED();  // not even clear a drag cursor will make sense for touch
Please check your comments that they begin with capitals and end with periods.

http://codereview.chromium.org/4319003/diff/18001/19010
File gfx/canvas.h (right):

http://codereview.chromium.org/4319003/diff/18001/19010#newcode104
gfx/canvas.h:104: // Fills the specified region with the specified color and
mode
Can you put a blank line above this?

[Alex Nicolaou, 2010-11-12 04:05:00]

Oshima/Brett: please see updates and let me know if the patch meets with your
satisfaction.

Rob: can you please download this and give it a more thorough once-over? Working
via NX from disney is latency hell and I didn't spend much time testing, though
the patch should be a no-op some more testing would be great. Also if more
revisions are needed that can't just get a TODO I leave it to you to judge if
you want to finish it for me or wait until I'm back from vacation - this patch
is OK to commit in my view (only really affects our build anyway) but of course
I don't want anything terrible getting committed.

thanks
alex

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/backing_store_x.h (right):

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/backing_store_x.h:17: }
On 2010/11/08 20:59:38, oshima wrote:
>    // namespace gfx

Done.

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
File chrome/browser/views/tab_contents/tab_contents_container.cc (right):

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
chrome/browser/views/tab_contents/tab_contents_container.cc:54: Layout();
On 2010/11/08 20:59:38, oshima wrote:
> do you need to set layout manager everytime?
> Also you may not need to re-layout when re-adding?

I moved these into the if(...)

Probably I don't need to set the layout manager every time but I don't know
where you'd prefer I set it - in the constructor?

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
File chrome/browser/views/tab_contents/tab_contents_view_views.cc (right):

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
chrome/browser/views/tab_contents/tab_contents_view_views.cc:35: #endif
On 2010/11/08 20:59:38, oshima wrote:
> do you really this? If there is good reason, then should this be in header
> rather than .cc?

Removed. Seemed like a harmless check to me.

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
chrome/browser/views/tab_contents/tab_contents_view_views.cc:221:
NOTIMPLEMENTED();  // not even clear a drag cursor will make sense for touch
On 2010/11/08 21:44:06, brettw wrote:
> Please check your comments that they begin with capitals and end with periods.

Done.

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
File chrome/browser/views/tab_contents/tab_contents_view_views.h (right):

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
chrome/browser/views/tab_contents/tab_contents_view_views.h:26: }
On 2010/11/08 20:59:38, oshima wrote:
>   // namespace gfx

Done.

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
chrome/browser/views/tab_contents/tab_contents_view_views.h:30: }
On 2010/11/08 20:59:38, oshima wrote:
>   // namespace views

Done.

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
chrome/browser/views/tab_contents/tab_contents_view_views.h:99: //
---------------------------------------------------------------------------
On 2010/11/08 20:59:38, oshima wrote:
> nuke this

Done. This is just a replica of what's been done in the extremely similar
TabContentsViewGtk header

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
chrome/browser/views/tab_contents/tab_contents_view_views.h:115: // TODO
scoped_ptr<TabContentsDragSource> drag_source_;
On 2010/11/08 20:59:38, oshima wrote:
> Use standard format for TODO.

Done.

http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
chrome/browser/views/tab_contents/tab_contents_view_views.h:122: // TODO
scoped_ptr<WebDragDestGtk> drag_dest_;
On 2010/11/08 20:59:38, oshima wrote:
> same here.

Done.

http://codereview.chromium.org/4319003/diff/18001/gfx/canvas.h
File gfx/canvas.h (right):

http://codereview.chromium.org/4319003/diff/18001/gfx/canvas.h#newcode104
gfx/canvas.h:104: // Fills the specified region with the specified color and
mode
On 2010/11/08 21:44:06, brettw wrote:
> Can you put a blank line above this?

Done, but that's not the way overloads are done elsewhere in this file (see
DrawBitmapInt for example).

[Alex Nicolaou, 2010-11-12 04:07:59]

[-rjrkoege, +rjkroege] <blush>

On Thu, Nov 11, 2010 at 8:05 PM, <anicolao@chromium.org> wrote:

> Oshima/Brett: please see updates and let me know if the patch meets with
> your
> satisfaction.
>
> Rob: can you please download this and give it a more thorough once-over?
> Working
> via NX from disney is latency hell and I didn't spend much time testing,
> though
> the patch should be a no-op some more testing would be great. Also if more
> revisions are needed that can't just get a TODO I leave it to you to judge
> if
> you want to finish it for me or wait until I'm back from vacation - this
> patch
> is OK to commit in my view (only really affects our build anyway) but of
> course
> I don't want anything terrible getting committed.
>
> thanks
> alex
>
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/backing_store_x.h (right):
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/renderer_hos...
>
> chrome/browser/renderer_host/backing_store_x.h:17: }
> On 2010/11/08 20:59:38, oshima wrote:
>
>>    // namespace gfx
>>
>
> Done.
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
> File chrome/browser/views/tab_contents/tab_contents_container.cc
> (right):
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
> chrome/browser/views/tab_contents/tab_contents_container.cc:54:
> Layout();
>
> On 2010/11/08 20:59:38, oshima wrote:
>
>> do you need to set layout manager everytime?
>> Also you may not need to re-layout when re-adding?
>>
>
> I moved these into the if(...)
>
> Probably I don't need to set the layout manager every time but I don't
> know where you'd prefer I set it - in the constructor?
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
>
> File chrome/browser/views/tab_contents/tab_contents_view_views.cc
> (right):
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
> chrome/browser/views/tab_contents/tab_contents_view_views.cc:35: #endif
>
> On 2010/11/08 20:59:38, oshima wrote:
>
>> do you really this? If there is good reason, then should this be in
>>
> header
>
>> rather than .cc?
>>
>
> Removed. Seemed like a harmless check to me.
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
>
> chrome/browser/views/tab_contents/tab_contents_view_views.cc:221:
> NOTIMPLEMENTED();  // not even clear a drag cursor will make sense for
> touch
> On 2010/11/08 21:44:06, brettw wrote:
>
>> Please check your comments that they begin with capitals and end with
>>
> periods.
>
> Done.
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
> File chrome/browser/views/tab_contents/tab_contents_view_views.h
> (right):
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
> chrome/browser/views/tab_contents/tab_contents_view_views.h:26: }
> On 2010/11/08 20:59:38, oshima wrote:
>
>>   // namespace gfx
>>
>
> Done.
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
> chrome/browser/views/tab_contents/tab_contents_view_views.h:30: }
> On 2010/11/08 20:59:38, oshima wrote:
>
>>   // namespace views
>>
>
> Done.
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
> chrome/browser/views/tab_contents/tab_contents_view_views.h:99: //
> ---------------------------------------------------------------------------
> On 2010/11/08 20:59:38, oshima wrote:
>
>> nuke this
>>
>
> Done. This is just a replica of what's been done in the extremely
> similar TabContentsViewGtk header
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
>
> chrome/browser/views/tab_contents/tab_contents_view_views.h:115: // TODO
> scoped_ptr<TabContentsDragSource> drag_source_;
> On 2010/11/08 20:59:38, oshima wrote:
>
>> Use standard format for TODO.
>>
>
> Done.
>
>
>
http://codereview.chromium.org/4319003/diff/18001/chrome/browser/views/tab_co...
>
> chrome/browser/views/tab_contents/tab_contents_view_views.h:122: // TODO
> scoped_ptr<WebDragDestGtk> drag_dest_;
> On 2010/11/08 20:59:38, oshima wrote:
>
>> same here.
>>
>
> Done.
>
> http://codereview.chromium.org/4319003/diff/18001/gfx/canvas.h
>
> File gfx/canvas.h (right):
>
> http://codereview.chromium.org/4319003/diff/18001/gfx/canvas.h#newcode104
>
> gfx/canvas.h:104: // Fills the specified region with the specified color
> and mode
> On 2010/11/08 21:44:06, brettw wrote:
>
>> Can you put a blank line above this?
>>
>
> Done, but that's not the way overloads are done elsewhere in this file
> (see DrawBitmapInt for example).
>
>
> http://codereview.chromium.org/4319003/
>

[brettw, 2010-11-12 21:55:51]

LGTM with these minor changes

http://codereview.chromium.org/4319003/diff/32001/chrome/browser/gtk/constrai...
File chrome/browser/gtk/constrained_window_gtk.h (right):

http://codereview.chromium.org/4319003/diff/32001/chrome/browser/gtk/constrai...
chrome/browser/gtk/constrained_window_gtk.h:21: typedef TabContentsViewViews
TabContentsViewType;
Can you put this typedef inside the class so it's scoped. Otherwise, you have
this weird typedef in the global scope. Normally we would put this at the top of
the public section.

http://codereview.chromium.org/4319003/diff/32001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/backing_store_x.h (right):

http://codereview.chromium.org/4319003/diff/32001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/backing_store_x.h:48: void XShowRect(const
gfx::Rect& damage, XID target);
The style guide says not to have overloaded functions like this. I'd just change
the callers to say Point(0, 0) which is pretty easy and obvious

[oshima, 2010-11-13 01:36:08]

LGTM

http://codereview.chromium.org/4319003/diff/32001/chrome/browser/gtk/constrai...
File chrome/browser/gtk/constrained_window_gtk.h (right):

http://codereview.chromium.org/4319003/diff/32001/chrome/browser/gtk/constrai...
chrome/browser/gtk/constrained_window_gtk.h:21: typedef TabContentsViewViews
TabContentsViewType;
On 2010/11/12 21:55:51, brettw wrote:
> Can you put this typedef inside the class so it's scoped. Otherwise, you have
> this weird typedef in the global scope. Normally we would put this at the top
of
> the public section.

agreed

http://codereview.chromium.org/4319003/diff/32001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/4319003/diff/32001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:3431:
'browser/ui/views/tab_contents/tab_contents_view_views.h',
replace tabs with space