Another stab at the Chromium side of storage events. The WebKit side can be ...

Another stab at the Chromium side of storage events.  The WebKit side can be found here: https://bugs.webkit.org/show_bug.cgi?id=29655

TEST=Manually inspected that storage events fired.  Will turn on more layout tests in a subsequent patch.
BUG=19972
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=27756

[jorlow, 2009-09-23 21:27:56]

The code review of the patch this replaces:
http://codereview.chromium.org/209047/show

[jorlow, 2009-09-23 21:59:54]

This version is _much_ easier to review.  :-)

I went through the diff and caught a bunch of silly stuff and refactored a
couple minor functions.

[michaeln, 2009-09-23 23:03:13]

This is a big improvement over the initial cut, thnx for putting it together, a
much happier reviewer here ;)

http://codereview.chromium.org/223013/diff/4001/76
File chrome/browser/in_process_webkit/browser_webkitclient_impl.h (right):

http://codereview.chromium.org/223013/diff/4001/76#newcode37
Line 37: const WebKit::WebString& origin, bool isLocalStorage);
ah... even better, no class instances involved at the source (browser process)
of the events.

http://codereview.chromium.org/223013/diff/4001/75
File chrome/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/223013/diff/4001/75#newcode54
Line 54: const DispatcherHostSet* GetDispatcherHostSet() const {
maybe dispatcher_hosts()

http://codereview.chromium.org/223013/diff/4001/75#newcode72
Line 72: // IO THREAD!
The all caps yelling isn't really needed, there are many many cases where the
same thread safety issue exists (afaik, this is true of all of the collections
in this class), but is not yelled about.

http://codereview.chromium.org/223013/diff/4001/75#newcode73
Line 73: DispatcherHostSet dispatcher_host_set_;
maybe dispatcher_hosts_

http://codereview.chromium.org/223013/diff/4001/71
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/223013/diff/4001/71#newcode19
Line 19: DOMStorageDispatcherHost* dispatcher_host) {
not sure how to reasonably wrap this line, but what you have looks odd

maybe...

DOMStorageDispatcherHost::
AutoSetCurrentDispatcherHost::AutoSetCurrentDispatcherHost(
    DOMStorageDispatcherHost* dispatcher_host) {
}

http://codereview.chromium.org/223013/diff/4001/71#newcode26
Line 26: ::~AutoSetCurrentDispatcherHost() {
ditto

http://codereview.chromium.org/223013/diff/4001/73
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h (right):

http://codereview.chromium.org/223013/diff/4001/73#newcode115
Line 115: struct hash<DOMStorageDispatcherHost*> {
nice to see this tucked away at the end of the .h file :)

http://codereview.chromium.org/223013/diff/4001/80
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/223013/diff/4001/80#newcode227
Line 227: dom_storage_dispatcher_host_->Init();
you're gonna have to merge with my recently submitted change right here

http://codereview.chromium.org/223013/diff/4001/69
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/223013/diff/4001/69#newcode94
Line 94: virtual void dispatchStorageEvent(const WebString& key, const
WebString& oldValue, const WebString& newValue, const WebString& origin, bool
isLocalStorage) = 0;
Comments about the 'directionaly' would be good. This is called when the
instance of webkit generates a storage event which must be propagated to all
child processes... only used in the browser process in chrome.

http://codereview.chromium.org/223013/diff/4001/68
File webkit/api/public/WebStorageEventDispatcher.h (right):

http://codereview.chromium.org/223013/diff/4001/68#newcode43
Line 43: virtual void dispatchStorageEvent(const WebString& key, const
WebString& oldValue, const WebString& newValue, const WebString& origin, bool
isLocalStorage) = 0;
Comments that indicate the 'directionality' could help. Not sure how to phrase
it, but this is called to poke events into an instance of webkit which will be
raised into documents running in that instance... only used child processes in
chrome.

http://codereview.chromium.org/223013/diff/4001/67
File webkit/api/public/WebStorageNamespace.h (right):

http://codereview.chromium.org/223013/diff/4001/67#newcode39
Line 39: class WebStorageEventDispatcher;
not needed here anymore

http://codereview.chromium.org/223013/diff/4001/67#newcode59
Line 59: // takes ownership of any WebStorageEventDispatcher* you pass in.
stale comment

[jorlow, 2009-09-23 23:13:32]

Nothing major, so I'll wait till Darin's review.

http://codereview.chromium.org/223013/diff/4001/75
File chrome/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/223013/diff/4001/75#newcode54
Line 54: const DispatcherHostSet* GetDispatcherHostSet() const {
On 2009/09/23 23:03:13, michaeln wrote:
> maybe dispatcher_hosts()

Actually, I think I'm going to move it into dom_storage_context_.h and add a
dcheck for our current thread...so it makes more sense to leave the name as is.

http://codereview.chromium.org/223013/diff/4001/75#newcode72
Line 72: // IO THREAD!
On 2009/09/23 23:03:13, michaeln wrote:
> The all caps yelling isn't really needed, there are many many cases where the
> same thread safety issue exists (afaik, this is true of all of the collections
> in this class), but is not yelled about.

Hmmm...but this is the only part of this class that should be done on the IO
thread.  I kind of feel like it is warranted.

http://codereview.chromium.org/223013/diff/4001/75#newcode73
Line 73: DispatcherHostSet dispatcher_host_set_;
On 2009/09/23 23:03:13, michaeln wrote:
> maybe dispatcher_hosts_

I've named a lot of the other stuff like this...if you guys feel strongly, I
should change it all....probably in a second patch.

I know type information generally doesn't need to be in the name or anything,
but whenever I see a name that's plural, I think it helps to know whether it's a
set, map, array, etc.  Maybe it's just me, though.

http://codereview.chromium.org/223013/diff/4001/71
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/223013/diff/4001/71#newcode19
Line 19: DOMStorageDispatcherHost* dispatcher_host) {
On 2009/09/23 23:03:13, michaeln wrote:
> not sure how to reasonably wrap this line, but what you have looks odd
> 
> maybe...
> 
> DOMStorageDispatcherHost::
> AutoSetCurrentDispatcherHost::AutoSetCurrentDispatcherHost(
>     DOMStorageDispatcherHost* dispatcher_host) {
> }
> 

This seems better.  Darin, thoughts?

http://codereview.chromium.org/223013/diff/4001/73
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h (right):

http://codereview.chromium.org/223013/diff/4001/73#newcode115
Line 115: struct hash<DOMStorageDispatcherHost*> {
On 2009/09/23 23:03:13, michaeln wrote:
> nice to see this tucked away at the end of the .h file :)

Yeah...no weird dependencies on DOMStorageContext

http://codereview.chromium.org/223013/diff/4001/80
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/223013/diff/4001/80#newcode227
Line 227: dom_storage_dispatcher_host_->Init();
On 2009/09/23 23:03:13, michaeln wrote:
> you're gonna have to merge with my recently submitted change right here

And another change of mine.  :-)

http://codereview.chromium.org/223013/diff/4001/69
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/223013/diff/4001/69#newcode94
Line 94: virtual void dispatchStorageEvent(const WebString& key, const
WebString& oldValue, const WebString& newValue, const WebString& origin, bool
isLocalStorage) = 0;
On 2009/09/23 23:03:13, michaeln wrote:
> Comments about the 'directionaly' would be good. This is called when the
> instance of webkit generates a storage event which must be propagated to all
> child processes... only used in the browser process in chrome.

The WebKit API has no notion of browser processes, render processes, etc....I
guess I could add some, but it won't add much information.

http://codereview.chromium.org/223013/diff/4001/68
File webkit/api/public/WebStorageEventDispatcher.h (right):

http://codereview.chromium.org/223013/diff/4001/68#newcode43
Line 43: virtual void dispatchStorageEvent(const WebString& key, const
WebString& oldValue, const WebString& newValue, const WebString& origin, bool
isLocalStorage) = 0;
On 2009/09/23 23:03:13, michaeln wrote:
> Comments that indicate the 'directionality' could help. Not sure how to phrase
> it, but this is called to poke events into an instance of webkit which will be
> raised into documents running in that instance... only used child processes in
> chrome. 

I do need a comment at the top, you're right.

http://codereview.chromium.org/223013/diff/4001/67
File webkit/api/public/WebStorageNamespace.h (right):

http://codereview.chromium.org/223013/diff/4001/67#newcode39
Line 39: class WebStorageEventDispatcher;
On 2009/09/23 23:03:13, michaeln wrote:
> not needed here anymore

True.

http://codereview.chromium.org/223013/diff/4001/67#newcode59
Line 59: // takes ownership of any WebStorageEventDispatcher* you pass in.
On 2009/09/23 23:03:13, michaeln wrote:
> stale comment

Ha...yes it is.

[michaeln, 2009-09-23 23:41:23]

http://codereview.chromium.org/223013/diff/4001/75
File chrome/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/223013/diff/4001/75#newcode72
Line 72: // IO THREAD!
oh... sorry... nm... maybe yell more loudly :)

[jorlow, 2009-09-25 20:15:43]

Ok.  I made the changes.

Can I get a LGTM?

[darin (slow to review), 2009-09-25 23:24:07]

my comment about long lines in webkit/api land applies to more than just the few
that i highlighted.  even though webkit style doesn't require line breaks, it
seems reasonable to break the lines to help readability.  it just doesn't have
to be at 80 chars!

http://codereview.chromium.org/223013/diff/10001/11023
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/223013/diff/10001/11023#newcode66
Line 66: extern const char* kPageGroupName;
this violates abstractions and would break a WebKit.dll build.   it seems like
there needs to be a proper WebKit API for getting the default page group name.

another choice would be to pass the page group name into WebView::create(..)

http://codereview.chromium.org/223013/diff/10001/11023#newcode253
Line 253: WebString(ASCIIToUTF16(kPageGroupName))));
WebString::fromUTF8

