Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TIMOB-10137: BlackBerry: Implement Ti.Network.Socket and Ti.Network.Socket.TCP [part2] #134

Merged
merged 6 commits into from
Aug 23, 2012
Merged

TIMOB-10137: BlackBerry: Implement Ti.Network.Socket and Ti.Network.Socket.TCP [part2] #134

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
38decd6
TIMOB-10137: BlackBerry: Implement Ti.Network.Socket and Ti.Network.S…
alexandergalstyan Aug 17, 2012
22266e7
TIMOB-10137: BlackBerry: Implement Ti.Network.Socket and Ti.Network.S…
alexandergalstyan Aug 20, 2012
c1b074b
TIMOB-10137: BlackBerry: Implement Ti.Network.Socket and Ti.Network.S…
alexandergalstyan Aug 20, 2012
3f57011
TIMOB-10137: BlackBerry: Implement Ti.Network.Socket and Ti.Network.S…
alexandergalstyan Aug 21, 2012
8f601f8
TIMOB-10137: BlackBerry: Implement Ti.Network.Socket and Ti.Network.S…
alexandergalstyan Aug 23, 2012
4729d15
TIMOB-10137: BlackBerry: Implement Ti.Network.Socket and Ti.Network.S…
alexandergalstyan Aug 23, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion blackberry/tibb/NativeMessageStrings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ N_MESSAGESTRINGS_CONST_DEF(char*, Out_of_bounds, "Out of bounds");
N_MESSAGESTRINGS_CONST_DEF(char*, Unknown_key_value_received, "Unknown key:value received");
N_MESSAGESTRINGS_CONST_DEF(char*, Unknown_value_received, "Unknown value received");
N_MESSAGESTRINGS_CONST_DEF(char*, Unsupported_event_name_, "Unsupported event name ");

N_MESSAGESTRINGS_CONST_DEF(char*, Invalid_hostname_or_port, "Invalid host name or port");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this alphabetical

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

N_MESSAGESTRINGS_CONST_DEF(char*, Invalid_socket_state, "Invalid socket state");
}
}

Expand Down
241 changes: 219 additions & 22 deletions blackberry/tibb/NativeTCPSocketObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@
#include "NativeTCPSocketObject.h"

#include "NativeControlObject.h"
#include "NativeException.h"
#include "NativeMessageStrings.h"
#include "TiObject.h"
#include "TiV8Event.h"

#pragma GCC diagnostic ignored "-Wunused-parameter"
#include <v8.h>
Expand All @@ -18,14 +21,14 @@ using namespace v8;

#define PROP_SETGET_FUNCTION(NAME) prop_##NAME

#define PROP_SETGET(NAME) static int prop_##NAME(NativeTCPSocketObject* tcpSocket, TiObject* obj) \
#define PROP_SETGET(NAME) static int prop_##NAME(NativeTCPSocketObject* tcpSocket, TiObject* obj, void * userContext) \
{\
return tcpSocket->NAME(obj);\
return tcpSocket->NAME(obj, userContext);\
}

#define GET_ARRAY_SIZE(ARRAY) (int)(sizeof(ARRAY)/sizeof(*(ARRAY)))

typedef int (*NATIVE_PROPSETGET_CALLBACK)(NativeTCPSocketObject*, TiObject*);
typedef int (*NATIVE_PROPSETGET_CALLBACK)(NativeTCPSocketObject*, TiObject*, void*);

struct NATIVE_PROPSETGET_SETTING
{
Expand Down Expand Up @@ -86,41 +89,179 @@ class SetGetSocketProperties
};

PROP_SETGET(setHost)
int NativeTCPSocketObject::setHost(TiObject* obj)
int NativeTCPSocketObject::setHost(TiObject* obj, void* /*userContext*/)
{
int error = NativeControlObject::getString(obj, hostName_);
if (!N_SUCCEEDED(error))
if (socketState_ == SOCKET_STATE_INITIALIZED)
{
return error;
int error = NativeControlObject::getString(obj, hostName_);
if (!N_SUCCEEDED(error))
{
return error;
}
return NATIVE_ERROR_OK;
}
return NATIVE_ERROR_OK;
return NATIVE_ERROR_INVALID_ARG;
}

PROP_SETGET(getHost)
int NativeTCPSocketObject::getHost(TiObject* obj)
int NativeTCPSocketObject::getHost(TiObject* obj, void* /*userContext*/)
{
obj->setValue(String::New(hostName_.toStdString().c_str()));
return NATIVE_ERROR_OK;
}

