[Webkit-unassigned] [Bug 26907] [Chromium] Upstream V8SVGPODTypeWrapper

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26907


levin at chromium.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32150|review?                     |review-
               Flag|                            |




------- Comment #2 from levin at chromium.org  2009-07-01 15:58 PDT -------
(From update of attachment 32150)
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


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.