[webkit-reviews] review denied: [Bug 35634] [DRT/Chromium] Add CppVariant and CppBoundClass : [Attachment 49892] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 3 09:56:50 PST 2010


Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied TAMURA, Kent
<tkent at chromium.org>'s request for review:
Bug 35634: [DRT/Chromium] Add CppVariant and CppBoundClass
https://bugs.webkit.org/show_bug.cgi?id=35634

Attachment 49892: Patch
https://bugs.webkit.org/attachment.cgi?id=49892&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
Minor comments overall. Do you think Cpp* prefix on these classes makes any
sense in the context of DRT? I am having naming doubts, but can't come up with
anything better. Maybe fishd would have some ideas.

> +// Copyright (c) 2010 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

Incorrect license header. Please use standard WebKit header.

> +#include "config.h"
> +#include "CppBoundClass.h"
> +
> +#include "base/compiler_specific.h"
> +#include "base/string_util.h"
> +#include "public/WebBindings.h"
> +#include "public/WebFrame.h"
> +#include "public/WebString.h"
> +#include <wtf/Assertions.h>

> +
> +private:
> +    scoped_ptr<CppBoundClass::GetterCallback> m_callback;

Since we're using WTF already, let's use OwnPtr here.

> +    // If the given method is exposed by the C++ class associated with this
> +    // NPObject, invokes it with the given args and returns a result. 
Otherwise,
> +    // returns "undefined" (in the JavaScript sense).  Called by the JS
runtime.
> +    static bool invoke(NPObject*, NPIdentifier,
> +			  const NPVariant* args, uint32_t arg_count,
> +			  NPVariant* result);

arguments, argumentCount

> +/* static */

No need for these comments in WebKit-land.

> +    PropertyCallback* propertyCallback =
> +	   callback ? new GetterPropertyCallback(callback) : 0;

Let's just keep these on one line.

> +
> +void CppBoundClass::bindProperty(const string& name, CppVariant* prop)
> +{
> +    PropertyCallback* propertyCallback =
> +	   prop ? new CppVariantPropertyCallback(prop) : 0;

Ditto.

> +void CppBoundClass::bindToJavascript(WebFrame* frame, const WebString&
classname)
> +{
> +#if USE(JSC)
> +#error "This is not going to work anymore...but it's not clear what the
solution is...or if it's still necessary."
> +    JSC::JSLock lock(false);
> +#endif

This is not needed here.

> +
> +    // BindToWindowObject will take its own reference to the NPObject, and
clean
> +    // up after itself.  It will also (indirectly) register the object with
V8,
> +    // so we must remember this so we can unregister it when we're
destroyed.
> +    frame->bindToWindowObject(classname,
> +				 NPVARIANT_TO_OBJECT(*getAsCppVariant()));

Keep on one line.

> @@ -0,0 +1,179 @@
> +// Copyright (c) 2010 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

Wrong header.

> +
> +/*
> +  CppBoundClass class:
> +  This base class serves as a parent for C++ classes designed to be bound to

> +  JavaScript objects.
> +
> +  Subclasses should define the constructor to build the property and method
> +  lists needed to bind this class to a JS object.  They should also declare
> +  and define member variables and methods to be exposed to JS through
> +  that object.
> +*/

> +
> +#ifndef CppBoundClass_h
> +#define CppBoundClass_h
> +
> +#include "CppVariant.h"
> +#include "base/callback.h"

This dependency can be broken fairly easily. We only use a very small subset of
Chromium callback machinery.

> +#include "base/scoped_ptr.h"

As mentioned before, might be good to break this dependency and use
wtf/OwnPtr.h

> +#include <map>
> +#include <vector>

Can use wtf primitives here as well.

> +
> +    DISALLOW_COPY_AND_ASSIGN(CppBoundClass);

Can use wtf/Noncopyable

> +++ b/WebKitTools/DumpRenderTree/chromium/CppVariant.cpp
> @@ -0,0 +1,284 @@
> +// Copyright (c) 2010 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

Wrong header.

> +
> +// This file contains definitions for CppVariant.
> +

Gratuitous comment :)

> @@ -0,0 +1,109 @@
> +// Copyright (c) 2010 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

Header fail.

> +
> +  For a usage example, see cpp_binding_example.{h|cc}.

Rename this?

> +#include "third_party/npapi/bindings/npruntime.h"

This seems odd. How will this build here?


More information about the webkit-reviews mailing list