Adds IDBKeyPath parser / extractor, and provides a mechanism to call it sandboxed.

Adds IDBKeyPath parser / extractor, and provides a mechanism to call it sandboxed.

TEST=idbkeypathextractor_browsertests.cc

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

[bulach, 2010-07-30 23:21:04]

This change depends on 
https://bugs.webkit.org/show_bug.cgi?id=42976 and
https://bugs.webkit.org/show_bug.cgi?id=43276.

The idea is that we'll run the IDBKeyPath parser / extractor inside a sandbox.

This is not yet optimized, we'll probably want to have some "bulk" mechanism of
some kind, i.e., pass KeyPath, a vector of SerializedScriptValues and get back a
vector of KeyValues..

However, this change sets up the basic infrastructure and tests needed, so it
may be worth to go ahead with this and optimize later.

[Paweł Hajdan Jr., 2010-07-30 23:25:39]

The way of running methods on the IO thread seems suspicious to me, and it's
surprising to the reader. How about using PostTask directly, possibly with some
notification?

http://codereview.chromium.org/3043037/diff/5001/6001
File chrome/browser/idbkeypathextractor_browsertest.cc (right):

http://codereview.chromium.org/3043037/diff/5001/6001#newcode39
chrome/browser/idbkeypathextractor_browsertest.cc:39: if
(!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
It seems we always know on which thread we run CheckExtract. Is there any caller
of this other than this test?

http://codereview.chromium.org/3043037/diff/5001/6001#newcode56
chrome/browser/idbkeypathextractor_browsertest.cc:56: if
(!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
It seems we always know on which thread we run CheckExtract. Is there any caller
of this other than this test?

[bulach, 2010-08-02 09:25:33]

Hi Pawel,

thanks for your comments! I'm not sure I understood, could you please clarify
what do you mean by running the methods on the IO thread seems suspicious? :)

more inline:

http://codereview.chromium.org/3043037/diff/5001/6001
File chrome/browser/idbkeypathextractor_browsertest.cc (right):

http://codereview.chromium.org/3043037/diff/5001/6001#newcode39
chrome/browser/idbkeypathextractor_browsertest.cc:39: if
(!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
On 2010/07/30 23:25:39, Paweł Hajdan Jr. wrote:
> It seems we always know on which thread we run CheckExtract. Is there any
caller
> of this other than this test?

I'm not sure I entirely understood your comment so I added more of my own
comments :) trying to clarify the mechanism here.
the fixture runs in the UI thread, and we need to effectively run this method in
the IO thread.
we could surely move this "posttask" to the test fixture below, but I think that
would make it less clear...
currently, the "helper" encapsulates all this knowledge about which thread to
run what, the "fixture" knows that it should run the message loop, and the test
itself is pretty straightforward.. I'm afraid if we move this "post" down to the
fixture, it wouldn't be so balanced.
thoughts?

http://codereview.chromium.org/3043037/diff/5001/6001#newcode56
chrome/browser/idbkeypathextractor_browsertest.cc:56: if
(!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
On 2010/07/30 23:25:39, Paweł Hajdan Jr. wrote:
> It seems we always know on which thread we run CheckExtract. Is there any
caller
> of this other than this test?

as above..

[andreip3000, 2010-08-02 10:18:01]

http://codereview.chromium.org/3043037/diff/5001/6008
File webkit/glue/webidbkeypathextractor.cc (right):

http://codereview.chromium.org/3043037/diff/5001/6008#newcode14
webkit/glue/webidbkeypathextractor.cc:14: class LocalContext {
Isn't this the same as v8::Context::Scope?

http://codereview.chromium.org/3043037/diff/10001/5003
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/3043037/diff/10001/5003#newcode65
chrome/browser/utility_process_host.cc:65: if (!StartProcess(FilePath()))
Is this starting a new process for each key path?

http://codereview.chromium.org/3043037/diff/10001/5004
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/10001/5004#newcode121
chrome/browser/utility_process_host.h:121: bool ExtractIDBKeyPath(int id,
Should be prefixed with "Start" to maintain consistency with the methods next to
this one?

http://codereview.chromium.org/3043037/diff/10001/5007
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/3043037/diff/10001/5007#newcode267
chrome/utility/utility_thread.cc:267: ChildProcess::current()->ReleaseProcess();
I wonder if it's possible to have a process that we can reuse (and potentially
shut down after some amount of inactivity) rather than spinning a new process
for every key path extraction?

http://codereview.chromium.org/3043037/diff/10001/5009
File webkit/glue/webidbkeypathextractor.cc (right):

http://codereview.chromium.org/3043037/diff/10001/5009#newcode14
webkit/glue/webidbkeypathextractor.cc:14: class LocalContext {
isn't this the same as v8::Context::Scope?

[Paweł Hajdan Jr., 2010-08-02 17:41:56]

http://codereview.chromium.org/3043037/diff/5001/6001
File chrome/browser/idbkeypathextractor_browsertest.cc (right):

http://codereview.chromium.org/3043037/diff/5001/6001#newcode39
chrome/browser/idbkeypathextractor_browsertest.cc:39: if
(!ChromeThread::CurrentlyOn(ChromeThread::IO)) {
On 2010/08/02 09:25:34, bulach wrote:
> On 2010/07/30 23:25:39, Paweł Hajdan Jr. wrote:
> > It seems we always know on which thread we run CheckExtract. Is there any
> caller
> > of this other than this test?
> 
> I'm not sure I entirely understood your comment so I added more of my own
> comments :) trying to clarify the mechanism here.
> the fixture runs in the UI thread, and we need to effectively run this method
in
> the IO thread.
> we could surely move this "posttask" to the test fixture below, but I think
that
> would make it less clear...
> currently, the "helper" encapsulates all this knowledge about which thread to
> run what, the "fixture" knows that it should run the message loop, and the
test
> itself is pretty straightforward.. I'm afraid if we move this "post" down to
the
> fixture, it wouldn't be so balanced.
> thoughts?

If you want that, then please make it separate methods, and in each DCHECK it's
called on correct thread. I think that is less error-prone and less "magical",
and actually closer to how we write other code.

[bulach, 2010-08-03 10:28:13]

andrei: thanks for the comments! all addressed, and also it's inline with the
new names from the webkit side. another look please? also, let me know if you're
ok with doing the keeping the process as a separate step.

pawel: sorry, I'm still confused on what is confusing you :)
do you mean this pattern?
if (!expected_thread) {
  Post(expected_thread);
  return;
}
DCHECK(expected_thread)

there are plenty of places where we do this.. :)
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/dom_ui/mediapl...
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/tab_contents/w...

please, let me know if that's what you're talking about, and how I could
clarify: I'm happy to change this if there's a clearer / more standard way of
doing this thread-dance.. :)

http://codereview.chromium.org/3043037/diff/10001/5003
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/3043037/diff/10001/5003#newcode65
chrome/browser/utility_process_host.cc:65: if (!StartProcess(FilePath()))
On 2010/08/02 10:18:01, andreip3000 wrote:
> Is this starting a new process for each key path?

yes. as mentioned before, this is far from being optimized.
I changed so that it now can pass a vector of serialized values in bulk, rather
than one by one.
I'll look into keeping the process alive for longer, but it may be better to do
separately?

http://codereview.chromium.org/3043037/diff/10001/5004
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/10001/5004#newcode121
chrome/browser/utility_process_host.h:121: bool ExtractIDBKeyPath(int id,
On 2010/08/02 10:18:01, andreip3000 wrote:
> Should be prefixed with "Start" to maintain consistency with the methods next
to
> this one?

Done.

http://codereview.chromium.org/3043037/diff/10001/5007
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/3043037/diff/10001/5007#newcode267
chrome/utility/utility_thread.cc:267: ChildProcess::current()->ReleaseProcess();
On 2010/08/02 10:18:01, andreip3000 wrote:
> I wonder if it's possible to have a process that we can reuse (and potentially
> shut down after some amount of inactivity) rather than spinning a new process
> for every key path extraction?

I changed now so that you can pass a vector of serialized script values, and
I'll investigate keeping the process running, but as above, perhaps we could
split this in a separate step? 

..I think it may be possible to remove this "Release" here, and add a new
message like "StopValueForKeyPath" to release the process. this way, whoever is
extracting the value, may be able to keep the UtilityProcess for as long as it
needs.

http://codereview.chromium.org/3043037/diff/10001/5009
File webkit/glue/webidbkeypathextractor.cc (right):

http://codereview.chromium.org/3043037/diff/10001/5009#newcode14
webkit/glue/webidbkeypathextractor.cc:14: class LocalContext {
On 2010/08/02 10:18:01, andreip3000 wrote:
> isn't this the same as v8::Context::Scope?

same case as https://bugs.webkit.org/show_bug.cgi?id=43276, I think we need a
v8::HandleScope as well, so I moved it here rather than on the method below.

[jorlow, 2010-08-03 14:31:59]

http://codereview.chromium.org/3043037/diff/17001/4004
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/17001/4004#newcode80
chrome/browser/utility_process_host.h:80: int id, const std::vector<IDBKey>&
value) {}
Why are these all {}..?

http://codereview.chromium.org/3043037/diff/17001/4005
File chrome/chrome_common.gypi (right):

http://codereview.chromium.org/3043037/diff/17001/4005#newcode62
chrome/chrome_common.gypi:62: 'common/idbkey.cc',
I already wrote this.  It's called indexed_db_key.  It's right below.  :-)

http://codereview.chromium.org/3043037/diff/17001/4008
File chrome/common/idbkey.h (right):

http://codereview.chromium.org/3043037/diff/17001/4008#newcode18
chrome/common/idbkey.h:18: enum Type {
Can't we just use the enums from it?  The only reason we can't just use the Web
types wholesale is that they are not threadsafe.

http://codereview.chromium.org/3043037/diff/17001/4009
File chrome/common/utility_messages.h (right):

http://codereview.chromium.org/3043037/diff/17001/4009#newcode82
chrome/common/utility_messages.h:82: } else if (p.type() == IDBKey::StringType)
{
NumberType!

Didn't I already write this code...?

http://codereview.chromium.org/3043037/diff/17001/4009#newcode117
chrome/common/utility_messages.h:117: } else if (p.type() == IDBKey::StringType)
{
NumberType!

http://codereview.chromium.org/3043037/diff/17001/4010
File chrome/common/utility_messages_internal.h (right):

http://codereview.chromium.org/3043037/diff/17001/4010#newcode54
chrome/common/utility_messages_internal.h:54:
IPC_MESSAGE_CONTROL3(UtilityMsg_IDBValueForKeyPath,
Why isn't this sync?

http://codereview.chromium.org/3043037/diff/17001/4011
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/3043037/diff/17001/4011#newcode262
chrome/utility/utility_thread.cc:262: int id, const std::vector<string16>&
serialized_script_values,
Why not keep them in SerializedScriptValue objects?

http://codereview.chromium.org/3043037/diff/17001/4011#newcode265
chrome/utility/utility_thread.cc:265: typedef
std::vector<string16>::const_iterator ci;
This name isn't very good.

http://codereview.chromium.org/3043037/diff/17001/4011#newcode266
chrome/utility/utility_thread.cc:266: for (ci
i(serialized_script_values.begin());
i think i = blah is a bit more readable and common

http://codereview.chromium.org/3043037/diff/17001/4011#newcode268
chrome/utility/utility_thread.cc:268: WebKit::WebIDBKey webValue(0);
do we need some sort of empty/invalid initializer?

And or should it return this value?

http://codereview.chromium.org/3043037/diff/17001/4011#newcode273
chrome/utility/utility_thread.cc:273: ChildProcess::current()->ReleaseProcess();
Where's the other half of this?

Do we want processes to be thrashing during IDB usage?

http://codereview.chromium.org/3043037/diff/17001/4013
File webkit/glue/webidbbindingutilities.cc (right):

http://codereview.chromium.org/3043037/diff/17001/4013#newcode5
webkit/glue/webidbbindingutilities.cc:5: #include "webidbbindingutilities.h"
does this match the style guide and/or how we've done it elsewhere?

http://codereview.chromium.org/3043037/diff/17001/4013#newcode16
webkit/glue/webidbbindingutilities.cc:16: class LocalContext {
You'd better rope in some people like dglazkov, mads, and/or darin to look at
this as well.

http://codereview.chromium.org/3043037/diff/17001/4013#newcode19
webkit/glue/webidbbindingutilities.cc:19: : context_(v8::Context::New()) {
Hm...can we make the context in the WebKit code instead?

http://codereview.chromium.org/3043037/diff/17001/4013#newcode36
webkit/glue/webidbbindingutilities.cc:36: LocalContext env;
Do you need to grab the lock?

[bulach, 2010-08-05 21:26:26]

thanks jeremy! comments addressed, would you please take another look before I
send to a wider audience?

http://codereview.chromium.org/3043037/diff/17001/4004
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/17001/4004#newcode80
chrome/browser/utility_process_host.h:80: int id, const std::vector<IDBKey>&
value) {}
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> Why are these all {}..?

I guess it's designed so that one can derive from this client and implement only
the methods one is interested..

http://codereview.chromium.org/3043037/diff/17001/4005
File chrome/chrome_common.gypi (right):

http://codereview.chromium.org/3043037/diff/17001/4005#newcode62
chrome/chrome_common.gypi:62: 'common/idbkey.cc',
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> I already wrote this.  It's called indexed_db_key.  It's right below.  :-)

ouch... :) oh, well, thanks for writing it! :)

http://codereview.chromium.org/3043037/diff/17001/4008
File chrome/common/idbkey.h (right):

http://codereview.chromium.org/3043037/diff/17001/4008#newcode18
chrome/common/idbkey.h:18: enum Type {
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> Can't we just use the enums from it?  The only reason we can't just use the
Web
> types wholesale is that they are not threadsafe.


deleted this class..

http://codereview.chromium.org/3043037/diff/17001/4009
File chrome/common/utility_messages.h (right):

http://codereview.chromium.org/3043037/diff/17001/4009#newcode82
chrome/common/utility_messages.h:82: } else if (p.type() == IDBKey::StringType)
{
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> NumberType!
> 
> Didn't I already write this code...?

yep, and apparently better than this.. :)

http://codereview.chromium.org/3043037/diff/17001/4009#newcode117
chrome/common/utility_messages.h:117: } else if (p.type() == IDBKey::StringType)
{
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> NumberType!

removed.

http://codereview.chromium.org/3043037/diff/17001/4010
File chrome/common/utility_messages_internal.h (right):

http://codereview.chromium.org/3043037/diff/17001/4010#newcode54
chrome/common/utility_messages_internal.h:54:
IPC_MESSAGE_CONTROL3(UtilityMsg_IDBValueForKeyPath,
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> Why isn't this sync?

none of these utility methods are sync.
if i understood correctly, this utility process is entirely async: you call the
StartFoo method, it launches the process, sends the message, and eventually
calls back via the Client interface.

http://codereview.chromium.org/3043037/diff/17001/4011
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/3043037/diff/17001/4011#newcode262
chrome/utility/utility_thread.cc:262: int id, const std::vector<string16>&
serialized_script_values,
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> Why not keep them in SerializedScriptValue objects?

Done.

http://codereview.chromium.org/3043037/diff/17001/4011#newcode265
chrome/utility/utility_thread.cc:265: typedef
std::vector<string16>::const_iterator ci;
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> This name isn't very good.

I moved this down to the glue layer so that we hold the v8 scope / lock only
once.

http://codereview.chromium.org/3043037/diff/17001/4011#newcode266
chrome/utility/utility_thread.cc:266: for (ci
i(serialized_script_values.begin());
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> i think i = blah is a bit more readable and common

done in the glue.

http://codereview.chromium.org/3043037/diff/17001/4011#newcode268
chrome/utility/utility_thread.cc:268: WebKit::WebIDBKey webValue(0);
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> do we need some sort of empty/invalid initializer?
> 
> And or should it return this value?

moved to the glue, making it return the value.

http://codereview.chromium.org/3043037/diff/17001/4011#newcode273
chrome/utility/utility_thread.cc:273: ChildProcess::current()->ReleaseProcess();
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> Where's the other half of this?
> 
> Do we want processes to be thrashing during IDB usage?

the other half is the utility_process.
I'll investigate whether or not it's possible to keep this process open rather
than releasing it per key path.

http://codereview.chromium.org/3043037/diff/17001/4013
File webkit/glue/webidbbindingutilities.cc (right):

http://codereview.chromium.org/3043037/diff/17001/4013#newcode5
webkit/glue/webidbbindingutilities.cc:5: #include "webidbbindingutilities.h"
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> does this match the style guide and/or how we've done it elsewhere?

this directory has plenty of webfoobar.*, I just followed the local convention.

http://codereview.chromium.org/3043037/diff/17001/4013#newcode16
webkit/glue/webidbbindingutilities.cc:16: class LocalContext {
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> You'd better rope in some people like dglazkov, mads, and/or darin to look at
> this as well.

yep, let's agree on the overall API / usage for indexed db, and then I'll ask
them to take a look once you're done.

http://codereview.chromium.org/3043037/diff/17001/4013#newcode19
webkit/glue/webidbbindingutilities.cc:19: : context_(v8::Context::New()) {
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> Hm...can we make the context in the WebKit code instead?

hmm, not sure.. I moved the vector traversal here, so seems a good place to hold
this context? otherwise, there'd be yet-another-copy from std::vector to
WebVector to pass to WebKit..

http://codereview.chromium.org/3043037/diff/17001/4013#newcode36
webkit/glue/webidbbindingutilities.cc:36: LocalContext env;
On 2010/08/03 14:31:59, Jeremy Orlow wrote:
> Do you need to grab the lock?

done.

[jorlow, 2010-08-06 14:42:09]

besides what i mentioned in person, looking pretty good

[bulach, 2010-08-10 16:41:09]

+avayvod, brettw

all: this change depends on
https://bugs.webkit.org/show_bug.cgi?id=42976
https://bugs.webkit.org/show_bug.cgi?id=43276
but I'd appreciate your thoughts on the chromium side.

brett / anton: would you mind taking a look at the UtilityProcess bits, please?
long story short: on IndexedDB, we need to parse "KeyPath" and extract a value
from a serialized script value both of these are external data (either from
disk, or from JS), so instead of running in the browser process, we need to run
inside a sandbox. please, ping me if you need more details.

andrei / jorlow: I added the ability to keep the utility process running, and
clarified the behavior for error conditions. both are covered by new tests.
another look please?

[jorlow, 2010-08-10 16:53:05]

LGTM.  My only concern is that whomever is using this needs to handle the case
that the process crashes, is killed, OOMs, etc.  Maybe at least add a comment
about this?

[whywhat, 2010-08-10 17:20:54]

http://codereview.chromium.org/3043037/diff/35001/36003
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/35001/36003#newcode15
chrome/browser/utility_process_host.h:15: #include
"chrome/common/serialized_script_value.h"
This is not in alphabetical order, should be after chrome/common/extensions/

http://codereview.chromium.org/3043037/diff/35001/36003#newcode77
chrome/browser/utility_process_host.h:77: // Called when IDBKeyPath has
extracted the value.
Would be good to have comments about each param for these methods.

http://codereview.chromium.org/3043037/diff/35001/36003#newcode121
chrome/browser/utility_process_host.h:121: // Extracts |key_path| from
|serialized_value|, and replies with the
I think this comment is for the next method.

http://codereview.chromium.org/3043037/diff/35001/36003#newcode124
chrome/browser/utility_process_host.h:124: bool ExtractIDBValueForKeyPath(
Why do we split Start and Extract here?

http://codereview.chromium.org/3043037/diff/35001/36008
File chrome/common/utility_messages_internal.h (right):

http://codereview.chromium.org/3043037/diff/35001/36008#newcode56
chrome/common/utility_messages_internal.h:56: int      /* id */,
I think it's better to have // comments like above.
Same about the definition below.

http://codereview.chromium.org/3043037/diff/35001/36010
File chrome/utility/utility_thread.h (right):

http://codereview.chromium.org/3043037/diff/35001/36010#newcode77
chrome/utility/utility_thread.h:77: void OnIDBValuesForKeyPathFinished();
Why do we need this?
If this is useful indeed to manage the life of sandboxed process, why not make
it a general method (with a general name) so other clients could use it?

[bulach, 2010-08-11 16:56:46]

thanks Anton!

I addressed your comments, and changed things a bit to reflect the latest
changes on the WebKit side.
would you mind taking another look, please?

http://codereview.chromium.org/3043037/diff/35001/36003
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/35001/36003#newcode15
chrome/browser/utility_process_host.h:15: #include
"chrome/common/serialized_script_value.h"
On 2010/08/10 17:20:55, whywhat wrote:
> This is not in alphabetical order, should be after chrome/common/extensions/

Done.

http://codereview.chromium.org/3043037/diff/35001/36003#newcode77
chrome/browser/utility_process_host.h:77: // Called when IDBKeyPath has
extracted the value.
On 2010/08/10 17:20:55, whywhat wrote:
> Would be good to have comments about each param for these methods.

Done.

http://codereview.chromium.org/3043037/diff/35001/36003#newcode121
chrome/browser/utility_process_host.h:121: // Extracts |key_path| from
|serialized_value|, and replies with the
On 2010/08/10 17:20:55, whywhat wrote:
> I think this comment is for the next method.

Done.

http://codereview.chromium.org/3043037/diff/35001/36003#newcode124
chrome/browser/utility_process_host.h:124: bool ExtractIDBValueForKeyPath(
On 2010/08/10 17:20:55, whywhat wrote:
> Why do we split Start and Extract here?

as you suggested, I made it generic with Start/EndUtilityProcessBatchMode().

http://codereview.chromium.org/3043037/diff/35001/36008
File chrome/common/utility_messages_internal.h (right):

http://codereview.chromium.org/3043037/diff/35001/36008#newcode56
chrome/common/utility_messages_internal.h:56: int      /* id */,
On 2010/08/10 17:20:55, whywhat wrote:
> I think it's better to have // comments like above.
> Same about the definition below.

Done.

http://codereview.chromium.org/3043037/diff/35001/36010
File chrome/utility/utility_thread.h (right):

http://codereview.chromium.org/3043037/diff/35001/36010#newcode77
chrome/utility/utility_thread.h:77: void OnIDBValuesForKeyPathFinished();
On 2010/08/10 17:20:55, whywhat wrote:
> Why do we need this?
> If this is useful indeed to manage the life of sandboxed process, why not make
> it a general method (with a general name) so other clients could use it?

yes, great idea! I renamed this to Start/EndUtilityProcessBatchMode(), and
changed things a bit to support such mode.

[whywhat, 2010-08-11 17:19:25]

LGTM with a couple of additional comments.
I'd ask Brett to take a quick look too, since I only touched the code once when
adding my own method and you're changing the whole semantics of the utility
process host now.

http://codereview.chromium.org/3043037/diff/46002/54002
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/3043037/diff/46002/54002#newcode67
chrome/browser/utility_process_host.cc:67: Send(new
UtilityMsg_IDBKeysFromValuesAndKeyPath(
All the rest StartXX methods start the process before sending message to it.
Here we should at least check that the process is started, right?

http://codereview.chromium.org/3043037/diff/46002/54003
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/46002/54003#newcode30
chrome/browser/utility_process_host.h:30: // use StartUtilityProcessBatchMode(),
then multiple calls to StartFooBar(p),
Maybe StartBatchMode is sufficient? Current name looks a bit too long to me.

http://codereview.chromium.org/3043037/diff/46002/54003#newcode149
chrome/browser/utility_process_host.h:149: bool StartProcessIfNeeded(const
FilePath& exposed_dir);
You could leave the one method but change it behavior so it doesn't start new
process if the flag is already true. And reflect this behavior in comments here.

[bulach, 2010-08-11 19:21:33]

thanks, Anton!

brett, would you mind taking a look, please? as Anton said, this is a reasonably
big semantic change, so comments and more eyes are very welcome! :)

http://codereview.chromium.org/3043037/diff/46002/54002
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/3043037/diff/46002/54002#newcode67
chrome/browser/utility_process_host.cc:67: Send(new
UtilityMsg_IDBKeysFromValuesAndKeyPath(
On 2010/08/11 17:19:25, whywhat wrote:
> All the rest StartXX methods start the process before sending message to it.
> Here we should at least check that the process is started, right?

Done.

http://codereview.chromium.org/3043037/diff/46002/54003
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/46002/54003#newcode30
chrome/browser/utility_process_host.h:30: // use StartUtilityProcessBatchMode(),
then multiple calls to StartFooBar(p),
On 2010/08/11 17:19:25, whywhat wrote:
> Maybe StartBatchMode is sufficient? Current name looks a bit too long to me.

sounds better, done.

http://codereview.chromium.org/3043037/diff/46002/54003#newcode149
chrome/browser/utility_process_host.h:149: bool StartProcessIfNeeded(const
FilePath& exposed_dir);
On 2010/08/11 17:19:25, whywhat wrote:
> You could leave the one method but change it behavior so it doesn't start new
> process if the flag is already true. And reflect this behavior in comments
here.

yeah, that looks better, thanks for the suggestion!
done.

[bulach, 2010-08-12 19:12:31]

hmm, I just realized a flaw in the utility_thread side: all the methods are
"ReleaseProcess()" regardless of it being in batch mode. will update a new patch
shortly.

On 2010/08/11 19:21:33, bulach wrote:
> thanks, Anton!
> 
> brett, would you mind taking a look, please? as Anton said, this is a
reasonably
> big semantic change, so comments and more eyes are very welcome! :)
> 
> http://codereview.chromium.org/3043037/diff/46002/54002
> File chrome/browser/utility_process_host.cc (right):
> 
> http://codereview.chromium.org/3043037/diff/46002/54002#newcode67
> chrome/browser/utility_process_host.cc:67: Send(new
> UtilityMsg_IDBKeysFromValuesAndKeyPath(
> On 2010/08/11 17:19:25, whywhat wrote:
> > All the rest StartXX methods start the process before sending message to it.
> > Here we should at least check that the process is started, right?
> 
> Done.
> 
> http://codereview.chromium.org/3043037/diff/46002/54003
> File chrome/browser/utility_process_host.h (right):
> 
> http://codereview.chromium.org/3043037/diff/46002/54003#newcode30
> chrome/browser/utility_process_host.h:30: // use
StartUtilityProcessBatchMode(),
> then multiple calls to StartFooBar(p),
> On 2010/08/11 17:19:25, whywhat wrote:
> > Maybe StartBatchMode is sufficient? Current name looks a bit too long to me.
> 
> sounds better, done.
> 
> http://codereview.chromium.org/3043037/diff/46002/54003#newcode149
> chrome/browser/utility_process_host.h:149: bool StartProcessIfNeeded(const
> FilePath& exposed_dir);
> On 2010/08/11 17:19:25, whywhat wrote:
> > You could leave the one method but change it behavior so it doesn't start
new
> > process if the flag is already true. And reflect this behavior in comments
> here.
> 
> yeah, that looks better, thanks for the suggestion!
> done.

[bulach, 2010-08-12 20:18:43]

uploaded a new patch fixing the utility_thread.

On 2010/08/12 19:12:31, bulach wrote:
> hmm, I just realized a flaw in the utility_thread side: all the methods are
> "ReleaseProcess()" regardless of it being in batch mode. will update a new
patch
> shortly.
> 
> On 2010/08/11 19:21:33, bulach wrote:
> > thanks, Anton!
> > 
> > brett, would you mind taking a look, please? as Anton said, this is a
> reasonably
> > big semantic change, so comments and more eyes are very welcome! :)
> > 
> > http://codereview.chromium.org/3043037/diff/46002/54002
> > File chrome/browser/utility_process_host.cc (right):
> > 
> > http://codereview.chromium.org/3043037/diff/46002/54002#newcode67
> > chrome/browser/utility_process_host.cc:67: Send(new
> > UtilityMsg_IDBKeysFromValuesAndKeyPath(
> > On 2010/08/11 17:19:25, whywhat wrote:
> > > All the rest StartXX methods start the process before sending message to
it.
> > > Here we should at least check that the process is started, right?
> > 
> > Done.
> > 
> > http://codereview.chromium.org/3043037/diff/46002/54003
> > File chrome/browser/utility_process_host.h (right):
> > 
> > http://codereview.chromium.org/3043037/diff/46002/54003#newcode30
> > chrome/browser/utility_process_host.h:30: // use
> StartUtilityProcessBatchMode(),
> > then multiple calls to StartFooBar(p),
> > On 2010/08/11 17:19:25, whywhat wrote:
> > > Maybe StartBatchMode is sufficient? Current name looks a bit too long to
me.
> > 
> > sounds better, done.
> > 
> > http://codereview.chromium.org/3043037/diff/46002/54003#newcode149
> > chrome/browser/utility_process_host.h:149: bool StartProcessIfNeeded(const
> > FilePath& exposed_dir);
> > On 2010/08/11 17:19:25, whywhat wrote:
> > > You could leave the one method but change it behavior so it doesn't start
> new
> > > process if the flag is already true. And reflect this behavior in comments
> > here.
> > 
> > yeah, that looks better, thanks for the suggestion!
> > done.

[brettw, 2010-08-13 05:44:11]

Sorry I took so long...

http://codereview.chromium.org/3043037/diff/66001/67002
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/3043037/diff/66001/67002#newcode93
chrome/browser/utility_process_host.cc:93: if (has_started_) return true;
Please use multiple lines here, we almost never do that.

http://codereview.chromium.org/3043037/diff/66001/67003
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/66001/67003#newcode167
chrome/browser/utility_process_host.h:167: bool has_started_;
Can you add a comment for this? Also, can you consider a better name? It's weird
it gets reset when batch mode completes. I would expect, given this name, for it
to remain true forever once it's started.

http://codereview.chromium.org/3043037/diff/66001/67007
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/3043037/diff/66001/67007#newcode31
chrome/utility/utility_thread.cc:31: : batch_mode_(false) {
Style nit: indent 2 more space.

http://codereview.chromium.org/3043037/diff/66001/67007#newcode289
chrome/utility/utility_thread.cc:289: ChildProcess::current()->ReleaseProcess();
Might this be implementable more simply as just taking another reference when
the batch mode starts, and releasing that reference when batch mode completes?

http://codereview.chromium.org/3043037/diff/66001/67008
File chrome/utility/utility_thread.h (right):

http://codereview.chromium.org/3043037/diff/66001/67008#newcode72
chrome/utility/utility_thread.h:72: void OnIDBKeysFromValuesAndKeyPath(
Can you please provide documentation for all of these functions and members you
added? For the cases where it's documented elsewhere, you can say "See Foo in
bar.h." ReleaseProcessIfNeeded should probably have a specific comment.

http://codereview.chromium.org/3043037/diff/66001/67009
File webkit/glue/webidbbindingutilities.cc (right):

http://codereview.chromium.org/3043037/diff/66001/67009#newcode5
webkit/glue/webidbbindingutilities.cc:5: #include "webidbbindingutilities.h"
Be sure to list the directory for all includes: "webkit/glue/..."

The naming of this file seems weird to me and hard to read. I tried to discern
the pattern. It looks like when we implement a WebKit::Web* interface, we use
all lowercase with no _ (I think this is a mistake but oh well) for the part
matching the WebKit name.

In this case, you're not exactly implementing a WebKit interface like
webclipboard_impl.h is, for example. Also, the "utilities" isn't really helping
here (I'm on a personal mission to delete "utils" files since they usually end
up being a vague dumping ground).

So... I think "idb_bindings.h" and "idb_bindings.cc" is easier and more clear,
and also not inconsistent with the rest of our naming.

http://codereview.chromium.org/3043037/diff/66001/67009#newcode23
webkit/glue/webidbbindingutilities.cc:23: class LocalContext {
I would probably put this in an anonymous namespace so it's not exposed.

http://codereview.chromium.org/3043037/diff/66001/67009#newcode26
webkit/glue/webidbbindingutilities.cc:26: : context_(v8::Context::New()) {
Style nit: two more space indent for the :

http://codereview.chromium.org/3043037/diff/66001/67009#newcode38
webkit/glue/webidbbindingutilities.cc:38: v8::Persistent<v8::Context> context_;
Does DISALLOW_COPY_AND_ASSIGN make sense here? We try to use that whenever
possible to try to prevent some simple mistakes.

http://codereview.chromium.org/3043037/diff/66001/67010
File webkit/glue/webidbbindingutilities.h (right):

http://codereview.chromium.org/3043037/diff/66001/67010#newcode19
webkit/glue/webidbbindingutilities.h:19: bool* error);
Is it practical to just return the error code rather than have an out param,
like you do for the other versions of this function?

[bulach, 2010-08-13 15:06:30]

thanks, Brett!

all comments addressed except one about adding a reference in the child thread
which I may not have understood your suggestion. would you mind another look,
please? thanks!

http://codereview.chromium.org/3043037/diff/66001/67002
File chrome/browser/utility_process_host.cc (right):

http://codereview.chromium.org/3043037/diff/66001/67002#newcode93
chrome/browser/utility_process_host.cc:93: if (has_started_) return true;
On 2010/08/13 05:44:12, brettw wrote:
> Please use multiple lines here, we almost never do that.

Done.

http://codereview.chromium.org/3043037/diff/66001/67003
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/66001/67003#newcode167
chrome/browser/utility_process_host.h:167: bool has_started_;
On 2010/08/13 05:44:12, brettw wrote:
> Can you add a comment for this? Also, can you consider a better name? It's
weird
> it gets reset when batch mode completes. I would expect, given this name, for
it
> to remain true forever once it's started.

Renamed and added some comments, let me know if it's any clearer.

http://codereview.chromium.org/3043037/diff/66001/67007
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/3043037/diff/66001/67007#newcode31
chrome/utility/utility_thread.cc:31: : batch_mode_(false) {
On 2010/08/13 05:44:12, brettw wrote:
> Style nit: indent 2 more space.

Done.

http://codereview.chromium.org/3043037/diff/66001/67007#newcode289
chrome/utility/utility_thread.cc:289: ChildProcess::current()->ReleaseProcess();
On 2010/08/13 05:44:12, brettw wrote:
> Might this be implementable more simply as just taking another reference when
> the batch mode starts, and releasing that reference when batch mode completes?

hmm, not sure I fully understood, but I guess that wouldn't work on these
scenarios:
1. non-batch mode:
OnFooBar(); // needs to ReleaseProcess() here.

2.
StartBatch(); // AddsRef
OnFooBar(); // should not call ReleaseProcess() here.
OnFooBar(); //
EndBatch(); // ReleaseProcess

please, let me know if I misunderstood your suggestion.

http://codereview.chromium.org/3043037/diff/66001/67008
File chrome/utility/utility_thread.h (right):

http://codereview.chromium.org/3043037/diff/66001/67008#newcode72
chrome/utility/utility_thread.h:72: void OnIDBKeysFromValuesAndKeyPath(
On 2010/08/13 05:44:12, brettw wrote:
> Can you please provide documentation for all of these functions and members
you
> added? For the cases where it's documented elsewhere, you can say "See Foo in
> bar.h." ReleaseProcessIfNeeded should probably have a specific comment.

Done.

http://codereview.chromium.org/3043037/diff/66001/67009
File webkit/glue/webidbbindingutilities.cc (right):

http://codereview.chromium.org/3043037/diff/66001/67009#newcode5
webkit/glue/webidbbindingutilities.cc:5: #include "webidbbindingutilities.h"
On 2010/08/13 05:44:12, brettw wrote:
> Be sure to list the directory for all includes: "webkit/glue/..."
> 
> The naming of this file seems weird to me and hard to read. I tried to discern
> the pattern. It looks like when we implement a WebKit::Web* interface, we use
> all lowercase with no _ (I think this is a mistake but oh well) for the part
> matching the WebKit name.
> 
> In this case, you're not exactly implementing a WebKit interface like
> webclipboard_impl.h is, for example. Also, the "utilities" isn't really
helping
> here (I'm on a personal mission to delete "utils" files since they usually end
> up being a vague dumping ground).
> 
> So... I think "idb_bindings.h" and "idb_bindings.cc" is easier and more clear,
> and also not inconsistent with the rest of our naming.

agreed, idb_bindings.* they are!

http://codereview.chromium.org/3043037/diff/66001/67009#newcode23
webkit/glue/webidbbindingutilities.cc:23: class LocalContext {
On 2010/08/13 05:44:12, brettw wrote:
> I would probably put this in an anonymous namespace so it's not exposed.

Done.

http://codereview.chromium.org/3043037/diff/66001/67009#newcode26
webkit/glue/webidbbindingutilities.cc:26: : context_(v8::Context::New()) {
On 2010/08/13 05:44:12, brettw wrote:
> Style nit: two more space indent for the :

Done.

http://codereview.chromium.org/3043037/diff/66001/67009#newcode38
webkit/glue/webidbbindingutilities.cc:38: v8::Persistent<v8::Context> context_;
On 2010/08/13 05:44:12, brettw wrote:
> Does DISALLOW_COPY_AND_ASSIGN make sense here? We try to use that whenever
> possible to try to prevent some simple mistakes.

Done.

http://codereview.chromium.org/3043037/diff/66001/67010
File webkit/glue/webidbbindingutilities.h (right):

http://codereview.chromium.org/3043037/diff/66001/67010#newcode19
webkit/glue/webidbbindingutilities.h:19: bool* error);
On 2010/08/13 05:44:12, brettw wrote:
> Is it practical to just return the error code rather than have an out param,
> like you do for the other versions of this function?

yep, done.

[brettw, 2010-08-13 15:48:14]

LGTM

http://codereview.chromium.org/3043037/diff/66001/67007
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/3043037/diff/66001/67007#newcode289
chrome/utility/utility_thread.cc:289: ChildProcess::current()->ReleaseProcess();
I get it. Then what you have is fine.

[bulach, 2010-08-17 15:04:57]

(ouch, the trybots caught my DEPS violation..).

brett, darin: your insight here will be greatly appreciated!
short summary: I'm implementing this functionality that will run under
UtilityThread. it depends on chrome/common (for some chrome wrapper on webkit
datatypes), v8 (for acquiring a lock and obtaining a value) and webkit (for
parsing a "keypath" and obtaining the actual value).

Issues:
webkit/glue can't depend on chrome/common.
chrome/utility can't depend on v8 (or chrome/browser).

any ideas on how to solve this?

if not changing DEPS, I could move the v8 dependency down to WebKit, but that
would require someone converting the vector 
 of chromium datatypes to webkit datatypes and acquiring the lock there (but I
can't find any other example of this..)

thoughts?

[jorlow, 2010-08-17 15:09:08]

On 2010/08/17 15:04:57, bulach wrote:
> (ouch, the trybots caught my DEPS violation..).
> 
> brett, darin: your insight here will be greatly appreciated!
> short summary: I'm implementing this functionality that will run under
> UtilityThread. it depends on chrome/common (for some chrome wrapper on webkit
> datatypes), v8 (for acquiring a lock and obtaining a value) and webkit (for
> parsing a "keypath" and obtaining the actual value).
> 
> Issues:
> webkit/glue can't depend on chrome/common.
> chrome/utility can't depend on v8 (or chrome/browser).
> 
> any ideas on how to solve this?
> 
> if not changing DEPS, I could move the v8 dependency down to WebKit, but that
> would require someone converting the vector 
>  of chromium datatypes to webkit datatypes and acquiring the lock there (but I
> can't find any other example of this..)
> 
> thoughts?

I'm against moving it into WebKit because it's just wrong to have it messing
with the V8 lock directly, the WebKit API would no longer match the design of
the WebCore one (which takes a single item, not a vector), and because of the
inefficiency.

Changing chrome/common's check against v8 is bad because then browser could
depend on v8.

So I think making a new chrome/sandboxed_common (or something like that) is the
right solution.  But I'm just a mere mortal, unlike Darin and Brett.  :-)

[bulach, 2010-08-17 16:33:15]

On 2010/08/17 15:09:08, jorlow wrote:
> On 2010/08/17 15:04:57, bulach wrote:
> > (ouch, the trybots caught my DEPS violation..).
> > 
> > brett, darin: your insight here will be greatly appreciated!
> > short summary: I'm implementing this functionality that will run under
> > UtilityThread. it depends on chrome/common (for some chrome wrapper on
webkit
> > datatypes), v8 (for acquiring a lock and obtaining a value) and webkit (for
> > parsing a "keypath" and obtaining the actual value).
> > 
> > Issues:
> > webkit/glue can't depend on chrome/common.
> > chrome/utility can't depend on v8 (or chrome/browser).
> > 
> > any ideas on how to solve this?
> > 
> > if not changing DEPS, I could move the v8 dependency down to WebKit, but
that
> > would require someone converting the vector 
> >  of chromium datatypes to webkit datatypes and acquiring the lock there (but
I
> > can't find any other example of this..)
> > 
> > thoughts?
> 
> I'm against moving it into WebKit because it's just wrong to have it messing
> with the V8 lock directly, the WebKit API would no longer match the design of
> the WebCore one (which takes a single item, not a vector), and because of the
> inefficiency.
> 
> Changing chrome/common's check against v8 is bad because then browser could
> depend on v8.
> 
> So I think making a new chrome/sandboxed_common (or something like that) is
the
> right solution.  But I'm just a mere mortal, unlike Darin and Brett.  :-)

I managed to work around hopefully in a nice way (i.e., doesn't add any overhead
and doesn't require additional deps):
1. instead of webkit/glue depending on chrome/common, it now takes WebKit types.
2. utilitythread does the conversion from chrome/common to webkit.

just sent to try servers, let's see what they think of it, but please give me a
shout if there's something intrinsically wrong with this workaround.

[brettw, 2010-08-17 17:12:19]

Just some style nits. The conversions and layering looks OK to me. In some
places, we pass the WebKit values around so I don't know whether we even really
need a conversion to SerializedScriptValue. Not knowing background on this or
how it's used, I don't have a strong opinion.

http://codereview.chromium.org/3043037/diff/39004/82003
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/39004/82003#newcode167
chrome/browser/utility_process_host.h:167: // True iff running in batch mode,
i.e., StartBatchMode() has been called
Nit you don't have to follow: I hate it when people write "iff". The "only if"
part really doesn't add any extra meaning in the context of a comment in source
code. "when" or "if" works just as well and is easier to read.

http://codereview.chromium.org/3043037/diff/39004/82009
File chrome/common/utility_messages_internal.h (right):

http://codereview.chromium.org/3043037/diff/39004/82009#newcode17
chrome/common/utility_messages_internal.h:17: #include
"chrome/common/serialized_script_value.h"
Is this include necessary? It looks like you can forward declare it in this
header.

http://codereview.chromium.org/3043037/diff/39004/82009#newcode60
chrome/common/utility_messages_internal.h:60: // Tells the utility process that
it's running in batch mode.
Check indenting for these two messages.

http://codereview.chromium.org/3043037/diff/39004/82010
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/3043037/diff/39004/82010#newcode264
chrome/utility/utility_thread.cc:264: namespace {
Normally we put the anonymous namespace at the beginning of the file. Also be
sure that there are blank lines around the beginning & end of the namespace, and
the ending one looks like this "}  // namespace"

http://codereview.chromium.org/3043037/diff/39004/82010#newcode274
chrome/utility/utility_thread.cc:274: int id, const
std::vector<SerializedScriptValue>& serialized_script_values,
I'd put "serialized_script_values" on the next line as I explain in the arg
wrapping comment below.

http://codereview.chromium.org/3043037/diff/39004/82012
File webkit/glue/idb_bindings.cc (right):

http://codereview.chromium.org/3043037/diff/39004/82012#newcode45
webkit/glue/idb_bindings.cc:45: } // namespace
Two spaces before end-of-line comments.

http://codereview.chromium.org/3043037/diff/39004/82012#newcode49
webkit/glue/idb_bindings.cc:49: const string16& idb_key_path,
std::vector<WebIDBKey>* values) {
Generally prefer to put args on separate lines unless everything fits on one or
there's some logical grouping (like "x" and "y"). So I'd put "values" on the
next line. Same in the header.

http://codereview.chromium.org/3043037/diff/39004/82012#newcode55
webkit/glue/idb_bindings.cc:55: int pos = 0;
It doesn't look like this is needed for anything. Can you just delete it?

http://codereview.chromium.org/3043037/diff/39004/82012#newcode57
webkit/glue/idb_bindings.cc:57: serialized_script_values.begin();
I'd indent this 4 spaces from the ( (so 3 more).

http://codereview.chromium.org/3043037/diff/39004/82013
File webkit/glue/idb_bindings.h (right):

http://codereview.chromium.org/3043037/diff/39004/82013#newcode20
webkit/glue/idb_bindings.h:20: const
std::vector<WebKit::WebSerializedScriptValue>& serialized_script_values,
I'd wrap the var name indented 4 more spaces from the "const" on the previous
line.

[jorlow, 2010-08-17 17:32:38]

http://codereview.chromium.org/3043037/diff/39004/82012
File webkit/glue/idb_bindings.cc (right):

http://codereview.chromium.org/3043037/diff/39004/82012#newcode49
webkit/glue/idb_bindings.cc:49: const string16& idb_key_path,
std::vector<WebIDBKey>* values) {
On 2010/08/17 17:12:19, brettw wrote:
> Generally prefer to put args on separate lines unless everything fits on one
or
> there's some logical grouping (like "x" and "y"). So I'd put "values" on the
> next line. Same in the header.

brettw: I know I've told people to do it this way (what Marcus did) in code
reviews before, and I'm pretty sure I've seen others do it as well.  If what
we've been saying is wrong, maybe you should write a short and sweet PSA to
chromium-dev to make sure everyone's on the same page?

[bulach, 2010-08-17 21:30:22]

many thanks for the detailed review, Brett! I'll submit iff^H^H^H when the bots
are green. :)

jokes apart:
the latest patch failed on windows.. for some bizarre reason MSVC tries to pull
in some webkit dependency in npchrome_frame due to the param trait being in
common.. :(
I created a indexed_db_param_traits.h, and now I'm only including it in
render_messages and utility_messages, let see if all the bots are happy now...

http://codereview.chromium.org/3043037/diff/39004/82003
File chrome/browser/utility_process_host.h (right):

http://codereview.chromium.org/3043037/diff/39004/82003#newcode167
chrome/browser/utility_process_host.h:167: // True iff running in batch mode,
i.e., StartBatchMode() has been called
On 2010/08/17 17:12:19, brettw wrote:
> Nit you don't have to follow: I hate it when people write "iff". The "only if"
> part really doesn't add any extra meaning in the context of a comment in
source
> code. "when" or "if" works just as well and is easier to read.

heh! :) I deleted an "iff" above so thought would balance by adding a new one..
;)
on a more serious note, done s/iff/when/

http://codereview.chromium.org/3043037/diff/39004/82009
File chrome/common/utility_messages_internal.h (right):

http://codereview.chromium.org/3043037/diff/39004/82009#newcode17
chrome/common/utility_messages_internal.h:17: #include
"chrome/common/serialized_script_value.h"
On 2010/08/17 17:12:19, brettw wrote:
> Is this include necessary? It looks like you can forward declare it in this
> header.

Done.

http://codereview.chromium.org/3043037/diff/39004/82009#newcode60
chrome/common/utility_messages_internal.h:60: // Tells the utility process that
it's running in batch mode.
On 2010/08/17 17:12:19, brettw wrote:
> Check indenting for these two messages.

Done.

http://codereview.chromium.org/3043037/diff/39004/82010
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/3043037/diff/39004/82010#newcode264
chrome/utility/utility_thread.cc:264: namespace {
On 2010/08/17 17:12:19, brettw wrote:
> Normally we put the anonymous namespace at the beginning of the file. Also be
> sure that there are blank lines around the beginning & end of the namespace,
and
> the ending one looks like this "}  // namespace"

Done.

http://codereview.chromium.org/3043037/diff/39004/82010#newcode274
chrome/utility/utility_thread.cc:274: int id, const
std::vector<SerializedScriptValue>& serialized_script_values,
On 2010/08/17 17:12:19, brettw wrote:
> I'd put "serialized_script_values" on the next line as I explain in the arg
> wrapping comment below.

Done.

http://codereview.chromium.org/3043037/diff/39004/82012
File webkit/glue/idb_bindings.cc (right):

http://codereview.chromium.org/3043037/diff/39004/82012#newcode45
webkit/glue/idb_bindings.cc:45: } // namespace
On 2010/08/17 17:12:19, brettw wrote:
> Two spaces before end-of-line comments.

Done.

http://codereview.chromium.org/3043037/diff/39004/82012#newcode49
webkit/glue/idb_bindings.cc:49: const string16& idb_key_path,
std::vector<WebIDBKey>* values) {
On 2010/08/17 17:12:19, brettw wrote:
> Generally prefer to put args on separate lines unless everything fits on one
or
> there's some logical grouping (like "x" and "y"). So I'd put "values" on the
> next line. Same in the header.

Done.

http://codereview.chromium.org/3043037/diff/39004/82012#newcode55
webkit/glue/idb_bindings.cc:55: int pos = 0;
On 2010/08/17 17:12:19, brettw wrote:
> It doesn't look like this is needed for anything. Can you just delete it?

Done.

http://codereview.chromium.org/3043037/diff/39004/82012#newcode57
webkit/glue/idb_bindings.cc:57: serialized_script_values.begin();
On 2010/08/17 17:12:19, brettw wrote:
> I'd indent this 4 spaces from the ( (so 3 more).

Done.

http://codereview.chromium.org/3043037/diff/39004/82013
File webkit/glue/idb_bindings.h (right):

http://codereview.chromium.org/3043037/diff/39004/82013#newcode20
webkit/glue/idb_bindings.h:20: const
std::vector<WebKit::WebSerializedScriptValue>& serialized_script_values,
On 2010/08/17 17:12:19, brettw wrote:
> I'd wrap the var name indented 4 more spaces from the "const" on the previous
> line.

Done.