Bug 27517 – Misc cleanup in DOM Storage.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

Description will be attached to patch shortly.

[Jeremy Orlow]

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.

[Darin Fisher (:fishd, Google)]

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.

[Jeremy Orlow]

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.

[Eric Seidel (no email)]

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.

[Jeremy Orlow]

Created attachment 33670 [details]
patch

[Jeremy Orlow]

Would be great if I could get a quick review.  This is the only thing standing between LocalStorage (mostly) working in Chromium.  :-)

[Jeremy Orlow]

Created attachment 33672 [details]
patch

[Eric Seidel (no email)]

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.

[Jeremy Orlow]

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.

[Eric Seidel (no email)]

I would rather replace it with an ASSERT(page) than just removing it. :)

[Jeremy Orlow]

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.

[Jeremy Orlow]

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