[webkit-reviews] review requested: [Bug 34994] Add sync bindings for Worker access to DB : [Attachment 54485] patch #1: stubs for storage classes

[bugzilla-daemon at webkit.org]

Dumitru Daniliuc <dumi at chromium.org> has asked	for review:
Bug 34994: Add sync bindings for Worker access to DB
https://bugs.webkit.org/show_bug.cgi?id=34994

Attachment 54485: patch #1: stubs for storage classes
https://bugs.webkit.org/attachment.cgi?id=54485&action=review

------- Additional Comments from Dumitru Daniliuc <dumi at chromium.org>
(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.