WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
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
Committed
r48937
: <
http://trac.webkit.org/changeset/48937
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug