[webkit-reviews] review requested: [Bug 28873] WebInspector: Migrate Databases tab to InjectedScript / serialized interaction. : [Attachment 38887] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has asked  for review:
Bug 28873: WebInspector: Migrate Databases tab to InjectedScript / serialized
interaction.
https://bugs.webkit.org/show_bug.cgi?id=28873

Attachment 38887: patch
https://bugs.webkit.org/attachment.cgi?id=38887&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
(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.