Add Custom V8 bindings for HTML5 Database.

R=mbelshe
BUG=4359

[dglazkov, 2009-01-16 00:17:24]

This is completely barren, non-functional, and Win-only at the moment, but it's
a start.

[darin (slow to review), 2009-01-16 08:09:59]

LGTM

http://codereview.chromium.org/18125/diff/1/4
File webkit/port/bindings/v8/V8DatabaseCustom.cpp (right):

http://codereview.chromium.org/18125/diff/1/4#newcode52
Line 52: }  // namespace WebCore
nit: i think webkit convention is to just have one space between the "}" and the
"// namespace WebCore" comment :)

[dglazkov, 2009-02-03 18:49:52]

No project changes to keep the review size manageable.

[Mike Belshe, 2009-02-04 00:07:45]

http://codereview.chromium.org/18125/diff/420/425
File webkit/port/bindings/v8/V8CustomSQLStatementCallback.cpp (right):

http://codereview.chromium.org/18125/diff/420/425#newcode1
Line 1: /*
What would you think about putting these into a subdirectory:

port/bindings/v8/custom/SQLStatementErrorCallback.cpp?

?

http://codereview.chromium.org/18125/diff/420/425#newcode47
Line 47: m_callback.Clear();
I think you want m_callback.Dispose() here?

http://codereview.chromium.org/18125/diff/420/428
File webkit/port/bindings/v8/V8CustomSQLStatementCallback.h (right):

http://codereview.chromium.org/18125/diff/420/428#newcode38
Line 38: using namespace v8;
I don't think we should ever have using in a header file.
Certainly against google standards.

Do this in all headers.

http://codereview.chromium.org/18125/diff/420/428#newcode46
Line 46: static PassRefPtr<V8CustomSQLStatementCallback> create(Handle<Value>
value, Frame* frame)
Use Local<>.  Using Local<> lets us use the compiler to make sure we don't mix
and match handle types.  Try to never use Handle<>

http://codereview.chromium.org/18125/diff/420/428#newcode48
Line 48: return adoptRef(new V8CustomSQLStatementCallback(value->ToObject(),
frame));
I suppose you could add 

ASSERT(value->IsObject());

http://codereview.chromium.org/18125/diff/420/431
File webkit/port/bindings/v8/V8CustomSQLStatementErrorCallback.cpp (right):

http://codereview.chromium.org/18125/diff/420/431#newcode47
Line 47: m_callback.Clear();
Dispose?

http://codereview.chromium.org/18125/diff/420/432
File webkit/port/bindings/v8/V8CustomSQLTransactionCallback.cpp (right):

http://codereview.chromium.org/18125/diff/420/432#newcode49
Line 49: m_callback.Clear();
Dispose?

http://codereview.chromium.org/18125/diff/420/424
File webkit/port/bindings/v8/V8CustomSQLTransactionErrorCallback.cpp (right):

http://codereview.chromium.org/18125/diff/420/424#newcode47
Line 47: m_callback.Clear();
Dispose()

http://codereview.chromium.org/18125/diff/420/426
File webkit/port/bindings/v8/V8CustomVoidCallback.cpp (right):

http://codereview.chromium.org/18125/diff/420/426#newcode48
Line 48: m_callback.Clear();
Dispose?

http://codereview.chromium.org/18125/diff/420/426#newcode68
Line 68: bool invokeCallback(Persistent<Object> callback, int argc,
Handle<Value> argv[], bool raisedException)
&

http://codereview.chromium.org/18125/diff/420/426#newcode94
Line 94: if (exceptionCatcher.HasCaught())
raisedException is not initialized in all cases.

Propose:
raisedException = exceptionCatcher.HasCaught();

http://codereview.chromium.org/18125/diff/420/426#newcode97
Line 97: if (!result.IsEmpty())
Should this be:
!result.IsEmpty() && result.IsBoolean()?

http://codereview.chromium.org/18125/diff/420/433
File webkit/port/bindings/v8/V8CustomVoidCallback.h (right):

http://codereview.chromium.org/18125/diff/420/433#newcode62
Line 62: bool invokeCallback(Persistent<Object> callback, int argc,
Handle<Value> argv[], bool raisedException);
Is this supposed to be bool &raisedException?  (pass by reference)

http://codereview.chromium.org/18125/diff/420/433#newcode62
Line 62: bool invokeCallback(Persistent<Object> callback, int argc,
Handle<Value> argv[], bool raisedException);
Add Comment about this function:  Returns true on success, false on failure,
sets the raisedException out parameter, etc etc.

http://codereview.chromium.org/18125/diff/420/421
File webkit/port/bindings/v8/V8DatabaseCustom.cpp (right):

http://codereview.chromium.org/18125/diff/420/421#newcode42
Line 42: using namespace v8;
I don't *like* the using in cpp files either, but I can live with them.  I don't
think we've been using them in general.

http://codereview.chromium.org/18125/diff/420/421#newcode92
Line 92: return Undefined();
Is returning Undefined() correct here (in the success case)?

http://codereview.chromium.org/18125/diff/420/423
File webkit/port/bindings/v8/V8SQLTransactionCustom.cpp (right):

http://codereview.chromium.org/18125/diff/420/423#newcode71
Line 71: TryCatch exceptionCatcher;
What is the purpose of this?

http://codereview.chromium.org/18125/diff/420/423#newcode82
Line 82: else
Should this specifically test for value->IsString()?

http://codereview.chromium.org/18125/diff/420/423#newcode117
Line 117: return v8::Undefined();
Returning undefined?

[dglazkov, 2009-02-05 19:10:44]

Ready for another look. Thank you so much for the review!

http://codereview.chromium.org/18125/diff/420/425
File webkit/port/bindings/v8/V8CustomSQLStatementCallback.cpp (right):

http://codereview.chromium.org/18125/diff/420/425#newcode1
Line 1: /*
On 2009/02/04 00:07:45, Mike Belshe wrote:
> What would you think about putting these into a subdirectory:
> 
> port/bindings/v8/custom/SQLStatementErrorCallback.cpp?
> 
> ?

I think it is a good idea -- this is how I will be upstreaming them.

http://codereview.chromium.org/18125/diff/420/425#newcode47
Line 47: m_callback.Clear();
On 2009/02/04 00:07:45, Mike Belshe wrote:
> I think you want m_callback.Dispose() here?

Yes! Thank you.

http://codereview.chromium.org/18125/diff/420/428
File webkit/port/bindings/v8/V8CustomSQLStatementCallback.h (right):

http://codereview.chromium.org/18125/diff/420/428#newcode38
Line 38: using namespace v8;
On 2009/02/04 00:07:45, Mike Belshe wrote:
> I don't think we should ever have using in a header file.
> Certainly against google standards.
> 
> Do this in all headers.

Done.

http://codereview.chromium.org/18125/diff/420/428#newcode46
Line 46: static PassRefPtr<V8CustomSQLStatementCallback> create(Handle<Value>
value, Frame* frame)
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Use Local<>.  Using Local<> lets us use the compiler to make sure we don't mix
> and match handle types.  Try to never use Handle<>

Done.

http://codereview.chromium.org/18125/diff/420/428#newcode48
Line 48: return adoptRef(new V8CustomSQLStatementCallback(value->ToObject(),
frame));
On 2009/02/04 00:07:45, Mike Belshe wrote:
> I suppose you could add 
> 
> ASSERT(value->IsObject());

Done.

http://codereview.chromium.org/18125/diff/420/431
File webkit/port/bindings/v8/V8CustomSQLStatementErrorCallback.cpp (right):

http://codereview.chromium.org/18125/diff/420/431#newcode47
Line 47: m_callback.Clear();
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Dispose?

Done.

http://codereview.chromium.org/18125/diff/420/432
File webkit/port/bindings/v8/V8CustomSQLTransactionCallback.cpp (right):

http://codereview.chromium.org/18125/diff/420/432#newcode49
Line 49: m_callback.Clear();
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Dispose?

Done.

http://codereview.chromium.org/18125/diff/420/424
File webkit/port/bindings/v8/V8CustomSQLTransactionErrorCallback.cpp (right):

http://codereview.chromium.org/18125/diff/420/424#newcode47
Line 47: m_callback.Clear();
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Dispose()
> 
> 

Done.

http://codereview.chromium.org/18125/diff/420/426
File webkit/port/bindings/v8/V8CustomVoidCallback.cpp (right):

http://codereview.chromium.org/18125/diff/420/426#newcode48
Line 48: m_callback.Clear();
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Dispose?
> 
> 

Done.

http://codereview.chromium.org/18125/diff/420/426#newcode68
Line 68: bool invokeCallback(Persistent<Object> callback, int argc,
Handle<Value> argv[], bool raisedException)
On 2009/02/04 00:07:45, Mike Belshe wrote:
> &

Done.

http://codereview.chromium.org/18125/diff/420/426#newcode94
Line 94: if (exceptionCatcher.HasCaught())
On 2009/02/04 00:07:45, Mike Belshe wrote:
> raisedException is not initialized in all cases.
> 
> Propose:
> raisedException = exceptionCatcher.HasCaught();

Done.

http://codereview.chromium.org/18125/diff/420/426#newcode97
Line 97: if (!result.IsEmpty())
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Should this be:
> !result.IsEmpty() && result.IsBoolean()?

Done.

http://codereview.chromium.org/18125/diff/420/433
File webkit/port/bindings/v8/V8CustomVoidCallback.h (right):

http://codereview.chromium.org/18125/diff/420/433#newcode62
Line 62: bool invokeCallback(Persistent<Object> callback, int argc,
Handle<Value> argv[], bool raisedException);
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Add Comment about this function:  Returns true on success, false on failure,
> sets the raisedException out parameter, etc etc.

Done -- added comment in the header.

http://codereview.chromium.org/18125/diff/420/433#newcode62
Line 62: bool invokeCallback(Persistent<Object> callback, int argc,
Handle<Value> argv[], bool raisedException);
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Is this supposed to be bool &raisedException?  (pass by reference)

Doh! Thanks. Fixed. Actually, I refactored it a bit, making function return
value indicate failure (exception, etc.) and accepting a &returnValue as a
parameter.

http://codereview.chromium.org/18125/diff/420/421
File webkit/port/bindings/v8/V8DatabaseCustom.cpp (right):

http://codereview.chromium.org/18125/diff/420/421#newcode92
Line 92: return Undefined();
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Is returning Undefined() correct here (in the success case)?

Yep.

http://codereview.chromium.org/18125/diff/420/423
File webkit/port/bindings/v8/V8SQLTransactionCustom.cpp (right):

http://codereview.chromium.org/18125/diff/420/423#newcode71
Line 71: TryCatch exceptionCatcher;
On 2009/02/04 00:07:45, Mike Belshe wrote:
> What is the purpose of this?

It was my understanding that I could overwrite a getter on an array and throw an
exception on retrieval, but it doesn't seem to be possible. Removed.

http://codereview.chromium.org/18125/diff/420/423#newcode82
Line 82: else
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Should this specifically test for value->IsString()?

They don't do it upstream currently, but there's a discussion on whether this is
the right thing to do.

http://codereview.chromium.org/18125/diff/420/423#newcode117
Line 117: return v8::Undefined();
On 2009/02/04 00:07:45, Mike Belshe wrote:
> Returning undefined?

Yep.

[Mike Belshe, 2009-02-05 19:20:05]

LGTM - you decide on the using v8 stuff.

http://codereview.chromium.org/18125/diff/442/445
File webkit/port/bindings/v8/V8SQLTransactionCustom.cpp (right):

http://codereview.chromium.org/18125/diff/442/445#newcode57
Line 57: String statement = ToWebCoreString(args[0]);
Sorry to nit on using namespaces.  This one has both WebCore::String and
v8::String.  I think it works here, but I find the code harder to read.

If you don't mind, I'd prefer to nix the using altogether.  But I leave the
decision to you.