Bitmap transport

Bitmap transport

This patch reworks bitmap transport on all platforms. Linux and Mac
are switched from serialising bitmaps over the IPC channel to using
shared memory. All platforms gain a shared memory mapping cache on the
host side.

The concept of a TransportDIB (device independent bitmap) is added to
encapsulate most of the platform specifics.

On Linux, we use SysV shared memory. This is because X shared pixmaps,
which predate POSIX SHM, can only use SysV. By using SysV between
renderer and browser, we open up the possibility to map the shared
memory directly from the renderer to the X server.

On Mac, we use POSIX shared memory. However, since this needs
filesystem access and the Mac renderer is sandboxed from the
filesystem, we add two new messages from renderer -> browser:

The first, AllocTransportDIB, synchronously creates a transport DIB in
the browser and passes a handle back to the renderer. The second,
FreeTransportDIB, asynchronously, notifies the browser that it may
close its handle to the shared memory region.

On Mac, the shared memory regions are identified by their inode
numbers on the wire. This means that the browser must keep handles
open to all the allocated shared memory regions (since an inode number
is insufficient to map the region). The alternative design is that the
renderer passes the file descriptor with each paint operation. Since
passing file descriptors is special case in the code, I felt that it
would be best to minimise their use. Creating and freeing transport
DIBs are relatively rare operations relative to paints and scrolls.

On Windows, most of the code remains the same, except that Windows now
uses the mapping cache added in this patch. This allows the browser to
maintain a shared memory mapping for a transport DIB over several
paints. Previously it mapped and unmapped for every operation, causing
lots of TLB and VM churn.

[darin (slow to review), 2009-02-19 01:13:52]

looks awesome overall.  sorry for the pile of nits.  the "spammage of revoked
tasks" issue can be postponed to a follow-up CL.  the biggest problem i think is
with the sync IPCs that terminate on the browser UI thread -- oops!

http://codereview.chromium.org/21485/diff/3001/3002
File base/shared_memory.h (right):

http://codereview.chromium.org/21485/diff/3001/3002#newcode27
Line 27: typedef HANDLE SharedMemoryHandle;
no SharedMemoryId here?  maybe the comment about SharedMemoryId should be moved
down into the OS_POSIX section?

http://codereview.chromium.org/21485/diff/3001/3002#newcode108
Line 108: SharedMemoryId id() const { return inode_; }
is this really a unique identifier?  i thought different filesystems could have
the same inode number.  maybe you mean "unique" ?

