Start writing the GTK code for test_shell.

Start writing the GTK code for test_shell.

[agl, 2008-10-28 00:03:47]

This allows test_shell to paint an empty background.

[Evan Martin, 2008-10-28 00:21:59]

I know it's hard habit to break, but we put * and & on the left everywhere else
in the code.

http://codereview.chromium.org/8000/diff/19/207
File base/gfx/bitmap_platform_device_linux.h (right):

http://codereview.chromium.org/8000/diff/19/207#newcode26
Line 26: // function to allocate in shared memory.
Perhaps this should be the other way around?  We could have our own memory here,
and then wrap a GdkPixbuf around it to make GTK happy.

http://library.gnome.org/devel/gdk-pixbuf/stable/gdk-pixbuf-creating.html "Image
Data in Memory — Creating a pixbuf from image data that is already in memory."

[tony, 2008-10-28 00:29:42]

http://codereview.chromium.org/8000/diff/19/206
File base/gfx/bitmap_platform_device_linux.cc (left):

http://codereview.chromium.org/8000/diff/19/206#oldcode24
Line 24: #ifndef NDEBUG
It seems like we would still want this or is the plan to keep it on all the time
for now?

http://codereview.chromium.org/8000/diff/19/210
File webkit/tools/test_shell/gtk/test_shell.cc (left):

http://codereview.chromium.org/8000/diff/19/210#oldcode83
Line 83: // It's funny, ok?
Ah, the price of progress.

http://codereview.chromium.org/8000/diff/19/211
File webkit/tools/test_shell/gtk/webview_host.cc (right):

http://codereview.chromium.org/8000/diff/19/211#newcode20
Line 20: //
/usr/local/google/agl/src/chrome/src/webkit/tools/test_shell/gtk/webview_host.cc:19:
error: no matching function for call to 'WebWidgetHost::Create(GtkWidget*&,
WebViewDelegate*&)'
Is this comment still valid?

http://codereview.chromium.org/8000/diff/19/211#newcode21
Line 21: WebViewHost* host = (WebViewHost *) WebWidgetHost::Create(box, NULL);
Can we use a c++ style cast or is this supposed to follow gtk style?

http://codereview.chromium.org/8000/diff/19/212
File webkit/tools/test_shell/gtk/webwidget_host.cc (right):

http://codereview.chromium.org/8000/diff/19/212#newcode19
Line 19: static gboolean configureEvent(GtkWidget *widget, GdkEventConfigure
*config,
Nit: You might want to namespace these.

http://codereview.chromium.org/8000/diff/19/212#newcode110
Line 110: void
Nit: The return type should be on the same line as the method declaration.

http://codereview.chromium.org/8000/diff/19/212#newcode119
Line 119: void
Same thing here.

http://codereview.chromium.org/8000/diff/19/212#newcode167
Line 167: void
One more.

http://codereview.chromium.org/8000/diff/19/213
File webkit/tools/test_shell/webwidget_host.h (right):

http://codereview.chromium.org/8000/diff/19/213#newcode77
Line 77: void Paint();
Paint() is on all platforms, can you pull it out?

[agl, 2008-10-28 01:09:56]

Should all be addressed.

http://codereview.chromium.org/8000/diff/19/206
File base/gfx/bitmap_platform_device_linux.cc (left):

http://codereview.chromium.org/8000/diff/19/206#oldcode24
Line 24: #ifndef NDEBUG
On 2008/10/28 00:29:43, tony wrote:
> It seems like we would still want this or is the plan to keep it on all the
time
> for now?

Unintended omission, thanks.

http://codereview.chromium.org/8000/diff/19/207
File base/gfx/bitmap_platform_device_linux.h (right):

http://codereview.chromium.org/8000/diff/19/207#newcode26
Line 26: // function to allocate in shared memory.
ack.

http://codereview.chromium.org/8000/diff/19/211
File webkit/tools/test_shell/gtk/webview_host.cc (right):

http://codereview.chromium.org/8000/diff/19/211#newcode20
Line 20: //
/usr/local/google/agl/src/chrome/src/webkit/tools/test_shell/gtk/webview_host.cc:19:
error: no matching function for call to 'WebWidgetHost::Create(GtkWidget*&,
WebViewDelegate*&)'
On 2008/10/28 00:29:43, tony wrote:
> Is this comment still valid?

For now, yes.

