[webkit-reviews] review denied: [Bug 27517] Misc cleanup in DOM Storage. : [Attachment 33212] v1

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 27517: Misc cleanup in DOM Storage.
https://bugs.webkit.org/show_bug.cgi?id=27517

Attachment 33212: v1
https://bugs.webkit.org/attachment.cgi?id=33212&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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.