[webkit-reviews] review granted: [Bug 31168] Add [HintAtomic] to IDLs to remove unnecessary string conversions in generated V8 DOM bindings : [Attachment 43381] path with different implementation

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jens Alfke <snej at chromium.org>'s
request for review:
Bug 31168: Add [HintAtomic] to IDLs to remove unnecessary string conversions in
generated V8 DOM bindings
https://bugs.webkit.org/show_bug.cgi?id=31168

Attachment 43381: path with different implementation
https://bugs.webkit.org/attachment.cgi?id=43381&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good. Good approach.

>      void setNameInternal(const String& name)
>      {   
> -	   m_name = name;
> +	   m_name = AtomicString(name);
>      }

This function is never called. It should be removed instead of modified.

> -	   static AtomicString toString(bool type) { return type ? "true" :
"false"; }
> +	   static AtomicString toString(bool type) { return AtomicString(type ?
"true" : "false"); }

> -	   static AtomicString toString(int type) { return
String::number(type); }
> +	   static AtomicString toString(int type) { return
AtomicString(String::number(type)); }

> -	   static AtomicString toString(long type) { return
String::number(type); }
> +	   static AtomicString toString(long type) { return
AtomicString(String::number(type)); }

> -	   static AtomicString toString(const SVGLength& type) { return
type.valueAsString(); }
> +	   static AtomicString toString(const SVGLength& type) { return
AtomicString(type.valueAsString()); }

> -	   static AtomicString toString(float type) { return
String::number(type); }
> +	   static AtomicString toString(float type) { return
AtomicString(String::number(type)); }

> -	   static AtomicString toString(const FloatRect& type) { return
String::format("%f %f %f %f", type.x(), type.y(), type.width(), type.height());
}
> +	   static AtomicString toString(const FloatRect& type) { return
AtomicString(String::format("%f %f %f %f", type.x(), type.y(), type.width(),
type.height())); }

> -	   static AtomicString toString(const String& type) { return type; }
> +	   static AtomicString toString(const String& type) { return
AtomicString(type); }

I do not understand why these functions return AtomicString objects! If an
AtomicString is really needed here, then we should have versions of these
functions that make the atomic string directly because making the String first
is not good for cases where the AtomicString already exists.

I haven't reviewed V8 binding code before, but elsewhere WebKit coding style
says the data member should be called m_v8Object, not _v8Object.

It also seems curious to me to use a template class instead of three different
classes for V8Parameter, and also a bit strange that class name doesn't mention
"string" at all.'

Otherwise seems fine. I'll say r=me but I would like some followup cleanup to
the AtomicString issues if you're willing.