[webkit-reviews] review denied: [Bug 37037] [Chrome] Clear browsing data dialog never closes if one of the WebSQLDatabase that should be removed is open at the time. : [Attachment 52446] fix a bug regression

[bugzilla-daemon at webkit.org]

Nate Chapin <japhet at chromium.org> has denied Michael Nordman
<michaeln at google.com>'s request for review:
Bug 37037: [Chrome] Clear browsing data dialog never closes if one of the
WebSQLDatabase that  should be removed is open at the time.
https://bugs.webkit.org/show_bug.cgi?id=37037

Attachment 52446: fix a bug regression
https://bugs.webkit.org/attachment.cgi?id=52446&action=review

------- Additional Comments from Nate Chapin <japhet at chromium.org>
A rather pedantic r- since we'll need to update the ChangeLog before setting
commit-queue+ anyway.

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 57019)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,21 @@
> +2010-04-02  Michael Nordman	<michaeln at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Set the close policy used the by the DatabaseCloseTask in a
constructor argument

Typo here and in the other ChangeLog.

> @@ -110,18 +110,20 @@ private:
>  
>  class DatabaseCloseTask : public DatabaseTask {
>  public:
> -    static PassOwnPtr<DatabaseCloseTask> create(Database* db,
DatabaseTaskSynchronizer* synchronizer)
> +    static PassOwnPtr<DatabaseCloseTask> create(Database* db,
Database::ClosePolicy closePolicy, DatabaseTaskSynchronizer* synchronizer)
>      { 
> -	   return new DatabaseCloseTask(db, synchronizer);
> +	   return new DatabaseCloseTask(db, closePolicy, synchronizer);
>      }
>  
>  private:
> -    DatabaseCloseTask(Database*, DatabaseTaskSynchronizer*);
> +    DatabaseCloseTask(Database*, Database::ClosePolicy,
DatabaseTaskSynchronizer*);
>  
>      virtual void doPerformTask();
>  #ifndef NDEBUG
>      virtual const char* debugTaskName() const;
>  #endif
> +
> +    Database::ClosePolicy m_closePolicy;
>  };

Perhaps we should support closePolicy as an optional parameter on the
constructor/create function? I assume
in the vast majority of cases we will want DoNotRemoveFromDatabaseContext. I'm
willing to be convinced otherwise