[Webkit-unassigned] [Bug 29909] [V8] Chromium's implementation of ScriptString is awful for XHR's responseText

[bugzilla-daemon at webkit.org]

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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

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




--- Comment #2 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2009-09-30 11:47:21 PDT ---
(From update of attachment 40340)
> 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

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