[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