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.
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"!!
Created attachment 53610 [details] Proposed patch.
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.
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.
Created attachment 53617 [details] Proposed patch (updated) Thanks Joseph for the comments.
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.
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!
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.
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.
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?
Created attachment 53619 [details] Proposed patch (updated, v3) Removed a rogue tab character. Sorry for overlooking this.
Comment on attachment 53619 [details] Proposed patch (updated, v3) Excellent. Thanks!
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!
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
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!
Nicely done Juan! Committed r57789 M WebCore/ChangeLog M WebCore/inspector/front-end/DatabaseQueryView.js r57789 = e794dcd6652a60d068b39acb7df614d34952aa19 (trunk) http://trac.webkit.org/changeset/57789
Created attachment 53624 [details] [PATCH] Patch that Landed This is the patch that landed. Attached for archival reasons.
Chris or Dan might know if this is something svn-apply should support.
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!
(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
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!
Seems we should fix our scripts to understand git diff --no-prefix diffs.
Looks like we already have a bug on this. bug 32438.
*** Bug 200220 has been marked as a duplicate of this bug. ***