[webkit-reviews] review denied: [Bug 26907] [Chromium] Upstream V8SVGPODTypeWrapper : [Attachment 32150] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 26907: [Chromium] Upstream V8SVGPODTypeWrapper
https://bugs.webkit.org/show_bug.cgi?id=26907

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

------- Additional Comments from David Levin <levin at chromium.org>
Just a few things to take care of.


> Index: WebCore/bindings/v8/V8SVGPODTypeWrapper.h
> +/*
> + * Copyright (C) 2006, 2008 Nikolas Zimmermann <zimmermann at kde.org>
> + * Copyright (C) 2008 Apple Inc. All rights reserved.
> + * Copyright (C) 2008 The Chromium Authors. All rights reserved.

I think this should be "Google" instead of "The Chromium Authors".  Also I'm
sure you did enough changes to add 2009.


> +#ifndef V8SVGPODTypeWrapper_h
> +#define V8SVGPODTypeWrapper_h
> +
> +#if ENABLE(SVG)
> +
> +#include "config.h"

I don't think header files are suppose to include config.h.



> +public:
> +    V8SVGStaticPODTypeWrapper(PODType type)
> +	   : m_podType(type)
> +    { }

Once you've put the { } on a new line, just put each on its own line.


> +private:
> +    // Update callbacks

Is this comment useful?



> +    explicit PODTypeWrapperCacheInfo(WTF::HashTableDeletedValueType)
> +	   : creator(reinterpret_cast<PODTypeCreator*>(-1))
> +	   , getter(0)
> +	   , setter(0)
> +    {
> +    }
> +    bool isHashTableDeletedValue() const

Add a blank line before this method.



> +template<typename PODType, typename PODTypeCreator>
> +struct PODTypeWrapperCacheInfoHash {
> +    static unsigned hash(const PODTypeWrapperCacheInfo<PODType,
PODTypeCreator>& info)
> +    {
> +	   unsigned creator = reinterpret_cast<unsigned>(info.creator);
> +	   unsigned getter = reinterpret_cast<unsigned>(*(void**)&info.getter);

> +	   unsigned setter = reinterpret_cast<unsigned>(*(void**)&info.setter);


Can reinterpret_cast be used for the void**?



> +template<typename PODType, typename PODTypeCreator>
> +struct PODTypeWrapperCacheInfoTraits :
WTF::GenericHashTraits<PODTypeWrapperCacheInfo<PODType, PODTypeCreator> > {
> +    typedef PODTypeWrapperCacheInfo<PODType, PODTypeCreator> CacheInfo;
> +
> +    static const bool emptyValueIsZero = true;
> +    static const bool needsDestruction = false;
> +
> +    static const CacheInfo& emptyValue()
> +    {
> +	   static CacheInfo key;

Consider using DEFINE_STATIC_LOCAL.



> +    static DynamicWrapperHashMap& dynamicWrapperHashMap()
> +    {
> +	   static DynamicWrapperHashMap _dynamicWrapperHashMap;

Consider using DEFINE_STATIC_LOCAL (and getting rid of the preceding "_").



> +template <class P>
> +P V8SVGPODTypeUtil::toSVGPODType(V8ClassIndex::V8WrapperType type,
v8::Handle<v8::Value> object, bool& ok) {

Brace style