[webkit-reviews] review denied: [Bug 20219] Databases pane doesn't recognize table creation/deletion : [Attachment 53617] Proposed patch (updated)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Apr 17 16:29:25 PDT 2010


Joseph Pecoraro <joepeck at webkit.org> has denied Juan C. Montemayor E
<jmonte03 at cs.tufts.edu>'s request for review:
Bug 20219: Databases pane doesn't recognize table creation/deletion
https://bugs.webkit.org/show_bug.cgi?id=20219

Attachment 53617: Proposed patch (updated)
https://bugs.webkit.org/attachment.cgi?id=53617&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
> +2010-04-17  Juan C. Montemayor  <jmonte03 at cs.tufts.edu>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Databases pane doesn't recognize table creation/deletion
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=20219

Nit: Its preferred that you don't have this newline.

	
>      _queryFinished: function(query, result)
>      {
>	   var dataGrid =
WebInspector.panels.storage.dataGridForResult(result);
> -	   if (!dataGrid)
> -	       return;
> -	   dataGrid.element.addStyleClass("inline");
> -	   this._appendQueryResult(query, dataGrid.element);
> -	   dataGrid.autoSizeColumns(5);
> +	   
> +	   if(dataGrid){
> +	       dataGrid.element.addStyleClass("inline");
> +	       this._appendQueryResult(query, dataGrid.element);
> +	       dataGrid.autoSizeColumns(5);
> +	   }

Nit: Trailing whitespace (like that empty line) is a pet peeve of mine. In
fact, the blank here isn't needed and can be removed.


>	   if (query.match(/^create /i) || query.match(/^drop table /i))
>	       WebInspector.panels.storage.updateDatabaseTables(this.database);


You should also update this code. If the user input has leading whitespace
these regex's don't match. Could you update this so it is something like the
following:

    var trimmedQuery = query.trim();
    if (trimmedQuery.match(/^create /i) || trimmedQuery.match(/^drop table /i))

	WebInspector.panels.storage.updateDatabaseTables(this.database);

Or better yet, trim the query somewhere earlier on (like as soon as we get the
user input) so mistakes like this will pop up less frequently.

r+ if you make these changes!


More information about the webkit-reviews mailing list