Bug 34994

*** Bug 38061 has been marked as a duplicate of this bug. ***

Created attachment 54212 [details] patch #1: stubs for storage classes JSC and V8 bindings are coming next, followed by IDL files which will connect the bindings to the respective class methods.

Attachment 54212 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/storage/DatabaseSync.cpp:36: Alphabetical sorting problem. [build/include_order] [4] WebCore/storage/SQLTransactionSync.h:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 54212 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1901002

Created attachment 54213 [details] patch #1: stubs for storage classes Same patch. Should fix Chromium and style problems.

Started doing reviewing. Haven't found anything that's for sure a bug yet, but I'm VERY concerned by your use of TheadSafeShared here. If something is ThreadSafeShared and owns any Strings, for example, you must be sure to only return copies to anything that even might take a ref to them for example. There's just a lot of ways to mess this stuff up. Is there any way to simplify the design so that objects can be created/refed/derefed on the same thread for simplicity? Maybe objects can be split up based on what thread they operate on? I know we already have a bunch of stuff on database that exists on multiple threads, but I'd really like to see us find ways to simplify this stuff. WebCore/WTF is just not designed very well for threadsafe programming.

Comment on attachment 54213 [details] patch #1: stubs for storage classes good to see this coming in.

Hey, the fancy review didn't take my comments :(

Comment on attachment 54213 [details] patch #1: stubs for storage classes Since Jerermy's reviewing and has concerns, I'll leave it to him.

I read Jeremy's review - which shows a lot of concern for thread safety and makes suggestions for refinement, then I see Dimitri r+ without further comment on the matter or Dumitri's response. I share Jeremy's concerns...

