Bug 35634 – [DRT/Chromium] Add CppVariant and CppBoundClass

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kent Tamura]

[DRT/Chromium] Add CppVariant and CppBoundClass

[Kent Tamura]

Created attachment 49892 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 49892 [details]
Patch

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?

[Darin Fisher (:fishd, Google)]

>> +#include "third_party/npapi/bindings/npruntime.h"
>
>This seems odd. How will this build here?

Use #include <bindings/npruntime.h> just like WebBindings.h, or just include WebBindings.h :)

[Kent Tamura]

(In reply to comment #2)
> (From update of attachment 49892 [details])
> 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.

Cpp* is not so meaningful, but I feel it is not bad.

> > +// 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.

These files have changes by non-Googler.  What header should we add?  Just replace "Google Inc" with "The Chromium Authors" in the usual header?

[Adam Barth]

> These files have changes by non-Googler.  What header should we add?  Just
> replace "Google Inc" with "The Chromium Authors" in the usual header?

In the past, folks in the WebKit community have expressed uncertainty about attributing copyright to "The Chromium Authors."  My understanding is that this is actually ok legally, but we might want to be more specific if its feasible.  There are a few options:

1) Attribute copyright explicitly to everyone in the Chromium AUTHORS file (i.e., by listing them in the copyright notice).

2) Use the revision history of this file to filter that list down to those who have actually contributed to this file.

3) Convince the WebKit community that "The Chromium Authors" is a find entity to which to attribute copyright.

[Adam Barth]

s/find/fine/

[Dimitri Glazkov (Google)]

Don't feel blocked by the name issues. If you have another patch ready today, I am fine with the timezone-friendly reviewer r+ing it today after the rest of the nits are addressed.

[Kent Tamura]

Created attachment 49991 [details]
Proposed patch (rev.2)

[Kent Tamura]

> > +// 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.

I replaced them with the standard one with "The Chromium Authors".

> > +    scoped_ptr<CppBoundClass::GetterCallback> m_callback;
> 
> Since we're using WTF already, let's use OwnPtr here.

Done.

> > +    static bool invoke(NPObject*, NPIdentifier,
> > +                       const NPVariant* args, uint32_t arg_count,
> > +                       NPVariant* result);
> 
> arguments, argumentCount

Done.  Fixed other occurrences.

> > +/* static */
> 
> No need for these comments in WebKit-land.

Done.

> > +    PropertyCallback* propertyCallback =
> > +        callback ? new GetterPropertyCallback(callback) : 0;
> 
> Let's just keep these on one line.

Done.

> > +    PropertyCallback* propertyCallback =
> > +        prop ? new CppVariantPropertyCallback(prop) : 0;
> 
> Ditto.

Ditto.

> > +#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.

Removed.

> > +    frame->bindToWindowObject(classname,
> > +                              NPVARIANT_TO_OBJECT(*getAsCppVariant()));
> 
> Keep on one line.

Done.

> > +#include "base/callback.h"
> 
> This dependency can be broken fairly easily. We only use a very small subset of
> Chromium callback machinery.

Ok, I made specialized callback classes and put them into this file.

> > +#include "base/scoped_ptr.h"
> 
> As mentioned before, might be good to break this dependency and use
> wtf/OwnPtr.h

Done.

> > +#include <map>
> > +#include <vector>
> 
> Can use wtf primitives here as well.

Done.

> > +    DISALLOW_COPY_AND_ASSIGN(CppBoundClass);
> 
> Can use wtf/Noncopyable

Done.

> > +
> > +// This file contains definitions for CppVariant.
> > +
> 
> Gratuitous comment :)

Removed.

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

Removed.

> > +#include "third_party/npapi/bindings/npruntime.h"
> 
> This seems odd. How will this build here?

I supposed WebKit/chromium was in the include paths and third_party/npapi was extracted to there.
I changed it to WebBindings.h.  It seems better.

[Dimitri Glazkov (Google)]

Comment on attachment 49991 [details]
Proposed patch (rev.2)

I think we Adam mentioned "Chromium Authors", he meant it as a more long-term solution. At this point, finding actual contributors through history and attributing them directly the only option.

[Kent Tamura]

Created attachment 50064 [details]
Proposed patch rev.3 (Copyright header change)

[Kent Tamura]

(In reply to comment #10)
> (From update of attachment 49991 [details])
> I think we Adam mentioned "Chromium Authors", he meant it as a more long-term
> solution. At this point, finding actual contributors through history and
> attributing them directly the only option.

ok, I updated so.

[Dimitri Glazkov (Google)]

Comment on attachment 50064 [details]
Proposed patch rev.3 (Copyright header change)

> +/*
> + * Copyright (C) 2010 The Chromium Authors. All rights reserved.

Please fix up on landing..

[Kent Tamura]

(In reply to comment #13)
> > + * Copyright (C) 2010 The Chromium Authors. All rights reserved.
> 
> Please fix up on landing..

Oops, sure.
I had a mistake on copying files...

[Kent Tamura]

Landed as r55563.