http://codereview.chromium.org/8000/diff/19/211#newcode21
Line 21: WebViewHost* host = (WebViewHost *) WebWidgetHost::Create(box, NULL);
On 2008/10/28 00:29:43, tony wrote:
> Can we use a c++ style cast or is this supposed to follow gtk style?

We can use a C++ cast, fixed.

http://codereview.chromium.org/8000/diff/19/212
File webkit/tools/test_shell/gtk/webwidget_host.cc (right):

http://codereview.chromium.org/8000/diff/19/212#newcode19
Line 19: static gboolean configureEvent(GtkWidget *widget, GdkEventConfigure
*config,
On 2008/10/28 00:29:43, tony wrote:
> Nit: You might want to namespace these.

Ack. Anonymous namespace added.

http://codereview.chromium.org/8000/diff/19/212#newcode110
Line 110: void
On 2008/10/28 00:29:43, tony wrote:
> Nit: The return type should be on the same line as the method declaration.

ack.

http://codereview.chromium.org/8000/diff/19/212#newcode119
Line 119: void
On 2008/10/28 00:29:43, tony wrote:
> Same thing here.

ack.

http://codereview.chromium.org/8000/diff/19/212#newcode167
Line 167: void
On 2008/10/28 00:29:43, tony wrote:
> One more.

ack.

http://codereview.chromium.org/8000/diff/19/213
File webkit/tools/test_shell/webwidget_host.h (right):

http://codereview.chromium.org/8000/diff/19/213#newcode77
Line 77: void Paint();
On 2008/10/28 00:29:43, tony wrote:
> Paint() is on all platforms, can you pull it out?

yep.

[tony, 2008-10-28 16:01:14]

LGTM.  Some style nits, there are still lots of Foo *bar args that should be
Foo* bar.  I will let evan review the gtk code in webwidget_host.cc.

http://codereview.chromium.org/8000/diff/22/23
File base/gfx/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/8000/diff/22/23#newcode20
Line 20: GdkPixbuf *pixbuf = gdk_pixbuf_new(GDK_COLORSPACE_RGB, true, 8, width,
height);
Nit: GdkPixbuf*

http://codereview.chromium.org/8000/diff/22/23#newcode21
Line 21: if (!pixbuf) return NULL;
Nit: Can you put the return on it's own line?

[agl, 2008-10-28 17:40:11]

Fixed style issues with a search and replace to put the asterisk in the wrong
place and commited. Cheers.

[Evan Martin, 2008-10-28 18:04:21]

No functional comments.  You also went over 80 columns in a bunch of places, for
what it's worth.

http://codereview.chromium.org/8000/diff/32/220
File base/gfx/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/8000/diff/32/220#newcode9
Line 9: #include "gdk/gdk.h"
I think we want <> around system headers, and these should be before L7.

http://codereview.chromium.org/8000/diff/32/226
File webkit/tools/test_shell/gtk/webwidget_host.cc (right):

http://codereview.chromium.org/8000/diff/32/226#newcode14
Line 14: #include <gtk/gtk.h>
system headers before our headers (should be on L7)

http://codereview.chromium.org/8000/diff/32/226#newcode21
Line 21: static gboolean configureEvent(GtkWidget* widget, GdkEventConfigure*
config,
Since these are in the anonymous namespace anyway, they're already static and
the "static" keyword isn't necessary.

It's a bit strange to have mixed webkitStyle and GoogleStyle function names in
this file -- I think it should all be GoogleStyle.

[agl, 2008-10-28 18:30:42]

cheers

http://codereview.chromium.org/8000/diff/32/220
File base/gfx/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/8000/diff/32/220#newcode9
Line 9: #include "gdk/gdk.h"
On 2008/10/28 18:04:21, Evan Martin wrote:
> I think we want <> around system headers, and these should be before L7.

ack.

http://codereview.chromium.org/8000/diff/32/226
File webkit/tools/test_shell/gtk/webwidget_host.cc (right):

http://codereview.chromium.org/8000/diff/32/226#newcode21
Line 21: static gboolean configureEvent(GtkWidget* widget, GdkEventConfigure*
config,
On 2008/10/28 18:04:21, Evan Martin wrote:
> Since these are in the anonymous namespace anyway, they're already static and
> the "static" keyword isn't necessary.
> 
> It's a bit strange to have mixed webkitStyle and GoogleStyle function names in
> this file -- I think it should all be GoogleStyle.

ack and ack.