http://codereview.chromium.org/21485/diff/3001/3003
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/21485/diff/3001/3003#newcode137
Line 137: bool SharedMemory::CreateOrOpen(const std::wstring &name,
this method is very long :(

http://codereview.chromium.org/21485/diff/3001/3004
File chrome/browser/renderer_host/backing_store.cc (right):

http://codereview.chromium.org/21485/diff/3001/3004#newcode63
Line 63: TransportDIB* dib,
maybe you should change the parameter name uniformly in the header file as well.
 how about using the name bitmap so that bitmap_rect still refers back to this
parameter?  or maybe change bitmap_rect to dib_rect.

http://codereview.chromium.org/21485/diff/3001/3006
File chrome/browser/renderer_host/backing_store_posix.cc (right):

http://codereview.chromium.org/21485/diff/3001/3006#newcode29
Line 29: LOG(WARNING) << "Submitted transport DIB is too small"
do you really need this logging in release mode?

http://codereview.chromium.org/21485/diff/3001/3008
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/21485/diff/3001/3008#newcode617
Line 617: dibid.first, GetRendererProcessHandle(), false /* read only? */);
dibid.first is really quite unreadable.  how about using a struct with named
fields instead?

why the question mark after "read only"?  i believe we only require read access
to the transport DIBs from the browser side.

http://codereview.chromium.org/21485/diff/3001/3008#newcode619
Line 619: #elif defined(OS_MACOSX)  // defined(OS_WIN)
nit: this // defined(OS_WIN) comment seems like noise to me.

http://codereview.chromium.org/21485/diff/3001/3008#newcode629
Line 629: #elif defined(OS_LINUX)  // defined(OS_MACOSX)
ditto

http://codereview.chromium.org/21485/diff/3001/3008#newcode649
Line 649: std::map<TransportDIB::Id, TransportDIB*>::iterator smallest_iterator;
here's where a typedef would be nice...

http://codereview.chromium.org/21485/diff/3001/3008#newcode677
Line 677: cached_dibs_clearer_factory_.RevokeAll();
i suspect that this function will get called a lot.  the result will be a flood
of Task objects in the MessageLoop that just do nothing.

i think we should use an approach such as the one used by
RenderWidgetHost::StartHangMonitorTimeout, which really ought to be factored out
into a helper class.

at the very least, it is cleaner to use OneShotTimer<T> here than an explicit
call to PostDelayedTask.  but, i really think we need something better than
spammage of revoked tasks.

http://codereview.chromium.org/21485/diff/3001/3008#newcode702
Line 702: IPC_MESSAGE_HANDLER(ViewHostMsg_AllocTransportDIB,
thanks for taking care of OSX with this patch!  we should really intercept these
messages on the IO thread, however, since it is not valid to terminate a
synchronous IPC on the browser UI thread.  else, you could get nasty deadlocks.

you can move these handlers to ResourceMessageFilter (which is very much poorly
named now-a-days).

http://codereview.chromium.org/21485/diff/3001/3008#newcode856
Line 856: allocated_dibs_[shmem->id()] = dup(result->a.fd);
ugh, having to mess with allocated_dibs_ from the IO thread may be nasty.  you
probably need to move allocated_dibs_ to the RenderWidgetHelper, which can be
accessed from either thread.

http://codereview.chromium.org/21485/diff/3001/3009
File chrome/browser/renderer_host/browser_render_process_host.h (right):

http://codereview.chromium.org/21485/diff/3001/3009#newcode66
Line 66: virtual TransportDIB* GetTransportDIB(TransportDIB::Id dibid);
dib_id

http://codereview.chromium.org/21485/diff/3001/3009#newcode147
Line 147: std::map<TransportDIB::Id, TransportDIB*> cached_dibs_;
how about a typedef here?  TransportDIBMap?

http://codereview.chromium.org/21485/diff/3001/3009#newcode154
Line 154: TransportDIB* MapTransportDIB(TransportDIB::Id dibid);
dib_id

http://codereview.chromium.org/21485/diff/3001/3009#newcode159
Line 159: std::Map<TransportDIB::Id, int> allocated_dibs_;
did you mean std::map?

typedef for this map?

http://codereview.chromium.org/21485/diff/3001/3009#newcode162
Line 162: void OnDetachTransportDIB(TransportDIB::Id dibid);
dib_id

http://codereview.chromium.org/21485/diff/3060/85
File chrome/browser/renderer_host/render_process_host.h (right):

http://codereview.chromium.org/21485/diff/3060/85#newcode135
Line 135: virtual TransportDIB* GetTransportDIB(TransportDIB::Id dibid) = 0;
dib_id

http://codereview.chromium.org/21485/diff/3060/86
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/21485/diff/3060/86#newcode432
Line 432: LOG(WARNING) << "Transport DIB too small for given rectangle";
this seems like it corresponds to a corrupt renderer, right?  we should probably
kill the renderer in this case.

at the very least, it is a bug that you don't ACK this PaintRect message.

instead of returning early, you may be better off just skipping the
PaintBackingStoreRect call.  i worry about the rest of this function not
running, which could leave the browser in a bad state.

http://codereview.chromium.org/21485/diff/3060/86#newcode491
Line 491: LOG(WARNING) << "Transport DIB too small for given rectangle";
ditto

http://codereview.chromium.org/21485/diff/3060/86#newcode583
Line 583: const gfx::Rect& bitmap_rect,
dib and bitmap_rect should be consistently named.  not going to repeat this nit.
 please fix all occurrences.

http://codereview.chromium.org/21485/diff/3060/87
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/21485/diff/3060/87#newcode272
Line 272: void ScrollBackingStoreRect(TransportDIB* dib,
dib and bitmap_rect should be named consistently

http://codereview.chromium.org/21485/diff/3060/92
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/21485/diff/3060/92#newcode1074
Line 1074: A a;
instead of .a, how about .data or .value?

also it is a little odd to define this in the mix of a bunch of ParamTraits
defines.  how about moving it up to an earlier location in the file?

http://codereview.chromium.org/21485/diff/3060/95
File chrome/common/transport_dib.h (right):

http://codereview.chromium.org/21485/diff/3060/95#newcode31
Line 31: typedef void* HANDLE;
seems strange to re-define HANDLE on TransportDIB.  how about just including
windows.h and using HANDLE from there?  or put the typedef for HANDLE at global
scope so it matches what windows.h would import.

http://codereview.chromium.org/21485/diff/3060/95#newcode41
Line 41: typedef std::pair<HANDLE, uint64> Id;
can we please have a struct here with named fields that are descriptive? 
.handle and .sequence_number perhaps?

http://codereview.chromium.org/21485/diff/3060/95#newcode48
Line 48: typedef int Id;
would be good to document what these "int" values correspond to.

http://codereview.chromium.org/21485/diff/3060/95#newcode56
Line 56: //
---------------------------------------------------------------------------
*wince* excessive dividers :)

http://codereview.chromium.org/21485/diff/3060/95#newcode57
Line 57: static TransportDIB* Create(size_t size, uint64 epoch);
maybe you should just use Time or TimeTicks here?  i'm not sure that Time::Now
is synchronized well across processes.  is this going to be a good sequence
number source?

how about if we just use IPC routing_id + counter for the sequence number?  in
other words, all we really need is for the renderer to maintain a simple counter
because the browser can always inspect the routing_id of the IPC message to see
what renderer is sending it the message.

http://codereview.chromium.org/21485/diff/3060/95#newcode92
Line 92: base::SharedMemory shm_;
shared_memory_ :)

http://codereview.chromium.org/21485/diff/3060/95#newcode93
Line 93: uint64 epoch_;  // sequence number
how about naming the variable sequence_number_ so this comment is unnecessary?

http://codereview.chromium.org/21485/diff/3060/95#newcode95
Line 95: int shmkey_;  // SysV SHM id
shared_memory_key_ or just key_

http://codereview.chromium.org/21485/diff/3060/96
File chrome/common/transport_dib_linux.cc (right):

http://codereview.chromium.org/21485/diff/3060/96#newcode12
Line 12: 
maybe this file should be named transport_dib_x11.cc since it is just specific
to using Xwindows.

http://codereview.chromium.org/21485/diff/3060/96#newcode36
Line 36: LOG(ERROR) << "Failed to create SysV shared memory region"
DLOG or do you really want this compiled into release?

http://codereview.chromium.org/21485/diff/3060/98
File chrome/common/transport_dib_win.cc (right):

http://codereview.chromium.org/21485/diff/3060/98#newcode19
Line 19: : shm_(handle, false /* read only? */) {
why the question about read only?

http://codereview.chromium.org/21485/diff/3060/98#newcode46
Line 46: LOG(ERROR) << "Failed to map transport DIB"
DLOG is fine

http://codereview.chromium.org/21485/diff/3060/100
File chrome/renderer/render_process.h (right):

http://codereview.chromium.org/21485/diff/3060/100#newcode13
Line 13: class Rect;
contents of namespace should not be indented per google style.  not your
mistake, but can you fix it?

http://codereview.chromium.org/21485/diff/3060/100#newcode73
Line 73: size_t FindFreeCacheSlot(size_t size);
can you just use int for the index?  and say that -1 is the invalid index?  less
verbose.

http://codereview.chromium.org/21485/diff/3060/101
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/21485/diff/3060/101#newcode406
Line 406: RenderProcess::GetDrawingCanvas(&current_paint_buf_, damaged_rect);
common style is to indent by 4 for line continuations (as you can see in the
code you replaced).

http://codereview.chromium.org/21485/diff/3060/101#newcode483
Line 483: RenderProcess::GetDrawingCanvas(&current_scroll_buf_, damaged_rect);
ditto

http://codereview.chromium.org/21485/diff/3060/103
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/21485/diff/3060/103#newcode98
Line 98: shared_buf_ = new base::SharedMemory;
yeah, it is OK for this to not be using AllocSharedMemory.

http://codereview.chromium.org/21485/diff/3060/103#newcode99
Line 99: shared_buf_->Create(L"", false /* read only? */, true /* existing? */,
oh... i think i get the notation with the question mark now.  you are saying
that false answers the question of whether or not we want "read only"... sorry,
that tripped me up.  i thought it was an annotation of uncertainty about whether
false was the right value or not :-/  maybe others would be similarly confused?

http://codereview.chromium.org/21485/diff/3060/106
File skia/ext/platform_canvas_linux.h (right):

http://codereview.chromium.org/21485/diff/3060/106#newcode33
Line 33: bool initialize(int width, int height, bool is_opaque, uint8_t* data);
it is against google style to have overloaded methods.  but this is Skia code,
so i don't know.  maybe you should ask brett.

[agl, 2009-02-19 04:47:42]

Big changes:
  * Added DelayTimer (base/timer.h)
  * Removed the Task storm and switched to DelayTimer
  * Made TransportDIB::Id a struct on Windows

http://codereview.chromium.org/21485/diff/3001/3002
File base/shared_memory.h (right):

http://codereview.chromium.org/21485/diff/3001/3002#newcode27
Line 27: typedef HANDLE SharedMemoryHandle;
On 2009/02/19 01:13:52, darin wrote:
> no SharedMemoryId here?  maybe the comment about SharedMemoryId should be
moved
> down into the OS_POSIX section?

Done.

http://codereview.chromium.org/21485/diff/3001/3002#newcode108
Line 108: SharedMemoryId id() const { return inode_; }
On 2009/02/19 01:13:52, darin wrote:
> is this really a unique identifier?  i thought different filesystems could
have
> the same inode number.  maybe you mean "unique" ?

Have updated comment. In short, inodes are unique to a given filesystem and we
always create the backing files in the same directory, so will get the same
filesystem. It's technically possible that someone could mount a new filesystem
which we're running, in which case there's a tiny chance that their scrolling
will go wrong for about 5 seconds. I'm ok with that.

http://codereview.chromium.org/21485/diff/3001/3003
File base/shared_memory_posix.cc (right):

http://codereview.chromium.org/21485/diff/3001/3003#newcode137
Line 137: bool SharedMemory::CreateOrOpen(const std::wstring &name,
On 2009/02/19 01:13:52, darin wrote:
> this method is very long :(

Agreed. Cleanup scheduled for another CL.

http://codereview.chromium.org/21485/diff/3001/3004
File chrome/browser/renderer_host/backing_store.cc (right):

http://codereview.chromium.org/21485/diff/3001/3004#newcode63
Line 63: TransportDIB* dib,
On 2009/02/19 01:13:52, darin wrote:
> maybe you should change the parameter name uniformly in the header file as
well.
>  how about using the name bitmap so that bitmap_rect still refers back to this
> parameter?  or maybe change bitmap_rect to dib_rect.

Changed to bitmap (and in the header).

http://codereview.chromium.org/21485/diff/3001/3006
File chrome/browser/renderer_host/backing_store_posix.cc (right):

http://codereview.chromium.org/21485/diff/3001/3006#newcode29
Line 29: LOG(WARNING) << "Submitted transport DIB is too small"
On 2009/02/19 01:13:52, darin wrote:
> do you really need this logging in release mode?

Removed since it's already covered, generally, higher up in the call stack.

http://codereview.chromium.org/21485/diff/3001/3008
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/21485/diff/3001/3008#newcode617
Line 617: dibid.first, GetRendererProcessHandle(), false /* read only? */);
> why the question mark after "read only"?  i believe we only require read
access
> to the transport DIBs from the browser side.

Since the /* read only? */ is confusing, I've change them all.

http://codereview.chromium.org/21485/diff/3001/3008#newcode619
Line 619: #elif defined(OS_MACOSX)  // defined(OS_WIN)
On 2009/02/19 01:13:52, darin wrote:
> nit: this // defined(OS_WIN) comment seems like noise to me.

Done.

http://codereview.chromium.org/21485/diff/3001/3008#newcode629
Line 629: #elif defined(OS_LINUX)  // defined(OS_MACOSX)
On 2009/02/19 01:13:52, darin wrote:
> ditto

Done.

http://codereview.chromium.org/21485/diff/3001/3008#newcode677
Line 677: cached_dibs_clearer_factory_.RevokeAll();
On 2009/02/19 01:13:52, darin wrote:
> i really think we need something better than
> spammage of revoked tasks.

Have added base::DelayTimer and switched to using it. I've not changed
StartHangMonitorTimeout to use it in this CL because because I don't want to add
another possible, subtle breakage to this CL.

http://codereview.chromium.org/21485/diff/3001/3008#newcode702
Line 702: IPC_MESSAGE_HANDLER(ViewHostMsg_AllocTransportDIB,
On 2009/02/19 01:13:52, darin wrote:
> thanks for taking care of OSX with this patch!  we should really intercept
these
> messages on the IO thread, however, since it is not valid to terminate a
> synchronous IPC on the browser UI thread.  else, you could get nasty
deadlocks.
> 
> you can move these handlers to ResourceMessageFilter (which is very much
poorly
> named now-a-days).

Done.

http://codereview.chromium.org/21485/diff/3001/3008#newcode856
Line 856: allocated_dibs_[shmem->id()] = dup(result->a.fd);
On 2009/02/19 01:13:52, darin wrote:
> ugh, having to mess with allocated_dibs_ from the IO thread may be nasty.  you
> probably need to move allocated_dibs_ to the RenderWidgetHelper, which can be
> accessed from either thread.

Done.

http://codereview.chromium.org/21485/diff/3001/3009
File chrome/browser/renderer_host/browser_render_process_host.h (right):

http://codereview.chromium.org/21485/diff/3001/3009#newcode66
Line 66: virtual TransportDIB* GetTransportDIB(TransportDIB::Id dibid);
On 2009/02/19 01:13:52, darin wrote:
> dib_id

Done.

http://codereview.chromium.org/21485/diff/3001/3009#newcode154
Line 154: TransportDIB* MapTransportDIB(TransportDIB::Id dibid);
On 2009/02/19 01:13:52, darin wrote:
> dib_id

Done.

http://codereview.chromium.org/21485/diff/3001/3009#newcode159
Line 159: std::Map<TransportDIB::Id, int> allocated_dibs_;
On 2009/02/19 01:13:52, darin wrote:
> did you mean std::map?
> 
> typedef for this map?

Done.

http://codereview.chromium.org/21485/diff/3001/3009#newcode162
Line 162: void OnDetachTransportDIB(TransportDIB::Id dibid);
On 2009/02/19 01:13:52, darin wrote:
> dib_id

Done.

http://codereview.chromium.org/21485/diff/3060/85
File chrome/browser/renderer_host/render_process_host.h (right):

http://codereview.chromium.org/21485/diff/3060/85#newcode135
Line 135: virtual TransportDIB* GetTransportDIB(TransportDIB::Id dibid) = 0;
On 2009/02/19 01:13:53, darin wrote:
> dib_id

Done.

http://codereview.chromium.org/21485/diff/3060/86
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/21485/diff/3060/86#newcode432
Line 432: LOG(WARNING) << "Transport DIB too small for given rectangle";
On 2009/02/19 01:13:53, darin wrote:
> this seems like it corresponds to a corrupt renderer, right?  we should
probably
> kill the renderer in this case.
> 
> at the very least, it is a bug that you don't ACK this PaintRect message.
> 
> instead of returning early, you may be better off just skipping the
> PaintBackingStoreRect call.  i worry about the rest of this function not
> running, which could leave the browser in a bad state.

Done.

http://codereview.chromium.org/21485/diff/3060/86#newcode491
Line 491: LOG(WARNING) << "Transport DIB too small for given rectangle";
On 2009/02/19 01:13:53, darin wrote:
> ditto

Done.

http://codereview.chromium.org/21485/diff/3060/86#newcode583
Line 583: const gfx::Rect& bitmap_rect,
On 2009/02/19 01:13:53, darin wrote:
> dib and bitmap_rect should be consistently named.  not going to repeat this
nit.
>  please fix all occurrences.

Done.

http://codereview.chromium.org/21485/diff/3060/87
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/21485/diff/3060/87#newcode272
Line 272: void ScrollBackingStoreRect(TransportDIB* dib,
On 2009/02/19 01:13:53, darin wrote:
> dib and bitmap_rect should be named consistently

Done.

http://codereview.chromium.org/21485/diff/3060/92
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/21485/diff/3060/92#newcode1074
Line 1074: A a;
On 2009/02/19 01:13:53, darin wrote:
> instead of .a, how about .data or .value?
> 
> also it is a little odd to define this in the mix of a bunch of ParamTraits
> defines.  how about moving it up to an earlier location in the file?

Move into its own header file (needed to compile on the Mac without pulling
ipc_message_utils.h in at places where it shouldn't be).

Changed to .value.

http://codereview.chromium.org/21485/diff/3060/95
File chrome/common/transport_dib.h (right):

http://codereview.chromium.org/21485/diff/3060/95#newcode31
Line 31: typedef void* HANDLE;
On 2009/02/19 01:13:53, darin wrote:
> seems strange to re-define HANDLE on TransportDIB.  how about just including
> windows.h and using HANDLE from there?  or put the typedef for HANDLE at
global
> scope so it matches what windows.h would import.

Done.

http://codereview.chromium.org/21485/diff/3060/95#newcode41
Line 41: typedef std::pair<HANDLE, uint64> Id;
On 2009/02/19 01:13:53, darin wrote:
> can we please have a struct here with named fields that are descriptive? 
> .handle and .sequence_number perhaps?

Done.

http://codereview.chromium.org/21485/diff/3060/95#newcode48
Line 48: typedef int Id;
On 2009/02/19 01:13:53, darin wrote:
> would be good to document what these "int" values correspond to.

Done.

http://codereview.chromium.org/21485/diff/3060/95#newcode56
Line 56: //
---------------------------------------------------------------------------
On 2009/02/19 01:13:53, darin wrote:
> *wince* excessive dividers :)

Oh, ok. Removed :)

http://codereview.chromium.org/21485/diff/3060/95#newcode57
Line 57: static TransportDIB* Create(size_t size, uint64 epoch);
On 2009/02/19 01:13:53, darin wrote:
> maybe you should just use Time or TimeTicks here?  i'm not sure that Time::Now
> is synchronized well across processes.  is this going to be a good sequence
> number source?
> 
> how about if we just use IPC routing_id + counter for the sequence number?  in
> other words, all we really need is for the renderer to maintain a simple
counter
> because the browser can always inspect the routing_id of the IPC message to
see
> what renderer is sending it the message.

Sorry, I was thinking epoch in the distributed systems sense, not the time
sense. It's a bad name, have changed it.

http://codereview.chromium.org/21485/diff/3060/95#newcode92
Line 92: base::SharedMemory shm_;
On 2009/02/19 01:13:53, darin wrote:
> shared_memory_ :)

Done.

http://codereview.chromium.org/21485/diff/3060/95#newcode93
Line 93: uint64 epoch_;  // sequence number
On 2009/02/19 01:13:53, darin wrote:
> how about naming the variable sequence_number_ so this comment is unnecessary?

Done.

http://codereview.chromium.org/21485/diff/3060/95#newcode95
Line 95: int shmkey_;  // SysV SHM id
On 2009/02/19 01:13:53, darin wrote:
> shared_memory_key_ or just key_

Done.

http://codereview.chromium.org/21485/diff/3060/96
File chrome/common/transport_dib_linux.cc (right):

http://codereview.chromium.org/21485/diff/3060/96#newcode12
Line 12: 
On 2009/02/19 01:13:53, darin wrote:
> maybe this file should be named transport_dib_x11.cc since it is just specific
> to using Xwindows.

Follow up patches are really going to tie us down to X, all guarded by
defined(OS_LINUX), so we're really accepting that Linux == X for now.

http://codereview.chromium.org/21485/diff/3060/96#newcode36
Line 36: LOG(ERROR) << "Failed to create SysV shared memory region"
On 2009/02/19 01:13:53, darin wrote:
> DLOG or do you really want this compiled into release?

Done.

http://codereview.chromium.org/21485/diff/3060/98
File chrome/common/transport_dib_win.cc (right):

http://codereview.chromium.org/21485/diff/3060/98#newcode19
Line 19: : shm_(handle, false /* read only? */) {
On 2009/02/19 01:13:53, darin wrote:
> why the question about read only?

Removed question mark.

http://codereview.chromium.org/21485/diff/3060/98#newcode46
Line 46: LOG(ERROR) << "Failed to map transport DIB"
On 2009/02/19 01:13:53, darin wrote:
> DLOG is fine

Done.

http://codereview.chromium.org/21485/diff/3060/100
File chrome/renderer/render_process.h (right):

http://codereview.chromium.org/21485/diff/3060/100#newcode13
Line 13: class Rect;
On 2009/02/19 01:13:53, darin wrote:
> contents of namespace should not be indented per google style.  not your
> mistake, but can you fix it?

Done.

http://codereview.chromium.org/21485/diff/3060/100#newcode13
Line 13: class Rect;
On 2009/02/19 01:13:53, darin wrote:
> contents of namespace should not be indented per google style.  not your
> mistake, but can you fix it?

Done.

http://codereview.chromium.org/21485/diff/3060/100#newcode73
Line 73: size_t FindFreeCacheSlot(size_t size);
On 2009/02/19 01:13:53, darin wrote:
> can you just use int for the index?  and say that -1 is the invalid index? 
less
> verbose.

Done.

http://codereview.chromium.org/21485/diff/3060/101
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/21485/diff/3060/101#newcode406
Line 406: RenderProcess::GetDrawingCanvas(&current_paint_buf_, damaged_rect);
On 2009/02/19 01:13:53, darin wrote:
> common style is to indent by 4 for line continuations (as you can see in the
> code you replaced).

Done.

http://codereview.chromium.org/21485/diff/3060/101#newcode483
Line 483: RenderProcess::GetDrawingCanvas(&current_scroll_buf_, damaged_rect);
On 2009/02/19 01:13:53, darin wrote:
> ditto

Done.

http://codereview.chromium.org/21485/diff/3060/103
File chrome/renderer/renderer_glue.cc (right):

http://codereview.chromium.org/21485/diff/3060/103#newcode99
Line 99: shared_buf_->Create(L"", false /* read only? */, true /* existing? */,
On 2009/02/19 01:13:53, darin wrote:
> oh... i think i get the notation with the question mark now.  you are saying
> that false answers the question of whether or not we want "read only"...
sorry,
> that tripped me up.  i thought it was an annotation of uncertainty about
whether
> false was the right value or not :-/  maybe others would be similarly
confused?

Have changed them all.

http://codereview.chromium.org/21485/diff/3060/106
File skia/ext/platform_canvas_linux.h (right):

http://codereview.chromium.org/21485/diff/3060/106#newcode33
Line 33: bool initialize(int width, int height, bool is_opaque, uint8_t* data);
On 2009/02/19 01:13:53, darin wrote:
> it is against google style to have overloaded methods.  but this is Skia code,
> so i don't know.  maybe you should ask brett.

The Linux Skia code needs a cleanup pretty badly. For the moment, this matches
what the other platforms are doing.

[darin (slow to review), 2009-02-19 17:41:43]

LGreatTM!

If you could do the follow-up change to use DelayTimer in more places, that'd be
great too.

http://codereview.chromium.org/21485/diff/3118/122
File base/timer.h (right):

http://codereview.chromium.org/21485/diff/3118/122#newcode202
Line 202: // A Delay timer is like The Button from Lost. Once started, you have
to keep
nice!

http://codereview.chromium.org/21485/diff/3118/122#newcode214
Line 214: DelayTimer(Receiver* receiver, ReceiverMethod method, TimeDelta delay)
nit: please list the delay parameter first to match BaseTimer::Start

i presume that the typical usage of this class is to heap allocate it.  maybe an
example in the comments would be good.

http://codereview.chromium.org/21485/diff/3118/122#newcode230
Line 230: if (timer_.IsRunning() &&
nit: no need for line wrapping here?

http://codereview.chromium.org/21485/diff/3118/144
File chrome/common/maybe.h (right):

http://codereview.chromium.org/21485/diff/3118/144#newcode10
Line 10: struct Maybe {
this used to be in the IPC namespace.  perhaps it should stay there?

the comments talk a great deal about IPC.  IPC::Maybe seems OK to me.

http://codereview.chromium.org/21485/diff/3118/147
File chrome/common/transport_dib.h (right):

http://codereview.chromium.org/21485/diff/3118/147#newcode55
Line 55: uint64 sequence_num;
a uint32 is probably fine for this, right?  roll over is probably not a problem.

[agl, 2009-02-19 20:16:05]

http://codereview.chromium.org/21485/diff/3118/122
File base/timer.h (right):

http://codereview.chromium.org/21485/diff/3118/122#newcode214
Line 214: DelayTimer(Receiver* receiver, ReceiverMethod method, TimeDelta delay)
On 2009/02/19 17:41:44, darin wrote:
> nit: please list the delay parameter first to match BaseTimer::Start
> 
> i presume that the typical usage of this class is to heap allocate it.  maybe
an
> example in the comments would be good.

Done.

http://codereview.chromium.org/21485/diff/3118/122#newcode230
Line 230: if (timer_.IsRunning() &&
On 2009/02/19 17:41:44, darin wrote:
> nit: no need for line wrapping here?

Done.

http://codereview.chromium.org/21485/diff/3118/144
File chrome/common/maybe.h (right):

http://codereview.chromium.org/21485/diff/3118/144#newcode10
Line 10: struct Maybe {
On 2009/02/19 17:41:44, darin wrote:
> this used to be in the IPC namespace.  perhaps it should stay there?
> 
> the comments talk a great deal about IPC.  IPC::Maybe seems OK to me.

Done.

http://codereview.chromium.org/21485/diff/3118/147
File chrome/common/transport_dib.h (right):

http://codereview.chromium.org/21485/diff/3118/147#newcode55
Line 55: uint64 sequence_num;
On 2009/02/19 17:41:44, darin wrote:
> a uint32 is probably fine for this, right?  roll over is probably not a
problem.

Done.

[darin (slow to review), 2009-02-19 20:18:21]

http://codereview.chromium.org/21485/diff/3119/174
File base/timer.h (right):

http://codereview.chromium.org/21485/diff/3119/174#newcode252
Line 252: Receiver *const receiver_;
extraneous constness that is not consistent with the other classes in this file.
 when in rome?

http://codereview.chromium.org/21485/diff/3119/197
File chrome/common/maybe.h (right):

http://codereview.chromium.org/21485/diff/3119/197#newcode12
Line 12: struct Maybe {
so this file should be renamed too.  ipc_maybe.h

[pink (ping after 24hrs), 2009-02-19 20:27:59]

http://codereview.chromium.org/21485/diff/3119/213
File skia/ext/platform_canvas_mac.cc (right):

http://codereview.chromium.org/21485/diff/3119/213#newcode56
Line 56: CGColorSpaceRef colourSpace;
we should probably be consistent with the spelling of "color" since the type has
it spelled one way on the same line.

http://codereview.chromium.org/21485/diff/3119/213#newcode61
Line 61: kCGImageAlphaPremultipliedFirst);
this should probably be kCGImageAlphaPremultipliedFirst |
kCGBitmapByteOrder32Host, as recommended by apple and used in the
bitmap_platform_device's create call.

http://codereview.chromium.org/21485/diff/3119/213#newcode66
Line 66: SkDevice* device = createPlatformDevice(width, height, is_opaque,
context);
who owns context after this call?

[agl, 2009-02-19 21:04:18]

Thanks for the Mac review, I'm going to try and get a Mac to debug on before
landing.

http://codereview.chromium.org/21485/diff/3119/213
File skia/ext/platform_canvas_mac.cc (right):

http://codereview.chromium.org/21485/diff/3119/213#newcode56
Line 56: CGColorSpaceRef colourSpace;
On 2009/02/19 20:27:59, pink wrote:
> we should probably be consistent with the spelling of "color" since the type
has
> it spelled one way on the same line.

Done.

http://codereview.chromium.org/21485/diff/3119/213#newcode61
Line 61: kCGImageAlphaPremultipliedFirst);
On 2009/02/19 20:27:59, pink wrote:
> this should probably be kCGImageAlphaPremultipliedFirst |
> kCGBitmapByteOrder32Host, as recommended by apple and used in the
> bitmap_platform_device's create call.
> 
> 

Done.

http://codereview.chromium.org/21485/diff/3119/213#newcode66
Line 66: SkDevice* device = createPlatformDevice(width, height, is_opaque,
context);
On 2009/02/19 20:27:59, pink wrote:
> who owns context after this call?

It ends up in a BitmapPlatformDeviceMacData, which is owned by the canvas and
it's deleted with the canvas.

[cpu_(ooo_6.6-7.5), 2009-02-20 00:28:07]

http://codereview.chromium.org/21485/diff/3123/230
File chrome/browser/renderer_host/backing_store_win.cc (right):

http://codereview.chromium.org/21485/diff/3123/230#newcode34
Line 34: TransportDIB* bitmap,
When merging my change and your change, we should try to map just the size as
computed in line 44, which is unused in this CL, I think is possible, we just
need to be lazy about the moment we map.
Also the mapping on the browser size is read-only and it would be very nice to
keep it this way.