(In reply to comment #9) > (From update of attachment 54213 [details]) > Since Jerermy's reviewing and has concerns, I'll leave it to him. Lol. I wonder why it didn't give me a mid-air collision on my comment. Thanks.

I think the mid-air collision thing is broken! It didn't warn me about Jeremy's comment either.

I think we can get away with accessing the DatabaseSync and SQLTransactionSyncCallback objects only on the context thread. However, SQLTransactionSync needs to be created on the main thread (when DatabaseSync::transaction() called), and needs to be able to run stuff on the DB thread. Would you be OK with making DatabaseSync and SQLTransactionSyncCallback not ThreadSafeShared, but leave SQLTransactionSync as it is? Also, no matter what we do, I plan to add ASSERTs to almost every single method of these classes to make sure they're called on the correct thread, when I add the actual implementations.

(In reply to comment #13) > I think we can get away with accessing the DatabaseSync and > SQLTransactionSyncCallback objects only on the context thread. However, > SQLTransactionSync needs to be created on the main thread (when > DatabaseSync::transaction() called), and needs to be able to run stuff on the > DB thread. > > Would you be OK with making DatabaseSync and SQLTransactionSyncCallback not > ThreadSafeShared, but leave SQLTransactionSync as it is? Is it possible for it to be created on the main thread and then be passed off to the background thread (which would hold all references to it)? If so, then even that could be just RefCounted. If not, then you'll need to make sure that it gives up any refs it might hold to objects on the DB thread only on the DB thread (or ensure the DB thread is dead before giving up such a ref).

(In reply to comment #14) > Is it possible for it to be created on the main thread and then be passed off > to the background thread (which would hold all references to it)? If so, then > even that could be just RefCounted. I don't think this is possible. We need to run the transaction callback on the main/context thread, so calls to SQLTransactionSync::executeSQL() will happen on the context thread. > If not, then you'll need to make sure that it gives up any refs it might hold > to objects on the DB thread only on the DB thread (or ensure the DB thread is > dead before giving up such a ref). That's the plan.

Created attachment 54331 [details] patch #1: stubs for storage classes

Attachment 54331 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/storage/SQLTransactionSync.cpp:38: Alphabetical sorting problem. [build/include_order] [4] WebCore/WebCore.vcproj/WebCore.vcproj:42850: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. Total errors found: 21 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #17) > Attachment 54331 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" > exit_code: 1 > WebCore/storage/SQLTransactionSync.cpp:38: Alphabetical sorting problem. > [build/include_order] [4] Will fix in the next upload or on landing. > WebCore/WebCore.vcproj/WebCore.vcproj:42850: One or more unexpected \r (^M) > found; better to use only a \n [whitespace/carriage_return] [1] > Suppressing further [whitespace/carriage_return] reports for this file. > Total errors found: 21 in 11 files This is a Windows file, it's supposed to have the ^Ms.

Created attachment 54371 [details] patch #1: stubs for storage classes After talking to Michael and Eric about this, we decided that it would be best have a single-threaded sync API implementation (everything's done on the worker's context thread). So I made small changes to the patch to account for that. In case you wonder why we don't want to use the DB thread: -- no benefit: the main thread cannot do anything while the task is processed on the DB thread. -- simpler code: no locks, thread-safe classes, tasks, thread switching, etc. -- faster: no need to wait for the DB thread to run the async DB tasks that are in the queue before getting to the sync task. -- fewer changes to the existing code: no need to modify DatabaseThread, for example, to support both types of databases.

Attachment 54371 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/WebCore.vcproj/WebCore.vcproj:42850: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. Total errors found: 20 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 54371 [details] patch #1: stubs for storage classes > Index: WebCore/storage/DatabaseSync.cpp > =================================================================== > --- WebCore/storage/DatabaseSync.cpp (revision 0) > +++ WebCore/storage/DatabaseSync.cpp (revision 0) > @@ -0,0 +1,120 @@ > +/* > + * Copyright (C) 2010 Google Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > + * its contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED > + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE > + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY > + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES > + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND > + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF > + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#include "config.h" > +#include "DatabaseSync.h" > + > +#include <wtf/StdLibExtras.h> Move this inside the block below. > + > +#if ENABLE(DATABASE) > +#include "DatabaseCallback.h" > +#include "ExceptionCode.h" > +#include "SQLTransactionSyncCallback.h" > +#include "ScriptExecutionContext.h" > +#include <wtf/PassRefPtr.h> > +#include <wtf/RefPtr.h> > + > +#endif // ENABLE(DATABASE) This is very confusing and ugly. Please find a better way. Ideally by not writing code that requires this function when database is not enabled. > + > +namespace WebCore { > + > +const String& DatabaseSync::databaseInfoTableName() > +{ > + DEFINE_STATIC_LOCAL(String, name, ("__WebKitDatabaseInfoTable__")); > + return name; > +} > + > +#if ENABLE(DATABASE) > + > +static bool isSyncDatabaseAvailable = true; > + > +void DatabaseSync::setIsAvailable(bool available) > +{ > + isSyncDatabaseAvailable = available; > +} > + > +bool DatabaseSync::isAvailable() > +{ > + return isSyncDatabaseAvailable; > +} > + > +PassRefPtr<DatabaseSync> DatabaseSync::openDatabaseSync( > + ScriptExecutionContext* context, const String& /*name*/, const String& /*expectedVersion*/, > + const String& /*displayName*/, unsigned long /*estimatedSize*/, > + PassRefPtr<DatabaseCallback> /*creationCallback*/, ExceptionCode& e) Is there any precedent for splitting up a line like this? I can think of cases like below, but not like this. Unless there's precent, please make this 1 or 2 lines, and if 2, align on the (. > +{ > + ASSERT(context->isContextThread()); > + > + e = SECURITY_ERR; > + return 0; > +} > + > +DatabaseSync::DatabaseSync(ScriptExecutionContext* context, const String& name, > + const String& expectedVersion, const String& displayName, > + unsigned long estimatedSize, PassRefPtr<DatabaseCallback> creationCallback) This could be done in 2 (if not 1) line(s). > + : m_scriptExecutionContext(context) > + , m_name(name.crossThreadString()) > + , m_expectedVersion(expectedVersion.crossThreadString()) > + , m_displayName(displayName.crossThreadString()) > + , m_estimatedSize(estimatedSize) > + , m_creationCallback(creationCallback) > +{ > + ASSERT(context); Not needed. You'd segfault on the next line anyway. Should you verify you got a callback tho? > + ASSERT(context->isContextThread()); > +} > + > +DatabaseSync::~DatabaseSync() > +{ > + ASSERT(m_scriptExecutionContext->isContextThread()); > +} > + > +String DatabaseSync::version() const > +{ > + ASSERT(m_scriptExecutionContext->isContextThread()); > + return String(); > +} > + > +void DatabaseSync::changeVersion(const String& /*oldVersion*/, const String& /*newVersion*/, PassRefPtr<SQLTransactionSyncCallback> /*callback*/) > +{ > + ASSERT(m_scriptExecutionContext->isContextThread()); > +} > + > +void DatabaseSync::transaction(PassRefPtr<SQLTransactionSyncCallback> /*callback*/, bool /*readOnly*/) > +{ > + ASSERT(m_scriptExecutionContext->isContextThread()); > +} > + > +ScriptExecutionContext* DatabaseSync::scriptExecutionContext() const > +{ > + ASSERT(m_scriptExecutionContext->isContextThread()); > + return m_scriptExecutionContext.get(); > +} > + > +#endif // ENABLE(DATABASE) > + > +} // namespace WebCore > Index: WebCore/storage/DatabaseSync.h > =================================================================== > --- WebCore/storage/DatabaseSync.h (revision 0) > +++ WebCore/storage/DatabaseSync.h (revision 0) > @@ -0,0 +1,97 @@ > +/* > + * Copyright (C) 2010 Google Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > + * its contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED > + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE > + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY > + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES > + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND > + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF > + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#ifndef DatabaseSync_h > +#define DatabaseSync_h > + > +#include "PlatformString.h" Move inside the guard. > + > +#if ENABLE(DATABASE) > +#include <wtf/Forward.h> > + > +namespace WebCore { > + > +class DatabaseCallback; > +class ScriptExecutionContext; > +class SQLTransactionSyncCallback; > + > +typedef int ExceptionCode; > + > +// Instances of this class should be created and used only on the worker's context thread. > +class DatabaseSync : public RefCounted<DatabaseSync> { > +public: > + static void setIsAvailable(bool); > + static bool isAvailable(); > + > + ~DatabaseSync(); > + > +// Direct support for the DOM API Tab these comments in. > + static PassRefPtr<DatabaseSync> openDatabaseSync( > + ScriptExecutionContext* context, const String& name, const String& expectedVersion, ScriptExecutionContext* doesn't need a name here > + const String& displayName, unsigned long estimatedSize, > + PassRefPtr<DatabaseCallback> creationCallback, ExceptionCode&); This doesn't need to be split across so many lines. > + String version() const; > + void changeVersion(const String& oldVersion, const String& newVersion, PassRefPtr<SQLTransactionSyncCallback> callback); Get rid of 'callback'. > + void transaction(PassRefPtr<SQLTransactionSyncCallback> callback, bool readOnly); ditto > + > +// Internal engine support > + ScriptExecutionContext* scriptExecutionContext() const; > + > + static const String& databaseInfoTableName(); > + > +private: > + DatabaseSync(ScriptExecutionContext* context, const String& name, get rid of context. > + const String& expectedVersion, const String& displayName, > + unsigned long estimatedSize, PassRefPtr<DatabaseCallback> creationCallback); Probably can be split across less lines. > + > + RefPtr<ScriptExecutionContext> m_scriptExecutionContext; > + String m_name; > + String m_expectedVersion; > + String m_displayName; > + unsigned long m_estimatedSize; > + RefPtr<DatabaseCallback> m_creationCallback; > + > +#ifndef NDEBUG > + String databaseDebugName() const { return String(); } > +#endif Move this above the variable names. > +}; > + > +} // namespace WebCore > + > +#else > + > +namespace WebCore { > +class DatabaseSync : public RefCounted<DatabaseSync> { > +public: > + static const String& databaseInfoTableName(); > +}; > +} // namespace WebCore I don't like this. See earlier comments. > + > +#endif // ENABLE(DATABASE) > + > +#endif // DatabaseSync_h > Index: WebCore/storage/SQLTransactionSync.cpp > =================================================================== > --- WebCore/storage/SQLTransactionSync.cpp (revision 0) > +++ WebCore/storage/SQLTransactionSync.cpp (revision 0) > @@ -0,0 +1,73 @@ > +/* > + * Copyright (C) 2010 Google Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > + * its contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED > + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE > + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY > + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES > + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND > + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF > + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#include "config.h" > +#include "SQLTransactionSync.h" > + > +#if ENABLE(DATABASE) > + > +#include "DatabaseSync.h" > +#include "ExceptionCode.h" > +#include "PlatformString.h" > +#include "SQLResultSet.h" > +#include "SQLTransactionSyncCallback.h" > +#include "SQLValue.h" > +#include "ScriptExecutionContext.h" Don't we do alphabetical order, not ASCII order? > +#include <wtf/PassRefPtr.h> > +#include <wtf/RefPtr.h> > + > +namespace WebCore { > + > +PassRefPtr<SQLTransactionSync> SQLTransactionSync::create(DatabaseSync* db, PassRefPtr<SQLTransactionSyncCallback> callback, bool readOnly) > +{ > + return adoptRef(new SQLTransactionSync(db, callback, readOnly)); > +} > + > +SQLTransactionSync::SQLTransactionSync(DatabaseSync* db, PassRefPtr<SQLTransactionSyncCallback> callback, bool readOnly) > + : m_database(db) > + , m_callback(callback) > + , m_readOnly(readOnly) > +{ > + ASSERT(m_database); Not really needed since you'd segfault on the next line anyway. > + ASSERT(m_database->scriptExecutionContext()->isContextThread()); Is it worth asserting the callback as well? > +} > + > +SQLTransactionSync::~SQLTransactionSync() > +{ > + ASSERT(m_database->scriptExecutionContext()->isContextThread()); > +} > + > +PassRefPtr<SQLResultSet> SQLTransactionSync::executeSQL(const String& /*sqlStatement*/, const Vector<SQLValue>& /*arguments*/, ExceptionCode& /*e*/) the /*arguments*/ and /*e*/ are not needed. sqlStatement is fine as long as there's precedent for something like that (style wise) elsewhere in the code. (I can't think of any, but there must be some convention for something like this.) > +{ > + ASSERT(m_database->scriptExecutionContext()->isContextThread()); > + return 0; > +} > + > +} // namespace WebCore > + > +#endif // ENABLE(DATABASE) > Index: WebCore/storage/SQLTransactionSync.h > =================================================================== > --- WebCore/storage/SQLTransactionSync.h (revision 0) > +++ WebCore/storage/SQLTransactionSync.h (revision 0) > @@ -0,0 +1,71 @@ > +/* > + * Copyright (C) 2010 Google Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > + * its contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY > + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED > + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE > + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY > + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES > + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; > + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND > + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF > + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > +#ifndef SQLTransactionSync_h > +#define SQLTransactionSync_h > + > +#if ENABLE(DATABASE) > + > +#include <wtf/Forward.h> > +#include <wtf/RefCounted.h> > +#include <wtf/Vector.h> > + > +namespace WebCore { > + > +typedef int ExceptionCode; > + > +class DatabaseSync; > +class SQLResultSet; > +class SQLTransactionSyncCallback; > +class SQLValue; > +class String; > + > +// Instances of this class should be created and used only on the worker's context thread. > +class SQLTransactionSync : public RefCounted<SQLTransactionSync> { > +public: > + static PassRefPtr<SQLTransactionSync> create(DatabaseSync*, PassRefPtr<SQLTransactionSyncCallback>, bool readOnly = false); > + > + ~SQLTransactionSync(); > + > + PassRefPtr<SQLResultSet> executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, ExceptionCode& e); delete 'e' > + > + DatabaseSync* database() { return m_database.get(); } > + bool isReadOnly() { return m_readOnly; } const

Created attachment 54485 [details] patch #1: stubs for storage classes (In reply to comment #21) > > Index: WebCore/storage/DatabaseSync.cpp > > =================================================================== > > +#include "config.h" > > +#include "DatabaseSync.h" > > + > > +#include <wtf/StdLibExtras.h> > > Move this inside the block below. done. > > +#if ENABLE(DATABASE) > > +#include "DatabaseCallback.h" > > +#include "ExceptionCode.h" > > +#include "SQLTransactionSyncCallback.h" > > +#include "ScriptExecutionContext.h" > > +#include <wtf/PassRefPtr.h> > > +#include <wtf/RefPtr.h> > > + > > +#endif // ENABLE(DATABASE) > > This is very confusing and ugly. Please find a better way. Ideally by not > writing code that requires this function when database is not enabled. i copy-pasted this function/style from Database.{h|cpp}. Removed this function for now; we can add it later if needed. > > +PassRefPtr<DatabaseSync> DatabaseSync::openDatabaseSync( > > + ScriptExecutionContext* context, const String& /*name*/, const String& /*expectedVersion*/, > > + const String& /*displayName*/, unsigned long /*estimatedSize*/, > > + PassRefPtr<DatabaseCallback> /*creationCallback*/, ExceptionCode& e) > > Is there any precedent for splitting up a line like this? I can think of cases > like below, but not like this. Unless there's precent, please make this 1 or 2 > lines, and if 2, align on the (. done... VERY reluctantly... i don't know if there's any precedent for splitting lines like this, and i know that no line is too long for WebKit. however, i believe that the intent of this rule is to save developers' time by not making them count the exact number of characters on every line, not to encourage 188-char lines just to keep the list of all parameters on <= 2 lines. what's wrong with 3+ shorter, easier to read lines of parameters? > > +{ > > + ASSERT(context->isContextThread()); > > + > > + e = SECURITY_ERR; > > + return 0; > > +} > > + > > +DatabaseSync::DatabaseSync(ScriptExecutionContext* context, const String& name, > > + const String& expectedVersion, const String& displayName, > > + unsigned long estimatedSize, PassRefPtr<DatabaseCallback> creationCallback) > > This could be done in 2 (if not 1) line(s). changed to 2 lines (but have the same objection). > > + : m_scriptExecutionContext(context) > > + , m_name(name.crossThreadString()) > > + , m_expectedVersion(expectedVersion.crossThreadString()) > > + , m_displayName(displayName.crossThreadString()) > > + , m_estimatedSize(estimatedSize) > > + , m_creationCallback(creationCallback) > > +{ > > + ASSERT(context); > > Not needed. You'd segfault on the next line anyway. removed. > Should you verify you got a callback tho? the callback is optional. > > Index: WebCore/storage/DatabaseSync.h > > =================================================================== > > +#ifndef DatabaseSync_h > > +#define DatabaseSync_h > > + > > +#include "PlatformString.h" > > Move inside the guard. done. > > +class DatabaseSync : public RefCounted<DatabaseSync> { > > +public: > > + static void setIsAvailable(bool); > > + static bool isAvailable(); > > + > > + ~DatabaseSync(); > > + > > +// Direct support for the DOM API > > Tab these comments in. done. and fixed this in Database.h too. > > + static PassRefPtr<DatabaseSync> openDatabaseSync( > > + ScriptExecutionContext* context, const String& name, const String& expectedVersion, > > ScriptExecutionContext* doesn't need a name here removed here and everywhere else. > > + const String& displayName, unsigned long estimatedSize, > > + PassRefPtr<DatabaseCallback> creationCallback, ExceptionCode&); > > This doesn't need to be split across so many lines. changed to 2-lines. again, same objection. > > + String version() const; > > + void changeVersion(const String& oldVersion, const String& newVersion, PassRefPtr<SQLTransactionSyncCallback> callback); > > Get rid of 'callback'. removed here and everywhere else. > > + void transaction(PassRefPtr<SQLTransactionSyncCallback> callback, bool readOnly); > > ditto done. > > +private: > > + DatabaseSync(ScriptExecutionContext* context, const String& name, > > get rid of context. done. > > + const String& expectedVersion, const String& displayName, > > + unsigned long estimatedSize, PassRefPtr<DatabaseCallback> creationCallback); > > Probably can be split across less lines. done... same objection. > > + RefPtr<ScriptExecutionContext> m_scriptExecutionContext; > > + String m_name; > > + String m_expectedVersion; > > + String m_displayName; > > + unsigned long m_estimatedSize; > > + RefPtr<DatabaseCallback> m_creationCallback; > > + > > +#ifndef NDEBUG > > + String databaseDebugName() const { return String(); } > > +#endif > > Move this above the variable names. not done. this location for this method is consistent with Database.h. > > +#else > > + > > +namespace WebCore { > > +class DatabaseSync : public RefCounted<DatabaseSync> { > > +public: > > + static const String& databaseInfoTableName(); > > +}; > > +} // namespace WebCore > > I don't like this. See earlier comments. removed. we can add this later if needed. i was just trying to stay as close as possible to Database.h. > > Index: WebCore/storage/SQLTransactionSync.cpp > > =================================================================== > > +#include "DatabaseSync.h" > > +#include "ExceptionCode.h" > > +#include "PlatformString.h" > > +#include "SQLResultSet.h" > > +#include "SQLTransactionSyncCallback.h" > > +#include "SQLValue.h" > > +#include "ScriptExecutionContext.h" > > Don't we do alphabetical order, not ASCII order? that's what i thought too, but check-webkit-style disagreed. > > +SQLTransactionSync::SQLTransactionSync(DatabaseSync* db, PassRefPtr<SQLTransactionSyncCallback> callback, bool readOnly) > > + : m_database(db) > > + , m_callback(callback) > > + , m_readOnly(readOnly) > > +{ > > + ASSERT(m_database); > > Not really needed since you'd segfault on the next line anyway. removed. > > + ASSERT(m_database->scriptExecutionContext()->isContextThread()); > > Is it worth asserting the callback as well? added. > > +PassRefPtr<SQLResultSet> SQLTransactionSync::executeSQL(const String& /*sqlStatement*/, const Vector<SQLValue>& /*arguments*/, ExceptionCode& /*e*/) > > the /*arguments*/ and /*e*/ are not needed. sqlStatement is fine as long as > there's precedent for something like that (style wise) elsewhere in the code. > (I can't think of any, but there must be some convention for something like > this.) removed for now, even though these arguments will be needed once i get to implementing this method. > > Index: WebCore/storage/SQLTransactionSync.h > > =================================================================== > > + PassRefPtr<SQLResultSet> executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, ExceptionCode& e); > > delete 'e' done. > > + bool isReadOnly() { return m_readOnly; } > > const done.

Attachment 54485 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/WebCore.vcproj/WebCore.vcproj:42850: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. Total errors found: 20 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

+ eric for the final question about check-webkit-style > > > +PassRefPtr<DatabaseSync> DatabaseSync::openDatabaseSync( > > > + ScriptExecutionContext* context, const String& /*name*/, const String& /*expectedVersion*/, > > > + const String& /*displayName*/, unsigned long /*estimatedSize*/, > > > + PassRefPtr<DatabaseCallback> /*creationCallback*/, ExceptionCode& e) > > > > Is there any precedent for splitting up a line like this? I can think of cases > > like below, but not like this. Unless there's precent, please make this 1 or 2 > > lines, and if 2, align on the (. > > done... VERY reluctantly... i don't know if there's any precedent for splitting > lines like this, and i know that no line is too long for WebKit. however, i > believe that the intent of this rule is to save developers' time by not making > them count the exact number of characters on every line, not to encourage > 188-char lines just to keep the list of all parameters on <= 2 lines. what's > wrong with 3+ shorter, easier to read lines of parameters? I think some reviewers would have asked for it all to be on 1, but that would have been a really long line, so I figured 2 was kind of a compromise. I don't like super long lines either, but a style guide is about consistency, not appealing to your (or my) sense of aesthetics. > > Don't we do alphabetical order, not ASCII order? > > that's what i thought too, but check-webkit-style disagreed. Really? Well, either all instances of SQLblah followed by SecurBlah need to be changed or we need to fix check-webkit-style.

Chris and Hamaji are the father's of check-webkit-style. We should make it match our style guidelines, whatever those are in this case. :)

+ Chris And Hamaji for the check-webkit-style question I don't see anything in http://webkit.org/coding/coding-style.html Requiring ASCII order just seems a little weird, but I guess I'm OK either way.

(In reply to comment #26) > + Chris And Hamaji for the check-webkit-style question > > I don't see anything in http://webkit.org/coding/coding-style.html Requiring > ASCII order just seems a little weird, but I guess I'm OK either way. http://webkit.org/coding/coding-style.html I think we have the rule for this? "3. Other #include statements should be in sorted order (case sensitive, as done by the command-line sort tool or the Xcode sort selection command). Don't bother to organize them in a logical order."

(In reply to comment #27) > (In reply to comment #26) > > + Chris And Hamaji for the check-webkit-style question > > > > I don't see anything in http://webkit.org/coding/coding-style.html Requiring > > ASCII order just seems a little weird, but I guess I'm OK either way. > > http://webkit.org/coding/coding-style.html > > I think we have the rule for this? > > "3. Other #include statements should be in sorted order (case sensitive, as > done by the command-line sort tool or the Xcode sort selection command). Don't > bother to organize them in a logical order." You're right. Dumi, please change all the orderings (including class declarations) over to ASCII order.

(In reply to comment #28) > Dumi, please change all the orderings (including class declarations) over to > ASCII order. All includes are in the ASCII order. check-webkit-style is happy with them.

Comment on attachment 54485 [details] patch #1: stubs for storage classes WebCore/storage/DatabaseSync.h:39 + class ScriptExecutionContext; not ascii order...I know it's not in the coding style, but might as well match #includes r=me

(In reply to comment #30) > (From update of attachment 54485 [details]) > WebCore/storage/DatabaseSync.h:39 > + class ScriptExecutionContext; > not ascii order...I know it's not in the coding style, but might as well match > #includes Oops, sorry, missed this one. Will fix on landing.

patch #1 landed as r58437. JSC and V8 bindings are coming next.

Created attachment 54661 [details] patch #2: IDL files + JSC bindings

Attachment 54661 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/bindings/js/JSWorkerContextCustom.cpp:34: Alphabetical sorting problem. [build/include_order] [4] WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 54661 [details] patch #2: IDL files + JSC bindings WebCore/bindings/js/JSCustomSQLTransactionSyncCallback.cpp:30 + #include "JSCustomSQLTransactionSyncCallback.h" I think this file is misnamed. We usually put the custom at the end of the file name. WebCore/bindings/js/JSCustomSQLTransactionSyncCallback.cpp:54 + void JSCustomSQLTransactionSyncCallback::handleEvent(ScriptExecutionContext* context, SQLTransactionSync* transaction, bool& raisedException) This code is all copy/paste from somewhere else. We need to abstract this code and not have duplication. WebCore/bindings/js/JSDatabaseSyncCustom.cpp:48 + String newVersion = ustringToString(args.at(1).toString(exec)); What about exceptions generated here? WebCore/bindings/js/JSDatabaseSyncCustom.cpp:45 + JSValue JSDatabaseSync::changeVersion(ExecState* exec, const ArgList& args) It's lame that we can't autogenerate this code, but I'll work on that in a separate bug. WebCore/bindings/js/JSDatabaseSyncCustom.cpp:51 + if (!(object = args.at(2).getObject())) { Can we do the assignment and the test separate statements? This is hard to read. WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:47 + setDOMException(exec, SYNTAX_ERR); Are you sure this is the right exception to throw here? WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:76 + if (value.isNull()) Why Null and not Undefined? WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:78 + else if (value.isNumber()) Do we need to call valueOf? WebCore/bindings/js/JSWorkerContextCustom.cpp:152 + if (args.size() < 4) Why do we need check the args.size() here? WebCore/bindings/js/JSWorkerContextCustom.cpp:159 + unsigned long estimatedSize = args.at(3).toInt32(exec); Exception handling? WebCore/bindings/js/JSWorkerContextCustom.cpp:150 + JSValue JSWorkerContext::openDatabaseSync(ExecState* exec, const ArgList& args) It's lame that we can't autogenerate this code. It's not doing anything complicated. Where's the V8 implementation of the custom bindings?

- People I added for the style questions.

(In reply to comment #35) > (From update of attachment 54661 [details]) > WebCore/bindings/js/JSCustomSQLTransactionSyncCallback.cpp:30 > + #include "JSCustomSQLTransactionSyncCallback.h" > I think this file is misnamed. We usually put the custom at the end of the > file name. Custom comes first when it's 100% custom and last when it's custom components of something partially generated. I believe his use of custom is correct here.

(In reply to comment #37) > (In reply to comment #35) > > (From update of attachment 54661 [details] [details]) > > WebCore/bindings/js/JSCustomSQLTransactionSyncCallback.cpp:30 > > + #include "JSCustomSQLTransactionSyncCallback.h" > > I think this file is misnamed. We usually put the custom at the end of the > > file name. > > Custom comes first when it's 100% custom and last when it's custom components > of something partially generated. I believe his use of custom is correct here. Yep.

> Custom comes first when it's 100% custom and last when it's custom components > of something partially generated. I believe his use of custom is correct here. Ah. I see there are other examples of this naming pattern.

Created attachment 55104 [details] patch #2: IDL files + JSC and V8 bindings tested the patch on webkit/win, webkit/mac and chromium/win. (In reply to comment #35) > WebCore/bindings/js/JSCustomSQLTransactionSyncCallback.cpp:30 > + #include "JSCustomSQLTransactionSyncCallback.h" > I think this file is misnamed. We usually put the custom at the end of the > file name. all these callbacks are auto-generated now. > WebCore/bindings/js/JSCustomSQLTransactionSyncCallback.cpp:54 > + void > JSCustomSQLTransactionSyncCallback::handleEvent(ScriptExecutionContext* > context, SQLTransactionSync* transaction, bool& raisedException) > This code is all copy/paste from somewhere else. We need to abstract this code > and not have duplication. done. this code is auto-generated. > WebCore/bindings/js/JSDatabaseSyncCustom.cpp:48 > + String newVersion = ustringToString(args.at(1).toString(exec)); > What about exceptions generated here? done. > WebCore/bindings/js/JSDatabaseSyncCustom.cpp:45 > + JSValue JSDatabaseSync::changeVersion(ExecState* exec, const ArgList& args) > It's lame that we can't autogenerate this code, but I'll work on that in a > separate bug. this is on my to-do list too. > WebCore/bindings/js/JSDatabaseSyncCustom.cpp:51 > + if (!(object = args.at(2).getObject())) { > Can we do the assignment and the test separate statements? This is hard to > read. fixed in all DB bindings. > WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:47 > + setDOMException(exec, SYNTAX_ERR); > Are you sure this is the right exception to throw here? yes, executeSQL() takes at least one mandatory argument. > WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:76 > + if (value.isNull()) > Why Null and not Undefined? changed to isUndefinedOrNull(). > WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:78 > + else if (value.isNumber()) > Do we need to call valueOf? i don't think so. isNumber() makes sure it's a number, and uncheckedGetNumber() gets it. > WebCore/bindings/js/JSWorkerContextCustom.cpp:152 > + if (args.size() < 4) > Why do we need check the args.size() here? yes, openDatabaseSync() has 4 mandatory arguments. > WebCore/bindings/js/JSWorkerContextCustom.cpp:159 > + unsigned long estimatedSize = args.at(3).toInt32(exec); > Exception handling? done. > WebCore/bindings/js/JSWorkerContextCustom.cpp:150 > + JSValue JSWorkerContext::openDatabaseSync(ExecState* exec, const ArgList& > args) > It's lame that we can't autogenerate this code. It's not doing anything > complicated. agree, will work on this in parallel to implementing the rest of the sync DB API. > Where's the V8 implementation of the custom bindings? added.

Attachment 55104 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:52: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/bindings/v8/custom/V8DatabaseSyncCustom.cpp:39: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 55104 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/2141036

Created attachment 55105 [details] patch #2: IDL files + JSC and V8 bindings Same patch, should make the GTK bot happy and the style bot almost happy.

Attachment 55105 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:52: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 55105 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/2153035

Created attachment 55110 [details] patch #2: IDL files + JSC and V8 bindings Copy-paste error in the GNUMakefile.am file. One more try to make the GTK bot happy.

Attachment 55110 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:52: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 55110 [details] patch #2: IDL files + JSC and V8 bindings No ChangeLog. No tests! There's tons of stuff you should be testing here. WebCore/bindings/js/JSDOMWindowCustom.cpp:977 + setDOMException(exec, TYPE_MISMATCH_ERR); Wrong indent WebCore/bindings/js/JSDatabaseSyncCustom.cpp:65 + RefPtr<SQLTransactionSyncCallback> callback(JSSQLTransactionSyncCallback::create(object, static_cast<JSDOMGlobalObject*>(exec->dynamicGlobalObject()))); Why the dynamic global object? WebCore/bindings/js/JSDatabaseSyncCustom.cpp:93 + return createTransaction(exec, args, m_impl.get(), static_cast<JSDOMGlobalObject*>(exec->dynamicGlobalObject()), false); It seems more likely that we want the global object associated with the JSDatabaseSync object. WebCore/bindings/js/JSWorkerContextCustom.cpp:164 + const UString& name = args.at(0).toString(exec); Exceptions? WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:722 + return throwError("openDatabaseSync() must be called with at least 4 parameters.", V8Proxy::SyntaxError); These exception messages aren't localized. Also, they differ from the JSC versions. I think we should just throw without a message. WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:157 + String name = toWebCoreString(args[0]); Again, no exception handling.

Created attachment 55292 [details] patch #2: IDL files + JSC and V8 bindings (In reply to comment #48) > No ChangeLog. oops, sorry about that. fixed. > No tests! There's tons of stuff you should be testing here. added a test to test the parameters passed to openDatabaseSync(). i'm not sure what else we can test at this point. once we have some implementation in place, i'll add a similar test for db.{read}transaction(), as well as more functional tests. > WebCore/bindings/js/JSDOMWindowCustom.cpp:977 > + setDOMException(exec, TYPE_MISMATCH_ERR); > Wrong indent fixed. > WebCore/bindings/js/JSDatabaseSyncCustom.cpp:65 > + RefPtr<SQLTransactionSyncCallback> > callback(JSSQLTransactionSyncCallback::create(object, > static_cast<JSDOMGlobalObject*>(exec->dynamicGlobalObject()))); > Why the dynamic global object? i copy-pasted this code from another class. fixed in all DB bindings. > WebCore/bindings/js/JSDatabaseSyncCustom.cpp:93 > + return createTransaction(exec, args, m_impl.get(), > static_cast<JSDOMGlobalObject*>(exec->dynamicGlobalObject()), false); > It seems more likely that we want the global object associated with the > JSDatabaseSync object. fixed. > WebCore/bindings/js/JSWorkerContextCustom.cpp:164 > + const UString& name = args.at(0).toString(exec); > Exceptions? handled. > WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:722 > + return throwError("openDatabaseSync() must be called with at least > 4 parameters.", V8Proxy::SyntaxError); > These exception messages aren't localized. Also, they differ from the JSC > versions. I think we should just throw without a message. removed the exception messages from all V8 DB bindings. > WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:157 > + String name = toWebCoreString(args[0]); > Again, no exception handling. done.

Attachment 55292 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:52: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebCore/bindings/js/JSWorkerContextCustom.cpp:190: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 in 43 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 55292 [details] patch #2: IDL files + JSC and V8 bindings Sorry for nit picking this patch to death, but writing custom bindings correctly is nigh unto impossible. Mostly r- for the lack of a toString call in the V8 bindings. You can test this with an object that has a toString function that actually returns a string. WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:54 + setDOMException(exec, TYPE_MISMATCH_ERR); It's unclear to me whether its better to magically transmute the exception to TYPE_MISMATCH_ERR or whether it's better to re-throw the same exception. WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:724 + if (!args[0]->IsString() || !args[1]->IsString() || !args[2]->IsString() || !args[3]->IsUint32()) Why don't we call toString() on these args like we do in the JSC version? WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:732 + String name = toWebCoreString(args[0]); I think you want to skip the check above and check for an exception here instead. WebCore/bindings/v8/custom/V8DatabaseSyncCustom.cpp:50 + if (!(args[0]->IsString() && args[1]->IsString())) Same comment here about calling toString() WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:63 + v8::Local<v8::Value> lengthGetter; Is this the getter or the gotten value? WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:80 + v8::TryCatch block; Can we condense this somehow? It's going to be a lot of code everywhere otherwise. WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:89 + sqlValues.append(SQLValue(value->NumberValue())); Does this call valueOf? If so, what if there's an exception? WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:91 + sqlValues.append(SQLValue(toWebCoreString(value))); Exception? WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:150 + if (!args[0]->IsString() || !args[1]->IsString() || !args[2]->IsString() || !args[3]->IsUint32()) String comment. WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:159 + unsigned long estimatedSize = args[3]->Uint32Value(); Does this call valueOf? Should we be calling valueOf? Does this match JSC?

Created attachment 55358 [details] patch #2: IDL files + JSC and V8 bindings (In reply to comment #51) > WebCore/bindings/js/JSSQLTransactionSyncCustom.cpp:54 > + setDOMException(exec, TYPE_MISMATCH_ERR); > It's unclear to me whether its better to magically transmute the exception to > TYPE_MISMATCH_ERR or whether it's better to re-throw the same exception. reverted, keeping the original exception around. > WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:724 > + if (!args[0]->IsString() || !args[1]->IsString() || > !args[2]->IsString() || !args[3]->IsUint32()) > Why don't we call toString() on these args like we do in the JSC version? i believe toString() is automatically called when needed. > WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:732 > + String name = toWebCoreString(args[0]); > I think you want to skip the check above and check for an exception here > instead. done here and everywhere else. > WebCore/bindings/v8/custom/V8DatabaseSyncCustom.cpp:50 > + if (!(args[0]->IsString() && args[1]->IsString())) > Same comment here about calling toString() done. > WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:63 > + v8::Local<v8::Value> lengthGetter; > Is this the getter or the gotten value? changed to 'length'. > WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:80 > + v8::TryCatch block; > Can we condense this somehow? It's going to be a lot of code everywhere > otherwise. added a macro to V8BindingMacros.h. > WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:89 > + sqlValues.append(SQLValue(value->NumberValue())); > Does this call valueOf? If so, what if there's an exception? wrapped in an EXCEPTION_BLOCK. > WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:91 > + sqlValues.append(SQLValue(toWebCoreString(value))); > Exception? handled. > WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:150 > + if (!args[0]->IsString() || !args[1]->IsString() || > !args[2]->IsString() || !args[3]->IsUint32()) > String comment. > > WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp:159 > + unsigned long estimatedSize = args[3]->Uint32Value(); > Does this call valueOf? Should we be calling valueOf? Does this match JSC? potential exceptions should be handled. JSC was changed to return a uint32 too.

Attachment 55358 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/bindings/v8/custom/V8BindingMacros.h:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. Total errors found: 38 in 44 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #53) > WebCore/bindings/v8/custom/V8BindingMacros.h:1: One or more unexpected \r (^M) > found; better to use only a \n [whitespace/carriage_return] [1] > Suppressing further [whitespace/carriage_return] reports for this file. > Total errors found: 38 in 44 files i guess i didn't fix this before uploading the patch... fixed now.

Comment on attachment 55358 [details] patch #2: IDL files + JSC and V8 bindings There are still a few more bugs, but this is vastly better than the rest of the custom bindings. Thanks so much for iterating on this patch. Please address the comments below before landing. WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:725 + EXCEPTION_BLOCK(String, name, toWebCoreString(args[0])); Nice. WebCore/bindings/v8/custom/V8DatabaseSyncCustom.cpp:52 + EXCEPTION_BLOCK(String, newVersion, toWebCoreString(args[0])); Are these both supposed to be args[0]? WebCore/bindings/v8/custom/V8DatabaseSyncCustom.cpp:65 + database->changeVersion(toWebCoreString(args[0]), toWebCoreString(args[1]), callback.release(), ec); We probably shouldn't call toString again. Why not use oldVersion and newVersion above? WebCore/bindings/v8/custom/V8SQLTransactionCustom.cpp:72 + sqlArgsLength = length->Uint32Value(); This doesn't call valueOf, right? WebCore/bindings/v8/custom/V8SQLTransactionCustom.cpp:78 + v8::TryCatch block; Why not use our fancy macro here?

Comment on attachment 55358 [details] patch #2: IDL files + JSC and V8 bindings I'd actually prefer a little more explanation and focus here before it lands (yes, it's much better, and thanks for sticking to it) The empty ChangeLog is not helpful. New files might be self explanatory, but I'd like to see comments on changes to existing code. > - RefPtr<SQLTransactionCallback> callback(JSSQLTransactionCallback::create(object, static_cast<JSDOMGlobalObject*>(exec->dynamicGlobalObject()))); > - > + > + RefPtr<SQLTransactionCallback> callback(JSSQLTransactionCallback::create(object, static_cast<JSDOMGlobalObject*>(globalObject()))); This replacement is done a lot in this patch. Why this change? There's no comment in the ChangeLog explaining why it's necessary. If it's just a style cleanup because we have some new helper-y way of doing this, that should be in a separate patch because it is unrelated to this task.

> This replacement is done a lot in this patch. Why this change? There's no > comment in the ChangeLog explaining why it's necessary. Yeah, I sort of pushed dumi into making this into an omnibus "fix the world" patch. Virtually every time you see dynamicGlobalObject in the bindings its a bug. In this case, we need to use the global object of the "this" object to get the prototype chain to lead to the correct place. We've been working on fixing pervasive bug in the bindings for about a year. We've got 90% of it fixed, but the last 10% is taking a long time. > If it's just a style cleanup because we have some new helper-y way of doing > this, that should be in a separate patch because it is unrelated to this task. It's not a style cleanup. The old code is wrong. This patch could be broken up into about six smaller patches if necessary.

(In reply to comment #57) > > If it's just a style cleanup because we have some new helper-y way of doing > > this, that should be in a separate patch because it is unrelated to this task. > > It's not a style cleanup. The old code is wrong. This patch could be broken > up into about six smaller patches if necessary. With those kind of changes intertwined, I'd personally prefer that. Dumi might prefer it as well. It's *a lot* easier to get a few 10+k patches solidified, reviewed, and landed than it is one 90k patch, as he's probably discovered by this point. :)

(In reply to comment #58) > > It's not a style cleanup. The old code is wrong. This patch could be broken > > up into about six smaller patches if necessary. > > With those kind of changes intertwined, I'd personally prefer that. > > Dumi might prefer it as well. It's *a lot* easier to get a few 10+k patches > solidified, reviewed, and landed than it is one 90k patch, as he's probably > discovered by this point. :) 10K patches are impossible when adding a new binding. It requires changes to 9 files: DerivedSources.cpp DerivedSources.make GNUmakefile.am WebCore.gypi WebCore.pri WebCore.pro WebCore.vcproj/WebCore.vcproj WebCore.xcodeproj/project.pbxproj bindings/js/JSBindingsAllInOne.cpp which already adds up to a 20K+ patch. On top of that, you have two copies of the new bindings themselves (JSC and V8). I'll try to break this patch into two: 1. Add the new IDL files and the new bindings, while keeping the existing code as unchanged as possible. 2. Clean up the existing code. Adam: I've addressed your comments. Uint32Value() doesn't seem to call valueOf(), but I'm not 100% sure. What's so special about that method? Does it matter if we have that inside an EXCEPTION_BLOCK?

(In reply to comment #59) > 10K patches are impossible when adding a new binding. I was just pulling 10k out of thin air as an example. I've added new bindings myself - such as all of the original database case. This can easily be split up. > I'll try to break this patch into two: > 1. Add the new IDL files and the new bindings, while keeping the existing code > as unchanged as possible. > 2. Clean up the existing code. You're not being visionary enough ;) When faced with large tasks like this, both myself and many others have followed a pattern of patches similar to the following: 1 - Land all of the new files, and their additions into the various build systems, with empty implementations of the idl interface itself. This is mechanical and non-controversial, and can help shave *many* k of the size of the patch. 2 - Land the actual code that implements the new bindings, making trivial changes to existing core code where needed to keep the build working. 3 - Land the more complex changes to existing core code required to support the new feature along with the layout tests. Even if you decided to combine steps 2 and 3, having step 1 be separate can be a huge help. These are just ideas, and I'm not going to be a hardass and make you do it like that, but I hope they illustrate how things could be broken up. Landing the cleanups to existing code that need to happen, but aren't actually directly related to the task at hand, *should* be a separate patch and I will insist on that.

It's important to distinguish process for the sake of process and process for some useful goal. It sounds like the useful goal you have in mind is higher quality code review. IMHO, this change to the custom bindings is substantially more correct than the rest of the custom bindings. For all the process used in the original custom database code, it's full of the kinds of errors I've been pushing to have fixed in this patch. That said, if you'd be interested in reviewing this change broken into small pieces, I'd also encourage dumi to break up the patch because custom bindings are essentially impossible to write correctly. The more review the better. :-)

Created attachment 55426 [details] patch #2: IDL files + JSC and V8 bindings stubs

(In reply to comment #61) > It's important to distinguish process for the sake of process and process for > some useful goal. It sounds like the useful goal you have in mind is higher > quality code review. Indeed. > IMHO, this change to the custom bindings is substantially > more correct than the rest of the custom bindings. For all the process used in > the original custom database code, it's full of the kinds of errors I've been > pushing to have fixed in this patch. The process has gotten better over time, allowing these errors to be caught now > That said, if you'd be interested in reviewing this change broken into small > pieces, I'd also encourage dumi to break up the patch because custom bindings > are essentially impossible to write correctly. The more review the better. > :-) That is why I'm interested. I'm not trying to enforce some arbitrary process. This is a 90k patch that: - Adds code for a new feature - Changes previous code to support the new feature - Corrects previous code that was incorrect, unrelated to the new feature - Is littered with little style cleanups (such as the many corrections of trailing whitespace) There's a handful of people on the project who are totally capable of giving such patches a effective review. I am admittedly not one of them. And for a patch like this that has been a struggle to iterate over multiple works... I just wish I'd stepped in and made the suggestions sooner

Comment on attachment 55426 [details] patch #2: IDL files + JSC and V8 bindings stubs Dumi, I really appreciate you breaking off this chunk. 50k of non-controversial, mechanical work is much easier for me to review. r+, baring any V8-ey, macro-ey changes Adam requested (which I didn't double check whether they apply here or not)

> The process has gotten better over time, allowing these errors to be caught now At some point, I should go through can correct a wide swath of these errors now that we have the tools to fix them. My plan was to try to get more of the bindings autogenerated so that the task would be smaller. We've made a lot of progress there, but there's still more to do.

(In reply to comment #64) > Dumi, I really appreciate you breaking off this chunk. 50k of > non-controversial, mechanical work is much easier for me to review. No problem. The easier it is for you to review my patches, the sooner I can land them. :) Landed patch #2 as r58989. A bunch of small patches will follow once the bots pick up r58989 and stay green: patch #3: JSC bindings implementation patch #4: V8 bindings implementation patch #5: A test that calls openDatabaseSync() with invalid inputs patch #6: A clean-up of the existing JSC bindings patch #7: A clean-up of the existing V8 bindings All these patches should be independent, except for patch #5 which will be blocked on patches #3 and #4.

Created attachment 55448 [details] patch #3: JSC bindings implementation

Created attachment 55453 [details] patch #4: V8 bindings implementation

Created attachment 55454 [details] patch #5: openDatabaseSync() inputs test

Attachment 55453 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2221054

Created attachment 55455 [details] patch #4: V8 bindings implementation Fixing V8DatabaseSyncCustom.cpp.

Created attachment 55456 [details] patch #4: V8 bindings implementation Forgot to add the implementation for V8WorkerContextCustom::openDatabaseSyncCallback().

Attachment 55456 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2173054

http://trac.webkit.org/changeset/58989 might have broken GTK Linux 32-bit Debug The following changes are on the blame list: http://trac.webkit.org/changeset/58987 http://trac.webkit.org/changeset/58988 http://trac.webkit.org/changeset/58989

Created attachment 55466 [details] patch #3: JSC bindings implementation Forgot to add the JSWorkerContextCustom::openDatabaseSync() implementation.

Created attachment 55467 [details] patch #4: V8 bindings implementation Should make the Chromium bot happy.

Attachment 55466 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/2174056

Created attachment 55469 [details] patch #3: JSC bindings implementation Should make the Mac bot happy.

Created attachment 55470 [details] patch #6: clean up of JSC DB bindings

Created attachment 55471 [details] patch #7: clean up of V8 DB bindings

Attachment 55467 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2197062

Created attachment 55472 [details] patch #4: V8 bindings implementation Typo.

Comment on attachment 55471 [details] patch #7: clean up of V8 DB bindings Thanks. Presumably bradee-oh isn't interested in reviewing V8 bindings changes.

Comment on attachment 55472 [details] patch #4: V8 bindings implementation Thanks! Please consider the comment below before landing. Again, assuming bradee-oh isn't interested in reviewing V8 bindings. WebCore/bindings/v8/custom/V8SQLTransactionSyncCustom.cpp:66 + EXCEPTION_BLOCK(v8::Local<v8::Value>, length, sqlArgsObject->Get(v8::String::New("length"))); This code looks very similar to the code for the async case. Can we call some common routine instead of copy/pasting the code?

(I am not)

patch #7 landed as r59095.

patch #4 landed as r59096. Waiting on Brady's review for patches 3, 5 and 6.

Comment on attachment 55470 [details] patch #6: clean up of JSC DB bindings All 3 reviewed. Thanks for breaking this up to individual commits!

Landed patch #3 as r59113.

Landed patch #6 (+ minor fix to a test, related to these changes) as r59118.

http://trac.webkit.org/changeset/59118 might have broken Qt Linux Release

(In reply to comment #91) > http://trac.webkit.org/changeset/59118 might have broken Qt Linux Release Updating Qt's expectations for a test fixed the problem.

http://trac.webkit.org/changeset/59136 might have broken SnowLeopard Intel Release (Tests)

Landed patch #5 as r59135. All patches are landed, closing bug.

http://trac.webkit.org/changeset/59135 might have broken GTK Linux 32-bit Debug The following changes are on the blame list: http://trac.webkit.org/changeset/59137 http://trac.webkit.org/changeset/59138 http://trac.webkit.org/changeset/59131 http://trac.webkit.org/changeset/59132 http://trac.webkit.org/changeset/59133 http://trac.webkit.org/changeset/59134 http://trac.webkit.org/changeset/59135