WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Proposed patch (updated)
(1.54 KB, patch)
2010-04-17 15:53 PDT
,
Juan C. Montemayor E
joepeck
: review-
Details
Formatted Diff
Diff
Proposed patch (updated, v2)
(1.67 KB, patch)
2010-04-17 16:53 PDT
,
Juan C. Montemayor E
no flags
Details
Formatted Diff
Diff
Proposed patch (updated, v3)
(1.67 KB, patch)
2010-04-17 17:03 PDT
,
Juan C. Montemayor E
joepeck
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
[PATCH] Patch that Landed
(1.68 KB, patch)
2010-04-17 22:05 PDT
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug