Bug 29536 - WebInspector: Migrate DOM storage inspection to serialized interaction
Summary: WebInspector: Migrate DOM storage inspection to serialized interaction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-21 00:52 PDT by Yury Semikhatsky
Modified: 2009-09-22 23:04 PDT (History)
4 users (show)

See Also:


Attachments
patch (36.37 KB, patch)
2009-09-21 00:53 PDT, Yury Semikhatsky
timothy: review-
Details | Formatted Diff | Diff
patch (36.24 KB, patch)
2009-09-22 12:17 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 Yury Semikhatsky 2009-09-21 00:52:01 PDT
This is next step in migrating Storage Panel to serialized interaction.
Comment 1 Yury Semikhatsky 2009-09-21 00:53:46 PDT
Created attachment 39842 [details]
patch
Comment 2 Pavel Feldman 2009-09-22 01:34:03 PDT
This looks good to me!
Comment 3 Timothy Hatcher 2009-09-22 09:51:00 PDT
Comment on attachment 39842 [details]
patch

> +    if (storageResourceId) {
> +        m_frontend->selectDOMStorage(storageResourceId);
> +    }

No need for braces.


> +    for (DOMStorageResourcesSet::iterator it = m_domStorageResources.begin(); it != domStorageEnd; ++it) {
> +        if ((*it)->id() == storageId) {
> +            return it->get();
> +        }
> +    }

Ditto.


> +    if (isSameHostAndType(storage->frame(), isLocalStorage)) {
> +        m_frontend->updateDOMStorage(m_id);
> +    }

Ditto.


> +            return listener->type() == InspectorDOMStorageResourceType
> +            ? static_cast<const InspectorDOMStorageResource*>(listener)
> +            : 0;

Just put put on one line.


> +    getEntriesAsync: function(callback)
> +    setItemAsync: function(key, value, callback)
> +    removeItemAsync: function(key, callback)

I am not sure "Async" is needed for these, since they take a callback, it is implied. All the other async calls Pavel has added don't have that suffix.


> +        if (columnIdentifier == 0) {
> +            if (this._keys.indexOf(newText) != -1) {

Would be good to use === or !==.


> +        for (var index = 0; index < entries.length; index++) {

Just use "i" instead of index and ++i.


> +            if (domStorage.id == storageId) {
> +                return domStorage;
>              }

No need for braces.


Otherwise will be r+.
Comment 4 Yury Semikhatsky 2009-09-22 12:17:16 PDT
Created attachment 39936 [details]
patch
Comment 5 Yury Semikhatsky 2009-09-22 12:19:08 PDT
(In reply to comment #3)
> (From update of attachment 39842 [details])
> > +    if (storageResourceId) {
> > +        m_frontend->selectDOMStorage(storageResourceId);
> > +    }
> 
> No need for braces.
> 
Done.

> 
> > +    for (DOMStorageResourcesSet::iterator it = m_domStorageResources.begin(); it != domStorageEnd; ++it) {
> > +        if ((*it)->id() == storageId) {
> > +            return it->get();
> > +        }
> > +    }
> 
> Ditto.
> 
Done.

> 
> > +    if (isSameHostAndType(storage->frame(), isLocalStorage)) {
> > +        m_frontend->updateDOMStorage(m_id);
> > +    }
> 
> Ditto.
> 
Done.

> 
> > +            return listener->type() == InspectorDOMStorageResourceType
> > +            ? static_cast<const InspectorDOMStorageResource*>(listener)
> > +            : 0;
> 
> Just put put on one line.
> 
Done.

> 
> > +    getEntriesAsync: function(callback)
> > +    setItemAsync: function(key, value, callback)
> > +    removeItemAsync: function(key, callback)
> 
> I am not sure "Async" is needed for these, since they take a callback, it is
> implied. All the other async calls Pavel has added don't have that suffix.
> 
Removed Async suffixes.

> 
> > +        if (columnIdentifier == 0) {
> > +            if (this._keys.indexOf(newText) != -1) {
> 
> Would be good to use === or !==.
> 
Done.

> 
> > +        for (var index = 0; index < entries.length; index++) {
> 
> Just use "i" instead of index and ++i.
> 
Done. It was called index in the original code.

> 
> > +            if (domStorage.id == storageId) {
> > +                return domStorage;
> >              }
> 
> No need for braces.
> 
Done.
Comment 6 WebKit Commit Bot 2009-09-22 16:03:00 PDT
Comment on attachment 39936 [details]
patch

Rejecting patch 39936 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11312 test cases.

httpd is already running: pid 37053, killing...
http/tests/loading/basic-credentials-sent-automatically.html -> failed

Exiting early after 1 failures. 8433 tests run.
209.08s total testing time

8432 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output
Comment 7 Eric Seidel (no email) 2009-09-22 16:14:36 PDT
Comment on attachment 39936 [details]
patch

Flakey test.  bug 29505.  Adding back to the commit-queue.
Comment 8 WebKit Commit Bot 2009-09-22 17:09:18 PDT
Comment on attachment 39936 [details]
patch

Clearing flags on attachment: 39936

Committed r48659: <http://trac.webkit.org/changeset/48659>
Comment 9 WebKit Commit Bot 2009-09-22 17:09:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Dimitri Glazkov (Google) 2009-09-22 17:32:32 PDT
This was a two-sided patch for Chromium. Yury, Pavel, please make sure to note this in the future. It's very hard for the webkit gardeners to keep up.
Comment 11 Dimitri Glazkov (Google) 2009-09-22 19:57:00 PDT
Chromium build fixes landed as http://trac.webkit.org/changeset/48665 and http://trac.webkit.org/changeset/48663. Come on guys.
Comment 12 Pavel Feldman 2009-09-22 23:04:56 PDT
(In reply to comment #10)
> This was a two-sided patch for Chromium. Yury, Pavel, please make sure to note
> this in the future. It's very hard for the webkit gardeners to keep up.

Sorry about that. It was not supposed to be two-sided, just the import bustage. Won't happen again.