Bug 84387 - [chromium] DomStorage events handling needs TLC (1)
Summary: [chromium] DomStorage events handling needs TLC (1)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Nordman
URL:
Keywords:
Depends on:
Blocks: 85215
  Show dependency treegraph
 
Reported: 2012-04-19 14:50 PDT by Michael Nordman
Modified: 2012-04-30 11:43 PDT (History)
7 users (show)

See Also:


Attachments
events (22.03 KB, patch)
2012-04-19 15:28 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
events (21.99 KB, patch)
2012-04-19 15:37 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
events (20.35 KB, patch)
2012-04-23 13:22 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
events (20.33 KB, patch)
2012-04-23 13:26 PDT, Michael Nordman
dglazkov: review+
Details | Formatted Diff | Diff
events - patch for landing (20.18 KB, patch)
2012-04-23 16:54 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
events3 (37.75 KB, patch)
2012-04-27 19:18 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Nordman 2012-04-19 14:50:43 PDT
Events are handled inconsistently. The dispatch of some events are initiated from within webkit/webcore, the dispatch of other events are initiated from the outside via the WebKit::WebStorageEventDispatcher interface. The existing WebStorageEventDispatcher is not expressive enough to handle initiated all events from the outside. There's a chunk of nearly replicated code in there that shouldn't be. The existing code has several FIXMEs related to making this better.

It could use some TLC for those reasons alone, but the current state of things in webkit/webcore is also blocking development of some overall performance improvements to chromium's implemention of this feature (getting rid of sync ipcs for each access and adding a renderer-side caching layer).

To facilitate the perf improvements, event dispatching needs to be initiated from the outside because there may be an async latency between setting an item and receiving the 'oldValue' from the main browser process which is needed to raise the mutation event.
Comment 1 Michael Nordman 2012-04-19 15:28:14 PDT
Created attachment 137993 [details]
events
Comment 2 WebKit Review Bot 2012-04-19 15:30:39 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 WebKit Review Bot 2012-04-19 15:30:59 PDT
Attachment 137993 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit/chromium/src/StorageAreaProxy.cpp:202:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/chromium/src/StorageAreaProxy.h:72:  The parameter name "storage" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Michael Nordman 2012-04-19 15:37:13 PDT
Created attachment 137994 [details]
events

style fixes
Comment 5 Michael Nordman 2012-04-20 13:13:04 PDT
ping somebody with a (r) next to your name
Comment 6 Michael Nordman 2012-04-21 11:13:33 PDT
Comment on attachment 137994 [details]
events

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

> Source/WebKit/chromium/public/WebStorageEventDispatcher.h:49
> +            bool originatedInProcess);

I should probably rename the 'sourceArea' param to 'sourceAreaInstance' to be more clear. The intent of that param is to identify the Document that caused the mutation.
Comment 7 Michael Nordman 2012-04-21 11:38:04 PDT
Comment on attachment 137994 [details]
events

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

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:200
> +            if (frame->document()->securityOrigin()->equal(securityOrigin) && !IsEventSource(frame->domWindow()->optionalLocalStorage(), sourceArea))

I suspect this SecurityOrigin::equal() test could give us some testing grief, some tests use file urls whose origins are considered unique (and things just behave differently). I'm working on the chrome-side changes to use the new dispatch API and will find out soon enough.
Comment 8 Eric Seidel (no email) 2012-04-21 18:02:54 PDT
Adam is on vacation for another week, but he's a great person to have review this patch.  If you can wait until May (and likely ping him again) that would probably be best.
Comment 9 Michael Nordman 2012-04-21 21:16:42 PDT
(In reply to comment #8)
> Adam is on vacation for another week, but he's a great person to have review this patch.  If you can wait until May (and likely ping him again) that would probably be best.

I'll be out that first week of May and I was kind of hoping to be wrapping up this project by the end of next week.

Maybe dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org could take a peek.
Comment 10 Dimitri Glazkov (Google) 2012-04-22 12:37:03 PDT
Comment on attachment 137994 [details]
events

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

Here are a few general comments.

> Source/WebCore/ChangeLog:18
> +        To facilitate the perf improvements, this patch adds WebKit API too allow all

too -> to

> Source/WebCore/ChangeLog:28
> +        xxxxxxx 4/5: cleanup, remove extra params from new API whose only purpose was to

was to ... what? :)

> Source/WebCore/page/DOMWindow.h:358
> +        Storage* optionalSessionStorage() const { return m_sessionStorage.get(); }
> +        Storage* optionalLocalStorage() const { return m_localStorage.get(); }

"optional" seems unclear in meaning. Does this goe around the RAII in the original accessor? In WebKit, we usually do the other way around, with "ensure" function to do the initialization. I guess we could name it something like sessionStorageOrNull?

> Source/WebCore/storage/Storage.h:54
> +        StorageArea* area() const { return m_storageArea.get(); }

I am not familiar with this code -- why was area kept private before?

> Source/WebKit/chromium/public/WebStorageEventDispatcher.h:58
> +    // DEPRECATED - The instance methods are going away soon in favor

