[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