Bug 29536 – WebInspector: Migrate DOM storage inspection to serialized interaction

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

[Yury Semikhatsky]

This is next step in migrating Storage Panel to serialized interaction.

[Yury Semikhatsky]

Created attachment 39842 [details]
patch

[Pavel Feldman]

This looks good to me!

[Timothy Hatcher]

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

[Yury Semikhatsky]

Created attachment 39936 [details]
patch

[Yury Semikhatsky]

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

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 39936 [details]
patch

Flakey test.  bug 29505.  Adding back to the commit-queue.

[WebKit Commit Bot]

Comment on attachment 39936 [details]
patch

Clearing flags on attachment: 39936

Committed r48659: <http://trac.webkit.org/changeset/48659>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

Chromium build fixes landed as http://trac.webkit.org/changeset/48665 and http://trac.webkit.org/changeset/48663. Come on guys.

[Pavel Feldman]

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