Perhaps link to a bug that aims to remove this?

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:186
> +    // FIXME: Multi-sided patch engineering alert !

Perhaps link to a meta bug that explains the effort?

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:203
> +    const HashSet<Page*>& pages = PageGroup::pageGroup(pageGroupName)->pages();
> +    for (HashSet<Page*>::const_iterator it = pages.begin(); it != pages.end(); ++it) {
> +        for (Frame* frame = (*it)->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
> +            if (frame->document()->securityOrigin()->equal(securityOrigin) && !IsEventSource(frame->domWindow()->optionalLocalStorage(), sourceArea))
> +                frames.append(frame);
> +        }
> +    }

This looks like an isolated helper function -- and eric@ is right that abarth@ is a good person to look at this.

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:210
> +            frames[i]->document()->enqueueWindowEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, pageURL, storage));

Interesting -- since we just enqueue events, do we need to worry about guarding the frames?

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:234
> +    Vector<RefPtr<Frame> > frames;
> +    for (Frame* frame = page->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
> +        if (frame->document()->securityOrigin()->equal(securityOrigin) && !IsEventSource(frame->domWindow()->optionalSessionStorage(), sourceArea))
> +            frames.append(frame);
> +    }

Ditto about the helper function.

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:241
> +            frames[i]->document()->enqueueWindowEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, pageURL, storage));

Ditto about async event dispatch.

> Source/WebKit/chromium/src/StorageAreaProxy.h:73
> +    static bool IsEventSource(Storage*, WebKit::WebStorageArea* sourceArea);
> +    static Page* FindPageWithSessionStorageNamespace(const String& pageGroupName, const WebKit::WebStorageNamespace& sessionNamespace);

lowercase names.

> Source/WebKit/chromium/src/StorageEventDispatcherImpl.cpp:44
> +// FIXME: delete this almost obsolete file soon

Link to a bug?

> Source/WebKit/chromium/src/WebStorageEventDispatcherImpl.cpp:45
> +// static

No need for this comment in WebKit-land.

> Source/WebKit/chromium/src/WebStorageEventDispatcherImpl.cpp:52
> +    RefPtr<WebCore::SecurityOrigin> securityOrigin = WebCore::SecurityOrigin::create(origin);

Creating a security origin? Must check with @abarth :)

> Source/WebKit/chromium/src/WebStorageEventDispatcherImpl.cpp:58
> +// static

Ditto.
Comment 11 Michael Nordman 2012-04-22 17:33:20 PDT
Comment on attachment 137994 [details]
events

>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:186
>> +    // FIXME: Multi-sided patch engineering alert !
> 
> Perhaps link to a meta bug that explains the effort?

The bug is this bug, this comment just describes the multi-sided sequence of patches to alter the interfaces as described in this bug. This bug mentions 'overall performance improvements' as a goal, with the exception of these api changes, that effort is in the chrome codebase so it doesn't make sense to have a meta bug in webkit for that effort. Here's some related work in progress over there.

http://codereview.chromium.org/10160003/
http://codereview.chromium.org/10162015/  

The first defines a set of more async ipcs. The second is very preliminary yet, but if you squint, you can see where a caching layer is to be inserted in the renderer.


>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:203
>> +    }
> 
> This looks like an isolated helper function -- and eric@ is right that abarth@ is a good person to look at this.

Its defined as a static class method for two reasons, it accesses private members of this class to do its work, and the function it performs is very much related to this class which is all about proxying between the embedder and webcore internals. Do you think it would be better to have these methods elsewhere, where, and why?

>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:210
>> +            frames[i]->document()->enqueueWindowEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, pageURL, storage));
> 
> Interesting -- since we just enqueue events, do we need to worry about guarding the frames?

I think you're right. I'll pull this up into the loop above on my next upload of this patch. Thnx.

>> Source/WebKit/chromium/src/StorageAreaProxy.h:73
>> +    static Page* FindPageWithSessionStorageNamespace(const String& pageGroupName, const WebKit::WebStorageNamespace& sessionNamespace);
> 
> lowercase names.

doh!
Comment 12 Dimitri Glazkov (Google) 2012-04-23 09:22:23 PDT
Comment on attachment 137994 [details]
events

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

>>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:186
>>> +    // FIXME: Multi-sided patch engineering alert !
>> 
>> Perhaps link to a meta bug that explains the effort?
> 
> The bug is this bug, this comment just describes the multi-sided sequence of patches to alter the interfaces as described in this bug. This bug mentions 'overall performance improvements' as a goal, with the exception of these api changes, that effort is in the chrome codebase so it doesn't make sense to have a meta bug in webkit for that effort. Here's some related work in progress over there.
> 
> http://codereview.chromium.org/10160003/
> http://codereview.chromium.org/10162015/  
> 
> The first defines a set of more async ipcs. The second is very preliminary yet, but if you squint, you can see where a caching layer is to be inserted in the renderer.

