Bug 84387

Created attachment 137993 [details] events

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.

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.

Created attachment 137994 [details] events style fixes

ping somebody with a (r) next to your name

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

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.

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

Created attachment 138410 [details] events

Created attachment 138411 [details] events

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 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 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!

Created attachment 138460 [details] events - patch for landing made findPageWithSessionStorageNamespace() a free standing static helper function instead of a static class method

Comment on attachment 138460 [details] events - patch for landing Clearing flags on attachment: 138460 Committed r114993: <http://trac.webkit.org/changeset/114993>

All reviewed patches have been landed. Closing bug.

reopening for the next in the sequence

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.

(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

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 on attachment 139320 [details] events3 Attachment 139320 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12555598

One patch per bug.

new sacrificial bug offerings created: 85215, 85221