Hi Sanjeev,
On Mon, Mar 26, 2012 at 8:54 AM, Enlightenment SVN
<no-re...@enlightenment.org> wrote:
> Log:
> CtxPopup support.
>
> Signed-off-by: Sanjeev BA <eflel...@gmail.com>
>
> Author: sanjeev
> Date: 2012-03-26 04:54:09 -0700 (Mon, 26 Mar 2012)
> New Revision: 69636
> Trac: http://trac.enlightenment.org/e/changeset/69636
>
> Added:
> trunk/PROTO/elev8/data/javascript/ctxpopup.js
> trunk/PROTO/elev8/src/modules/elm/CElmCtxPopup.cc
> trunk/PROTO/elev8/src/modules/elm/CElmCtxPopup.h
Please take a look on the previous clean up that we did. Some things
to take in consideration for new widgets like this:
1. Return early if argument is not of the expected type. So instead of
+void CElmCtxPopup::hover_parent_set(Handle<Value> val)
+{
+ if (val->IsObject())
+ {
+ .... long code
+ }
we have:
+void CElmCtxPopup::hover_parent_set(Handle<Value> val)
+{
+ if (!val->IsObject()) return;
+
+ ... long code
2. Don't use braces for only 1 statements in if branch:
+ if (p)
+ {
+ elm_ctxpopup_hover_parent_set(eo, p->get());
+ }
3. Beware of V8's stack:
+Handle<Value> CElmCtxPopup::direction_priority_get() const
+{
+ Elm_Ctxpopup_Direction f, s, q, t;
+ elm_ctxpopup_direction_priority_get(eo, &f, &s, &t, &q);
+ Local<Object> obj = Object::New();
+ obj->Set(String::New("first"), Number::New(f));
+ obj->Set(String::New("second"), Number::New(s));
+ obj->Set(String::New("third"), Number::New(t));
+ obj->Set(String::New("fourth"), Number::New(q));
+ return obj;
+}
You are creating several Local<> objects here that will never get
release or they will survive longer than needed. Every time you call
String::New() you are creating a Local<String> that will be allocated
with the last HandleScope. If you have a "HandleScope scope;" in your
function, when this function returns the destructor will run and these
objects will be marked for deletion. Since you need to return an
object, instead of "return obj;" you need to do "return
scope.Close(obj);"
4. Use String::NewSymbol() whenever possible
String::NewSymbol() is like eina's stringshare: it will avoid
allocating strings that already exist. Instead it will reuse them.
This is very suited for code like the ones in callbacks.
5. Stick to coding style. We had a hard time to make the coding style
uniform in elev8. Let's agree on it and stick to that? For example,
take a look on how headers are formatted, PROPERTIES_OF(), etc.
6. Most of the time there's no need to create tmp String::Utf8Value objects.
String::Utf8Value exists to convert from JS to plain char* and can be
used just like a function call if you are not doing anything else with
it. E.g.:
elm_object_part_content_set(eo, *String::Utf8Value(element), child->get());
7. Why are you mixing JS strings, char* and std::string? This makes
very little sense:
+ String::Utf8Value str(obj->Get(String::New("label")));
+ itc->label = std::string(*str);
If you need to store the value you can: Save the String::Utf8Value or
dup it in char*. But it looks like you are never re-using this string
so why bother storing it? Elementary is already adding it to a
stringshare, so you don't need to keep another copy.
8. Don't make public methods that can be private or protected. For elm
widgets this means the constructor, static methods given to elementary
and some other helpers.
Lucas De Marchi
------------------------------------------------------------------------------
This SF email is sponsosred by:
Try Windows Azure free for 90 days Click Here
http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
