Bug 88512 – Move Quota related code out of DOMWindow and into the quota/ folder

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Move Quota related code out of DOMWindow and into the quota/ folder.

(Separated out from bug 88396)

[Kinuko Yasuda]

Created attachment 146245 [details]
Patch

[Kinuko Yasuda]

Created attachment 146246 [details]
Patch

[WebKit Review Bot]

Comment on attachment 146246 [details]
Patch

Attachment 146246 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12921196

New failing tests:
fast/table/multiple-captions-display.xhtml

[WebKit Review Bot]

Created attachment 146295 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kinuko Yasuda]

(In reply to comment #3)
> (From update of attachment 146246 [details])
> Attachment 146246 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12921196
> 
> New failing tests:
> fast/table/multiple-captions-display.xhtml

This test failure is unrelated to the change.

[Adam Barth]

Comment on attachment 146246 [details]
Patch

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...

[Kinuko Yasuda]

Created attachment 146503 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 146246 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146246&action=review

>> 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

Done.

>> Source/WebCore/Modules/quota/DOMWindowStorageInfo.h:43
>> +class DOMWindowStorageInfo {
> 
> DOMWindowStorageInfo -> DOMWindowQuota (we're trying to have these names match the module name)

Done.

>> Source/WebCore/Modules/quota/StorageInfo.cpp:64
>> +#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.

Good point, I don't think we need this actually.  Just dropped them.

>> Source/WebCore/Modules/quota/StorageInfo.idl:30
>> +        JSGenerateToNativeObject
> 
> Why is this needed?

Hmm.. doesn't look we need it.  Removed.

[Kinuko Yasuda]

Created attachment 146506 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #6)
> (From update of attachment 146246 [details])
> 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.

Updated the patch to use the Supplement pattern to cache the StorageInfo object.  Adam, can you take one more quick look?  Thanks!

[Adam Barth]

Comment on attachment 146506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146506&action=review

Looks great.  Thanks!

> Source/WebCore/Modules/quota/DOMWindowQuota.cpp:51
> +// static

We usually skip these sorts of comments.

[Kinuko Yasuda]

Committed r119954: <http://trac.webkit.org/changeset/119954>