Changes made in one renderer are propagated to all renderes associated with the profile of that originating renderer. That's overkill. Only those renderers containing pages that are using the domstorage area for the origin or are listening for storage events via window.onstorage need to receive these mutation events. As it stands right now, all processes 'light up' a little on any domstorage mutation (which is pretty ugly if you have a lot of tabs open on unrelated sites). See http://code.google.com/p/chromium/issues/detail?id=128482 for more info.
Created attachment 143073 [details] lessChatty
Comment on attachment 143073 [details] lessChatty Attachment 143073 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12731715
Comment on attachment 143073 [details] lessChatty Attachment 143073 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12731716
Comment on attachment 143073 [details] lessChatty Attachment 143073 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12738532
Comment on attachment 143073 [details] lessChatty Attachment 143073 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12741353
Comment on attachment 143073 [details] lessChatty Attachment 143073 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12744293
Created attachment 143076 [details] lessChatty
Created attachment 143122 [details] lessChatty
cc'ing adam too
Comment on attachment 143122 [details] lessChatty View in context: https://bugs.webkit.org/attachment.cgi?id=143122&action=review > Source/WebCore/page/DOMWindow.cpp:1550 > +#if PLATFORM(CHROMIUM) We shouldn't be adding PLATFORM(CHROMIUM) ifdefs to DOMWindow.cpp. > Source/WebKit/chromium/src/StorageAreaProxy.cpp:128 > + Storage* storage = frame->domWindow()->optionalLocalStorage(); What is optionalLocalStorage ?
Comment on attachment 143122 [details] lessChatty The general idea of this patch is fine. The only wonky part is didAddStorageEventListener being conditional on PLATFORM(CHROMIUM). It's also a pretty subtle change that's likely to be broken if we don't add a test upstream that checks that we continue to do this correctly.
Comment on attachment 143122 [details] lessChatty Good question about a test. 5 of the existing test fail if the Storage instances are not created when a handler are attached, so a new test would add no value. storage/domstorage/events/basic-body-attribute.html -> unexpected test timed out storage/domstorage/events/basic-setattribute.html -> unexpected test timed out storage/domstorage/events/basic.html -> unexpected text diff mismatch storage/domstorage/events/case-sensitive.html -> unexpected text diff mismatch storage/domstorage/events/documentURI.html -> unexpected test timed out View in context: https://bugs.webkit.org/attachment.cgi?id=143122&action=review >> Source/WebCore/page/DOMWindow.cpp:1550 >> +#if PLATFORM(CHROMIUM) > > We shouldn't be adding PLATFORM(CHROMIUM) ifdefs to DOMWindow.cpp. I'm not super happy about that either. I'm not sure any of the alternatives are better. I'm pretty certain some of them are not. Some of the options as i see them... 0) Like it is, conditional on PLATFORM. Pros: most minimal and simple and obvious and constrained change that can be made Cons: risks gripes about PLATFORM conditionals 1) Do this unconditionally regardless of PLATFORM. Pros: no gripes about conditional complication Cons: doesn't really need to be there for other PLATFORMs (but it "shouldn't" hurt) 2) Hoist the inline conditional logic into either Storage.h or StorageArea.h as a static class method on one of those two classes. Pros: * removes the conditional from the monster DOMWindow.cpp file and puts it closer to the source of platform specific'ness. * expresses in the dom storage related .h file that something special happens at didAddStorageEventListener(window) time Cons: Storage|StorageArea.h would have to #include DOMWindow.h (that inclusion could also be conditional) 3) Other more invasive forms of indirection involving new source files and/or new virtual methods on a 'client' object somewhere in the webcore object graph. Pros: none Cons: obscurity, and unecessary runtime and congnitive complexity I'm open to suggestions. I put the patch up with #0 because it's the simplest possible to read/write/and review. #2 seems like a reasonable option to me. >> Source/WebKit/chromium/src/StorageAreaProxy.cpp:128 >> + Storage* storage = frame->domWindow()->optionalLocalStorage(); > > What is optionalLocalStorage ? Can you make a more constructive comment? It's simply a getter that doesn't force the creation of the underlying instance whereas the typically getter does. The name was patterned off of the pre-existing optionalApplicationCache getter. Storage* optionalSessionStorage() const { return m_sessionStorage.get(); } Storage* optionalLocalStorage() const { return m_localStorage.get(); } DOMApplicationCache* optionalApplicationCache() const { return m_applicationCache.get(); }
> >> Source/WebKit/chromium/src/StorageAreaProxy.cpp:128 > >> + Storage* storage = frame->domWindow()->optionalLocalStorage(); > > > > What is optionalLocalStorage ? > > Can you make a more constructive comment? It's simply a getter that doesn't force the creation of > the underlying instance whereas the typically getter does. The name was patterned off of the > pre-existing optionalApplicationCache getter. > Storage* optionalSessionStorage() const { return m_sessionStorage.get(); } > Storage* optionalLocalStorage() const { return m_localStorage.get(); } > DOMApplicationCache* optionalApplicationCache() const { return m_applicationCache.get(); } Ah, I see. There's a similar function for getting DOMWindow from Frame called existingDOMWindow. That might be a slightly clearer name, but we can worry about that later.
> Good question about a test. 5 of the existing test fail if the Storage instances are not created when a handler are attached, so a new test would add no value. Great. > >> Source/WebCore/page/DOMWindow.cpp:1550 > >> +#if PLATFORM(CHROMIUM) > > > > We shouldn't be adding PLATFORM(CHROMIUM) ifdefs to DOMWindow.cpp. > > I'm not super happy about that either. I'm not sure any of the alternatives are better. I'm pretty certain some of them are not. Some of the options as i see them... Can you help me understand why creating these objects is helpful? Is there some side effect of creating them that we might want to cause happen explicitly rather than implicitly?
> Can you help me understand why creating these objects is helpful? Is there some side effect of creating them that we might want to cause happen explicitly rather than implicitly? First, see http://code.google.com/p/chromium/issues/detail?id=128482. If that doesn't answer your "why" question, please come back with a more specific question. I don't see any reason to add more webkit API and plumbing than we already have. Three small changes amount to a large reduction in IPC spam. cr change 1) [commtted] https://chromiumcodereview.appspot.com/10413018 wk change 2) this one cr change 3) a call to the HasAreaOpen() method introduced in #1 void DOMStorageMessageFilter::SendDomStorageEvent( const dom_storage::DomStorageArea* area, const GURL& page_url, const NullableString16& key, const NullableString16& new_value, const NullableString16& old_value) { DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)); // Only send mutation events to processes which have the area open. bool originated_in_process = connection_dispatching_message_for_ != 0; if (originated_in_process || host_->HasAreaOpen(area->namespace_id(), area->origin())) { DOMStorageMsg_Event_Params params; params.origin = area->origin(); params.page_url = page_url; params.connection_id = connection_dispatching_message_for_; params.key = key; params.new_value = new_value; params.old_value = old_value; params.namespace_id = area->namespace_id(); Send(new DOMStorageMsg_Event(params)); } }
> First, see http://code.google.com/p/chromium/issues/detail?id=128482. If that doesn't answer your "why" question, please come back with a more specific question. Based on reading that bug and grepping around a bit, the answer to my question is that instantiating the WebCore::Storage object causes WebKit to ask the embedder for a WebStorageArea, which calls the WebStorageImpl constructor in webstoragearea_impl.cc, which sends the DOMStorageHostMsg_OpenStorageArea to the host, which causes HasAreaOpen to return true. It sounds like another approach would be to notify the embedder explicitly that someone is listening for storage events. The embedder could then send DOMStorageHostMsg_OpenStorageArea (or a similar message) to the host. The advantage of the second approach is that it's explicit rather than implicit and it avoiding having a wonky special case for PLATFORM(CHROMIUM) in DOMWindow.cpp. The disadvantage is that there's more total machinery since the notification travels on a different pathway. As a thought experiment, it might be worth considering how WebKit2 would approach this optimization if/when they support multiple WebProcesses. I suspect that approach is also more consistent with what would work for WebKit2.
(In reply to comment #16) > > First, see http://code.google.com/p/chromium/issues/detail?id=128482. If that doesn't answer your "why" question, please come back with a more specific question. > > Based on reading that bug and grepping around a bit, the answer to my question is that instantiating the WebCore::Storage object causes WebKit to ask the embedder for a WebStorageArea, which calls the WebStorageImpl constructor in webstoragearea_impl.cc, which sends the DOMStorageHostMsg_OpenStorageArea to the host, which causes HasAreaOpen to return true. > > It sounds like another approach would be to notify the embedder explicitly that someone is listening for storage events. The embedder could then send DOMStorageHostMsg_OpenStorageArea (or a similar message) to the host. As mentioned earlier, I don't see any reason to add more webkit API and plumbing than we already have. The disadvantages outweigh the advantages, a parallel mechanism just makes more room for error. There would also the balancing "no longer listening" call plumb thru and get right. > The advantage of the second approach is that it's explicit rather than implicit and it avoiding having a wonky special case for PLATFORM(CHROMIUM) in DOMWindow.cpp. The disadvantage is that there's more total machinery since the notification travels on a different pathway. We can do something reasonable with the PLATFORM conditional to eliminate it. > As a thought experiment, it might be worth considering how WebKit2 would approach this optimization if/when they support multiple WebProcesses. I suspect that approach is also more consistent with what would work for WebKit2. webkit2... sorry... no data
Also something that is not apparent by looking at code currently in the codebase is that the need to send events is not purely a function of whether there are handlers attached. The mutation events are also utilized (or will be soon) to maintain consistency of the renderer side cache. You can see this here... https://chromiumcodereview.appspot.com/10383123/
Ok. It sounds like we should try option (1): > 1) Do this unconditionally regardless of PLATFORM. > Pros: no gripes about conditional complication > Cons: doesn't really need to be there for other PLATFORMs (but it "shouldn't" hurt) Would you also be willing to update the comment to explain why we're doing this with less Chromium-specific jargon? Here's my hamfisted attempt: // Creating these WebCore::Storage objects informs the client that we'd like to receive // notifications about storage events that might be triggered in other processes. Rather // than subscribe to these notifications explicitly, we subscribe to them implicitly to // simply the work done by the client. I'm not 100% convinced that doing this implicitly is better, but I'd rather not argue about it.
(In reply to comment #19) > Ok. It sounds like we should try option (1): I'll run webkit/mac tests and let you know how it goes. > // Creating these WebCore::Storage objects informs the client that we'd like to receive > // notifications about storage events that might be triggered in other processes. Rather > // than subscribe to these notifications explicitly, we subscribe to them implicitly to > // simply the work done by the client. Comment sgtm. > I'm not 100% convinced that doing this implicitly is better, but I'd rather not argue about it. Thnx.
Created attachment 143644 [details] lessChatty New patch with option #1 (no longer PLATFORM conditional). Also updated the ChangeLog to describe the additional side effect of scheduling the area to be read from disk for non-chromium ports.
Comment on attachment 143644 [details] lessChatty Clearing flags on attachment: 143644 Committed r118254: <http://trac.webkit.org/changeset/118254>
All reviewed patches have been landed. Closing bug.