RESOLVED FIXED 20219
Databases pane doesn't recognize table creation/deletion
https://bugs.webkit.org/show_bug.cgi?id=20219
Summary Databases pane doesn't recognize table creation/deletion
Matt Lilek
Reported 2008-07-29 20:44:31 PDT
The Inspector's Databases panel doesn't recognize when a table is created or deleted. 1. Load <http://webkit.org/misc/DatabaseExample.html> 2. Open the Inspector and switch to the Databases panel 3. Expand the NoteTest database (notice the WebKitStickyNotes table) and then focus it 4. Enter the following in the db console: DROP TABLE WebKitStickyNotes The WebKitStickyNotes table should disappear from the sidebar, instead, clicking on it brings up the stale datagrid and switching again gives an error that the table could not be read. Two empty error lines are also added to the global console.
Attachments
Proposed patch. (1.56 KB, patch)
2010-04-17 14:53 PDT, Juan C. Montemayor E
joepeck: review-
Proposed patch (updated) (1.54 KB, patch)
2010-04-17 15:53 PDT, Juan C. Montemayor E
joepeck: review-
Proposed patch (updated, v2) (1.67 KB, patch)
2010-04-17 16:53 PDT, Juan C. Montemayor E
no flags
Proposed patch (updated, v3) (1.67 KB, patch)
2010-04-17 17:03 PDT, Juan C. Montemayor E
joepeck: review+
commit-queue: commit-queue-
[PATCH] Patch that Landed (1.68 KB, patch)
2010-04-17 22:05 PDT, Joseph Pecoraro
no flags
Joseph Pecoraro
Comment 1 2009-06-17 11:19:21 PDT
CREATE/DELETE table statements aren't working at all at the moment in r44702: > CREATE TABLE x (id REAL); An unexpected error 0 occured. > DROP TABLE WebKitStickyNotes An unexpected error 0 occured. Its worth mentioning that "occured" is a typo and should be spelled "occurred"!!
Juan C. Montemayor E
Comment 2 2010-04-17 14:53:46 PDT
Created attachment 53610 [details] Proposed patch.
WebKit Review Bot
Comment 3 2010-04-17 14:58:11 PDT
Attachment 53610 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 4 2010-04-17 15:18:39 PDT
Comment on attachment 53610 [details] Proposed patch. Looks good. I haven't actually tried it locally yet but it looks simple enough. Some minor issues that would need to be fixed in order for this to get accepted. > +2010-04-17 Juan C. Montemayor <jmonte03@cs.tufts.edu> > + > + Reviewed by NOBODY (OOPS!). > + > + Bug 20219 - Databases pane doesn't recognize table creation/deletion > + https://bugs.webkit.org/show_bug.cgi?id=20219 Nit: prepare-ChangeLog should not have added the "Bug 20219" part, because the link itself has that. You can remove that text. > _queryFinished: function(query, result) > { > var dataGrid = WebInspector.panels.storage.dataGridForResult(result); > - if (!dataGrid) > - return; > + > + if(dataGrid){ > dataGrid.element.addStyleClass("inline"); > this._appendQueryResult(query, dataGrid.element); > dataGrid.autoSizeColumns(5); > + } The code inside the if should be indented (4 spaces) as per the style guidelines: http://webkit.org/coding/coding-style.html > if (query.match(/^create /i) || query.match(/^drop table /i)) > WebInspector.panels.storage.updateDatabaseTables(this.database); > + > + if(!dataGrid) > + return; > + No need to move this code here, since its the end of the function anyways. The code can just be deleted.
Juan C. Montemayor E
Comment 5 2010-04-17 15:53:40 PDT
Created attachment 53617 [details] Proposed patch (updated) Thanks Joseph for the comments.
WebKit Review Bot
Comment 6 2010-04-17 15:57:02 PDT
Attachment 53617 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 7 2010-04-17 16:29:26 PDT
Comment on attachment 53617 [details] Proposed patch (updated) > +2010-04-17 Juan C. Montemayor <jmonte03@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!
Juan C. Montemayor E
Comment 8 2010-04-17 16:53:50 PDT
Created attachment 53618 [details] Proposed patch (updated, v2) I hope I got rid of all the unwanted whitespace that was mentioned. However, I separated the variables from the if statements, *hopefully* a good idea. Let me know. I also made the changes you suggested replacing query with trimmedQuery.
WebKit Review Bot
Comment 9 2010-04-17 16:57:51 PDT
Attachment 53618 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 10 2010-04-17 17:01:27 PDT
Aha. I see why the style bot is yelling. Because you're using tabs and not spaces! Sorry I didn't notice this before. You have some options. Either I can land this patch for you (keeping you in the ChangeLog so you get credit of course) or you can replace the tabs with 4 spaces. Do you have a preference?
Juan C. Montemayor E
Comment 11 2010-04-17 17:03:14 PDT
Created attachment 53619 [details] Proposed patch (updated, v3) Removed a rogue tab character. Sorry for overlooking this.
Joseph Pecoraro
Comment 12 2010-04-17 17:06:04 PDT
Comment on attachment 53619 [details] Proposed patch (updated, v3) Excellent. Thanks!
Joseph Pecoraro
Comment 13 2010-04-17 17:09:24 PDT
Ahh, it looks like I just missed you in IRC. You can always join #webkit or #webkit-inspector if you want to talk directly to someone. I am JoePeck!
WebKit Commit Bot
Comment 14 2010-04-17 19:44:24 PDT
Comment on attachment 53619 [details] Proposed patch (updated, v3) Rejecting patch 53619 from commit-queue. Found no modified ChangeLogs, cannot create a commit message. All changes require a ChangeLog. See: http://webkit.org/coding/contributing.html
Joseph Pecoraro
Comment 15 2010-04-17 21:58:41 PDT
Comment on attachment 53619 [details] Proposed patch (updated, v3) > diff --git WebCore/ChangeLog WebCore/ChangeLog > index e0aa101..082f4f6 100644 > --- WebCore/ChangeLog > +++ WebCore/ChangeLog Very Interesting. I haven't seen a git patch like this before. Juan, what version of git are you using? I would recommend using at least version 1.6.2. Although the diff is correct, the output should actually look like this. Note that line 2 is irrelevant, its the "a/" and "b/" that are missing that worry me and confused the bot. > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index de3b493..9e5ed89 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog I am going to land this manually, and I'll put a patch up of what I landed (identical to Juan's but with a style change and usable by git in the future). Weird!
Joseph Pecoraro
Comment 16 2010-04-17 22:04:33 PDT
Nicely done Juan! Committed r57789 M WebCore/ChangeLog M WebCore/inspector/front-end/DatabaseQueryView.js r57789 = e794dcd6652a60d068b39acb7df614d34952aa19 (trunk) http://trac.webkit.org/changeset/57789
Joseph Pecoraro
Comment 17 2010-04-17 22:05:41 PDT
Created attachment 53624 [details] [PATCH] Patch that Landed This is the patch that landed. Attached for archival reasons.
Eric Seidel (no email)
Comment 18 2010-04-17 22:21:20 PDT
Chris or Dan might know if this is something svn-apply should support.
Joseph Pecoraro
Comment 19 2010-04-17 22:29:32 PDT
I actually tried to manually apply this patch locally. First with `git apply` then with `svn-apply` and both failed. My guess is that this is an older form of git patch which we need not be concerned with. But, I'd love to hear more from others. Thanks for following up on this Eric!
Chris Jerdonek
Comment 20 2010-04-18 02:01:06 PDT
(In reply to comment #15) > (From update of attachment 53619 [details]) > > diff --git WebCore/ChangeLog WebCore/ChangeLog > > index e0aa101..082f4f6 100644 > > --- WebCore/ChangeLog > > +++ WebCore/ChangeLog > > Very Interesting. I haven't seen a git patch like this before. Juan, what > version of git are you using? I would recommend using at least version 1.6.2. > Although the diff is correct, the output should actually look like this. Note > that line 2 is irrelevant, its the "a/" and "b/" that are missing that worry me > and confused the bot. This past change might be relevant: https://bugs.webkit.org/show_bug.cgi?id=32820
Juan C. Montemayor E
Comment 21 2010-04-18 13:23:46 PDT
Thanks for all your help guys. I was actually calling "git diff --no-prefix > file" I called git diff without the no-prefix flag and it now includes the "a/" and "b/" Won't happen again!
Eric Seidel (no email)
Comment 22 2010-04-18 13:25:38 PDT
Seems we should fix our scripts to understand git diff --no-prefix diffs.
Eric Seidel (no email)
Comment 23 2010-04-18 13:26:15 PDT
Looks like we already have a bug on this. bug 32438.
Maciej Stachowiak
Comment 24 2019-08-09 14:13:45 PDT
*** Bug 200220 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.