Bug 26907 – [Chromium] Upstream V8SVGPODTypeWrapper

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

See summary.

[Nate Chapin]

Created attachment 32150 [details]
patch

[David Levin]

Comment on attachment 32150 [details]
patch

Just a few things to take care of.


> Index: WebCore/bindings/v8/V8SVGPODTypeWrapper.h
> +/*
> + * Copyright (C) 2006, 2008 Nikolas Zimmermann <[e-mail]>
> + * 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

[Nate Chapin]

Created attachment 32158 [details]
patch2

[Dimitri Glazkov (Google)]

This has landed, right?

[Nate Chapin]

Yes, this landed at r45659.  Sorry about that.