Adding srpc example to naclports.

Adding srpc example to naclports.  Updated scriptable to contain method and
function specific functionality.  Scriptables now only have two virtual 
functions to tell them what to implement.  Cleaned up style points and obsolete
comments.  Made some style changes to scriptable.

[David Springer, 2010-07-21 17:29:13]

We talked off-line about the separation of Scriptable and ScriptingBridge.  I
tried to capture that conversation in the large comments at the beginning of
scripting.h.

http://codereview.chromium.org/2856050/diff/1/2
File examples/srpc/duality/Makefile (right):

http://codereview.chromium.org/2856050/diff/1/2#newcode8
examples/srpc/duality/Makefile:8: # builds untrusted only.  No need for
npn_bridge
Obsolete comment, please remove.

http://codereview.chromium.org/2856050/diff/1/3
File examples/srpc/duality/duality.cc (right):

http://codereview.chromium.org/2856050/diff/1/3#newcode1
examples/srpc/duality/duality.cc:1: // Copyright 2008 The Native Client Authors.
All rights reserved.
"Native CLient SDK Authors" - in all files.

http://codereview.chromium.org/2856050/diff/1/4
File examples/srpc/duality/duality.h (right):

http://codereview.chromium.org/2856050/diff/1/4#newcode1
examples/srpc/duality/duality.h:1: // Copyright 2010 The Native Client Authors.
All rights reserved.
"Native Client SDK Authors".

http://codereview.chromium.org/2856050/diff/1/5
File examples/srpc/duality/duality.html (right):

http://codereview.chromium.org/2856050/diff/1/5#newcode16
examples/srpc/duality/duality.html:16: // the shared state of the two
application objects
Style Guide: comments should be Proper Sentences in whatever natural language
you use.  "Container for the shared state of the two application objects."

http://codereview.chromium.org/2856050/diff/1/6
File examples/srpc/duality/loader.cc (right):

http://codereview.chromium.org/2856050/diff/1/6#newcode14
examples/srpc/duality/loader.cc:14: // These functions are required by both the
develop and publish versions,
Obsolete facts, maybe "These functions are called when ..."

http://codereview.chromium.org/2856050/diff/1/6#newcode25
examples/srpc/duality/loader.cc:25: }
Style Guide: indentation.

http://codereview.chromium.org/2856050/diff/1/8
File examples/srpc/duality/npp_gate.cc (right):