PROP_SETGET(setPort)
int NativeTCPSocketObject::setPort(TiObject* obj)
int NativeTCPSocketObject::setPort(TiObject* obj, void* /*userContext*/)
{
int error = NativeControlObject::getInteger(obj, &port_);
if (!N_SUCCEEDED(error))
if (socketState_ == SOCKET_STATE_INITIALIZED)
{
return error;
int error = NativeControlObject::getInteger(obj, &port_);
if (!N_SUCCEEDED(error))
{
return error;
}
return NATIVE_ERROR_OK;
}
return NATIVE_ERROR_OK;
return NATIVE_ERROR_INVALID_ARG;
}

PROP_SETGET(getPort)
int NativeTCPSocketObject::getPort(TiObject* obj)
int NativeTCPSocketObject::getPort(TiObject* obj, void* /*userContext*/)
{
obj->setValue(Number::New(port_));
return NATIVE_ERROR_OK;
}

PROP_SETGET(getState)
int NativeTCPSocketObject::getState(TiObject* obj, void* /*userContext*/)
{
obj->setValue(Number::New(socketState_));
return NATIVE_ERROR_OK;
}

PROP_SETGET(setListenQueueSize)
int NativeTCPSocketObject::setListenQueueSize(TiObject* obj, void* /*userContext*/)
{
if (socketState_ == SOCKET_STATE_LISTENING)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this restriction in the docs and it would make sense to be able to set this in other states too. Please remove it, unless I missed something.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
int maxCount;
int error = NativeControlObject::getInteger(obj, &maxCount);
if (!N_SUCCEEDED(error))
{
return error;
}
tcpServer_->setMaxPendingConnections(maxCount);
return NATIVE_ERROR_OK;
}
return NATIVE_ERROR_INVALID_ARG;
}

PROP_SETGET(getListenQueueSize)
int NativeTCPSocketObject::getListenQueueSize(TiObject* obj, void* /*userContext*/)
{
if (socketState_ == SOCKET_STATE_LISTENING)
{
obj->setValue(Number::New(tcpServer_->maxPendingConnections()));
return NATIVE_ERROR_OK;
}
return NATIVE_ERROR_INVALID_ARG;
}

PROP_SETGET(setConnectedCallback)
int NativeTCPSocketObject::setConnectedCallback(TiObject* obj, void* userContext)
{
Handle<Value> value = obj->getValue();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the docs: Can only be modified when this socket is in the INITIALIZED state

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (value->IsFunction())
{
TiTCPSocketObject* context = (TiTCPSocketObject*)userContext;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like how this is being done. Firstly, tons of stuff is getting leaked. I'd prefer this to be done like the others (timeout is another good example) in setupEvents. I'm not sure how the source property is set for button clicks but it seems to be correct so maybe it's automatically set by V8. Then you might just need to set the socket property to that value and clear the source like timeout does.
Same for the others.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using eventContainer it fires the events with eventData, which uses 'source' as well. So, cannot put that empty

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant about clearing source is that the event fired does not have the source property.

Pleas refactor all these callbacks to be more like what is done for NativeButtonObject. All this new complicated code seems to be necessary because you need the Socket.TCP object. If that's the case then check if V8 is actually setting the "source" property for us since that should be the same thing. If not then expose a setter for passing the object to NativeTCPSocketObject.
Please try to use the existing stuff in TiProxy (the events_ member, setupEvents, etc). This will (hopefully) make sure event handlers, etc get deleted properly and connections are only made once.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored

TiEventContainerFactory* eventFactory = context->getNativeObjectFactory()->getEventContainerFactory();
if (!eventContainer_)
{
eventContainer_ = eventFactory->createEventContainer();
}
Handle<Object> source = Handle<Object>::Cast(context->getValue());
TiV8Event* connectEvent = TiV8Event::createEvent("connected", Handle<Function>::Cast(value), source);
eventContainer_->addListener(connectEvent);
TCPSocketEventHandler* eHandler = new TCPSocketEventHandler(eventContainer_, this);
eventContainer_->setV8ValueProperty("socket", context->getValue());
QObject::connect(tcpClient_, SIGNAL(connected()), eHandler, SLOT(connected()));
return NATIVE_ERROR_OK;
}
return NATIVE_ERROR_INVALID_ARG;
}

