Bug 28873 - WebInspector: Migrate Databases tab to InjectedScript / serialized interaction.
Summary: WebInspector: Migrate Databases tab to InjectedScript / serialized interaction.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-01 07:08 PDT by Pavel Feldman
Modified: 2009-09-21 00:56 PDT (History)
2 users (show)

See Also:


Attachments
patch (10.83 KB, patch)
2009-09-01 10:32 PDT, Pavel Feldman
timothy: review-
Details | Formatted Diff | Diff
patch (12.01 KB, patch)
2009-09-01 14:55 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
patch: serialize dom storage access (35.33 KB, patch)
2009-09-17 06:42 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
patch: serialize dom storage access (36.37 KB, patch)
2009-09-18 02:41 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2009-09-01 07:08:04 PDT
This will consist of several steps.
Comment 1 Pavel Feldman 2009-09-01 10:32:49 PDT
Created attachment 38869 [details]
patch

This is a first step in serializing db access. It also contains couple of drive-by fixes.
Comment 2 Timothy Hatcher 2009-09-01 10:56:27 PDT
Comment on attachment 38869 [details]
patch

> +    isDB: function(db)

I assume this will do somthing more interesting later? If not, I prefer the callers just doing === or ==.

If this will be needed later, I think isDatabase is better.
> +    runWithTableNames: function(callback)

This name confused me when I saw it. I think getTableNames with a callback is better wording and more consistent with the other callback functions.

> +        function withTablesCallback(tableNames)

Starting a function with "with" is odd naming. Just tableNameCallback would work, or somthing more descriptive on what the function does like accumulateCompletions.

> -    _queryFinished: function(query, tx, result)
> +    _queryFinished: function(query, result)

Is this correct? I thought tx was required here…

> +        function withTableNames(tableNames)

Again about "with".

> +        function withTableNames(tableNames)

Ditto.
Comment 3 Pavel Feldman 2009-09-01 14:55:43 PDT
Created attachment 38887 [details]
patch

(In reply to comment #2)
> (From update of attachment 38869 [details])
> > +    isDB: function(db)
> 
> I assume this will do somthing more interesting later? If not, I prefer the
> callers just doing === or ==.
> 

The idea is to encapsulate Database::database. This quarantined object will be replaced with the proxy in the next step. I actually can already mark this member as private (updated the change).

> If this will be needed later, I think isDatabase is better.

Done. 

> > +    runWithTableNames: function(callback)
> 
> This name confused me when I saw it. I think getTableNames with a callback is
> better wording and more consistent with the other callback functions.
> 

Done.

> > +        function withTablesCallback(tableNames)
> 
> Starting a function with "with" is odd naming. Just tableNameCallback would
> work, or somthing more descriptive on what the function does like
> accumulateCompletions.
> 

Done.

> > -    _queryFinished: function(query, tx, result)
> > +    _queryFinished: function(query, result)
> 
> Is this correct? I thought tx was required here…
>

Yes, Database.prototype.executeSql encapsulates callback wrapping and drops this parameters since it is not used in the actual callbacks.
 
> > +        function withTableNames(tableNames)
> 
> Again about "with".
> 
> > +        function withTableNames(tableNames)
> 
> Ditto.

Done.
Comment 4 Pavel Feldman 2009-09-02 03:00:21 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorDOMAgent.cpp
	M	WebCore/inspector/front-end/Database.js
	M	WebCore/inspector/front-end/DatabaseQueryView.js
	M	WebCore/inspector/front-end/DatabaseTableView.js
	M	WebCore/inspector/front-end/StoragePanel.js
Committed r47960
Comment 5 Yury Semikhatsky 2009-09-17 06:41:07 PDT
DOM storage access is not serialized yet.
Comment 6 Yury Semikhatsky 2009-09-17 06:42:16 PDT
Created attachment 39692 [details]
patch: serialize dom storage access
Comment 7 Yury Semikhatsky 2009-09-18 02:41:07 PDT
Created attachment 39754 [details]
patch: serialize dom storage access

Fixed some issues in the previous patch:
* Implemented InspectorDOMStorageResource::operator== which is declared as pure virtual in EventListener. Also added new EventListener type.
* Removed trailing whitespaces in several places.
Comment 8 Eric Seidel (no email) 2009-09-18 12:15:59 PDT
Comment on attachment 38887 [details]
patch

Obsoleting this patch which has been landed.  It's better to use a new bug for a new change.
Comment 9 Yury Semikhatsky 2009-09-21 00:55:15 PDT
(In reply to comment #8)
> (From update of attachment 38887 [details])
> Obsoleting this patch which has been landed.  It's better to use a new bug for
> a new change.
Filed a new bug:
https://bugs.webkit.org/show_bug.cgi?id=29536
Comment 10 Yury Semikhatsky 2009-09-21 00:56:41 PDT
Comment on attachment 39754 [details]
patch: serialize dom storage access

Send this patch to its own bug: 29536