Re: [RFC 2/4] stk: Handle Launch Browser proactive command

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2730 bytes --]

Hi Jeevaka,

> +static gboolean handle_command_launch_browser(const struct stk_command *cmd,
> +						struct stk_response *rsp,
> +						struct ofono_stk *stk)
> +{
> +	const struct stk_command_launch_browser *lb = &cmd->launch_browser;
> +	struct ofono_modem *modem = __ofono_atom_get_modem(stk->atom);
> +	static unsigned char no_cause[] = { 0x00 };
> +	static unsigned char no_service[] = { 0x04 };
> +	struct ofono_atom *netreg_atom;
> +	struct ofono_netreg *netreg;
> +	int qualifier = cmd->qualifier;
> +	char *alpha_id;
> +	int err;
> +	int status;
> +
> +	if (qualifier > 3 || qualifier == 1) {
> +		rsp->result.type = STK_RESULT_TYPE_COMMAND_NOT_UNDERSTOOD;
> +		return TRUE;
> +	}
> +
> +	if ( lb->browser_id > 4) {
> +		rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD;
> +		return TRUE;
> +	}
> +
> +	if (ofono_modem_get_online(modem) == FALSE) {
> +		ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_TERMINAL_BUSY,
> +					no_service);
> +		return TRUE;
> +	}
> +
> +	netreg_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_NETREG);
> +	if (netreg_atom == NULL) {
> +		rsp->result.type = STK_RESULT_TYPE_NOT_CAPABLE;
> +		return TRUE;
> +	}
> +
> +	netreg = __ofono_atom_get_data(netreg_atom);
> +	status = __ofono_netreg_registration_status(netreg);
> +
> +	if (status != NETWORK_REGISTRATION_STATUS_REGISTERED &&
> +			status != NETWORK_REGISTRATION_STATUS_ROAMING) {
> +		ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_TERMINAL_BUSY,
> +					no_service);
> +		return TRUE;
> +	}
> +

So a question about this.  Why are we checking the netreg atom?
Shouldn't we be checking the gprs atom's attached property?  But this
brings up an even more interesting question, do we even need to assume
that we have to be attached to the cellular network to perform these
operations?  We could in theory go over wifi.

Thoughts?

> +	alpha_id = dbus_apply_text_attributes(lb->alpha_id ? lb->alpha_id : "",
> +							&lb->text_attr);
> +	if (alpha_id == NULL) {
> +		rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD;
> +		return TRUE;
> +	}
> +
> +	err = stk_agent_launch_browser(stk->current_agent, alpha_id,
> +					lb->icon_id.id, lb->url,
> +					confirm_launch_browser_cb,
> +					stk, NULL, stk->timeout * 1000);
> +	g_free(alpha_id);
> +
> +	if (err < 0) {
> +		/*
> +		 * We most likely got an out of memory error, tell SIM
> +		 * to retry
> +		 */
> +		ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_TERMINAL_BUSY,
> +					no_cause);
> +		return TRUE;
> +	}
> +
> +	stk->respond_on_exit = TRUE;
> +	stk->cancel_cmd = stk_request_cancel;
> +
> +	return FALSE;
> +}
> +

Rest seems reasonable to me.

Regards,
-Denis