Bug 128620 - Web Inspector: DOMStorageView should listen for events from DOMStorageObject
Summary: Web Inspector: DOMStorageView should listen for events from DOMStorageObject
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-11 13:30 PST by BJ Burg
Modified: 2014-02-11 18:47 PST (History)
4 users (show)

See Also:


Attachments
patch (15.63 KB, patch)
2014-02-11 13:45 PST, BJ Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2014-02-11 13:30:09 PST
There's a layering violation now; the StorageManager explicitly maintains a map of available views and forwards events to them directly.
Comment 1 Radar WebKit Bug Importer 2014-02-11 13:31:10 PST
<rdar://problem/16040514>
Comment 2 BJ Burg 2014-02-11 13:45:47 PST
Created attachment 223894 [details]
patch
Comment 3 Timothy Hatcher 2014-02-11 16:29:26 PST
Comment on attachment 223894 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223894&action=review

> Source/WebInspectorUI/ChangeLog:8
> +        The storage manager used to have a map of DOMStorageViews and delivered events

Yeah that was gross!

> Source/WebInspectorUI/UserInterface/DOMStorageObserver.js:70
> -        WebInspector.storageManager.domStorageItemsCleared(storageId);
> +        var object = WebInspector.storageManager.domStorageForId(storageId);
> +        object.itemsCleared(storageId);
>      },
>  
>      domStorageItemRemoved: function(storageId, key)
>      {
> -        WebInspector.storageManager.domStorageItemRemoved(storageId, key);
> +        var object = WebInspector.storageManager.domStorageForId(storageId);
> +        object.itemRemoved(storageId, key);
>      },
>  
>      domStorageItemAdded: function(storageId, key, value)
>      {
> -        WebInspector.storageManager.domStorageItemAdded(storageId, key, value);
> +        var object = WebInspector.storageManager.domStorageForId(storageId);
> +        object.itemAdded(storageId, key, value);
>      },
>  
>      domStorageItemUpdated: function(storageId, key, oldValue, value)
>      {
> -        WebInspector.storageManager.domStorageItemUpdated(storageId, key, oldValue, value);
> +        var object = WebInspector.storageManager.domStorageForId(storageId);
> +        object.itemUpdated(storageId, key, oldValue, value);

This differs from other observers. Usually the protocol observers are very thin and do no work other than calling the right manager(s). That way any compatibility work happens in the manager. Not that this way is entirely bad. It would just push down compatibility massaging into the observer.

> Source/WebInspectorUI/UserInterface/StorageManager.js:108
> +    domStorageForId: function(id)

domStorageForIdentifier
Comment 4 BJ Burg 2014-02-11 16:53:53 PST
Comment on attachment 223894 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223894&action=review

>> Source/WebInspectorUI/UserInterface/DOMStorageObserver.js:70
>> +        object.itemUpdated(storageId, key, oldValue, value);
> 
> This differs from other observers. Usually the protocol observers are very thin and do no work other than calling the right manager(s). That way any compatibility work happens in the manager. Not that this way is entirely bad. It would just push down compatibility massaging into the observer.

I don't have strong opinions here, except that as written there are 4 fewer dummy functions. But the compatibility argument makes sense to me.

>> Source/WebInspectorUI/UserInterface/StorageManager.js:108
>> +    domStorageForId: function(id)
> 
> domStorageForIdentifier

Oh, I was going to get that.
Comment 5 BJ Burg 2014-02-11 18:47:15 PST
Committed r163927: <http://trac.webkit.org/changeset/163927>