http://codereview.chromium.org/223013/diff/10001/11024
File chrome/renderer/renderer_webstoragenamespace_impl.cc (right):

http://codereview.chromium.org/223013/diff/10001/11024#newcode32
Line 32: const WebString& origin) {
should this be using the recently added WebSecurityOrigin type?

http://codereview.chromium.org/223013/diff/10001/11011
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/223013/diff/10001/11011#newcode94
Line 94: virtual void dispatchStorageEvent(const WebString& key, const
WebString& oldValue, const WebString& newValue, const WebString& origin, bool
isLocalStorage) = 0;
some line wrapping here would help readability imo

http://codereview.chromium.org/223013/diff/10001/11010
File webkit/api/public/WebStorageEventDispatcher.h (right):

http://codereview.chromium.org/223013/diff/10001/11010#newcode43
Line 43: static WebStorageEventDispatcher* create(const WebString&
pageGroupName);
nit: insert newline after this line

http://codereview.chromium.org/223013/diff/10001/11010#newcode47
Line 47: virtual void dispatchStorageEvent(const WebString& key, const
WebString& oldValue, const WebString& newValue, const WebString& origin, bool
isLocalStorage) = 0;
nit: some line wrapping would help readability imo

http://codereview.chromium.org/223013/diff/10001/11009
File webkit/api/src/StorageEventDispatcherChromium.cpp (right):

http://codereview.chromium.org/223013/diff/10001/11009#newcode45
Line 45: void StorageEventDispatcher::dispatch(const String& key, const String&
oldValue, const String& newValue, StorageType storageType, SecurityOrigin*
origin, Frame* sourceFrame)
nit: same nit about doing some line breaking

[michaeln, 2009-09-25 23:24:32]

LGTM

[jorlow, 2009-09-25 23:42:23]

One outstanding question about the default page group name.

http://codereview.chromium.org/223013/diff/10001/11023
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/223013/diff/10001/11023#newcode66
Line 66: extern const char* kPageGroupName;
On 2009/09/25 23:24:07, darin wrote:
> this violates abstractions and would break a WebKit.dll build.

It would?  Glue will be on the Chromium side of the DLL break, right?  Or are
you anticipating where it'll go in the future?

> it seems like
> there needs to be a proper WebKit API for getting the default page group name.

Which direction?  It can be injected via the constructor or we can use the
webKitClient to grab it on demand.
 
> another choice would be to pass the page group name into WebView::create(..)

http://codereview.chromium.org/223013/diff/10001/11023#newcode253
Line 253: WebString(ASCIIToUTF16(kPageGroupName))));
On 2009/09/25 23:24:07, darin wrote:
> WebString::fromUTF8