http://codereview.chromium.org/2856050/diff/1/8#newcode17
examples/srpc/duality/npp_gate.cc:17: struct PluginContext {
"Plugin" is an overloaded term.  It refers to something build as a trusted NPAPI
module.  I prefer NaClContext or something that implies we have something
different and that runs in a different environment from trusted plugins.

http://codereview.chromium.org/2856050/diff/1/8#newcode19
examples/srpc/duality/npp_gate.cc:19: NPObject *npobject;
Is this weak reference necessary?  The browser should handle all the ref
counting and memory for it, and actually "owns" the reference.

http://codereview.chromium.org/2856050/diff/1/8#newcode23
examples/srpc/duality/npp_gate.cc:23: // that the browser can all them.  All of
them are required to be implemented
"can all" -> "can <what? call?> all"

http://codereview.chromium.org/2856050/diff/1/8#newcode24
examples/srpc/duality/npp_gate.cc:24: // regardless of whether this is a trusted
or untrusted build of the module.
"trusted or untrusted" - obsolete comment.  Please remove.

http://codereview.chromium.org/2856050/diff/1/8#newcode62
examples/srpc/duality/npp_gate.cc:62: fflush(stdout);
How do you handle plugin_context->npobject here?  The browser *should* have
released it by now - if you need to keep a weak reference, maybe assert that
it's NULL at this point?

http://codereview.chromium.org/2856050/diff/1/8#newcode78
examples/srpc/duality/npp_gate.cc:78: // Helper function for NPP_GetValue to
create this plugin's NPObject.
This sentence is not really needed.

http://codereview.chromium.org/2856050/diff/1/8#newcode160
examples/srpc/duality/npp_gate.cc:160: // When the browser called
NP_GetMIMEDescription, it calls a function of this
This function is only needed for Linux trusted plugins.  Please remove.

http://codereview.chromium.org/2856050/diff/1/9
File examples/srpc/duality/scriptable.h (right):

http://codereview.chromium.org/2856050/diff/1/9#newcode16
examples/srpc/duality/scriptable.h:16: // method table and property table.
We talked about this class, and why it's separated from ScriptingBridge.  I
understand the separation and it seems workable.  The scenario we want to
support is this:

1. The NaCl instance (or application or module) publishes an interface to the
browser (this is NPP_*).
2. An object in the NaCl module's task space can publish an interface that
provides access to it from JavaScript (via NPClass, this is the
ScriptingBridge).
3. A NaCl instance can publish a JS scripting bridge interface as well as it's
browser interface.
4. The NaCl instance can vend objects to the browser that conform to the JS
scripting interface.

Consider this HTML/JS application code:

<embed id="my_module" nexes="x86-32: my_module_x86_32.nexe">

var foo = my_module.getBar();
foo.someMethod();

In MyModule's code, there are three distinct interfaces:

1. MyModule's NPP interface, returned via NP_Initialize.
2. MyModule's scripting bridge interface, returned via
NPP_GetScriptableObject().  One method in this interface is MyModule.GetBar(),
which is bound to "getBar".
3. GetBar's scripting bridge, also returned via NPP_GetScriptableObject().

http://codereview.chromium.org/2856050/diff/1/9#newcode35
examples/srpc/duality/scriptable.h:35: // TODO(mlinck) make this use MemFree
You probably don't need to manage this memory in the browser.  It is fine - and
a lot simpler - to use malloc/new for memory owned by this object.

http://codereview.chromium.org/2856050/diff/1/9#newcode48
examples/srpc/duality/scriptable.h:48: // TODO(mlinck) make this use MemAlloc
(See above) The browser should not manage this memory.

http://codereview.chromium.org/2856050/diff/1/9#newcode61
examples/srpc/duality/scriptable.h:61: // table if it is uninitialized.
I would prefer overriding Init() to populate the table, and then provide two
accessors:
  Method* MethodForIdentifier(NPIdentifier np_id) const;
  Property* PropertyForItentifier(NPIdentifier np_id) const;

This encapsulates the table data and implementation.  The kind of container you
use for your method <-> id map should not be part of your API.

http://codereview.chromium.org/2856050/diff/1/10
File examples/srpc/duality/scripting_bridge.cc (right):

http://codereview.chromium.org/2856050/diff/1/10#newcode42
examples/srpc/duality/scripting_bridge.cc:42: fflush(stdout);
See comment in HasProperty - it seems that the separation between Scriptable and
ScriptingBridge could be better.

http://codereview.chromium.org/2856050/diff/1/10#newcode55
examples/srpc/duality/scripting_bridge.cc:55: fflush(stdout);
return ToInstance(object)->HasMethod(name);

http://codereview.chromium.org/2856050/diff/1/10#newcode66
examples/srpc/duality/scripting_bridge.cc:66: fflush(stdout);
Shouldn't this look up the method name in the method map?

http://codereview.chromium.org/2856050/diff/1/10#newcode85
examples/srpc/duality/scripting_bridge.cc:85: bool have_property = false;
Your separation of Scriptable and ScriptingBridge doesn't seem complete here.  I
would add an accessor on Scriptable that finds a method given an NPIdentifier. 
Same goes for properties (below).

http://codereview.chromium.org/2856050/diff/1/10#newcode101
examples/srpc/duality/scripting_bridge.cc:101: property_table_ =
application_->GetPropertyTable();
Consider adding API to the underlying object for property and method discovery,
so you don't have to cache references to these tables here.

http://codereview.chromium.org/2856050/diff/1/10#newcode132
examples/srpc/duality/scripting_bridge.cc:132: bool rval = false;
As with HasProperty, it seems that your separation of Scriptable and
ScriptingBridge is not complete.  This object has to know about the container
used by Scriptable, which should all be encapsulated in Scriptable.  An accessor
that returns a Scriptable::Method* would be better, e.g.:

Scriptable::Method* invoke_method =
application_->MethodForIdentifier(method_name);
if (invoke_method) {
  invoke_method(application_, args, arg_count, result);
}

http://codereview.chromium.org/2856050/diff/1/11
File examples/srpc/duality/scripting_bridge.h (right):

http://codereview.chromium.org/2856050/diff/1/11#newcode17
examples/srpc/duality/scripting_bridge.h:17: class ScriptingBridge : public
NPObject {
This can't have any virtual functions if it's a subclass of NPObject, since the
browser expects the memory to be laid out like a struct.  The vtable can't be
present.

http://codereview.chromium.org/2856050/diff/1/11#newcode23
examples/srpc/duality/scripting_bridge.h:23: // this plugin. The browser may
dereference the returned pointer any time
"plugin" -> "object" or "exposed object" or "published object"

http://codereview.chromium.org/2856050/diff/1/11#newcode25
examples/srpc/duality/scripting_bridge.h:25: // when it unloads the plugin.
The browser manages the memory for the scripting bridge object instance.  It
does not necessarily hang onto these objects until the NaCl module is unloaded -
it might dealloc them earlier than that.  It largely depends on the JS garbage
collection machinery.

http://codereview.chromium.org/2856050/diff/1/11#newcode42
examples/srpc/duality/scripting_bridge.h:42: // Documentation URL:
https://developer.mozilla.org/en/NPClass
Doesn't the dealloc method have to be a template as well?

http://codereview.chromium.org/2856050/diff/1/11#newcode129
examples/srpc/duality/scripting_bridge.h:129: NPObject* window_object_;
Why is this ivar here?  I don't see it used anywhere.

http://codereview.chromium.org/2856050/diff/1/11#newcode131
examples/srpc/duality/scripting_bridge.h:131: Scriptable * application_;
It might make more sense to call this "object_" or something more generic than
"application_" -  to make it clear that a ScriptingBridge can wrap any object,
not just the main application object.

Style Guide: Scriptbale*, no space before the *

http://codereview.chromium.org/2856050/diff/1/11#newcode163
examples/srpc/duality/scripting_bridge.h:163: ScriptableType *tApp = new
ScriptableType();
Style Guide: "tApp" -> "app".

"app" might be misleading here, given that *any* object can publish methods
across the scripting bridge.  This mechanism is not limited to the main
application object.

[mlinck, 2010-07-21 21:12:02]

I made the recommended changes.  Uploading new changelist momentarily.

http://codereview.chromium.org/2856050/diff/1/2
File examples/srpc/duality/Makefile (right):

http://codereview.chromium.org/2856050/diff/1/2#newcode8
examples/srpc/duality/Makefile:8: # builds untrusted only.  No need for
npn_bridge
On 2010/07/21 17:29:13, David Springer wrote:
> Obsolete comment, please remove.

Done.

http://codereview.chromium.org/2856050/diff/1/3
File examples/srpc/duality/duality.cc (right):

http://codereview.chromium.org/2856050/diff/1/3#newcode1
examples/srpc/duality/duality.cc:1: // Copyright 2008 The Native Client Authors.
All rights reserved.
On 2010/07/21 17:29:13, David Springer wrote:
> "Native CLient SDK Authors" - in all files.

Done.

http://codereview.chromium.org/2856050/diff/1/4
File examples/srpc/duality/duality.h (right):

http://codereview.chromium.org/2856050/diff/1/4#newcode1
examples/srpc/duality/duality.h:1: // Copyright 2010 The Native Client Authors.
All rights reserved.
On 2010/07/21 17:29:13, David Springer wrote:
> "Native Client SDK Authors".

Done.

http://codereview.chromium.org/2856050/diff/1/5
File examples/srpc/duality/duality.html (right):

http://codereview.chromium.org/2856050/diff/1/5#newcode16
examples/srpc/duality/duality.html:16: // the shared state of the two
application objects
On 2010/07/21 17:29:13, David Springer wrote:
> Style Guide: comments should be Proper Sentences in whatever natural language
> you use.  "Container for the shared state of the two application objects."

Done.

http://codereview.chromium.org/2856050/diff/1/5#newcode16
examples/srpc/duality/duality.html:16: // the shared state of the two
application objects
On 2010/07/21 17:29:13, David Springer wrote:
> Style Guide: comments should be Proper Sentences in whatever natural language
> you use.  "Container for the shared state of the two application objects."

Done.

http://codereview.chromium.org/2856050/diff/1/6
File examples/srpc/duality/loader.cc (right):

http://codereview.chromium.org/2856050/diff/1/6#newcode14
examples/srpc/duality/loader.cc:14: // These functions are required by both the
develop and publish versions,
On 2010/07/21 17:29:13, David Springer wrote:
> Obsolete facts, maybe "These functions are called when ..."

Done.

http://codereview.chromium.org/2856050/diff/1/6#newcode25
examples/srpc/duality/loader.cc:25: }
On 2010/07/21 17:29:13, David Springer wrote:
> Style Guide: indentation.

Done.

http://codereview.chromium.org/2856050/diff/1/8
File examples/srpc/duality/npp_gate.cc (right):

http://codereview.chromium.org/2856050/diff/1/8#newcode17
examples/srpc/duality/npp_gate.cc:17: struct PluginContext {
On 2010/07/21 17:29:13, David Springer wrote:
> "Plugin" is an overloaded term.  It refers to something build as a trusted
NPAPI
> module.  I prefer NaClContext or something that implies we have something
> different and that runs in a different environment from trusted plugins.

Done.

http://codereview.chromium.org/2856050/diff/1/8#newcode19
examples/srpc/duality/npp_gate.cc:19: NPObject *npobject;
This is the plugin-side reference to the browser-side instance.  It was weak in
the example I was using so I kept it that way.

On 2010/07/21 17:29:13, David Springer wrote:
> Is this weak reference necessary?  The browser should handle all the ref
> counting and memory for it, and actually "owns" the reference.

http://codereview.chromium.org/2856050/diff/1/8#newcode23
examples/srpc/duality/npp_gate.cc:23: // that the browser can all them.  All of
them are required to be implemented
On 2010/07/21 17:29:13, David Springer wrote:
> "can all" -> "can <what? call?> all"

Done.

http://codereview.chromium.org/2856050/diff/1/8#newcode24
examples/srpc/duality/npp_gate.cc:24: // regardless of whether this is a trusted
or untrusted build of the module.
On 2010/07/21 17:29:13, David Springer wrote:
> "trusted or untrusted" - obsolete comment.  Please remove.

Done.

http://codereview.chromium.org/2856050/diff/1/8#newcode62
examples/srpc/duality/npp_gate.cc:62: fflush(stdout);
On 2010/07/21 17:29:13, David Springer wrote:
> How do you handle plugin_context->npobject here?  The browser *should* have
> released it by now - if you need to keep a weak reference, maybe assert that
> it's NULL at this point?

Done.

http://codereview.chromium.org/2856050/diff/1/8#newcode78
examples/srpc/duality/npp_gate.cc:78: // Helper function for NPP_GetValue to
create this plugin's NPObject.
On 2010/07/21 17:29:13, David Springer wrote:
> This sentence is not really needed.

Done.

http://codereview.chromium.org/2856050/diff/1/8#newcode160
examples/srpc/duality/npp_gate.cc:160: // When the browser called
NP_GetMIMEDescription, it calls a function of this
On 2010/07/21 17:29:13, David Springer wrote:
> This function is only needed for Linux trusted plugins.  Please remove.

Done.

http://codereview.chromium.org/2856050/diff/1/9
File examples/srpc/duality/scriptable.h (right):

http://codereview.chromium.org/2856050/diff/1/9#newcode16
examples/srpc/duality/scriptable.h:16: // method table and property table.
On 2010/07/21 17:29:13, David Springer wrote:
> We talked about this class, and why it's separated from ScriptingBridge.  I
> understand the separation and it seems workable.  The scenario we want to
> support is this:
> 
> 1. The NaCl instance (or application or module) publishes an interface to the
> browser (this is NPP_*).
> 2. An object in the NaCl module's task space can publish an interface that
> provides access to it from JavaScript (via NPClass, this is the
> ScriptingBridge).
> 3. A NaCl instance can publish a JS scripting bridge interface as well as it's
> browser interface.
> 4. The NaCl instance can vend objects to the browser that conform to the JS
> scripting interface.
> 
> Consider this HTML/JS application code:
> 
> <embed id="my_module" nexes="x86-32: my_module_x86_32.nexe">
> 
> var foo = my_module.getBar();
> foo.someMethod();
> 
> In MyModule's code, there are three distinct interfaces:
> 
> 1. MyModule's NPP interface, returned via NP_Initialize.
> 2. MyModule's scripting bridge interface, returned via
> NPP_GetScriptableObject().  One method in this interface is MyModule.GetBar(),
> which is bound to "getBar".
> 3. GetBar's scripting bridge, also returned via NPP_GetScriptableObject().
> 

I don't think that my implementation prohibits the scenario you described.  In
fact, I think I'm going to turn this code example into one that implements this
scenario.

http://codereview.chromium.org/2856050/diff/1/9#newcode35
examples/srpc/duality/scriptable.h:35: // TODO(mlinck) make this use MemFree
On 2010/07/21 17:29:13, David Springer wrote:
> You probably don't need to manage this memory in the browser.  It is fine -
and
> a lot simpler - to use malloc/new for memory owned by this object.

Done.

http://codereview.chromium.org/2856050/diff/1/9#newcode48
examples/srpc/duality/scriptable.h:48: // TODO(mlinck) make this use MemAlloc
On 2010/07/21 17:29:13, David Springer wrote:
> (See above) The browser should not manage this memory.

Done.

http://codereview.chromium.org/2856050/diff/1/9#newcode61
examples/srpc/duality/scriptable.h:61: // table if it is uninitialized.
OK, so I'll make the containers private, override init, and expose two protected
methods that allow the deriving class to populate the tables.

On 2010/07/21 17:29:13, David Springer wrote:
> I would prefer overriding Init() to populate the table, and then provide two
> accessors:
>   Method* MethodForIdentifier(NPIdentifier np_id) const;
>   Property* PropertyForItentifier(NPIdentifier np_id) const;
> 
> This encapsulates the table data and implementation.  The kind of container
you
> use for your method <-> id map should not be part of your API.

http://codereview.chromium.org/2856050/diff/1/10
File examples/srpc/duality/scripting_bridge.cc (right):

http://codereview.chromium.org/2856050/diff/1/10#newcode42
examples/srpc/duality/scripting_bridge.cc:42: fflush(stdout);
On 2010/07/21 17:29:13, David Springer wrote:
> See comment in HasProperty - it seems that the separation between Scriptable
and
> ScriptingBridge could be better.

Done.

http://codereview.chromium.org/2856050/diff/1/10#newcode55
examples/srpc/duality/scripting_bridge.cc:55: fflush(stdout);
On 2010/07/21 17:29:13, David Springer wrote:
> return ToInstance(object)->HasMethod(name);

Done.

http://codereview.chromium.org/2856050/diff/1/10#newcode66
examples/srpc/duality/scripting_bridge.cc:66: fflush(stdout);
Oops, forgot a TODO there
On 2010/07/21 17:29:13, David Springer wrote:
> Shouldn't this look up the method name in the method map?

http://codereview.chromium.org/2856050/diff/1/10#newcode85
examples/srpc/duality/scripting_bridge.cc:85: bool have_property = false;
On 2010/07/21 17:29:13, David Springer wrote:
> Your separation of Scriptable and ScriptingBridge doesn't seem complete here. 
I
> would add an accessor on Scriptable that finds a method given an NPIdentifier.

> Same goes for properties (below).

Done.

http://codereview.chromium.org/2856050/diff/1/10#newcode101
examples/srpc/duality/scripting_bridge.cc:101: property_table_ =
application_->GetPropertyTable();
On 2010/07/21 17:29:13, David Springer wrote:
> Consider adding API to the underlying object for property and method
discovery,
> so you don't have to cache references to these tables here.

Done.

http://codereview.chromium.org/2856050/diff/1/10#newcode132
examples/srpc/duality/scripting_bridge.cc:132: bool rval = false;
On 2010/07/21 17:29:13, David Springer wrote:
> As with HasProperty, it seems that your separation of Scriptable and
> ScriptingBridge is not complete.  This object has to know about the container
> used by Scriptable, which should all be encapsulated in Scriptable.  An
accessor
> that returns a Scriptable::Method* would be better, e.g.:
> 
> Scriptable::Method* invoke_method =
> application_->MethodForIdentifier(method_name);
> if (invoke_method) {
>   invoke_method(application_, args, arg_count, result);
> }
> 

Done.

http://codereview.chromium.org/2856050/diff/1/11
File examples/srpc/duality/scripting_bridge.h (right):

http://codereview.chromium.org/2856050/diff/1/11#newcode17
examples/srpc/duality/scripting_bridge.h:17: class ScriptingBridge : public
NPObject {
On 2010/07/21 17:29:13, David Springer wrote:
> This can't have any virtual functions if it's a subclass of NPObject, since
the
> browser expects the memory to be laid out like a struct.  The vtable can't be
> present.

I thought this was an odd comment because the scripting_bridge samples in the
SDK examples all had virtual methods.  What do you mean?

http://codereview.chromium.org/2856050/diff/1/11#newcode23
examples/srpc/duality/scripting_bridge.h:23: // this plugin. The browser may
dereference the returned pointer any time
On 2010/07/21 17:29:13, David Springer wrote:
> "plugin" -> "object" or "exposed object" or "published object"

Done.

http://codereview.chromium.org/2856050/diff/1/11#newcode25
examples/srpc/duality/scripting_bridge.h:25: // when it unloads the plugin.
On 2010/07/21 17:29:13, David Springer wrote:
> The browser manages the memory for the scripting bridge object instance.  It
> does not necessarily hang onto these objects until the NaCl module is unloaded
-
> it might dealloc them earlier than that.  It largely depends on the JS garbage
> collection machinery.

Done.

http://codereview.chromium.org/2856050/diff/1/11#newcode42
examples/srpc/duality/scripting_bridge.h:42: // Documentation URL:
https://developer.mozilla.org/en/NPClass
No, thanks to virtual destructors.
On 2010/07/21 17:29:13, David Springer wrote:
> Doesn't the dealloc method have to be a template as well?

http://codereview.chromium.org/2856050/diff/1/11#newcode129
examples/srpc/duality/scripting_bridge.h:129: NPObject* window_object_;
On 2010/07/21 17:29:13, David Springer wrote:
> Why is this ivar here?  I don't see it used anywhere.

Done.

http://codereview.chromium.org/2856050/diff/1/11#newcode131
examples/srpc/duality/scripting_bridge.h:131: Scriptable * application_;
done

On 2010/07/21 17:29:13, David Springer wrote:
> It might make more sense to call this "object_" or something more generic than
> "application_" -  to make it clear that a ScriptingBridge can wrap any object,
> not just the main application object.
> 
> Style Guide: Scriptbale*, no space before the *

http://codereview.chromium.org/2856050/diff/1/11#newcode163
examples/srpc/duality/scripting_bridge.h:163: ScriptableType *tApp = new
ScriptableType();
Ooops, beware the invasion of the old style guide =)

On 2010/07/21 17:29:13, David Springer wrote:
> Style Guide: "tApp" -> "app".
> 
> "app" might be misleading here, given that *any* object can publish methods
> across the scripting bridge.  This mechanism is not limited to the main
> application object.

[mlinck, 2010-07-21 22:08:29]

More changes:  I added a couple of pure vitual methods to scriptable:
InitializeMethodTable() and InitializePropertyTable()  I felt this clarifies the
interface for the scriptable object.

[David Springer, 2010-07-21 23:41:14]

Couple of questions and comments, and there are still a bunch of Style Guide
issues (looks like the lint-picker found most of them).

Otherwise, LGTM.

http://codereview.chromium.org/2856050/diff/24001/25003
File examples/srpc/duality/duality.h (right):

http://codereview.chromium.org/2856050/diff/24001/25003#newcode20
examples/srpc/duality/duality.h:20: static bool SayHello(Scriptable * instance,
Why is this a static function?  Please excuse my ignorance of C++, but is there
a way to make this a regular method?

http://codereview.chromium.org/2856050/diff/24001/25011
File examples/srpc/duality/scripting_bridge.h (right):

http://codereview.chromium.org/2856050/diff/24001/25011#newcode20
examples/srpc/duality/scripting_bridge.h:20: virtual ~ScriptingBridge();
Will this cause a vtable to get plopped into the memory of this object?  Th
browser expects NPObject to have a certain layout, and in particular to not have
a vtable.

[mlinck, 2010-07-22 02:16:56]

http://codereview.chromium.org/2856050/diff/24001/25003
File examples/srpc/duality/duality.h (right):

http://codereview.chromium.org/2856050/diff/24001/25003#newcode20
examples/srpc/duality/duality.h:20: static bool SayHello(Scriptable * instance,
On 2010/07/21 23:41:14, David Springer wrote:
> Why is this a static function?  Please excuse my ignorance of C++, but is
there
> a way to make this a regular method?
There is no way to make this a regular method without making the entire
Scriptable and ScriptingBridge an ugly template.  So instead the class provides
a callback and gets an instance pointer which it could use to reference an
object-bound function when necessary.  The reason for this is that a function of
signature:
typedef bool (Scriptable::*Method)(const NPVariant* args,
                                   uint32_t arg_count,
                                   NPVariant* result);
won't allow you to bind to a function on a class that *inherits* from
Scriptable.  This is why I went with:
typedef bool (*Method)(Scriptable * instance,
                       const NPVariant* args,
                       uint32_t arg_count,
                       NPVariant* result);

It would be possible to work around this with functor templates and letting
whatever class inherits from the Scriptable (which would also be a template)
specialize the type of the functor.  I considered it.  I didn't end up doing it
because I don't know if we should be showing off like that.  It could scare off
more people than it attracts. (though it would be fun!)

http://codereview.chromium.org/2856050/diff/24001/25011
File examples/srpc/duality/scripting_bridge.h (right):

http://codereview.chromium.org/2856050/diff/24001/25011#newcode20
examples/srpc/duality/scripting_bridge.h:20: virtual ~ScriptingBridge();
On 2010/07/21 23:41:14, David Springer wrote:
> Will this cause a vtable to get plopped into the memory of this object?  Th
> browser expects NPObject to have a certain layout, and in particular to not
have
> a vtable.
No.  you can't add a vtable to a struct.  In other words this virtual destructor
isn't as virtual as it looks because it's only virtual for any classes that
might inherit from this one.  Not down below.   A popular c++ interview question
right there, btw.  This also mean that we *have* to make sure that the actual
scripting bridge's destructor is called directly.  That's why ScriptingBridge
has to static_cast in the deallocate function before it calls delete.

[mlinck, 2010-07-22 02:19:25]

On 2010/07/21 23:41:14, David Springer wrote:
> Couple of questions and comments, and there are still a bunch of Style Guide
> issues (looks like the lint-picker found most of them).
> 
> Otherwise, LGTM.
> 
> http://codereview.chromium.org/2856050/diff/24001/25003
> File examples/srpc/duality/duality.h (right):
> 
> http://codereview.chromium.org/2856050/diff/24001/25003#newcode20
> examples/srpc/duality/duality.h:20: static bool SayHello(Scriptable *
instance,
> Why is this a static function?  Please excuse my ignorance of C++, but is
there
> a way to make this a regular method?
> 
There is no way to make this a regular method without making the entire
Scriptable and ScriptingBridge an ugly template.  So instead the class provides
a callback and gets an instance pointer which it could use to reference an
object-bound function when necessary.  The reason for this is that a function of
signature:
typedef bool (Scriptable::*Method)(const NPVariant* args,
                                   uint32_t arg_count,
                                   NPVariant* result);
won't allow you to bind to a function on a class that *inherits* from
Scriptable.  This is why I went with:
typedef bool (*Method)(Scriptable * instance,
                       const NPVariant* args,
                       uint32_t arg_count,
                       NPVariant* result);

It would be possible to work around this with functor templates and letting
whatever class inherits from the Scriptable (which would also be a template)
specialize the type of the functor.  I considered it.  I didn't end up doing it
because I don't know if we should be showing off like that.  It could scare off
more people than it attracts. (though it would be fun!)

> http://codereview.chromium.org/2856050/diff/24001/25011
> File examples/srpc/duality/scripting_bridge.h (right):
> 
> http://codereview.chromium.org/2856050/diff/24001/25011#newcode20
> examples/srpc/duality/scripting_bridge.h:20: virtual ~ScriptingBridge();
> Will this cause a vtable to get plopped into the memory of this object?  Th
> browser expects NPObject to have a certain layout, and in particular to not
have
> a vtable.
No.  you can't add a vtable to a struct.  In other words this virtual destructor
isn't as virtual as it looks because it's only virtual for any classes that
might inherit from this one.  Not down below.   A popular c++ interview question
right there, btw.  This also mean that we *have* to make sure that the actual
scripting bridge's destructor is called directly.  That's why ScriptingBridge
has to static_cast in the deallocate function before it calls delete.

[mlinck, 2010-07-22 02:29:58]

On 2010/07/22 02:19:25, mlinck wrote:
> On 2010/07/21 23:41:14, David Springer wrote:
> > Couple of questions and comments, and there are still a bunch of Style Guide
> > issues (looks like the lint-picker found most of them).
> > 
> > Otherwise, LGTM.
> > 
> > http://codereview.chromium.org/2856050/diff/24001/25003
> > File examples/srpc/duality/duality.h (right):
> > 
> > http://codereview.chromium.org/2856050/diff/24001/25003#newcode20
> > examples/srpc/duality/duality.h:20: static bool SayHello(Scriptable *
> instance,
> > Why is this a static function?  Please excuse my ignorance of C++, but is
> there
> > a way to make this a regular method?
> > 
> There is no way to make this a regular method without making the entire
> Scriptable and ScriptingBridge an ugly template.  So instead the class
provides
> a callback and gets an instance pointer which it could use to reference an
> object-bound function when necessary.  The reason for this is that a function
of
> signature:
> typedef bool (Scriptable::*Method)(const NPVariant* args,
>                                    uint32_t arg_count,
>                                    NPVariant* result);
> won't allow you to bind to a function on a class that *inherits* from
> Scriptable.  This is why I went with:
> typedef bool (*Method)(Scriptable * instance,
>                        const NPVariant* args,
>                        uint32_t arg_count,
>                        NPVariant* result);
> 
> It would be possible to work around this with functor templates and letting
> whatever class inherits from the Scriptable (which would also be a template)
> specialize the type of the functor.  I considered it.  I didn't end up doing
it
> because I don't know if we should be showing off like that.  It could scare
off
> more people than it attracts. (though it would be fun!)
> 

I thought about this before hand.  I basically had the choice of making the
whole ScriptingBridge a template and letting the ScriptableObject simply be any
type that can be used to specialize that template.  I decided against this
because the ScriptingBridge interface is too complex.  This is a problem because
templates define the interface that has to be implemented implicitly (by trying
to use it) rather than explicitly, by providing it as abstract methods.  I opted
for an inheritable class with a simple interface instead.  

> > http://codereview.chromium.org/2856050/diff/24001/25011
> > File examples/srpc/duality/scripting_bridge.h (right):
> > 
> > http://codereview.chromium.org/2856050/diff/24001/25011#newcode20
> > examples/srpc/duality/scripting_bridge.h:20: virtual ~ScriptingBridge();
> > Will this cause a vtable to get plopped into the memory of this object?  Th
> > browser expects NPObject to have a certain layout, and in particular to not
> have
> > a vtable.
> No.  you can't add a vtable to a struct.  In other words this virtual
destructor
> isn't as virtual as it looks because it's only virtual for any classes that
> might inherit from this one.  Not down below.   A popular c++ interview
question
> right there, btw.  This also mean that we *have* to make sure that the actual
> scripting bridge's destructor is called directly.  That's why ScriptingBridge
> has to static_cast in the deallocate function before it calls delete.