Replaced BaseNPObject with DefaultNPObject because......

Replaced BaseNPObject with DefaultNPObject because...
DefaultNPObject can derive from bases other than NPObject, allowing concrete NPObjects to choose their own NPObject subclass.
DefaultNPObject does not rely on virtual functions. See change to NPGetClass in np_class.h.

Removed count() from MockNPObject because I discovered gmock checks that no mock objects exist after a test completes anyway.

TEST=none
BUG=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25632

[apatrick, 2009-09-04 22:43:22]

It's not as bad as it looks. Just a refactor.

[gman, 2009-09-04 23:48:52]

LGTM

I don't personally have a problem with the multiple inheritance...Just wondering
if there is an easy workaround.

http://codereview.chromium.org/196032/diff/1/28
File o3d/gpu_plugin/command_buffer.h (right):

http://codereview.chromium.org/196032/diff/1/28#newcode15
Line 15: class CommandBuffer : public NPObject, public DispatchedNPObject {
I see that Chrome is breaking the rules. The problem with breaking the rules is
it makes it hard to know when they okay to break and when they are not. If some
are okay to break then they are all okay to break.

The rule is "We allow multiple inheritance only when at most one of the base
classes has an implementation; all other base classes must be pure interface
classes tagged with the Interface suffix."

A pure interface class is defined as:

 A class is a pure interface if it meets the following requirements:

    * It has only public pure virtual ("= 0") methods and static methods (but
see below for destructor).
    * It may not have non-static data members.
    * It need not have any constructors defined. If a constructor is provided,
it must take no arguments and it must be protected.
    * If it is a subclass, it may only be derived from classes that satisfy
these conditions and are tagged with the Interface suffix.


All of those rules seem to be broken here.

"Decision:  Multiple inheritance is allowed only when all superclasses, with the
possible exception of the first one, are pure interfaces. In order to ensure
that they remain pure interfaces, they must end with the Interface suffix. "

The guideline seems pretty clear "When multiple implementation inheritance seems
like the solution, you can usually find a different, more explicit, and cleaner
solution."

The clearly don't want multiple inheritance and otherwise they want classes
except 1 named "Interface" and that classes named "Interface" follow specific
rules. No constructor arguments, no non-static data, etc...


Could you not just as easily make them members?

class CommandBuffer : public NPObject {
 private:
   DispatchedNPObject mixin;

NP_UTILS_BEGIN_DISPATCHER_CHAIN(CommandBuffer, mixin)
NP_UTILS_DISPATCHER(Initialize, bool(int32))	      
NP_UTILS_DISPATCHER(mixin.SetPutOffset, void(int32))
NP_UTILS_DISPATCHER(GetGetOffset, int32())

Just an idea.

It seems like the guideline is really trying to discourage multiple inheritance
so if there is some simple solution to removing it it might be a good idea.

Otherwise, I see what your doing and it makes sense. At the same time it could
lead to the situations they are trying to avoid.

http://codereview.chromium.org/196032/diff/1/18
File o3d/gpu_plugin/np_utils/dispatched_np_object.h (right):

http://codereview.chromium.org/196032/diff/1/18#newcode53
Line 53: // Return the names of all the methods int he dispatcher chain.
int he -> in the

[apatrick, 2009-09-05 00:44:05]

Do you prefer this? It also gets rid of the virtual call to
GetDynamicDispatcherChain.

http://codereview.chromium.org/196032/diff/1/18
File o3d/gpu_plugin/np_utils/dispatched_np_object.h (right):

http://codereview.chromium.org/196032/diff/1/18#newcode53
Line 53: // Return the names of all the methods int he dispatcher chain.
On 2009/09/04 23:48:52, gman wrote:
> int he -> in the

Done.

[gman, 2009-09-05 01:01:07]

hmmm, that seems more in the spirit of the style guide although it's still a
little funky in that you are counting of the hiding of base class functions
instead of being explicit. Then again, not being explicit means it's more
expandable I guess so yea. Looks good.