[webkit-reviews] review denied: [Bug 57927] Expose unified Quota API if QUOTA build flag is enabled : [Attachment 88442] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Kinuko Yasuda
<kinuko at chromium.org>'s request for review:
Bug 57927: Expose unified Quota API if QUOTA build flag is enabled
https://bugs.webkit.org/show_bug.cgi?id=57927

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

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88442&action=review

> Source/WebCore/ChangeLog:15
> +	   No new tests: tests will be added when we add the implementation.

Usually the idl is the last thing added (after the implementation) and tests
are required when the idl is added.


Maybe you should do the changes without the idl and add it in with tests when
the impl is there.

> Source/WebCore/page/DOMWindow.cpp:111
> +#if ENABLE(QUOTA)

No if needed here. (The header has it.) Just put the include with the others.

> Source/WebCore/storage/StorageInfo.cpp:38
> +#include "ScriptExecutionContext.h"

Not needed a fwd decl will do.

> Source/WebCore/storage/StorageInfo.h:49
> +	   TEMPORARY,

Enum members should user InterCaps with an initial capital letter. (I think
you're using a chromium style.)

> Source/WebCore/storage/StorageInfo.h:58
> +    ~StorageInfo();

Consider making this private and making RefCounted<StorageInfo> a friend.

> Source/WebCore/storage/StorageInfo.idl:2
> + * Copyright (C) 2008, 2009 Apple Inc. All rights reserved.

This is an odd copyright header.

> Source/WebKit/chromium/src/StorageInfoChromium.cpp:57
> +    }

What if !isDocument?

I'm bothered by the raw pointer (new...) but I understand. I wonder if there is
another way to do this.

> Source/WebKit/chromium/src/StorageInfoChromium.cpp:67
> +    }

Ditto.