Bug 29655 – Chromium needs to be able to override the way storage events are delivered

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

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

[Jeremy Orlow]

Created attachment 39951 [details]
Patch v1

[Jeremy Orlow]

What do you think?

[Michael Nordman]

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]

(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)]

Big patch in an area I don't know much about.  Who is the storage expert @ Apple?  Can you CC them?

[Jeremy Orlow]

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]

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]

Committed r48937: <http://trac.webkit.org/changeset/48937>