Description will be attached to patch shortly.
Created attachment 33212 [details] v1 The StorageAreaImpl changes are all for Chromium. Because the DOM Storage implementation runs in a different process from where the Frame object lives, Chromium passes in NULL for the sourceFrame. This affects events and handling privateBrowsing. Chromium's incognito mode does not use the private browsing setting, so that's not a concern. As for events, I've decided to simply disable them for now. The StorageNamespaceImpl changes get rid of a stale comment (path is .copy'ed for thread-safety) and to add an assert that .copy is only ever called on a SessionStorage namespace.
Comment on attachment 33212 [details] v1 > Index: WebCore/storage/StorageAreaImpl.cpp ... > + // The frame pointer can be NULL in Chromium since this call is made in a > + // different process from where the Frame object exists. Luckily private > + // browsing is implemented differently in Chromium, so it'd never return > + // true anyway. This is a good comment for the ChangeLog since it is the kind of thing that could get stale, and it refers to things like Chrome's multi-process architecture that are not visible to someone just looking at the WebKit code base. We have made a concerted effort to avoid putting comments into WebKit that refer to Chrome and Chrome's architecture as much as we can. If we can instead have WebKit "make sense" standalone then it is a good thing. It means WebKit will be easier to understand. Ultimately, I think we should probably revise StorageAreaImpl to not depend directly on having a Page with Settings. Or, in Chromium we should have a dummy Page so that we can provide Settings. Or, finally... the simplest thing is to just have a comment that says StorageAreaImpl should be usable when there is no associated Frame. We could have a single comment in the header say that documents that. We could add that Chromium uses it that way without needing to give much more detail. A blanket comment like that would help someone else who is tempted to add additional code to touch the Frame, Page, or Settings. Otherwise, looks good.
Created attachment 33501 [details] v2 I haven't decided what approach I'm going to take with events yet. I'll have to think carefully about how it can be tied in with Chromium in a way that's still very clean from the WebKit side of things. Until then, I've placed FIXME's where we're dong the frame checks. I'll come up with a better long term solution soon, but since we're the only implementation that would supply NULLS, I think this is an acceptable short-term solution. So...changed the comments in StorageAreaImpl.cpp. Also cleaned up a bunch of trailing whitespace because once I started I couldn't stop.
Comment on attachment 33501 [details] v2 I would rather you abstract this into a function, something like this: static bool privateBrowsingEnabled(frame) { #ifdef CHROMIUM // comment explaing why it's OK for frame to be null return false; #else return frame->.... #endif } Otherwis this looks fine.
Created attachment 33670 [details] patch
Would be great if I could get a quick review. This is the only thing standing between LocalStorage (mostly) working in Chromium. :-)
Created attachment 33672 [details] patch
Comment on attachment 33672 [details] patch Why remove: // FIXME: When can this occur? Also, instead of removing // FIXME: Is the .copy necessary? it seems replacing it with // copy for safe muti-threaded access or some similar comment which explains the "why" since clearly the author of that FIXME didn't understand "why" :) Otherwise looks fine.
Added in the comment on the .copy(). As for the other FIXME, I still don't know when it's possible, but I suspect this was put in because page() can return NULL. I added the fixme originally and I don't think anyone's touched the code since, so I figured it was OK to remove.
I would rather replace it with an ASSERT(page) than just removing it. :)
I'll create another bug to review this and a couple other places where I think there are unnecessary if statements. I think it'll need careful review.
Sending WebCore/ChangeLog Deleting WebCore/storage/StorageArea.cpp Sending WebCore/storage/StorageAreaImpl.cpp Sending WebCore/storage/StorageNamespaceImpl.cpp Transmitting file data ... Committed revision 46502. http://trac.webkit.org/changeset/46502