[webkit-reviews] review denied: [Bug 111805] DatabaseTracker::origins() is missing some databases in a multi-process scenario : [Attachment 192340] the patch.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 111805: DatabaseTracker::origins() is missing some databases in a
multi-process scenario
https://bugs.webkit.org/show_bug.cgi?id=111805

Attachment 192340: the patch.
https://bugs.webkit.org/attachment.cgi?id=192340&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=192340&action=review


> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:260
> +    statement.bindText(1, origin->databaseIdentifier());
> +    int result;
> +    int rows = 0;
> +    while ((result = statement.step()) == SQLResultRow)
> +	   rows++;
> +    ASSERT(rows <= 1);
> +    if (result != SQLResultDone)
> +	   LOG_ERROR("Failed to read in origins from the database.");

Is it an SQL API requirement that we step through each statement result like
this, even though we only care about whether there were any results? This code
looks strange.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:426
> +    size_t capacityIncrement = 50;
> +    originsResult.reserveCapacity(capacityIncrement);
>      while ((result = statement.step()) == SQLResultRow) {
>	   RefPtr<SecurityOrigin> origin =
SecurityOrigin::createFromDatabaseIdentifier(statement.getColumnText(0));
> -	   m_quotaMap->set(origin.get()->isolatedCopy(),
statement.getColumnInt64(1));
> +	   size_t capacity = originsResult.capacity();
> +	   if (capacity == originsResult.size())
> +	       originsResult.reserveCapacity(capacity + capacityIncrement);
> +
> +	   originsResult.append(origin->isolatedCopy());

It looks like you've duplicated the Vector growth algorithm by hand here, and
in a way that changes the built-in O(log(n)) algorithm into an O(N^2)
algorithm. This is almost certainly wrong. Why can't we just use the built-in
algorithm?

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:744
> +    openTrackerDatabase(DontCreateIfDoesNotExist);
> +    if (!m_database.isOpen())
> +	   return quota;
> +
> +    SQLiteStatement statement(m_database, "SELECT quota FROM Origins where
origin=?;");
> +    if (statement.prepare() != SQLResultOk) {
> +	   LOG_ERROR("Failed to prepare statement.");
> +	   return quota;
> +    }
> +
> +    statement.bindText(1, origin->databaseIdentifier());

This code should be a helper function, shared with the function above.