[webkit-reviews] review denied: [Bug 108759] webdatabase: Introduce back-end database classes and a few small fixes : [Attachment 186268] Revised backend classes that are friendlier for chromium.

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 108759: webdatabase: Introduce back-end database classes and a few small
fixes
https://bugs.webkit.org/show_bug.cgi?id=108759

Attachment 186268: Revised backend classes that are friendlier for chromium.
https://bugs.webkit.org/attachment.cgi?id=186268&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=186268&action=review


Most of my comments are about comments.

But I also have very real concerns.  Possibly ones that could be resolved
with... comments.

> Source/WebCore/Modules/webdatabase/Database.cpp:72
> -Database::Database(PassRefPtr<DatabaseContext> databaseContext, const
String& name, const String& expectedVersion, const String& displayName,
unsigned long estimatedSize)
> -    : DatabaseBackend(databaseContext, name, expectedVersion, displayName,
estimatedSize, AsyncDatabase)
> +Database::Database(ScriptExecutionContext* scriptExecutionContext,
PassRefPtr<DatabaseBackendContext> databaseContext,
> +    const String& name, const String& expectedVersion, const String&
displayName, unsigned long estimatedSize)
> +    : DatabaseBase(scriptExecutionContext)
> +    , DatabaseBackendAsync(databaseContext, name, expectedVersion,
displayName, estimatedSize)

Presumably the DatabaseBackendContext here refers to the same
ScriptExecutionContext being passed in.  If that's the case, no need for both
arguments.

> Source/WebCore/Modules/webdatabase/Database.cpp:-77
> -    ScriptController::initializeThreading();

This is the type of mysterious change I'd hope to see an explanation of in the
ChangeLog.  Why was it needed before, and why is it not needed now?

> Source/WebCore/Modules/webdatabase/Database.h:94
> -    Database(PassRefPtr<DatabaseContext>, const String& name, const String&
expectedVersion,
> -		const String& displayName, unsigned long estimatedSize);
> +    Database(ScriptExecutionContext*, PassRefPtr<DatabaseBackendContext>,
const String& name,
> +	   const String& expectedVersion, const String& displayName, unsigned
long estimatedSize);
> +    PassRefPtr<DatabaseBackendAsync> backend();
> +

Same comment about DatabaseBackendContext and ScriptExecutionContext possibly
being duplicate parameters.

> Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp:246
>  DatabaseBackend::~DatabaseBackend()
>  {
> -    ASSERT(!m_opened);
> +    if (opened())
> +	   closeDatabase();
>  }

In WebCore we try to avoid doing much work in destructors.  The ASSERT here was
to help assure the heavyweight work had already been completed.

This change moves the heavyweight work into the destructor, which might be
problematic.

Why is this necessary now when it wasn't before?

> Source/WebCore/Modules/webdatabase/DatabaseContext.cpp:162
> +PassRefPtr<DatabaseBackendContext> DatabaseContext::backend()
> +{
> +    ref();
> +    DatabaseBackendContext* backend;
> +    backend = reinterpret_cast<DatabaseBackendContext*>(this);
> +    RefPtr<DatabaseBackendContext> backendRef = adoptRef(backend);
> +    return backendRef.release();
> +}

The ref() is super mysterious here.  Why is it necessary?  Can lifetime not be
managed another way?

Any time we do manual ref()/deref()s deserves and explanation.

> Source/WebCore/Modules/webdatabase/DatabaseSync.h:79
> +    DatabaseSync(ScriptExecutionContext*,
PassRefPtr<DatabaseBackendContext>, const String& name,
> +	   const String& expectedVersion, const String& displayName, unsigned
long estimatedSize);
> +    PassRefPtr<DatabaseBackendSync> backend();

Same comment about duplicating DatabaseBackendContext and
ScriptExecutionContext

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:111
> -	   return;
> +	   return; // Already open. Nothing more to do.

We don't normally put comments on the same line as code.

It is perfectly acceptable - and expected - to add a {...} block to
conditionals like this to contain the comment on its own line.

That said, I don't think this comment is necessary at all, as it's more of a
"WHAT this code does" comment instead of a "WHY this code does it" comment.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:124
> +    // Open the database unconditionally. If one does not exists already,
open
> +    // a new one.

Not really convinced this comment needs to be here.  I think the paragraphing
was enough to make this code easily readable.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:132
> +
> +    // Ensure that the 'Origins' table exists. Create it if it doesn't exist
yet.

same here.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:140
> +
> +    // Ensure that the 'Databases' table exists. Create it if it doesn't
exist yet.

same here.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:214
>      if (!m_database.isOpen())
> -	   return false;
> +	   return false; // No "tracker database". Hence, no entry for the
database of interest.

This comment feels useful to me.  But it needs to be on its own line, and
therefore the if() needs a {...} block to contain the two lines.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:375
> -    openTrackerDatabase(false);
> +    openTrackerDatabase(false); // Don't create one if it doesn't already
exist.

If this comment is necessary, it needs to be on its own line.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:380
> +    // Setup an SQL query to fetch the databases for the specified origin:
>      SQLiteStatement statement(m_database, "SELECT name FROM Databases where
origin=?;");

Don't think this one is needed.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:390
> +    // Execute the query and fetch each relevant db into the result:
>      int result;
>      while ((result = statement.step()) == SQLResultRow)
>	   resultVector.append(statement.getColumnText(0));

Ditto.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:634
>      if (!originQuotaManager().tracksOrigin(origin))
> -	   return 0;
> +	   return 0; // Still no db's for this origin. Hence, total usage is 0.


Not convinced this one is needed.  If it is, it needs its own line.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:637
> +    // Compute the total disk usage of all db's from the specified origin:
>      return originQuotaManager().diskUsage(origin);

If this is just saying what diskUsage() does, it's not necessary.

If the intention is to point out that diskUsage involves computation and is not
simply an accessor, I think it needs to more explicitly state that.