use reference instead of value in JSAPI.h

[GoogleCodeExporter]

Brief Description:
        virtual void registerEvent(const std::string& name);
        virtual void registerEventMethod(std::string name, BrowserObjectAPI *event);
        virtual void unregisterEventMethod(std::string name, BrowserObjectAPI *event);
        virtual void registerEventInterface(BrowserObjectAPI *event);
        virtual void unregisterEventInterface(BrowserObjectAPI *event);
        virtual BrowserObjectAPI *getDefaultEventMethod(std::string name);
        virtual void setDefaultEventMethod(std::string name, BrowserObjectAPI *event);

        // Methods for enumeration
        virtual void getMemberNames(std::vector<std::string> &nameVector) = 0;
        virtual size_t getMemberCount() = 0;

        // Methods to query existance of members on the API
        virtual bool HasMethod(std::string methodName) = 0;
        virtual bool HasProperty(std::string propertyName) = 0;
        virtual bool HasProperty(int idx) = 0;
        virtual bool HasEvent(std::string eventName);

        // Methods to manage properties on the API
        virtual variant GetProperty(std::string propertyName) = 0;
        virtual void SetProperty(std::string propertyName, const variant value) = 0;
        virtual variant GetProperty(int idx) = 0;
        virtual void SetProperty(int idx, const variant value) = 0;

        // Methods to manage methods on the API
        virtual variant Invoke(std::string methodName, std::vector<variant>& args) = 0;

should changed to

       virtual void registerEvent(const std::string& name);
        virtual void registerEventMethod(std::string& name, BrowserObjectAPI *event);
        virtual void unregisterEventMethod(std::string& name, BrowserObjectAPI *event);
        virtual void registerEventInterface(BrowserObjectAPI *event);
        virtual void unregisterEventInterface(BrowserObjectAPI *event);
        virtual BrowserObjectAPI *getDefaultEventMethod(std::string& name);
        virtual void setDefaultEventMethod(std::string& name, BrowserObjectAPI *event);

        // Methods for enumeration
        virtual void getMemberNames(std::vector<std::string> &nameVector) = 0;
        virtual size_t getMemberCount() = 0;

        // Methods to query existance of members on the API
        virtual bool HasMethod(std::string& methodName) = 0;
        virtual bool HasProperty(std::string& propertyName) = 0;
        virtual bool HasProperty(int idx) = 0;
        virtual bool HasEvent(std::string& eventName);

        // Methods to manage properties on the API
        virtual variant GetProperty(std::string& propertyName) = 0;
        virtual void SetProperty(std::string& propertyName, const variant& value) = 0;
        virtual variant GetProperty(int idx) = 0;
        virtual void SetProperty(int idx, const variant& value) = 0;

        // Methods to manage methods on the API
        virtual variant Invoke(std::string& methodName, std::vector<variant>& args) = 0;

Use case:

we we call firebreath from another dll, if we post string value, it will 
crashed. so in  JSAPI.h, we should use reference instead of value,and it will 
run faster and use less memory.

Additional information:

Original issue reported on code.google.com by qifuren1...@gmail.com on 30 Jun 2010 at 4:27

[GoogleCodeExporter]

Most of these should actually be const references, only getMemberNames() needs 
to mutate its arguments. Its one of the things i plan to clean up when i've got 
time.
Alternatively tested patches are welcome.

Original comment by georg.fritzsche on 30 Jun 2010 at 4:50

• Changed state: Accepted

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Actually, there is a good reason why most of them are not.  If you make these 
references, and certainly if you make them const references, they will break 
all the (many) places where we call them with a const char *.  Perhaps that 
means that we should add wrappers to allow const char* explicitely instead of 
implicitly to better solve this problem, but as written the patch will not be 
accepted because it would break too much backwards compatibility.

For example:
this->registerEvent("myevent");

will break if it expects a std::string& because it cannot implicitly convert 
"myevent" to a std::string&, but it can to a std::string.

Suggestions?

Original comment by taxilian on 30 Jun 2010 at 12:59

[GoogleCodeExporter]

You can construct temporaries of type std::string implicitly from string 
literals and temporaries/rvalues can be bound to const references - so the 
following is fine:

  void f(const std::string& s) {}
  f("moo");

The more interesting thing is that it 
 a) requires going through all derived classes
 b) can introduce bugs not caught at compile-time

I am worried about  b), e.g. by forgetting to change the signature in a 
derivative of a non-abstract class:

  struct A { virtual void f(const std::string&) = 0; };
  struct B : A { void f(const std::string&) { log("in B"); } };
  struct C : B { void f(std::string) { log("in C"); } };
  A* a = new C;
  a->f("foo"); // prints "in B"

Original comment by georg.fritzsche on 30 Jun 2010 at 6:41

[GoogleCodeExporter]

When I tried using const std::string& when I originally wrote it, it didn't 
work.

However, perhaps the correct way to fix that would be to make an extra method 
that explicitly converts it to a std::string and calls the main function on the 
JSAPI object.  That stub function could accept a const char *, thus eliminating 
the implicit behavior.  What do you think?

Original comment by taxilian on 30 Jun 2010 at 7:01

[GoogleCodeExporter]

Hm, i'd be curious to see a case where that fails :)
I have converted a part of the classes involved in FB and don't see anything - 
maybe that was with overload ambiguities? In the worst case you could always 
explicitly construct a temporary using "f(std::string(someRawCString));".

Original comment by georg.fritzsche on 30 Jun 2010 at 7:07

[GoogleCodeExporter]

I don't remember for sure; maybe I was doing something else weird and didn't 
realize it.

Mainly I just don't want to change something and have it break existing code.

Original comment by taxilian on 30 Jun 2010 at 7:12

[GoogleCodeExporter]

I went through our source and made the changes, could you test?
http://code.google.com/r/georgfritzsche-fb-interface-changes/source/checkout

Original comment by georg.fritzsche on 9 Aug 2010 at 1:51

[GoogleCodeExporter]

Its now in the trunk.

Original comment by georg.fritzsche on 10 Aug 2010 at 10:07

• Changed state: Fixed

[GoogleCodeExporter]

It works. Thanks very much!

Original comment by qifuren1...@gmail.com on 11 Aug 2010 at 3:21