Linux build fixes for webframe_imple.cc

Fix compiler warnings
Implement copy constructor for the bitmap
there is no operator= for the rect, use the set functions

[Evan Martin, 2008-10-13 16:59:58]

+Brett to comment on copy constructors in graphics code.

http://codereview.chromium.org/7244/diff/1/3
File base/gfx/bitmap_platform_device_linux.h (right):

http://codereview.chromium.org/7244/diff/1/3#newcode27
Line 27: BitmapPlatformDeviceLinux(const BitmapPlatformDeviceLinux& other);
We avoid copy constructors everywhere else in the code, but it seems the classes
on other platforms just use the implicit ones.

http://codereview.chromium.org/7244/diff/1/4
File webkit/glue/webframe_impl.cc (left):

http://codereview.chromium.org/7244/diff/1/4#oldcode727
Line 727: FrameView* view = frame()->view();
It looks like this variable is needed in the OS_WIN case (?)

http://codereview.chromium.org/7244/diff/1/4#oldcode756
Line 756: FrameView* view = frame()->view();
Here too.

http://codereview.chromium.org/7244/diff/1/4
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/7244/diff/1/4#newcode1532
Line 1532: frameview()->height(), true);
can you line this up with the paren on the previous line?

[icefox::TorchMobile, 2008-10-13 17:19:39]

http://codereview.chromium.org/7244/diff/1/4
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/7244/diff/1/4#newcode1532
Line 1532: frameview()->height(), true);
On 2008/10/13 16:59:58, Evan Martin wrote:
> can you line this up with the paren on the previous line?

Should I modify the windows and mac code to also line up?

[Evan Martin, 2008-10-13 17:36:56]

http://codereview.chromium.org/7244/diff/1/4
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/7244/diff/1/4#newcode1532
Line 1532: frameview()->height(), true);
On 2008/10/13 17:19:39, icefox::TorchMobile wrote:
> On 2008/10/13 16:59:58, Evan Martin wrote:
> > can you line this up with the paren on the previous line?
> 
> Should I modify the windows and mac code to also line up?

As far as I can tell they already do?
I'm just suggesting one extra space at the beginning of L1532.

[icefox::TorchMobile, 2008-10-13 18:29:25]

On 2008/10/13 17:36:56, Evan Martin wrote:
> http://codereview.chromium.org/7244/diff/1/4
> File webkit/glue/webframe_impl.cc (right):
> 
> http://codereview.chromium.org/7244/diff/1/4#newcode1532
> Line 1532: frameview()->height(), true);
> On 2008/10/13 17:19:39, icefox::TorchMobile wrote:
> > On 2008/10/13 16:59:58, Evan Martin wrote:
> > > can you line this up with the paren on the previous line?
> > 
> > Should I modify the windows and mac code to also line up?
> 
> As far as I can tell they already do?
> I'm just suggesting one extra space at the beginning of L1532.

Uploaded new patch
Add spaces so they line up
Moves the Frame variable inside of the windows ifdef rather then removing them.

[brettw, 2008-10-13 23:17:40]

http://codereview.chromium.org/7244/diff/7/8
File base/gfx/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/7244/diff/7/8#newcode41
Line 41: BitmapPlatformDeviceLinux::BitmapPlatformDeviceLinux(const
BitmapPlatformDeviceLinux& other)
80 cols?

http://codereview.chromium.org/7244/diff/7/10
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/7244/diff/7/10#newcode114
Line 114: #include "PlatformScrollBar.h"
If we're not going to keep things alphabetical, I'd prefer to move the
platform-specific includes to a separate section after the other ones.

http://codereview.chromium.org/7244/diff/7/10#newcode757
Line 757: FrameView* view = frame()->view();
This change is to just fix the warnings, not because you don't have FrameView on
Linux, right?

