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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Apr 6 11:06:09 PDT 2011

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

Attachment 88442: Patch

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

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

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


More information about the webkit-reviews mailing list