RESOLVED FIXED 29655
Chromium needs to be able to override the way storage events are delivered
https://bugs.webkit.org/show_bug.cgi?id=29655
Summary Chromium needs to be able to override the way storage events are delivered
Jeremy Orlow
Reported 2009-09-22 15:23:08 PDT
Chromium needs to be able to override the way storage events are delivered. This is a possible alternative to https://bugs.webkit.org/show_bug.cgi?id=29257 that would be faster (no vtables and extra allocation) and somewhat cleaner (no dependency injection). This is necessary because Chromium needs to transport events across a process barrier and then dispatch them without use of a Frame*.
Attachments
Patch v1 (31.10 KB, patch)
2009-09-22 15:27 PDT, Jeremy Orlow
fishd: review+
jorlow: commit-queue-
Jeremy Orlow
Comment 1 2009-09-22 15:27:51 PDT
Created attachment 39951 [details] Patch v1
Jeremy Orlow
Comment 2 2009-09-22 15:28:40 PDT
What do you think?
Michael Nordman
Comment 3 2009-09-22 16:01:43 PDT
I'm not looking at the other change side-by-side with this one, but this one seems super straight forward. So +1 in my book. ************************* I think getting rid of all of these... 31 #include "DOMWindow.h" 32 #include "EventNames.h" 33 #include "ExceptionCode.h" 34 #include "Frame.h" 35 #include "Page.h" 36 #include "PageGroup.h" 37 #include "SecurityOrigin.h" 38 #include "Settings.h" 39 #include "StorageEvent.h" 40 #include "StorageAreaSync.h" And replacing them with just this... 35 #include "StorageEventDispatcher.h" ... is a really good sign *********************** Not a big deal, you could make the StorageAreaImpl changes even smaller by retaining the dispatchStorageEvent() method, but implementing it in terms of your new static method. void StorageAreaImpl::dispatchStorageEvent(...) { StorageEventDispatcher::dispatch(...); }
Jeremy Orlow
Comment 4 2009-09-22 16:08:59 PDT
(In reply to comment #3) > I'm not looking at the other change side-by-side with this one, but this one > seems super straight forward. So +1 in my book. > > ************************* > I think getting rid of all of these... > > 31 #include "DOMWindow.h" > 32 #include "EventNames.h" > 33 #include "ExceptionCode.h" > 34 #include "Frame.h" > 35 #include "Page.h" > 36 #include "PageGroup.h" > 37 #include "SecurityOrigin.h" > 38 #include "Settings.h" > 39 #include "StorageEvent.h" > 40 #include "StorageAreaSync.h" > > And replacing them with just this... > > 35 #include "StorageEventDispatcher.h" > > ... is a really good sign Well, to be fair, essentially I split StorageAreaImpl into that and StorageEventDispatcher. It just so happens that half the includes were only necessary for that one function. :-) > *********************** > Not a big deal, you could make the StorageAreaImpl changes even smaller by > retaining the dispatchStorageEvent() method, but implementing it in terms of > your new static method. > > void StorageAreaImpl::dispatchStorageEvent(...) > { > StorageEventDispatcher::dispatch(...); > } I thought about it, but it didn't really seem worth it. I can change if anyone feels strongly.
Eric Seidel (no email)
Comment 5 2009-09-23 17:41:31 PDT
Big patch in an area I don't know much about. Who is the storage expert @ Apple? Can you CC them?
Jeremy Orlow
Comment 6 2009-09-23 17:45:05 PDT
Sure, but Darin's actually done 95% of the reviews of this stuff lately. If Brady has time to review a DOM Storage patch, looking at https://bugs.webkit.org/show_bug.cgi?id=29265 would be much more valuable.
Jeremy Orlow
Comment 7 2009-09-23 17:47:26 PDT
Also, if you look closely, the actual change is quite small and obviously correct. 90% of the patch is just a svn cp since I'm splitting DOM Storage event dispatching into its own file. Btw, in case anyone's interested in the Chromium side of this (which is mostly finished): http://codereview.chromium.org/223013/show
Jeremy Orlow
Comment 8 2009-09-30 11:17:36 PDT
Note You need to log in before you can comment on or make changes to this bug.