[webkit-reviews] review granted: [Bug 27836] Stale database version persists through browser refresh (changeVersion doesn't work) : [Attachment 40445] Revised patch.

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Ben Murdoch
<benm at google.com>'s request for review:
Bug 27836: Stale database version persists through browser refresh
(changeVersion doesn't work)
https://bugs.webkit.org/show_bug.cgi?id=27836

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

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> Index: WebCore/ChangeLog
> [...]
> +	   * bindings/v8/custom/V8DatabaseCustom.cpp:
> +	   (WebCore::CALLBACK_FUNC_DECL): implement the V8 binding for
database.changeVersion
> +	   (WebCore::createTransaction): Fix a bug that was checking the wrong
argument index to save the success callback

Would be nice if these comments both started with capital letters and ended
with periods (like the ones below).

> +	   * storage/Database.cpp:
> +	   (WebCore::updateGuidVersionMap): Safely update the Guid/version hash
map.
> +	   (WebCore::Database::~Database): Remove code that removes the
database from the guid->database and guid->version maps.
> +	   (WebCore::Database::setVersionInDatabase): Add a comment to explain
some behaviour.
> +	   (WebCore::Database::close): Move the code that updates the maps from
the destructor to here.
> +	   (WebCore::Database::performOpenAndVerify): Call updateGuidVersionMap
instead of setting the hash map directly.
> +	   (WebCore::Database::setExpectedVersion): Update the in memory
guid->version map when we want to update the database version.


> Index: WebCore/storage/Database.cpp
> [...]
> +static inline void updateGuidVersionMap(int guid, String newVersion)
> +{
> +    // Note: It is not safe to put an empty string into the
guidToVersionMap() map.
> +    // That's because the map is cross-thread, but empty strings are
per-thread.
> +    // The copy() function makes a version of the string you can use on the
current
> +    // thread, but we need a string we can keep in a cross-thread data
structure.
> +    // FIXME: This is a quite-awkward restriction to have to program with.
> +
> +    // NOTE: Caller must lock guidMutex().

Oops, I see what Dumi meant.  This one-line comment could go above the 'static
inline' method declaration.

It would be nice to add this comment above the code (in case someone skips over
the large block above):

    // Map null string to empty string (see comment above).

> +    guidToVersionMap().set(guid, newVersion.isEmpty() ? String() :
newVersion.copy());
> +}

>  bool Database::setVersionInDatabase(const String& version)
>  {
> +    // The INSERT will replace an existing entry for the database with the
new version number, due to the UNIQUE ON CONFLICT REPLACE
> +    // clause in the CREATE statement (see Database::performOpenAndVerify)

This comment needs a period.  Would be nice to add "()" after
"performOpenAndVerify" so you know you're looking for a method.

> @@ -447,12 +461,6 @@ bool Database::performOpenAndVerify(Exce
>      {
>	   MutexLocker locker(guidMutex());
>  
> -	   // Note: It is not safe to put an empty string into the
guidToVersionMap() map.
> -	   // That's because the map is cross-thread, but empty strings are
per-thread.
> -	   // The copy() function makes a version of the string you can use on
the current
> -	   // thread, but we need a string we can keep in a cross-thread data
structure.
> -	   // FIXME: This is a quite-awkward restriction to have to program
with.
> -
>	   GuidVersionMap::iterator entry = guidToVersionMap().find(m_guid);
>	   if (entry != guidToVersionMap().end()) {
>	       // Map null string to empty string (see comment above).

This comment needs to change since you moved the big comment:

    // Map null string to empty string (see updateGuidVersionMap()).

r=me with the above changes.