PROP_SETGET(getConnectedCallback)
int NativeTCPSocketObject::getConnectedCallback(TiObject* /*obj*/, void* /*userContext*/)
{
// TODO: Implement this
return NATIVE_ERROR_NOTSUPPORTED;
}

PROP_SETGET(setErrorCallback)
int NativeTCPSocketObject::setErrorCallback(TiObject* obj, void* userContext)
{
Handle<Value> value = obj->getValue();
if (value->IsFunction())
{
TiTCPSocketObject* context = (TiTCPSocketObject*)userContext;
TiEventContainerFactory* eventFactory = context->getNativeObjectFactory()->getEventContainerFactory();
if (!eventContainer_)
{
eventContainer_ = eventFactory->createEventContainer();
}
Handle<Object> source = Handle<Object>::Cast(context->getValue());
TiV8Event* connectEvent = TiV8Event::createEvent("error", Handle<Function>::Cast(value), source);
eventContainer_->addListener(connectEvent);
TCPSocketEventHandler* eHandler = new TCPSocketEventHandler(eventContainer_, this);
eventContainer_->setV8ValueProperty("socket", context->getValue());
QObject::connect(tcpClient_, SIGNAL(error()), eHandler, SLOT(error()));
return NATIVE_ERROR_OK;
}
return NATIVE_ERROR_INVALID_ARG;
}

PROP_SETGET(getErrorCallback)
int NativeTCPSocketObject::getErrorCallback(TiObject* /*obj*/, void* /*userContext*/)
{
// TODO: Implement this
return NATIVE_ERROR_NOTSUPPORTED;
}

PROP_SETGET(setAcceptedCallback)
int NativeTCPSocketObject::setAcceptedCallback(TiObject* obj, void* userContext)
{
Handle<Value> value = obj->getValue();
if (value->IsFunction())
{
TiTCPSocketObject* context = (TiTCPSocketObject*)userContext;
TiEventContainerFactory* eventFactory = context->getNativeObjectFactory()->getEventContainerFactory();
if (!eventContainer_)
{
eventContainer_ = eventFactory->createEventContainer();
}
Handle<Object> source = Handle<Object>::Cast(context->getValue());
TiV8Event* connectEvent = TiV8Event::createEvent("accepted", Handle<Function>::Cast(value), source);
eventContainer_->addListener(connectEvent);
eventContainer_->setV8ValueProperty("socket", context->getValue());
return NATIVE_ERROR_OK;
}
return NATIVE_ERROR_INVALID_ARG;
}

PROP_SETGET(getAcceptedCallback)
int NativeTCPSocketObject::getAcceptedCallback(TiObject* /*obj*/, void* /*userContext*/)
{
// TODO: Implement this
return NATIVE_ERROR_NOTSUPPORTED;
}

NativeTCPSocketObject::NativeTCPSocketObject()
{
socketState_ = SOCKET_STATE_INITIALIZED;
port_ = -1;
eventContainer_ = NULL;
}

NativeTCPSocketObject::~NativeTCPSocketObject()
{
}
Expand All @@ -137,43 +278,99 @@ NativeTCPSocketObject* NativeTCPSocketObject::createTCPSocket()

const static NATIVE_PROPSETGET_SETTING g_SocketPropSetGet[] =
{
{N_SOCKET_PROP_ACCEPTED, PROP_SETGET_FUNCTION(setAcceptedCallback), PROP_SETGET_FUNCTION(getAcceptedCallback)},
{N_SOCKET_PROP_CONNECTED, PROP_SETGET_FUNCTION(setConnectedCallback), PROP_SETGET_FUNCTION(getConnectedCallback)},
{N_SOCKET_PROP_ERROR, PROP_SETGET_FUNCTION(setErrorCallback), PROP_SETGET_FUNCTION(getErrorCallback)},
{N_SOCKET_PROP_HOST, PROP_SETGET_FUNCTION(setHost), PROP_SETGET_FUNCTION(getHost)},
{N_SOCKET_PROP_PORT, PROP_SETGET_FUNCTION(setPort), PROP_SETGET_FUNCTION(getPort)}
{N_SOCKET_PROP_LISTENQUEUESIZE, PROP_SETGET_FUNCTION(setListenQueueSize), PROP_SETGET_FUNCTION(getListenQueueSize)},
{N_SOCKET_PROP_PORT, PROP_SETGET_FUNCTION(setPort), PROP_SETGET_FUNCTION(getPort)},
{N_SOCKET_PROP_STATE, NULL, PROP_SETGET_FUNCTION(getState)}
};