http://codereview.chromium.org/7244/diff/7/10#newcode1529
Line 1529: gfx::BitmapPlatformDevice WebFrameImpl::CaptureImage(bool
scroll_to_zero) {
I'm not very happy with this set of ifdefs for this function being totally
separate. The code is very similar. Can you put them all together? You should be
able to use the "PlatformCanvas" typedef to declare the correct class for each
platform. The only difference will be how "gc" is constructed and the addition
of "fixupAlpha..." on Windows at the end.

Layout()

gfx::PlatformCanvas canvas(...)

http://codereview.chromium.org/7244/diff/7/10#newcode1862
Line 1862: page_size->SetRect(r.x(), r.y(), r.width(), r.height());
How does this conversion happen on Mac & Windows. I'm not sure why Linux would
be different in this regard.

[icefox::TorchMobile, 2008-10-14 15:38:44]

Uploaded a new patch and commented on the comments.

[icefox::TorchMobile, 2008-10-14 15:39:54]

http://codereview.chromium.org/7244/diff/7/8
File base/gfx/bitmap_platform_device_linux.cc (right):

http://codereview.chromium.org/7244/diff/7/8#newcode41
Line 41: BitmapPlatformDeviceLinux::BitmapPlatformDeviceLinux(const
BitmapPlatformDeviceLinux& other)
On 2008/10/13 23:17:40, brettw wrote:
> 80 cols?

Done.

http://codereview.chromium.org/7244/diff/7/10
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/7244/diff/7/10#newcode114
Line 114: #include "PlatformScrollBar.h"
On 2008/10/13 23:17:40, brettw wrote:
> If we're not going to keep things alphabetical, I'd prefer to move the
> platform-specific includes to a separate section after the other ones.

Moved this back to keep it in alphabetical order

http://codereview.chromium.org/7244/diff/7/10#newcode757
Line 757: FrameView* view = frame()->view();
On 2008/10/13 23:17:40, brettw wrote:
> This change is to just fix the warnings, not because you don't have FrameView
on
> Linux, right?

Correct, just a warning fix.

http://codereview.chromium.org/7244/diff/7/10#newcode1529
Line 1529: gfx::BitmapPlatformDevice WebFrameImpl::CaptureImage(bool
scroll_to_zero) {
On 2008/10/13 23:17:40, brettw wrote:
> I'm not very happy with this set of ifdefs for this function being totally
> separate. The code is very similar. Can you put them all together? You should
be
> able to use the "PlatformCanvas" typedef to declare the correct class for each
> platform. The only difference will be how "gc" is constructed and the addition
> of "fixupAlpha..." on Windows at the end.
> 
> Layout()
> 
> gfx::PlatformCanvas canvas(...)

This code has been refactored into something much smaller and cleaner.

http://codereview.chromium.org/7244/diff/7/10#newcode1862
Line 1862: page_size->SetRect(r.x(), r.y(), r.width(), r.height());
On 2008/10/13 23:17:40, brettw wrote:
> How does this conversion happen on Mac & Windows. I'm not sure why Linux would
> be different in this regard.

gfx::Rect doesn't have a operator overload for WebCore::IntRect on Linux.  On
windows there is a constructor for a RECT and on OS X there is a constructor for
CGRect (same with operator=)  On Linux I think we are using a GdkRectangle so we
could add support for that to gfx::Rect

[brettw, 2008-10-14 15:56:19]

> On Linux I think we are using a GdkRectangle so we
> could add support for that to gfx::Rect

If the other platforms have this, we should add it for Gdk as well. I feel like
we do this in other places, so it will be less pain to make it work like
everybody else.

http://codereview.chromium.org/7244/diff/210/16
File base/gfx/bitmap_platform_device_linux.h (right):

http://codereview.chromium.org/7244/diff/210/16#newcode27
Line 27: BitmapPlatformDeviceLinux(const BitmapPlatformDeviceLinux& other);
Is this copy constructor necessary? Where is it required?

http://codereview.chromium.org/7244/diff/210/17
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/7244/diff/210/17#newcode1500
Line 1500: frameview()->height(), true);
Can you align this with the previous "frameview" after the (

Thanks for cleaning this function up.

[icefox::TorchMobile, 2008-10-14 20:34:31]

On 2008/10/14 15:56:19, brettw wrote:
> > On Linux I think we are using a GdkRectangle so we
> > could add support for that to gfx::Rect
> 
> If the other platforms have this, we should add it for Gdk as well. I feel
like
> we do this in other places, so it will be less pain to make it work like
> everybody else.

I have added the functions to gfx/rect and changed that bit of code back.

> http://codereview.chromium.org/7244/diff/210/16
> File base/gfx/bitmap_platform_device_linux.h (right):
> 
> http://codereview.chromium.org/7244/diff/210/16#newcode27
> Line 27: BitmapPlatformDeviceLinux(const BitmapPlatformDeviceLinux& other);
> Is this copy constructor necessary? Where is it required?

In the return statement.

> http://codereview.chromium.org/7244/diff/210/17
> File webkit/glue/webframe_impl.cc (right):
> 
> http://codereview.chromium.org/7244/diff/210/17#newcode1500
> Line 1500: frameview()->height(), true);
> Can you align this with the previous "frameview" after the (

Done

> Thanks for cleaning this function up.

No problem

[icefox::TorchMobile, 2008-10-15 20:04:13]

On 2008/10/14 20:34:31, icefox::TorchMobile wrote:
> On 2008/10/14 15:56:19, brettw wrote:
> > > On Linux I think we are using a GdkRectangle so we
> > > could add support for that to gfx::Rect
> > 
> > If the other platforms have this, we should add it for Gdk as well. I feel
> like
> > we do this in other places, so it will be less pain to make it work like
> > everybody else.
> 
> I have added the functions to gfx/rect and changed that bit of code back.
> 
> > http://codereview.chromium.org/7244/diff/210/16
> > File base/gfx/bitmap_platform_device_linux.h (right):
> > 
> > http://codereview.chromium.org/7244/diff/210/16#newcode27
> > Line 27: BitmapPlatformDeviceLinux(const BitmapPlatformDeviceLinux& other);
> > Is this copy constructor necessary? Where is it required?
> 
> In the return statement.
> 
> > http://codereview.chromium.org/7244/diff/210/17
> > File webkit/glue/webframe_impl.cc (right):
> > 
> > http://codereview.chromium.org/7244/diff/210/17#newcode1500
> > Line 1500: frameview()->height(), true);
> > Can you align this with the previous "frameview" after the (
> 
> Done
> 
> > Thanks for cleaning this function up.
> 
> No problem

Closing as Tony's patch went in already.