Done.

http://codereview.chromium.org/223013/diff/10001/11024
File chrome/renderer/renderer_webstoragenamespace_impl.cc (right):

http://codereview.chromium.org/223013/diff/10001/11024#newcode32
Line 32: const WebString& origin) {
On 2009/09/25 23:24:07, darin wrote:
> should this be using the recently added WebSecurityOrigin type?

Maybe.  If so, it shoudln't be part of this CL though.

http://codereview.chromium.org/223013/diff/10001/11011
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/223013/diff/10001/11011#newcode94
Line 94: virtual void dispatchStorageEvent(const WebString& key, const
WebString& oldValue, const WebString& newValue, const WebString& origin, bool
isLocalStorage) = 0;
On 2009/09/25 23:24:07, darin wrote:
> some line wrapping here would help readability imo

Done.

http://codereview.chromium.org/223013/diff/10001/11010
File webkit/api/public/WebStorageEventDispatcher.h (right):

http://codereview.chromium.org/223013/diff/10001/11010#newcode43
Line 43: static WebStorageEventDispatcher* create(const WebString&
pageGroupName);
On 2009/09/25 23:24:07, darin wrote:
> nit: insert newline after this line

Done.

Is that actually convention?  I feel like I've seen it both ways.

http://codereview.chromium.org/223013/diff/10001/11010#newcode47
Line 47: virtual void dispatchStorageEvent(const WebString& key, const
WebString& oldValue, const WebString& newValue, const WebString& origin, bool
isLocalStorage) = 0;
On 2009/09/25 23:24:07, darin wrote:
> nit: some line wrapping would help readability imo

K...note that the original WebCore code did not, so I didn't here mainly so it
looked similar.  It's not a big deal, thoguh.

http://codereview.chromium.org/223013/diff/10001/11009
File webkit/api/src/StorageEventDispatcherChromium.cpp (right):

http://codereview.chromium.org/223013/diff/10001/11009#newcode45
Line 45: void StorageEventDispatcher::dispatch(const String& key, const String&
oldValue, const String& newValue, StorageType storageType, SecurityOrigin*
origin, Frame* sourceFrame)
On 2009/09/25 23:24:07, darin wrote:
> nit: same nit about doing some line breaking

Done.

[darin (slow to review), 2009-09-26 04:50:48]

http://codereview.chromium.org/223013/diff/10001/11023
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/223013/diff/10001/11023#newcode66
Line 66: extern const char* kPageGroupName;
On 2009/09/25 23:42:23, Jeremy Orlow wrote:
> On 2009/09/25 23:24:07, darin wrote:
> > this violates abstractions and would break a WebKit.dll build.
> 
> It would?  Glue will be on the Chromium side of the DLL break, right?  Or are
> you anticipating where it'll go in the future?

The WebView implementation will need to move into webkit/api before we can
declare victory.



> 
> > it seems like
> > there needs to be a proper WebKit API for getting the default page group
name.
> 
> Which direction?  It can be injected via the constructor or we can use the
> webKitClient to grab it on demand.

In general, I prefer for settings to be push instead of pull.  Adding a new
parameter to WebView::create seems best so that we can leave it up to the
consumer to define the page group name.  This could cause problems if we assume
one global page group elsewhere in webkit/{api,glue}.

If that is the case, then we could just have a static method on WebView that
returns the default page group name.  But, once we leak the concept of page
group name through the WebKit API, we might as well allow for the usage of
different page groups :-/

Hmm...

Actually, I just thought of something (sorry for the train of thought):  why do
you need the page group name here?  Why not just have the implementation of
WebStorageEventDispatcher::create know the default page group name?  Yeah, I
think that would be better.

Ultimately, both the WebStorageEventDispatcher and WebViewImpl will live within
the WebKit API implementation, so it is easy for them to share this constant.


>  
> > another choice would be to pass the page group name into WebView::create(..)
> 
>

http://codereview.chromium.org/223013/diff/10001/11024
File chrome/renderer/renderer_webstoragenamespace_impl.cc (right):

http://codereview.chromium.org/223013/diff/10001/11024#newcode32
Line 32: const WebString& origin) {
On 2009/09/25 23:42:23, Jeremy Orlow wrote:
> On 2009/09/25 23:24:07, darin wrote:
> > should this be using the recently added WebSecurityOrigin type?
> 
> Maybe.  If so, it shoudln't be part of this CL though.

yeah, definitely ok to punt.

http://codereview.chromium.org/223013/diff/10001/11010
File webkit/api/public/WebStorageEventDispatcher.h (right):

http://codereview.chromium.org/223013/diff/10001/11010#newcode43
Line 43: static WebStorageEventDispatcher* create(const WebString&
pageGroupName);
On 2009/09/25 23:42:23, Jeremy Orlow wrote:
> On 2009/09/25 23:24:07, darin wrote:
> > nit: insert newline after this line
> 
> Done.
> 
> Is that actually convention?  I feel like I've seen it both ways.

separation between static methods and non-static methods seems good for
readability to me.  maybe i'm just strange like that :-/

[jorlow, 2009-09-28 21:36:20]

New version uploaded.

http://codereview.chromium.org/223013/diff/10001/11023
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/223013/diff/10001/11023#newcode66
Line 66: extern const char* kPageGroupName;
On 2009/09/26 04:50:49, darin wrote:
> On 2009/09/25 23:42:23, Jeremy Orlow wrote:
> > On 2009/09/25 23:24:07, darin wrote:
> > > this violates abstractions and would break a WebKit.dll build.
> > 
> > It would?  Glue will be on the Chromium side of the DLL break, right?  Or
are
> > you anticipating where it'll go in the future?
> 
> The WebView implementation will need to move into webkit/api before we can
> declare victory.
> 
> 
> 
> > 
> > > it seems like
> > > there needs to be a proper WebKit API for getting the default page group
> name.
> > 
> > Which direction?  It can be injected via the constructor or we can use the
> > webKitClient to grab it on demand.
> 
> In general, I prefer for settings to be push instead of pull.  Adding a new
> parameter to WebView::create seems best so that we can leave it up to the
> consumer to define the page group name.  This could cause problems if we
assume
> one global page group elsewhere in webkit/{api,glue}.
> 
> If that is the case, then we could just have a static method on WebView that
> returns the default page group name.  But, once we leak the concept of page
> group name through the WebKit API, we might as well allow for the usage of
> different page groups :-/
> 
> Hmm...
> 
> Actually, I just thought of something (sorry for the train of thought):  why
do
> you need the page group name here?  Why not just have the implementation of
> WebStorageEventDispatcher::create know the default page group name?  Yeah, I
> think that would be better.
> 
> Ultimately, both the WebStorageEventDispatcher and WebViewImpl will live
within
> the WebKit API implementation, so it is easy for them to share this constant.
> 
> 
> >  
> > > another choice would be to pass the page group name into
WebView::create(..)
> > 
> > 
> 
> 

I moved it into the API and changed the constant to use WebKit style since it'll
soon be in the API and not glue.

[darin (slow to review), 2009-09-28 21:51:13]

LGTM

http://codereview.chromium.org/223013/diff/10014/10018
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/223013/diff/10014/10018#newcode134
Line 134: // TODO(port): This needs to go into the WebKit API implementation and
be hidden
nit: TODO(port) is for things that need to be done as part of porting to Mac or
Linux.  Also, this file is going to be in the WebKit API shortly, so it isn't
really a TODO to change this, right?

[jorlow, 2009-09-28 21:58:57]

http://codereview.chromium.org/223013/diff/10014/10018
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/223013/diff/10014/10018#newcode134
Line 134: // TODO(port): This needs to go into the WebKit API implementation and
be hidden
On 2009/09/28 21:51:13, darin wrote:
> nit: TODO(port) is for things that need to be done as part of porting to Mac
or
> Linux.

Oops.  Got mixed up.

> Also, this file is going to be in the WebKit API shortly, so it isn't
> really a TODO to change this, right?

I thought it was worth the note for whomever moves it...even though it'll
probably be you.

I made it a FIXME since the rest is in WebKit style anyway.

[darin (slow to review), 2009-09-28 22:00:56]

OK

On Mon, Sep 28, 2009 at 2:58 PM, <jorlow@chromium.org> wrote:

>
> http://codereview.chromium.org/223013/diff/10014/10018
> File webkit/glue/webview_impl.cc (right):
>
> http://codereview.chromium.org/223013/diff/10014/10018#newcode134
> Line 134: // TODO(port): This needs to go into the WebKit API
> implementation and be hidden
> On 2009/09/28 21:51:13, darin wrote:
>
>> nit: TODO(port) is for things that need to be done as part of porting
>>
> to Mac or
>
>> Linux.
>>
>
> Oops.  Got mixed up.
>
>  Also, this file is going to be in the WebKit API shortly, so it isn't
>> really a TODO to change this, right?
>>
>
> I thought it was worth the note for whomever moves it...even though
> it'll probably be you.
>
> I made it a FIXME since the rest is in WebKit style anyway.
>
>
> http://codereview.chromium.org/223013
>