Bug 29655 - Chromium needs to be able to override the way storage events are delivered
Summary: Chromium needs to be able to override the way storage events are delivered
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-22 15:23 PDT by Jeremy Orlow
Modified: 2009-09-30 11:17 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (31.10 KB, patch)
2009-09-22 15:27 PDT, Jeremy Orlow
fishd: review+
jorlow: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 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*.
Comment 1 Jeremy Orlow 2009-09-22 15:27:51 PDT
Created attachment 39951 [details]
Patch v1
Comment 2 Jeremy Orlow 2009-09-22 15:28:40 PDT
What do you think?
Comment 3 Michael Nordman 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(...);
}
Comment 4 Jeremy Orlow 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.
Comment 5 Eric Seidel (no email) 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?
Comment 6 Jeremy Orlow 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.
Comment 7 Jeremy Orlow 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
Comment 8 Jeremy Orlow 2009-09-30 11:17:36 PDT
Committed r48937: <http://trac.webkit.org/changeset/48937>