[webkit-reviews] review denied: [Bug 29909] [V8] Chromium's implementation of ScriptString is awful for XHR's responseText : [Attachment 40340] One possible ScriptString implementation for v8

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied James Robinson
<jamesr at chromium.org>'s request for review:
Bug 29909: [V8] Chromium's implementation of ScriptString is awful for XHR's
responseText
https://bugs.webkit.org/show_bug.cgi?id=29909

Attachment 40340: One possible ScriptString implementation for v8
https://bugs.webkit.org/attachment.cgi?id=40340&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebCore/bindings/v8/ScriptString.h
...
> +// This class is not thread-safe.  To access the data from multiple threads
> +// the caller must serialize into a String object and explicitly copy() the
> +// data.
>  class ScriptString {
>  public:
> +    ScriptString() : m_impl(new ScriptStringImpl) {}

it seems like it would be nice if ScriptString's default constructor did no
allocations.  that's how String works, and it seems nice for cases where you
have a temporary ScriptString on the stack or a ScriptString as a member var.


> Index: WebCore/bindings/v8/ScriptStringImpl.h
...
> + * Copyright (C) 2008, 2009 Google Inc. All rights reserved.

2008?


> +class ScriptStringImpl : public RefCounted<ScriptStringImpl> {
...
> +private:
> +  // Forces all appended strings into a flat buffer (stored as m_buffer).
> +  void UpdateBuffer() const;

nit: indent by 4 spaces


> +  mutable Vector<String> m_appended_strings;

nit: m_appendedStrings per webkit style


> Index: WebCore/bindings/v8/ScriptStringImpl.cpp
...
> + * Copyright (C) 2008, 2009 Google Inc. All rights reserved.

2008?


> +#ifndef ScriptString_h
> +#define ScriptString_h
> +
> +#include "PlatformString.h"
> +#include "ScriptStringImpl.h"
> +
> +namespace WebCore {
> +
> +// This implementation is based on StringBuilder.h with some caching added.
> +
> +ScriptStringImpl::ScriptStringImpl()
> +    : m_length(0)
> +    , m_isNull(false) {
> +}
> +ScriptStringImpl::ScriptStringImpl(const String& s)
> +    : m_buffer(s)
> +    , m_length(s.length())
> +    , m_isNull(s.isNull()) {
> +}

nit: webkit style is to put the opening "{" on a new line


> +const UChar* ScriptStringImpl::data() const {
> +  UpdateBuffer();

nit: indent by 4 spaces


> +  if (!m_buffer.characters()) {
> +    // oh snap!
> +    return NULL;
> +  }
> +  return m_buffer.characters();

nit: the above null check and explicit null returns seems unnecessary.

maybe the data method should just be implemented inline as a call to
toString().characters().


> +// This function should never change any observable state, hence the
const-ness.
> +void ScriptStringImpl::UpdateBuffer() const {
> +    int count = m_appended_strings.size();
> +
> +    // No append() calls have been made since either construction or the
last UpdateBuffer() call, so
> +    // m_buffer is already up to date.
> +    if (count == 0) {
> +	 return;

nit: indent by 4 spaces
nit: do not use {}'s when the enclosed statement is one line


> Index: WebCore/bindings/v8/V8Binding.cpp

> +	   // TODO(jamesr): get this accounting right

nit: use FIXME instead of TODO(jamesr) per webkit style


> +	   // TODO(jamesr): get this accounting right

ditto


> +    //String webcoreString() { return String(m_scriptString); }

delete this commented out code?  it is good practice to avoid leaving
commented out code around unless it has a FIXME comment explaining
what needs to be done before it can be uncommented.


> Index: WebCore/bindings/v8/V8Binding.h

> +    // Convert a ScriptString to a V8 string.
> +    inline v8::Handle<v8::String> v8ScriptString(const ScriptString& string)

> +    {
> +	 return v8ExternalScriptString(string);
> +    }

nit: 4 space indent


> +    inline v8::Handle<v8::Value> v8StringOrNull(const ScriptString& str) {
> +	  return str.isNull() ? v8::Handle<v8::Value>(v8::Null()) :
v8::Handle<v8::Value>(v8ScriptString(str));
> +    }

nit: fix indentation