Dispatch IndexedDB IPC messages to worker threads

Async IDB host messages now contain a thread_id corresponding to the requesting thread.  The id is 0 for the main thread and >0 for worker threads.  The browser process includes the thread_id in its reply, allowing the IO thread in the renderer to look up the right worker thread.

BUG=64054
TEST=


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

[dgrogan, 2011-11-30 03:11:56]

This is the mega patch.  I'm going to see how much I can break off into
bite-sized reviews.

[hans, 2011-11-30 15:05:40]

Looks fine as far as I can tell, but Michael should review as well.

Just some very minor comments below.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
File content/renderer/indexed_db_dispatcher.h (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
content/renderer/indexed_db_dispatcher.h:195: // * Ensures, through IDMap, that
it is only called on a single thread.
mabye point out in the comments that this map is intended to map from ids to
callbacks? or maybe rename the T typename to CallbacksType or something?

I was just confused below because it looked like a generic map, but the
parameters were called "callbacks".
Or maybe just rename those parameters to "value" or something?

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
content/renderer/indexed_db_dispatcher.h:197: class ThreadSafeIDMap
ultra nit: curly brace on same line

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
content/renderer/indexed_db_dispatcher.h:201: filter_ = filter;
nit: maybe use constructor initializer list?

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
File content/renderer/indexed_db_message_filter.h (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:47: const WebKit::WebWorkerRunLoop&
loop) OVERRIDE;
I was confused by the "Did" in these names... but I suppose the point is that
they get called by something when "loop" has been started or stopped? Maybe add
a comment?

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:104: typedef std::map<int,
WebKit::WebWorkerRunLoop*> IDToRunLoop;
maybe IDToRunLoopMap?

[dgrogan, 2011-11-30 21:34:35]

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
File content/renderer/indexed_db_dispatcher.h (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
content/renderer/indexed_db_dispatcher.h:195: // * Ensures, through IDMap, that
it is only called on a single thread.
On 2011/11/30 15:05:40, hans wrote:
> mabye point out in the comments that this map is intended to map from ids to
> callbacks? or maybe rename the T typename to CallbacksType or something?
> 
> I was just confused below because it looked like a generic map, but the
> parameters were called "callbacks".
> Or maybe just rename those parameters to "value" or something?

You're right.  Updated the comments and name to reflect that this is only
intended to be used with callbacks.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
content/renderer/indexed_db_dispatcher.h:197: class ThreadSafeIDMap
On 2011/11/30 15:05:40, hans wrote:
> ultra nit: curly brace on same line

Done.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
content/renderer/indexed_db_dispatcher.h:201: filter_ = filter;
On 2011/11/30 15:05:40, hans wrote:
> nit: maybe use constructor initializer list?

Done.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
File content/renderer/indexed_db_message_filter.h (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:104: typedef std::map<int,
WebKit::WebWorkerRunLoop*> IDToRunLoop;
On 2011/11/30 15:05:40, hans wrote:
> maybe IDToRunLoopMap?

Done.

[michaeln, 2011-11-30 21:48:14]

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
File content/renderer/indexed_db_dispatcher.h (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
content/renderer/indexed_db_dispatcher.h:195: // * Ensures, through IDMap, that
it is only called on a single thread.
"ThreadSafe" usually means safe to call from multiple threads, but that's not
your intent here.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
File content/renderer/indexed_db_message_filter.cc (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.cc:43:
tls_worker_run_loop_.Set(NULL);
Looks like there can tbe other instances of WebWorkerRunLoop stashed in the
various id_maps that are not safe to use upon return from this method. What
prevents their use?

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.cc:125: ForwardToMainThread(msg);
Do we need a stronger test for whether or not the msg should be delivered to a
worker? what if the worker exited prior to a callback, it wouldn't be found in
the map, and it shouldn't be delivered to the main thread.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.cc:129: iter->second->postTask(task);
Should this be under the run_loops_lock_? What guarantee is there that the loop
isn't exiting?

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.cc:218: dispatchers_[current_loop] =
new IndexedDBDispatcher(this);
where is this map cleaned up?

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
File content/renderer/indexed_db_message_filter.h (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:57:
WebKit::WebIDBDatabaseCallbacks* callbacks);
Feels tedious to maintain maps per callback/object type. Did you consider
assigning a workerId for each unique worker, and having a single map<id,
webworkerloop> and adding that id to the IPC messages. There's be need for per
call map maintance, and message specific map lookup.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:97: LoopSet run_loops_;
I'd vote to factor this collection and the TLS value out of this class and into
something that's not IDB specific. I think that could help simplify this class
and also make it easier for another consumer to use this WebWorkerRunLoop class.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:99: virtual bool Contains(const
WebKit::WebWorkerRunLoop& run_loop);
why virtual?

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:108: void RemoveFromMap(int32 id,
IDToRunLoop*);
please put typedefs, methods, members in order

http://codereview.chromium.org/8747002/diff/1/content/renderer/render_thread_...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/render_thread_...
content/renderer/render_thread_impl.cc:46: #include
"content/renderer/indexed_db_message_filter.h"
is this needed?

http://codereview.chromium.org/8747002/diff/1/content/renderer/renderer_webki...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/renderer_webki...
content/renderer/renderer_webkitplatformsupport_impl.h:131: IDMap<void>
webkit_to_chrome_thread_id_;
i don't see where this is used?

[michaeln, 2011-11-30 22:40:55]

http://codereview.chromium.org/8747002/diff/15001/content/renderer/indexed_db...
File content/renderer/indexed_db_message_filter.cc (right):

http://codereview.chromium.org/8747002/diff/15001/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:71: bool
IndexedDBMessageFilter::OnMessageReceived(const IPC::Message& msg) {
If the workerId could be determined w/o deserializing the entire message that
would be handy here. Instead of having an explicit hander per message, this one
bottleneck could extract the id from the msg and forward it accordingly.

http://codereview.chromium.org/8747002/diff/15001/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:101:
base::Bind(&IndexedDBDispatcher::OnMessageReceived,
ah... here's a place where refcounting comes into play.

Would it work to have a seperate DispatchMessage which gets the 'current'
instance from tls and call the OnMessage instance method? Assuming we use
base::Bind closures for worker threads too, the same closure could work in both
cases.

http://codereview.chromium.org/8747002/diff/15001/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:113: IndexedDBDispatcher*
dispatcher = filter_->thread_specific_idb_dispatcher();
Given how IndexedDBDispatcher is used, maybe it doesn't need to be threadsafe
refcounted. I think you only need to get a ptr to the disp for the current
thread and only use it on that thread. Isn't it completely single-threaded?

[dgrogan, 2011-11-30 23:25:18]

New patch with just a few of the trivial fixes.

I'll start separating WorkerTaskRunner from IDBMessageFilter, and add an
observer list to RendererWebKitPlatformSupport that notifies when workers are
started and stopped.

Not yet sure if I'm going to get rid of the maps that associate callback ids to
runloops in favor of ferrying the runloop id over IPC.  If we _could_ extract
the worker id from an IPC message without deserializing the whole thing then I
think I'd be sold.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
File content/renderer/indexed_db_dispatcher.h (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_dis...
content/renderer/indexed_db_dispatcher.h:195: // * Ensures, through IDMap, that
it is only called on a single thread.
On 2011/11/30 21:48:14, michaeln wrote:
> "ThreadSafe" usually means safe to call from multiple threads, but that's not
> your intent here.

You're right.  I renamed it CallbacksIDMap in a later patch.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
File content/renderer/indexed_db_message_filter.cc (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.cc:125: ForwardToMainThread(msg);
On 2011/11/30 21:48:14, michaeln wrote:
> Do we need a stronger test for whether or not the msg should be delivered to a
> worker? what if the worker exited prior to a callback, it wouldn't be found in
> the map, and it shouldn't be delivered to the main thread.

As it stands now these maps aren't cleaned up, so in the case you describe the
message would be forwarded to a dead runloop.

But assuming that these maps are cleaned up when a worker shuts down, you're
right, this would be bad.  Tracking main-thread object ids in a separate data
structure is one option.  Another would be to store the id of the run loop, and
push the task over to WorkerTaskRunner.  The main thread could have its own
magic constant.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.cc:129: iter->second->postTask(task);
On 2011/11/30 21:48:14, michaeln wrote:
> Should this be under the run_loops_lock_?

map_lock_, but yes.  Fixed.

> What guarantee is there that the loop isn't exiting?

I don't think there is one.  If the runloop is exiting I don't think it matters
if this message goes undelivered.  But after looking at WorkerRunLoop and
WTF::MessageQueue, I'm not sure the tasks's memory gets reclaimed.  I should
probably deal with that in webkit.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.cc:218: dispatchers_[current_loop] =
new IndexedDBDispatcher(this);
On 2011/11/30 21:48:14, michaeln wrote:
> where is this map cleaned up?

It appears that it isn't.  I had intended to clean it up in
DidStopWorkerRunLoop, which will be complicated by the move of those methods to
WorkerTaskRunner.  Maybe WebKitPlatformSupport will notify an observer list when
worker runloops are started and stopped.

It looks like I can get rid of dispatchers_ and dispatchers_lock_ in favor of a
TLS holding the current IDBDispatcher.  This class would still have to listen
for RunLoops stopping to deallocate a stopping-worker's IndexedDBDispatcher
object.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
File content/renderer/indexed_db_message_filter.h (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:47: const WebKit::WebWorkerRunLoop&
loop) OVERRIDE;
On 2011/11/30 15:05:40, hans wrote:
> I was confused by the "Did" in these names... but I suppose the point is that
> they get called by something when "loop" has been started or stopped? Maybe
add
> a comment?

Comment added.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:57:
WebKit::WebIDBDatabaseCallbacks* callbacks);
On 2011/11/30 21:48:14, michaeln wrote:
> Feels tedious to maintain maps per callback/object type.

Agreed.

> Did you consider
> assigning a workerId for each unique worker, and having a single map<id,
> webworkerloop> and adding that id to the IPC messages. There's be need for per
> call map maintance, and message specific map lookup.

Yeah, that's what I wanted to do initially.  The issue was that the ids for
WebIDBTransactionCallbacks and WebIDBDatabaseCallbacks are generated by the
browser process.  I didn't want to mess with the browser side too much, though
it's arguable that transferring control over assigning ids to the renderer
process would be less invasive than these maps.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:97: LoopSet run_loops_;
On 2011/11/30 21:48:14, michaeln wrote:
> I'd vote to factor this collection and the TLS value out of this class and
into
> something that's not IDB specific. I think that could help simplify this class
> and also make it easier for another consumer to use this WebWorkerRunLoop
class.

The idea behind the WebCoreWorkerTracker interface was to allow future reuse
without burdening this change too much.  I suppose the burden would be that this
class would still have to be notified when worker runloops stop so that it could
manage the IndexedDBDispatcher objects' memory.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:99: virtual bool Contains(const
WebKit::WebWorkerRunLoop& run_loop);
On 2011/11/30 21:48:14, michaeln wrote:
> why virtual?

Mistake.  Removed.

http://codereview.chromium.org/8747002/diff/1/content/renderer/indexed_db_mes...
content/renderer/indexed_db_message_filter.h:108: void RemoveFromMap(int32 id,
IDToRunLoop*);
On 2011/11/30 21:48:14, michaeln wrote:
> please put typedefs, methods, members in order

will do

http://codereview.chromium.org/8747002/diff/1/content/renderer/render_thread_...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/render_thread_...
content/renderer/render_thread_impl.cc:46: #include
"content/renderer/indexed_db_message_filter.h"
On 2011/11/30 21:48:14, michaeln wrote:
> is this needed?

Nope, deleted.

http://codereview.chromium.org/8747002/diff/1/content/renderer/renderer_webki...
File content/renderer/renderer_webkitplatformsupport_impl.h (right):

http://codereview.chromium.org/8747002/diff/1/content/renderer/renderer_webki...
content/renderer/renderer_webkitplatformsupport_impl.h:131: IDMap<void>
webkit_to_chrome_thread_id_;
On 2011/11/30 21:48:14, michaeln wrote:
> i don't see where this is used?

Oops, remnant of a previous strategy.  Deleted.

http://codereview.chromium.org/8747002/diff/15001/content/renderer/indexed_db...
File content/renderer/indexed_db_message_filter.cc (right):

http://codereview.chromium.org/8747002/diff/15001/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:71: bool
IndexedDBMessageFilter::OnMessageReceived(const IPC::Message& msg) {
On 2011/11/30 22:40:55, michaeln wrote:
> If the workerId could be determined w/o deserializing the entire message that
> would be handy here. Instead of having an explicit hander per message, this
one
> bottleneck could extract the id from the msg and forward it accordingly.

Agreed.  I looked around for an obvious way to do this but didn't find one.  As
it stands now, it's annoying to have to update this message map whenever
messages change.  Which Hans has been doing with the cursor stuff, and which
we're about to do with IDB.next.

http://codereview.chromium.org/8747002/diff/15001/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:101:
base::Bind(&IndexedDBDispatcher::OnMessageReceived,
On 2011/11/30 22:40:55, michaeln wrote:
> ah... here's a place where refcounting comes into play.
> 
> Would it work to have a seperate DispatchMessage which gets the 'current'
> instance from tls and call the OnMessage instance method?

You mean something like this?  I think it would work.

void IndexedDBMessageFilter::ForwardToMainThread(const IPC::Message& msg) {
main_thread_loop_proxy_->PostTask(FROM_HERE,
base::Bind(&IndexedDBMessageFilter::DispatchMessage, this, msg))
}

void IndexedDBMessageFilter::DispatchMessage(const IPC::Message& msg) {
  idb_dispatcher_tls_.Get()->OnMessageReceived(msg);
}


> Assuming we use base::Bind closures for worker threads too, the same closure
could
> work in both cases.

http://codereview.chromium.org/8747002/diff/15001/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:113: IndexedDBDispatcher*
dispatcher = filter_->thread_specific_idb_dispatcher();
On 2011/11/30 22:40:55, michaeln wrote:
> Given how IndexedDBDispatcher is used, maybe it doesn't need to be threadsafe
> refcounted. I think you only need to get a ptr to the disp for the current
> thread and only use it on that thread. Isn't it completely single-threaded?

I think the line-101 comment came after this one.  It is intended to be used on
a single thread but needed to be threadsafe refcounted for the above PostTask,
which I think you figured out.

[michaeln, 2011-11-30 23:51:35]

> Not yet sure if I'm going to get rid of the maps that associate callback ids
to
> runloops in favor of ferrying the runloop id over IPC.  If we _could_ extract
> the worker id from an IPC message without deserializing the whole thing then I
> think I'd be sold.

See class IPC::MessageInterator. Provided you know the structure of a message,
looks like you can iterate over the fields up to an arbitrary point and stop. If
workerId were in a fixed location in all IDBMessages (say the first field)...

   int workerId = MessageIterator(msg).NextInt()

> I'll start separating WorkerTaskRunner from IDBMessageFilter, and add an
> observer list to RendererWebKitPlatformSupport that notifies when workers are
> started and stopped.

If you used TLS for the dispatcher instances, you may not need stopped
notifications in IDB'land. When the thread really does unwind, the TLS
infrastructure will delete it.

[dgrogan, 2011-11-30 23:56:33]

On 2011/11/30 23:51:35, michaeln wrote:
> > I'll start separating WorkerTaskRunner from IDBMessageFilter, and add an
> > observer list to RendererWebKitPlatformSupport that notifies when workers
are
> > started and stopped.
> 
> If you used TLS for the dispatcher instances, you may not need stopped
> notifications in IDB'land. When the thread really does unwind, the TLS
> infrastructure will delete it.

I read this paragraph as saying the opposite of that, that the TLS
infrastructure doesn't delete anything.  Maybe I'm misunderstanding?
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/threading/thread...

[michaeln, 2011-12-01 00:01:59]

>> notifies when workers are started and stopped
>
> If you used TLS for the dispatcher instances, you may not need stopped
> notifications in IDB'land. When the thread really does unwind, the TLS
> infrastructure will delete it.

Ooops... just looked at our thread_local.h... it does not provide cleanup on
exit'abilities. So looks like you will need notifications to maintain these data
structures.

[michaeln, 2011-12-01 21:42:05]

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
File content/common/worker_task_runner.h (right):

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
content/common/worker_task_runner.h:20: class WorkerTaskRunner : public
base::RefCountedThreadSafe<WorkerTaskRunner> {
why refcounted?

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
content/common/worker_task_runner.h:63: void UnRegisterCurrentWorkerLoop(const
WebWorkerRunLoop& loop) {
there's probably enough code in here to warrant a .cc file

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
content/common/worker_task_runner.h:80: ObserverList<Observer> observer_list_;
I think ObserverList is not thread-safe.

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.cc (left):

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:119: if (!web_frame)
does this no longer apply?

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
File content/renderer/indexed_db_message_filter.cc (right):

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:49: DCHECK(thread_id <= 0xFF);
(stashing this value in the high nibble :)

it looks like the classes in indexed_db_callbacks.h and
indexed_db_database_callbacks.h could work as a place on the server side to
stash the full integer value along side the 'response_id' value

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:56: int id = ++unique_id_;
take a look at AtomicSequenceNumber

[dgrogan, 2011-12-01 22:49:00]

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
File content/common/worker_task_runner.h (right):

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
content/common/worker_task_runner.h:20: class WorkerTaskRunner : public
base::RefCountedThreadSafe<WorkerTaskRunner> {
On 2011/12/01 21:42:05, michaeln wrote:
> why refcounted?
 
I was imagining that ChildThread would be handing this out, possibly to threads
that would outlive the main thread.  In my case it's just the IO thread.  Do you
know if there is a well-defined shutdown order between the main and IO threads?

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
content/common/worker_task_runner.h:63: void UnRegisterCurrentWorkerLoop(const
WebWorkerRunLoop& loop) {
On 2011/12/01 21:42:05, michaeln wrote:
> there's probably enough code in here to warrant a .cc file

Agreed.

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
content/common/worker_task_runner.h:80: ObserverList<Observer> observer_list_;
On 2011/12/01 21:42:05, michaeln wrote:
> I think ObserverList is not thread-safe.

Argh, quite right.  ObserverListThreadSafe doesn't look like it's what I want
either.  I suppose I'll add a lock around this observer_list_ and forbid clients
from removing themselves during iteration to prevent lock reacquisition.

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.cc (left):

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:119: if (!web_frame)
On 2011/12/01 21:42:05, michaeln wrote:
> does this no longer apply?

This was to make the webkit side easier, so I didn't have to put in a frame for
the workers case.  I haven't convinced myself that it doesn't apply, I'll
probably have to figure something out.

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
File content/renderer/indexed_db_message_filter.cc (right):

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:49: DCHECK(thread_id <= 0xFF);
On 2011/12/01 21:42:05, michaeln wrote:
> (stashing this value in the high nibble :)

High byte, but yeah.

> it looks like the classes in indexed_db_callbacks.h and
> indexed_db_database_callbacks.h could work as a place on the server side to
> stash the full integer value along side the 'response_id' value

I just didn't want to add the extra field to all the messages, handlers, and to
various classes on the browser side.  This is hacky, but it's probably good
enough for the matrix guys to test with (why we're rushing to get this into
m17.)  I added a TODO.

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:56: int id = ++unique_id_;
On 2011/12/01 21:42:05, michaeln wrote:
> take a look at AtomicSequenceNumber

Ah nice, thanks for the pointer.

[michaeln, 2011-12-02 00:15:02]

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
File content/common/worker_task_runner.h (right):

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
content/common/worker_task_runner.h:20: class WorkerTaskRunner : public
base::RefCountedThreadSafe<WorkerTaskRunner> {
On 2011/12/01 22:49:00, dgrogan wrote:
> On 2011/12/01 21:42:05, michaeln wrote:
> > why refcounted?
>  
> I was imagining that ChildThread would be handing this out, possibly to
threads
> that would outlive the main thread.  In my case it's just the IO thread.  Do
you
> know if there is a well-defined shutdown order between the main and IO
threads?

Take a look a renderer_main.cc, worker_main.cc, ChildThread, and ChildProcess.

Looks like the the 'main' thread is the primary thread of the process, the
ChildThread class does not really start it, it just gloms onto it and putting a
MessageLoop and ChildProcess on the stack and heap alloc'ing the ChildThread
object. The seperate io thread is started/stopped by the ChildProcess class. The
ChildThread object is killed in the ChildProcess dtor (prior to io_thread_'s
dtor being run).

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
File content/renderer/indexed_db_message_filter.cc (right):

http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:49: DCHECK(thread_id <= 0xFF);
On 2011/12/01 22:49:00, dgrogan wrote:
> On 2011/12/01 21:42:05, michaeln wrote:
> > (stashing this value in the high nibble :)
> 
> High byte, but yeah.
> 
> > it looks like the classes in indexed_db_callbacks.h and
> > indexed_db_database_callbacks.h could work as a place on the server side to
> > stash the full integer value along side the 'response_id' value
> 
> I just didn't want to add the extra field to all the messages, handlers, and
to
> various classes on the browser side.  This is hacky, but it's probably good
> enough for the matrix guys to test with (why we're rushing to get this into
> m17.)  I added a TODO.

Seems like it'd be fairly mechanical and limited to messages.h, dispatcher_host,
and the handful of callback classes mentioned earlier. The additional maps and
high-order bits and stuff are kind of gunky?

[dgrogan, 2011-12-02 00:50:15]

I think this is mostly complete.

I tried to rework how transaction callback ids are assigned but stopped when it
felt like I was pulling a never-ending string.  Regular callback and database
callback ids no longer have to be tracked in maps by IDBMessageFilter though,
their thread ids get sent across the wire and back.

The two remaining issues are whether to refcount WorkerTaskRunner and adding a
separate ipc field for thread ids.  I'm going to work a bit on the webkit side
first though.

[dgrogan, 2011-12-02 11:57:44]

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
File content/common/worker_task_runner.h (right):

http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
content/common/worker_task_runner.h:20: class WorkerTaskRunner : public
base::RefCountedThreadSafe<WorkerTaskRunner> {
On 2011/12/02 00:15:03, michaeln wrote:
> On 2011/12/01 22:49:00, dgrogan wrote:
> > On 2011/12/01 21:42:05, michaeln wrote:
> > > why refcounted?
> Take a look a renderer_main.cc, worker_main.cc, ChildThread, and ChildProcess.
> 
> Looks like the the 'main' thread is the primary thread of the process, the
> ChildThread class does not really start it, it just gloms onto it and putting
a
> MessageLoop and ChildProcess on the stack and heap alloc'ing the ChildThread
> object. The seperate io thread is started/stopped by the ChildProcess class.
The
> ChildThread object is killed in the ChildProcess dtor (prior to io_thread_'s
> dtor being run).

I interpret this to mean that the ref counting is needed, to protect the io
thread's continued access to this class after the child thread is destroyed. 
Are we on the same page?

[dgrogan, 2011-12-02 21:35:56]

On 2011/12/02 00:15:02, michaeln wrote:
>
http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
> File content/common/worker_task_runner.h (right):
> 
>
http://codereview.chromium.org/8747002/diff/12032/content/common/worker_task_...
> content/common/worker_task_runner.h:20: class WorkerTaskRunner : public
> base::RefCountedThreadSafe<WorkerTaskRunner> {
> On 2011/12/01 22:49:00, dgrogan wrote:
> > On 2011/12/01 21:42:05, michaeln wrote:
> > > why refcounted?
> >  
> > I was imagining that ChildThread would be handing this out, possibly to
> threads
> > that would outlive the main thread.  In my case it's just the IO thread.  Do
> you
> > know if there is a well-defined shutdown order between the main and IO
> threads?
> 
> Take a look a renderer_main.cc, worker_main.cc, ChildThread, and ChildProcess.
> 
> Looks like the the 'main' thread is the primary thread of the process, the
> ChildThread class does not really start it, it just gloms onto it and putting
a
> MessageLoop and ChildProcess on the stack and heap alloc'ing the ChildThread
> object. The seperate io thread is started/stopped by the ChildProcess class.
The
> ChildThread object is killed in the ChildProcess dtor (prior to io_thread_'s
> dtor being run).
> 
>
http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
> File content/renderer/indexed_db_message_filter.cc (right):
> 
>
http://codereview.chromium.org/8747002/diff/12032/content/renderer/indexed_db...
> content/renderer/indexed_db_message_filter.cc:49: DCHECK(thread_id <= 0xFF);
> On 2011/12/01 22:49:00, dgrogan wrote:
> > On 2011/12/01 21:42:05, michaeln wrote:
> > > (stashing this value in the high nibble :)
> > 
> > High byte, but yeah.
> > 
> > > it looks like the classes in indexed_db_callbacks.h and
> > > indexed_db_database_callbacks.h could work as a place on the server side
to
> > > stash the full integer value along side the 'response_id' value
> > 
> > I just didn't want to add the extra field to all the messages, handlers, and
> to
> > various classes on the browser side.  This is hacky, but it's probably good
> > enough for the matrix guys to test with (why we're rushing to get this into
> > m17.)  I added a TODO.
> 
> Seems like it'd be fairly mechanical and limited to messages.h,
dispatcher_host,
> and the handful of callback classes mentioned earlier. The additional maps and
> high-order bits and stuff are kind of gunky?

You're right on both counts, the mechanical and gunky.  I just don't want to
explode this patch even more, especially as we get ever so closer to the m17
freeze.  Can it wait for another patch?

[dgrogan, 2011-12-02 21:40:21]

John, could you take a look at this, at least as OWNER of content/public?

This patch can't land until webkit.org/b/73609 lands and is rolled.

[jam, 2011-12-02 23:56:14]

Can you break this up into smaller pieces? This will make it much easier to
review, allow more people to review it, and make it easier to land.

Why do you need a WorkerTaskRunner interface in content\common? I don't have a
lot of time right now to look into this in detail unfortunately. But why can't
this be done with MessageLoop etc?

http://codereview.chromium.org/8747002/diff/27023/content/public/common/conte...
File content/public/common/content_switches.h (right):

http://codereview.chromium.org/8747002/diff/27023/content/public/common/conte...
content/public/common/content_switches.h:48: extern const char
kEnableIndexedDatabaseOnWorkers[];
nit: ForWorkers

[dgrogan, 2011-12-03 00:14:19]

On 2011/12/02 23:56:14, John Abd-El-Malek wrote:
> Can you break this up into smaller pieces? This will make it much easier to
> review, allow more people to review it, and make it easier to land.

Sure.  I broke off the content/public stuff into
http://codereview.chromium.org/8789004/.  Could you take a look at that one?

> Why do you need a WorkerTaskRunner interface in content\common? I don't have a
> lot of time right now to look into this in detail unfortunately. But why can't
> this be done with MessageLoop etc?

The worker threads are created in webcore. They have their own webcore runloop
and don't use base::MessageLoops.

>
http://codereview.chromium.org/8747002/diff/27023/content/public/common/conte...
> File content/public/common/content_switches.h (right):
> 
>
http://codereview.chromium.org/8747002/diff/27023/content/public/common/conte...
> content/public/common/content_switches.h:48: extern const char
> kEnableIndexedDatabaseOnWorkers[];
> nit: ForWorkers

Changed in http://codereview.chromium.org/8789004/

[jam, 2011-12-03 00:25:18]

On 2011/12/03 00:14:19, dgrogan wrote:
> On 2011/12/02 23:56:14, John Abd-El-Malek wrote:
> > Can you break this up into smaller pieces? This will make it much easier to
> > review, allow more people to review it, and make it easier to land.
> 
> Sure.  I broke off the content/public stuff into
> http://codereview.chromium.org/8789004/.  Could you take a look at that one?
> 
> > Why do you need a WorkerTaskRunner interface in content\common? I don't have
a
> > lot of time right now to look into this in detail unfortunately. But why
can't
> > this be done with MessageLoop etc?
> 
> The worker threads are created in webcore. They have their own webcore runloop
> and don't use base::MessageLoops.

It seems unfortunate to have two different message loop implementations in
chrome.

I will defer to Darin since he's much more familiar with both the chrome+webkit
sides.
> 
> >
>
http://codereview.chromium.org/8747002/diff/27023/content/public/common/conte...
> > File content/public/common/content_switches.h (right):
> > 
> >
>
http://codereview.chromium.org/8747002/diff/27023/content/public/common/conte...
> > content/public/common/content_switches.h:48: extern const char
> > kEnableIndexedDatabaseOnWorkers[];
> > nit: ForWorkers
> 
> Changed in http://codereview.chromium.org/8789004/

[dgrogan, 2011-12-14 01:46:15]

Hans, Josh and Michael,

This has been revamped and could use another look.

[michaeln, 2011-12-14 02:15:53]

i like the looks of the new CL, the lean and mean MessageFilter class is way
better :)

http://codereview.chromium.org/8747002/diff/47001/content/common/child_thread.cc
File content/common/child_thread.cc (right):

http://codereview.chromium.org/8747002/diff/47001/content/common/child_thread...
content/common/child_thread.cc:66:
channel_->AddFilter(idb_message_filter_.get());
The data member isn't used, would it work to just add the filter and not hang on
to a reference in this class (as is done for ChildTraceMessageFilter).
Alternatively, I'm not sure why the SyncMessageFilter is explicitly removed,
maybe this filter should also be explicitly removed in the dtor?

http://codereview.chromium.org/8747002/diff/47001/content/common/indexed_db_m...
File content/common/indexed_db_message_filter.h (right):

http://codereview.chromium.org/8747002/diff/47001/content/common/indexed_db_m...
content/common/indexed_db_message_filter.h:16: virtual bool
OnMessageReceived(const IPC::Message& msg) OVERRIDE;
much nicer to see this class trimmed down to size

http://codereview.chromium.org/8747002/diff/47001/content/common/indexed_db_m...
File content/common/indexed_db_messages.h (right):

http://codereview.chromium.org/8747002/diff/47001/content/common/indexed_db_m...
content/common/indexed_db_messages.h:25: // The response should have this id.
misplaced comment?

http://codereview.chromium.org/8747002/diff/47001/content/common/indexed_db_m...
content/common/indexed_db_messages.h:143: // Indexed DB messages sent from the
browser to the renderer.
maybe add a comment about the need for thread_id to be the first parameter in
all IDB messages that destined for the renderer

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.cc (right):

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:33:
base::ThreadLocalPointer<IndexedDBDispatcher>*
our convention is to use LazyInstance<> for things like this

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:120: new
IndexedDBHostMsg_CursorUpdate(idb_cursor_id, thread_id, response_id,
maybe call your CurrentWorkerId() helper here as well (like all of the other
usages) instead of having the local thread_id variable

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:473: void
IndexedDBDispatcher::OnSuccessIDBDatabase(int32 thread_id,
maybe DCHECK_EQ(thread_id, CurrentWorkerId()) here and in all other incoming
message handlers

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.h (right):

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.h:43: // Handle the indexed db related
communication for this entire renderer.
comment might be misleading now that there are thread-specific instances of this
class

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.h:48: // This returns the dispatcher
associated with the current WebWorkerRunLoop,
The comment is a little misleading since it will also create and return the
'main' thread's instance. I see how worker instances are deleted (via the
stopped notification) but how is the main thread's instance deleted? Maybe
something (ChildThread?) should own that ptr and explicitly delete it.

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.h:50: static IndexedDBDispatcher*
Instance();
maybe name this as ThreadSpecificInstance() for clarity?

http://codereview.chromium.org/8747002/diff/47001/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/8747002/diff/47001/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:45: #include
"content/renderer/indexed_db_dispatcher.h"
can this include be removed now?

http://codereview.chromium.org/8747002/diff/47001/content/renderer/render_thr...
File content/renderer/render_thread_impl.h (right):

http://codereview.chromium.org/8747002/diff/47001/content/renderer/render_thr...
content/renderer/render_thread_impl.h:33: class IndexedDBDispatcher;
can this forward decl be removed now?

[jsbell, 2011-12-15 00:08:42]

Overall LGTM.

I concur with michaeln@'s comments, especially that additional documentation in
indexed_db_messages.h indicating which messages are renderer->browser and
vice-versa, and indicating that all browser->renderer messages must have
thread_id as the first member.

http://codereview.chromium.org/8747002/diff/46029/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.cc (right):

http://codereview.chromium.org/8747002/diff/46029/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:528: int32 repsonse_id =
p.response_id;
typo: "repsonse_id" (not new, but should fix)

[jam, 2011-12-15 01:20:09]

(didn't review in detail)

indexed_db_message_filter.h and indexed_db_message_filter.cc need to be in the
same directory. i understand why you put them separately because you're getting
deps failure until you move the rest of the indexeddb code to common. instead,
you should put them both in content\renderer, and only add it in
RenderThreadImpl for now. once you move all the dependencies to common, then you
can move those files to common and ChildThread

http://codereview.chromium.org/8747002/diff/46029/content/renderer/indexed_db...
File content/renderer/indexed_db_message_filter.cc (right):

http://codereview.chromium.org/8747002/diff/46029/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:21: 
nit: blank line

[michaeln, 2011-12-15 02:20:47]

http://codereview.chromium.org/8747002/diff/57002/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.cc (right):

http://codereview.chromium.org/8747002/diff/57002/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:65:
g_idb_dispatcher_tls.Pointer()->Set(NULL);
consider putting this line in the dtor so when the main thread's instance is
deleted, the tls ptr also gets reset for that instance (as it is now, the tls
ptr for that guy is left dangling).

and if the reset goes in the dtor, consider putting the Set(tosomething) call in
the ctor for the sake of symmetry

http://codereview.chromium.org/8747002/diff/57002/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.h (right):

http://codereview.chromium.org/8747002/diff/57002/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.h:48: static void Init();
the Init() method declaration can be removed now too

[dgrogan, 2011-12-15 02:47:44]

I think I addressed everything, including putting the IDBDispatcher's tls stuff
in the ctor/dtor.  I had forgotten about that after we talked about it
yesterday.

Another look?

http://codereview.chromium.org/8747002/diff/47001/content/common/child_thread.cc
File content/common/child_thread.cc (right):

http://codereview.chromium.org/8747002/diff/47001/content/common/child_thread...
content/common/child_thread.cc:66:
channel_->AddFilter(idb_message_filter_.get());
On 2011/12/14 02:15:53, michaeln wrote:
> The data member isn't used, would it work to just add the filter and not hang
on
> to a reference in this class (as is done for ChildTraceMessageFilter).

Yes, this works, it causes the ipc channel to be the only owner.  So the
IDBMessageFilter is deleted when the ipc channel is.  Which seems to happen for
the utility process (when this was in child_thread), but the filter isn't
deleted in the renderer process.  I think I disabled sudden termination but I
couldn't get RenderThreadImpl's dtor to run.  I looked to the Audio filters as a
control and they were also not deleted.

> Alternatively, I'm not sure why the SyncMessageFilter is explicitly removed,
> maybe this filter should also be explicitly removed in the dtor?

I'm also not sure why SyncMessageFilter is explicitly removed, but
ChildTraceMessageFilter was explicitly removed until jam said it didn't need to
be, albeit indirectly:

http://codereview.chromium.org/6694004/diff2/6017:17001/content/common/child_...
http://codereview.chromium.org/6694004/diff2/6017:17001/content/common/child_...

http://codereview.chromium.org/8747002/diff/47001/content/common/indexed_db_m...
File content/common/indexed_db_messages.h (right):

http://codereview.chromium.org/8747002/diff/47001/content/common/indexed_db_m...
content/common/indexed_db_messages.h:25: // The response should have this id.
On 2011/12/14 02:15:53, michaeln wrote:
> misplaced comment?

Updated.

http://codereview.chromium.org/8747002/diff/47001/content/common/indexed_db_m...
content/common/indexed_db_messages.h:143: // Indexed DB messages sent from the
browser to the renderer.
On 2011/12/14 02:15:53, michaeln wrote:
> maybe add a comment about the need for thread_id to be the first parameter in
> all IDB messages that destined for the renderer

Ah yes, good idea.

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.cc (right):

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:33:
base::ThreadLocalPointer<IndexedDBDispatcher>*
On 2011/12/14 02:15:53, michaeln wrote:
> our convention is to use LazyInstance<> for things like this

Done.

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:120: new
IndexedDBHostMsg_CursorUpdate(idb_cursor_id, thread_id, response_id,
On 2011/12/14 02:15:53, michaeln wrote:
> maybe call your CurrentWorkerId() helper here as well (like all of the other
> usages) instead of having the local thread_id variable

Done.

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:473: void
IndexedDBDispatcher::OnSuccessIDBDatabase(int32 thread_id,
On 2011/12/14 02:15:53, michaeln wrote:
> maybe DCHECK_EQ(thread_id, CurrentWorkerId()) here and in all other incoming
> message handlers

Done.

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.h (right):

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.h:43: // Handle the indexed db related
communication for this entire renderer.
Updated.

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.h:48: // This returns the dispatcher
associated with the current WebWorkerRunLoop,
Renaming Instance to ThreadSpecificInstance should obviate the need for the
comment.

I made RenderThreadImpl own the main thread's dispatcher.

http://codereview.chromium.org/8747002/diff/47001/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.h:50: static IndexedDBDispatcher*
Instance();
On 2011/12/14 02:15:53, michaeln wrote:
> maybe name this as ThreadSpecificInstance() for clarity?

Done.

http://codereview.chromium.org/8747002/diff/47001/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/8747002/diff/47001/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:45: #include
"content/renderer/indexed_db_dispatcher.h"
On 2011/12/14 02:15:53, michaeln wrote:
> can this include be removed now?

Done.

http://codereview.chromium.org/8747002/diff/47001/content/renderer/render_thr...
File content/renderer/render_thread_impl.h (right):

http://codereview.chromium.org/8747002/diff/47001/content/renderer/render_thr...
content/renderer/render_thread_impl.h:33: class IndexedDBDispatcher;
On 2011/12/14 02:15:53, michaeln wrote:
> can this forward decl be removed now?

Done.

http://codereview.chromium.org/8747002/diff/46029/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.cc (right):

http://codereview.chromium.org/8747002/diff/46029/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.cc:528: int32 repsonse_id =
p.response_id;
On 2011/12/15 00:08:42, jsbell wrote:
> typo: "repsonse_id" (not new, but should fix)

Done.

http://codereview.chromium.org/8747002/diff/46029/content/renderer/indexed_db...
File content/renderer/indexed_db_message_filter.cc (right):

http://codereview.chromium.org/8747002/diff/46029/content/renderer/indexed_db...
content/renderer/indexed_db_message_filter.cc:21: 
On 2011/12/15 01:20:09, John Abd-El-Malek wrote:
> nit: blank line

Done.

http://codereview.chromium.org/8747002/diff/57002/content/renderer/indexed_db...
File content/renderer/indexed_db_dispatcher.h (right):

http://codereview.chromium.org/8747002/diff/57002/content/renderer/indexed_db...
content/renderer/indexed_db_dispatcher.h:48: static void Init();
On 2011/12/15 02:20:48, michaeln wrote:
> the Init() method declaration can be removed now too

Done.

[michaeln, 2011-12-15 20:55:09]

LGTM2

[jam, 2011-12-15 23:41:14]

I didn't really review this in detail, but I had scanned it earlier and the only
thing that stuck out was the file in common which you fixed. I defer to others,
so here's a lgtm for the owners check :) I think it's been mentioned before, but
you guys will benefit from an indexeddb subdirectory in common/renderer/browser
so that you can be OWNERS in them.

[commit-bot: I haz the power, 2011-12-16 00:21:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/8747002/70001

[commit-bot: I haz the power, 2011-12-16 02:04:41]

Change committed as 114747