[webkit-reviews] review granted: [Bug 88512] Move Quota related code out of DOMWindow and into the quota/ folder : [Attachment 146246] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 88512: Move Quota related code out of DOMWindow and into the quota/ folder
https://bugs.webkit.org/show_bug.cgi?id=88512

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146246&action=review


The main thing to address before landing is caching the StorageInfo object. 
I'm happy to take a look at an updated patch if you're unsure about the
Supplement pattern.

> Source/WebCore/Modules/quota/DOMWindowStorageInfo.cpp:45
> +    return StorageInfo::create();

Do we really want to return a new one each time?  I would have thought we'd
want to cache it so that window.webkitStorageInfo === window.webkitStorageInfo

> Source/WebCore/Modules/quota/DOMWindowStorageInfo.h:43
> +class DOMWindowStorageInfo {

DOMWindowStorageInfo -> DOMWindowQuota (we're trying to have these names match
the module name)

> Source/WebCore/Modules/quota/StorageInfo.cpp:64
> +#if !PLATFORM(CHROMIUM)
> +void StorageInfo::queryUsageAndQuota(ScriptExecutionContext*, int,
PassRefPtr<StorageInfoUsageCallback>, PassRefPtr<StorageInfoErrorCallback>)
> +{
> +    notImplemented();
> +}
> +
> +void StorageInfo::requestQuota(ScriptExecutionContext*, int, unsigned long
long, PassRefPtr<StorageInfoQuotaCallback>,
PassRefPtr<StorageInfoErrorCallback>)
> +{
> +    notImplemented();
> +}
> +#endif

Is there a reason to turn ENABLE(QUOTA) on without implementing these
functions?  If so, we do this is with a StorageInfoNone.cpp that folks can link
in if they don't implement the feature.

> Source/WebCore/Modules/quota/StorageInfo.idl:30
> +	   JSGenerateToNativeObject

Why is this needed?

> Source/WebCore/page/DOMWindow.cpp:-1946
> -StorageInfo* DOMWindow::webkitStorageInfo() const
> -{
> -    if (!isCurrentlyDisplayedInFrame())
> -	   return 0;
> -    if (!m_storageInfo)
> -	   m_storageInfo = StorageInfo::create();
> -    return m_storageInfo.get();
> -}

Ah, so it used to be cached.  We'll need to implement something like this logic
in DOMWindowQuota.  If you look at some of the other modules, you can see how
to do this with the Supplement pattern.

> Source/WebCore/storage/StorageInfo.cpp:-54
> -#if !PLATFORM(CHROMIUM)

Ah, I see this already existed.  It's fine to keep this in this patch since
you're just moving the file, but consider a followup patch that removes the
PLATFORM(CHROMIUM) ifdefs.

> Source/WebCore/storage/StorageInfo.idl:-30
> -	   JSGenerateToNativeObject

Ah, this is here too.  Ok.  I wonder if its needed...