I was more thinking along the lines of linking to a bug that tracks changes in WebKit. Here's an example of what I was describing: http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/ShadowRoot.cpp&exact_package=chromium&q=authorshadow&type=cs&l=75

Here, we aim to eventually get rid of this whole function, but can't at the moment. Each condition is thus tracked by a bug, so that those reading or refactoring the code could understand the wider context. It is totally uncritical for this patch, since you probably will have this done quickly.

>>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:203
>>> +    }
>> 
>> This looks like an isolated helper function -- and eric@ is right that abarth@ is a good person to look at this.
> 
> Its defined as a static class method for two reasons, it accesses private members of this class to do its work, and the function it performs is very much related to this class which is all about proxying between the embedder and webcore internals. Do you think it would be better to have these methods elsewhere, where, and why?

The function of populating vector of frame ref ptrs seems like a  nicely isolatable free-standing function (same code is also present in dispatchSessionStorageEvent), but I think you may not need it at all, since the event dispatch is async.
Comment 13 Michael Nordman 2012-04-23 13:22:57 PDT
Created attachment 138410 [details]
events
Comment 14 Michael Nordman 2012-04-23 13:26:19 PDT
Created attachment 138411 [details]
events
Comment 15 Michael Nordman 2012-04-23 13:42:15 PDT
New patch
- lowered uppercase method names
- removed // static code comments
- removed second loop in each of the dispatch[Type]StorageEvents methods
- fixed ChangeLog'isms
- removed a couple more param names from method declarations
- renamed sourceArea params to sourceAreaInstance (not sure it helps that much, but a little)

Some responses to earlier comments...

> "optional" seems unclear in meaning
For consistency, these names are being patterned off of an existing method named in this same way. Widen the diff context a little and see optionalApplicationCache().

> I am not familiar with this code -- why was area kept private before?
There simply was no need for it to be exposed before. The webcore ChangeLog comment has been modified to explain the reason for these changes.

> The function of populating vector of frame ref ptrs seems like...
I see, I misunderstood your meaning and thought you were referring to the dispatch[]StorageEvents() methods as a whole. In any case, that loop populating the separate vector of frames has been removed in the latest snapshot.
Comment 16 Dimitri Glazkov (Google) 2012-04-23 16:25:48 PDT
Comment on attachment 138411 [details]
events

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

ok with a nit.

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:243
> +Page* StorageAreaProxy::findPageWithSessionStorageNamespace(const String& pageGroupName, const WebKit::WebStorageNamespace& sessionNamespace)

This looks like it can be just a freestanding static function.
Comment 17 Michael Nordman 2012-04-23 16:41:19 PDT
Comment on attachment 138411 [details]
events

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

>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:243
>> +Page* StorageAreaProxy::findPageWithSessionStorageNamespace(const String& pageGroupName, const WebKit::WebStorageNamespace& sessionNamespace)
> 
> This looks like it can be just a freestanding static function.

yup, thnx!
Comment 18 Michael Nordman 2012-04-23 16:54:04 PDT
Created attachment 138460 [details]
events - patch for landing

made findPageWithSessionStorageNamespace() a free standing static helper function instead of a static class method
Comment 19 WebKit Review Bot 2012-04-23 21:20:09 PDT
Comment on attachment 138460 [details]
events - patch for landing

Clearing flags on attachment: 138460

Committed r114993: <http://trac.webkit.org/changeset/114993>
Comment 20 WebKit Review Bot 2012-04-23 21:20:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Michael Nordman 2012-04-27 19:16:06 PDT
reopening for the next in the sequence
Comment 22 Michael Nordman 2012-04-27 19:18:14 PDT
Created attachment 139320 [details]
events3

Now that http://codereview.chromium.org/10201010/ has landed, some cleanup and next steps can be taken back here in webkit land.
Comment 23 Dimitri Glazkov (Google) 2012-04-27 19:18:58 PDT
(In reply to comment #21)
> reopening for the next in the sequence

Don't do that. In WebKit, a bug is a patch. Umbrella bugs serve the purpose of grouping bugs. See this for example: https://bugs.webkit.org/showdependencytree.cgi?id=52962&hide_resolved=1
Comment 24 WebKit Review Bot 2012-04-27 19:20:54 PDT
Attachment 139320 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/storage/StorageArea.h:52:  The parameter name "ec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/StorageAreaProxy.h:54:  The parameter name "ec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/StorageAreaProxy.h:62:  The parameter name "pageGroup" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/StorageAreaProxy.h:65:  The parameter name "pageGroup" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/storage/StorageAreaImpl.h:49:  The parameter name "ec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/public/WebStorageArea.h:66:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebStorageArea.h:75:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebStorageArea.h:83:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebStorageArea.h:92:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 9 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 WebKit Review Bot 2012-04-28 05:03:25 PDT
Comment on attachment 139320 [details]
events3

Attachment 139320 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12555598
Comment 26 James Robinson 2012-04-30 09:45:40 PDT
One patch per bug.
Comment 27 Michael Nordman 2012-04-30 11:43:15 PDT
new sacrificial bug offerings created: 85215, 85221