static SetGetSocketProperties g_SocketProps(g_SocketPropSetGet, GET_ARRAY_SIZE(g_SocketPropSetGet));

int NativeTCPSocketObject::setPropertyValue(size_t propertyNumber, TiObject* obj)
int NativeTCPSocketObject::setPropertyValue(size_t propertyNumber, TiObject* obj, void* userContext)
{
NATIVE_PROPSETGET_CALLBACK cb = g_SocketProps.GetSetterCallback(propertyNumber);
if (cb == NULL)
{
return NATIVE_ERROR_NOTSUPPORTED;
}
return (cb)(this, obj);
return (cb)(this, obj, userContext);
}

int NativeTCPSocketObject::getPropertyValue(size_t propertyNumber, TiObject* obj)
int NativeTCPSocketObject::getPropertyValue(size_t propertyNumber, TiObject* obj, void* userContext)
{
NATIVE_PROPSETGET_CALLBACK cb = g_SocketProps.GetGetterCallback(propertyNumber);
if (cb == NULL)
{
return NATIVE_ERROR_NOTSUPPORTED;
}
return (cb)(this, obj);
return (cb)(this, obj, userContext);
}

void NativeTCPSocketObject::setupEvents(TiEventContainerFactory* containerFactory)
{
NativeProxyObject::setupEvents(containerFactory);
if (!eventContainer_)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use != NULL instead

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with == NULL.

{
eventContainer_ = containerFactory->createEventContainer();
}
}

void NativeTCPSocketObject::connect()
{
tcpSocket_.connectToHost(hostName_, port_);
if (hostName_.isEmpty() || port_ == -1)
{
throw NativeException(QString(Native::Msg::Invalid_hostname_or_port).toStdString());
}
if (socketState_ == SOCKET_STATE_CONNECTED || socketState_ == SOCKET_STATE_LISTENING)
{
throw NativeException(QString(Native::Msg::Invalid_socket_state).toStdString());
}
tcpClient_->connectToHost(hostName_, port_);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this initialized and deleted?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

void NativeTCPSocketObject::close()
{
tcpSocket_.close();
if (socketState_ != SOCKET_STATE_CONNECTED || socketState_ != SOCKET_STATE_LISTENING)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be &&?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed

{
throw NativeException(QString(Native::Msg::Invalid_socket_state).toStdString());
}
tcpClient_->close();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to close server too, when appropriate

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, based on state

socketState_ = SOCKET_STATE_CLOSED;
}

void NativeTCPSocketObject::listen()
{
if (hostName_.isEmpty() || port_ == -1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the apidoc: The listen call will attempt to listen on the specified host and/or port property for the socket if they are set.
Anvil seems to only set the port (util.js line 63) so this probably needs to be reworked.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into it and you need to use QHostAddress::Any if hostname is empty and 0 for the port if it's not set

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

{
throw NativeException(QString(Native::Msg::Invalid_hostname_or_port).toStdString());
}
if (socketState_ == SOCKET_STATE_CONNECTED || socketState_ == SOCKET_STATE_LISTENING)
{
throw NativeException(QString(Native::Msg::Invalid_socket_state).toStdString());
}
tcpServer_ = new QTcpServer();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to get deleted. Also, should only create if tcpServer_ == NULL

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

bool bSuccess = tcpServer_->listen(QHostAddress(hostName_), port_);
socketState_ = bSuccess ? SOCKET_STATE_LISTENING : SOCKET_STATE_ERROR;
}

void NativeTCPSocketObject::accept(TiEvent* /*errorCallback*/, int /*timeout*/)
{
// TODO: Use errorCallback and timeout
if (socketState_ != SOCKET_STATE_LISTENING)
{
throw NativeException(QString(Native::Msg::Invalid_socket_state).toStdString());
}
TCPSocketEventHandler* eHandler = new TCPSocketEventHandler(eventContainer_, this);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this get deleted?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe QObject::connect will take ownership.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QObject::connect will not take ownership, it only connects the signals to slots. Please see my comment about refactoring.

if (tcpServer_->hasPendingConnections())
{
eHandler->accepted();
}
else
{
// Accept next incoming connection immediately
QObject::connect(tcpServer_, SIGNAL(newConnection()), eHandler, SLOT(accepted()));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will keep adding new connections, need to make sure it is only done once or the others dosconnected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qt::UniqueConnection only prevents duplicate connections if both objects are the same, but since you're creating new handlers every time this will never be the case. Please see my comment about refactoring.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes handler member

}
}