Databases pane doesn't recognize table creation/deletion

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Matt Lilek]

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.

[Joseph Pecoraro]

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]

Created attachment 53610 [details]
Proposed patch.

[WebKit Review Bot]

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 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.


> [phone]  Juan C. Montemayor  <[e-mail]>
> +
> +        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]

Created attachment 53617 [details]
Proposed patch (updated)

Thanks Joseph for the comments.

[WebKit Review Bot]

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 on attachment 53617 [details]
Proposed patch (updated)

> [phone]  Juan C. Montemayor  <[e-mail]>
> +
> +        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]

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]

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]

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]

Created attachment 53619 [details]
Proposed patch (updated, v3)

Removed a rogue tab character. Sorry for overlooking this.

[Joseph Pecoraro]

Comment on attachment 53619 [details]
Proposed patch (updated, v3)

Excellent. Thanks!

[Joseph Pecoraro]

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 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 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]

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]

Created attachment 53624 [details]
[PATCH] Patch that Landed

This is the patch that landed. Attached for archival reasons.

[Eric Seidel (no email)]

Chris or Dan might know if this is something svn-apply should support.

[Joseph Pecoraro]

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]

(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]

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)]

Seems we should fix our scripts to understand git diff --no-prefix diffs.

[Eric Seidel (no email)]

Looks like we already have a bug on this.  bug 32438.

[Maciej Stachowiak]

*** Bug 200220 has been marked as a duplicate of this bug. ***