Skip to content

First Attempt to implement Payment SDK for WebWorks. #455

Closed
wants to merge 1 commit into from

7 participants

@shamim1
shamim1 commented Dec 10, 2012

Implemented API method for "Purchase" method in Payment Service.

(First Push attempt didn't have any changes as I forgot to check-in code before pushing to github.)

@nukulb
nukulb commented Dec 10, 2012

@rwmtse @pagey - can we review this please?

@nukulb
nukulb commented Dec 10, 2012

there is an .so file in there too, please remove that. Thats a build time artifact and doesn't need to be a part of the repo

@pagey
pagey commented Dec 10, 2012

we don't use cmake anymore, this need to be converted to use the qnx make system

@shamim1
shamim1 commented Dec 10, 2012

ok

@pagey pagey commented on an outdated diff Dec 10, 2012
ext/Payment/client.js
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+var _self = {},
+ _ID = require("./manifest.json").namespace,
+ onCreateSuccess = null,
+ onCreateFail = null;
+
+
+function webworksPurchaseCallback(result) {
+
+ paymentJNext.stopEvents();
+
+ //if (result === SUCCESS) {
@pagey
pagey added a note Dec 10, 2012

please remove commented code that is not used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 10, 2012
ext/Payment/client.js
+// }
+ }
+ onCreateSuccess = null;
+ onCreateFail = null;
+}
+
+_self.purchase = function (purchase_arguments_t, successCallback, failCallback) {
+ var args = { "digitalGoodID" : purchase_arguments_t.digitalGoodID || "",
+ "digitalGoodSKU" : purchase_arguments_t.digitalGoodSKU || "",
+ "digitalGoodName" : purchase_arguments_t.digitalGoodName || "",
+ "metaData" : purchase_arguments_t.metaData || "",
+ "purchaseAppName" : purchase_arguments_t.purchaseAppName || "",
+ "purchaseAppIcon" : purchase_arguments_t.purchaseAppIcon || "",
+ "extraParameters" : purchase_arguments_t.extraParameters || "" };
+
+// // Check if create() called more than once
@pagey
pagey added a note Dec 10, 2012

remove commented code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 10, 2012
ext/Payment/native/payment_bps.cpp
+
+
+void PaymentBPS::onPurchaseSuccess(bps_event_t *event)
+{
+ if (event == NULL) {
+ fprintf(stderr, "Invalid event.\n");
+ return;
+ }
+
+ std::stringstream ss;
+ std::string separator= " ;;;; ";
+ Json::Value purchasedItem;
+ Json::Value extraParameter;
+ Json::FastWriter writer;
+
+ try
@pagey
pagey added a note Dec 10, 2012

please do not use exceptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 10, 2012
ext/Payment/native/payment_bps.cpp
+ ss.str("");
+ }
+
+ //std::string result = ss.str();
+ //m_parent->NotifyEvent(result);
+
+ m_parent->NotifyEvent("payment.purchase.callback", ss.str());
+}
+
+void PaymentBPS::onFailureCommon(bps_event_t *event)
+{
+ std::stringstream ss;
+ std::string separator= " ;;;; ";
+ Json::FastWriter writer;
+
+ try
@pagey
pagey added a note Dec 10, 2012

don't use exceptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 10, 2012
ext/Payment/native/payment_bps.cpp
+ int error_id = paymentservice_event_get_error_id(event);
+ const char* error_text = paymentservice_event_get_error_text(event);
+
+ Json::Value error;
+ error["errorID"] = Json::Value(error_id);
+ error["errorText"] = Json::Value(error_text);
+
+ // fprintf(stderr, "Purchase Failed.
+ // error_id,
+ // error_text ? error_text : "N/A",
+
+ // Convert to string
+
+ ss << "FAILURE";
+ ss << separator;
+ ss << writer.write(error);
@pagey
pagey added a note Dec 10, 2012

use Json::FastWriter().write instead of creating an object for one time use

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey and 1 other commented on an outdated diff Dec 10, 2012
ext/Payment/native/payment_js.cpp
+ // Parse JSON object
+ Json::Value obj;
+
+ if (command.length() > index) {
+ std::string jsonObject = command.substr(index + 1, command.length());
+ Json::Reader reader;
+
+ bool parse = reader.parse(jsonObject, obj);
+ if (!parse) {
+ fprintf(stderr, "%s", "error parsing\n");
+ return "Cannot parse JSON object";
+ }
+ }
+
+ if (strCommand == "purchase") {
+ StartEvents();
@pagey
pagey added a note Dec 10, 2012

it looks like StartEvents is being called every time purchase is called, it would be better to check a flag rather than call this each time

@shamim1
shamim1 added a note Dec 10, 2012

At this moment we are basically thinking that - if I method call to the API need to listen for events, we can call start event (before we do any native code execution). Once the method call is done returning, we can call stop event.

The stop event is going to be called from the JavaScript side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 10, 2012
ext/Payment/native/payment_js.hpp
+
+#include <pthread.h>
+#include <plugin.h>
+#include <sstream>
+#include <string>
+
+void* PaymentEventThread(void *args);
+
+class Payment : public JSExt
+{
+public:
+ explicit Payment(const std::string& id);
+ virtual ~Payment();
+ virtual std::string InvokeMethod(const std::string& command);
+ virtual bool CanDelete();
+ //void NotifyEvent(const std::string& event);
@pagey
pagey added a note Dec 10, 2012

remove this line if you aren't using it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey and 1 other commented on an outdated diff Dec 10, 2012
ext/Payment/client.js
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+var _self = {},
+ _ID = require("./manifest.json").namespace,
+ onCreateSuccess = null,
+ onCreateFail = null;
+
+
+function webworksPurchaseCallback(result) {
+
+ paymentJNext.stopEvents();
@pagey
pagey added a note Dec 10, 2012

not sure how this is supposed to work? JNEXT runs on the server (index) and this variable is not defined anywhere

@shamim1
shamim1 added a note Dec 10, 2012

Ok...

I changed the code from

    if (strEventId === "payment.purchase.callback") {
        _event.trigger("payment.purchase.callback", JSON.parse(args));
    }

to

    if (strEventId === "payment.purchase.callback") {
        stopEvents();
        _event.trigger("payment.purchase.callback", JSON.parse(args));
    }

That way the code will stop event (from the Native side) and then trigger the event to the client side. Hopefully that will work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey and 1 other commented on an outdated diff Dec 10, 2012
ext/Payment/client.js
+
+
+function webworksPurchaseCallback(result) {
+
+ paymentJNext.stopEvents();
+
+ //if (result === SUCCESS) {
+ if (result && result.length > 0) { //making sure that we get a value from the result
+ // if onCreateSuccess is defined (I assume)
+ var arData = result.split(" ;;;; ");
+ var isSuccessful = arData[0];
+
+ if (onCreateSuccess) {
+ if (isSuccessful === "SUCCESS")
+ {
+ onCreateSuccess(JSON.parse(Purchase));
@pagey
pagey added a note Dec 10, 2012

Purchase is not defined anywhere

@shamim1
shamim1 added a note Dec 10, 2012

It should be arData1

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey
pagey commented Dec 10, 2012

please use jake lint to fix your lint errors

@rwmtse rwmtse and 1 other commented on an outdated diff Dec 10, 2012
ext/Payment/native/payment_bps.cpp
+ {
+ //unsigned request_id = paymentservice_event_get_request_id(event); //NOT USED YET
+ //We are using PurchaseID instead of TransactionID
+ purchasedItem["purchase_id"] = Json::Value(paymentservice_event_get_purchase_id(event, 0));
+ purchasedItem["date"] = Json::Value(paymentservice_event_get_date(event, 0));
+ purchasedItem["digitalGoodID"] = Json::Value(paymentservice_event_get_digital_good_id(event, 0));
+ purchasedItem["digitalGoodSKU"] = Json::Value(paymentservice_event_get_digital_good_sku(event, 0));
+ purchasedItem["licenseKey"] = Json::Value(paymentservice_event_get_license_key(event, 0));
+ purchasedItem["metaData"] = Json::Value(paymentservice_event_get_metadata(event, 0));
+
+ int extra_parameter_count = paymentservice_event_get_extra_parameter_count(event, 0);
+ //if (extra_parameter_count != NULL)
+ if (extra_parameter_count)
+ {
+ //int i;
+ for (int i = 0; i > extra_parameter_count; i++)
@rwmtse
rwmtse added a note Dec 10, 2012

Are we sure this loop is correct? What are the possible values of extra_parameter_count? I think it will be > 0 right? If so the loop will never be entered.

@shamim1
shamim1 added a note Dec 10, 2012

ya... I see that. sorry. Will fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shamim1
shamim1 commented Dec 10, 2012

RE "please use jake lint to fix your lint errors"

I didn't understand what exactly I need to do to address that. Sorry.

@pagey
pagey commented Dec 10, 2012

run jake lint in your framework folder to see the lint problems with your code, it will display which files and lines need to be fixed, this branch will not build on our CI server until the lint problems are fixed

@shamim1
shamim1 commented Dec 10, 2012

I found few code areas to fix....
I will submit a code review early tomorrow morning.

Thanks

@shamim1
shamim1 commented Dec 11, 2012

I made bunch of changes. But, I am having hard time to amend the commit on "'next-payment-sdk'" branch. Looks like that the "amend" worked fine locally. But, I can't seem to push the changes.

@nukulb
nukulb commented Dec 11, 2012

need to force push because you changed the history
but make sure you only push to your branch

so use

git push -f origin next-payment-sdk

@shamim1
shamim1 commented Dec 11, 2012

Ok, thanks. Not sure how I changed the history, I may have done it accidentally. I am still learning Git.

I made bunch of changes in the "amended version". I am still working on "make".

@nukulb
nukulb commented Dec 11, 2012

you change the history every time you do git commit --amend as you are changing an already pushed commit.

@shamim1
shamim1 commented Dec 11, 2012

ok, thanks.

@shamim1
shamim1 commented Dec 11, 2012

Seems like nobody is reviewing latest code. Do I need to do anything else or do I just need to wait?

Also, I am working on "make" and fixing few things found by "make".

@pagey
pagey commented Dec 11, 2012

this code still doesn't pass lint

@shamim1
shamim1 commented Dec 11, 2012

well I don't see any payment problem. I haven't touched any other folder except "payment". The only thing I find is -

ext\Payment\index.js :
Unused Variables:
_methods(17), _exports(19), info(95)

But that doesn't show up in the "error" section.

@pagey
pagey commented Dec 11, 2012

it's not passing cpplint, are you running this on windows?

@pagey
pagey commented Dec 11, 2012

also missing unit tests

@shamim1
shamim1 commented Dec 11, 2012

yes... The only thing I can say is that "Payment" folder has upper case "P". It may need to be renamed as "payment" with lower case "p". Other than that I don't know.

@pagey pagey commented on an outdated diff Dec 11, 2012
ext/Payment/native/payment_js.cpp
+ std::string jsonObject = command.substr(index + 1, command.length());
+ Json::Reader reader;
+
+ bool parse = reader.parse(jsonObject, obj);
+ if (!parse) {
+ fprintf(stderr, "%s", "error parsing\n");
+ return "Cannot parse JSON object";
+ }
+ }
+
+ if (strCommand == "purchase") {
+ StartEvents();
+
+ purchase_arguments_t* args = NULL;
+ std::stringstream ss;
+ try{
@pagey
pagey added a note Dec 11, 2012

please remove the c++ exceptions, we do not use c++ exceptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shamim1
shamim1 commented Dec 11, 2012

Ya, I am not sure who will write unit test for that. At this moment I am very interested to make sure that you guys don't find any problem in code and I can move forward.

@pagey pagey and 1 other commented on an outdated diff Dec 11, 2012
ext/Payment/native/payment_js.cpp
+ paymentservice_purchase_arguments_set_extra_parameter(args, itr->c_str(), obj["extraParameters"].operator[](itr->c_str()).asCString());
+ }
+
+ webworks::PaymentBPS *payment = new webworks::PaymentBPS();
+ ss << payment->purchase(args);
+ paymentservice_purchase_arguments_destroy(args);
+ delete payment;
+ return ss.str();
+ //return "";
+ }
+ catch (int e)
+ {
+ StopEvents();
+
+ ss.str("");
+ ss << BPS_FAILURE;
@pagey
pagey added a note Dec 11, 2012

this should be converted to use our utilities class Utils::IntToStr() you can find source for this in the ext/utils folder

@shamim1
shamim1 added a note Dec 11, 2012

Having hard time to #include

I see few other extension included them and used the method you mentioned. I am wondering what I am missing.

@pagey
pagey added a note Dec 11, 2012

as long as you have UTILS=yes in your common.mk you can just use #include and it should work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 11, 2012
ext/Payment/native/payment_js.cpp
+ static char name[] = "Payment";
+ return name;
+}
+
+JSExt* onCreateObject(const std::string& className, const std::string& id)
+{
+ if (className != "Payment") {
+ return NULL;
+ }
+
+ return new Payment(id);
+}
+
+std::string Payment::InvokeMethod(const std::string& command)
+{
+// int index = command.find_first_of(" ");
@pagey
pagey added a note Dec 11, 2012

remove lines that aren't being used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 11, 2012
ext/Payment/native/payment_js.cpp
+#include <string>
+#include "payment_js.hpp"
+#include "payment_bps.hpp"
+
+bool eventsInitialized = false;
+
+void* PaymentEventThread(void *args)
+{
+ Payment *parent = static_cast<Payment *>(args);
+ webworks::PaymentBPS *payment = new webworks::PaymentBPS(parent);
+
+ if (payment) {
+ if (payment->InitializeEvents() == 0) {
+ eventsInitialized = true;
+
+ // Poll for events in ConnectionBPS. This will run until StopEvents() disables events.
@pagey
pagey added a note Dec 11, 2012

remove or update the comment, this is talking about the connection extension

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 11, 2012
ext/Payment/native/payment_bps.hpp
+#include <bps/paymentservice.h>
+
+class Payment;
+
+namespace webworks {
+
+class PaymentBPS {
+public:
+ explicit PaymentBPS(Payment *parent = NULL);
+ ~PaymentBPS();
+ int InitializeEvents();
+ int WaitForEvents();
+ static void SendEndEvent();
+
+ //Method call from JavaScript side
+ BPS_API int purchase(purchase_arguments_t *purchase_arguments);
@pagey
pagey added a note Dec 11, 2012

public methods start with capital letters, should be Purchase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shamim1
shamim1 commented Dec 11, 2012

One more note... I made 2 very minor change on .cpp files. After I ran "make" for first time I saw those 2 errors. If you want, I can upload that change. But, I am not being able to run "make"properly yet. And I am not sure at this moment how to fix that. I will need somebody's help on that.

@rwmtse
rwmtse commented Dec 11, 2012

@shamim1 You need to run jake lint to see the list of all formatting errors in both JavaScript and C++

@shamim1
shamim1 commented Dec 11, 2012

I did. There were quite a few errors from Payment yesterday, the code I checked-in didn't have any error from "payment" folder. There was one warning though -

ext\Payment\index.js :
Unused Variables:
_methods(17), _exports(19), info(95)

There were 4 other error (not from Payment folder. I assume that they will not be a concern).

Note: I am running on windows. And I will run "jake lint" again after I fix the latest findings.

@rwmtse
rwmtse commented Dec 11, 2012

For those unused variables reported from lint, it means you are not using them anywhere, you should just removed them from your code. As @pagey previously said, there must be no lint errors in order for CI server to build your branch.

@shamim1
shamim1 commented Dec 11, 2012

Ok, thanks. But, the output don't really give a clue where I can find those unused variables. I just felt that since there are "4 specific error shows up before the unused variables notification", it could still work since there is no error. Now, I need help to understand how I can find the unused variable comes from "jake lint" output.

@pagey
pagey commented Dec 11, 2012

Your C++ code has a lot of lint problems, but for some reason c++ was disabled on Windows. Do you have access to a mac or linux machine? Otherwise you can look at the CI output for your build here: http://ci0000003863287:8080/hudson/job/BB10-Webworks-Framework-next-payment-sdk/

@shamim1
shamim1 commented Dec 11, 2012

Ok, I see them. Oh it was painful to fix all the javascript lint problem. At this moment, I don't have a linux machine. We may be able to manage one.

In order to fix the problem, I will need to be able to run the result every times after I make a small changes. I don't think "hudson jobs" would help on that.

It appears that unless we find why lint isn't working on windows, we will need to find a linux machine to fix lint problem.

@pagey
pagey commented Dec 11, 2012

it does the lint on the CI server, if you look at the console output you can see which lines have a problem and you should be able to fix it with one change

@shamim1
shamim1 commented Dec 11, 2012

I remember using Eclipse "correct indentation" did help me quite a bit. I wonder if I should use Eclipse "Format" option as well or would create more problem. I will use Eclipse build-in formatter.

@pagey
pagey commented Dec 11, 2012

I don't know what Eclipse will do, the only way to be sure is to look at the CI output for the lint problems it spots. This is currently the only way to see lint problems on Windows. We do not have a fix for Windows linting at the moment.

@shamim1
shamim1 commented Dec 11, 2012

Just checked-in some code. See how far the "cpp lint" problem is fixed. Waiting for the build result.

@shamim1
shamim1 commented Dec 12, 2012

CI build 6 says pending... I tried using "build now". But, that didn't work. Is there something you guys do to fix it?

@nukulb
@shamim1
shamim1 commented Dec 12, 2012

The build seems to work. But, I don't see output like before. I am interested to find out what error cpplint have now.

@shamim1
shamim1 commented Dec 12, 2012

My local build seem to be failing as well. Not sure why. Here is the output -

...
make[3]: *** No rule to make target C:/GitRepo/blackberry-webworks/BB10-WebWorks-Framework/ext/payment/native/paymentjnext.use', needed byC:/GitRepo/blackberry-webworks/BB10-WebWorks-Framework/ext
payment/native/arm/o.le-v7/paymentjnext'. Stop.
make[3]: Leaving directory C:/GitRepo/blackberry-webworks/BB10-WebWorks-Framework/ext/payment/native/arm/o.le-v7'
make[2]: *** [all] Error 2
make[2]: Leaving directory
C:/GitRepo/blackberry-webworks/BB10-WebWorks-Framework/ext/payment/native/arm'
make[1]: *** [all] Error 2
make[1]: Leaving directory `C:/GitRepo/blackberry-webworks/BB10-WebWorks-Framework/ext/payment/native'
make: *** [all] Error 2

@rwmtse
rwmtse commented Dec 12, 2012

@shamim1 I found out how to fix your build errors:

  • In common.mk, you should remove the following lines, because you don't have any native unit test
ifeq ($(UNITTEST),yes)
NAME=test
SRCS+=test_main.cpp
USEFILE=
endif
  • Delete this folder: ext/payment/native/arm/o.le-v7/ this o.le-v7 target is only needed when you have native unit test

  • Delete this folder: ext/payment/native/x86/o/ this o target is only needed when you have native unit test

After doing the above, I could build your branch on my Mac. But there are still lint errors, I've pasted the errors here:
http://git-mirror-torch.rim.net:5000/show/127/

@shamim1
shamim1 commented Dec 12, 2012

Ok, my build error was fixed.

Now

[ext/Payment/native/payment_bps.cpp:22: Found C system header after other header. Should be: payment_bps.h, c system, c++ system, other. [build/include_order] [4]]

I changed the order of include. Hopefully that will fix it.

[ext/Payment/native/payment_bps.cpp:82: If an else has a brace on one side, it should have it on both [readability/braces] [5]]

Not sure what it is asking. Do I have to have "else" for every "if" case in the code?

[ext/Payment/native/payment_js.cpp:103: Add #include for vector<> [build/include_what_you_use] [4]]

I added #include even though I was using std::vector which is not really native "vector" class.

@jeffheifetz jeffheifetz commented on an outdated diff Dec 12, 2012
ext/Payment/client.js
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+var _self = {},
+_ID = require("./manifest.json").namespace,
+onCreateSuccess = null,
+onCreateFail = null;
@jeffheifetz
jeffheifetz added a note Dec 12, 2012

Please indent lines 18-20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeffheifetz

Please use 4 spaces for tabs (not two) and trim your lines as well.

@jeffheifetz jeffheifetz commented on an outdated diff Dec 12, 2012
ext/Payment/client.js
+ {
+ throw arData[1];
+ }
+ }
+ }
+ else {
+ //something wrong happneed. throw to user
+ throw "Purchase Failed. Unexpected Error Occured.";
+ }
+
+ onCreateSuccess = null;
+ onCreateFail = null;
+}
+
+_self.purchase = function (purchase_arguments_t, successCallback, failCallback) {
+ var args = { "digitalGoodID" : purchase_arguments_t.digitalGoodID || "",
@jeffheifetz
jeffheifetz added a note Dec 12, 2012

What if purchase_arguments_t is not defined? Then you will have an error.

Also are only string values accepted, because 0 || "" === ""

Lastly purchase_arguments_t is not a very "javascripty" name but thats mainly a nitpick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeffheifetz jeffheifetz commented on an outdated diff Dec 12, 2012
ext/Payment/client.js
+ onCreateSuccess = null;
+ onCreateFail = null;
+}
+
+_self.purchase = function (purchase_arguments_t, successCallback, failCallback) {
+ var args = { "digitalGoodID" : purchase_arguments_t.digitalGoodID || "",
+ "digitalGoodSKU" : purchase_arguments_t.digitalGoodSKU || "",
+ "digitalGoodName" : purchase_arguments_t.digitalGoodName || "",
+ "metaData" : purchase_arguments_t.metaData || "",
+ "purchaseAppName" : purchase_arguments_t.purchaseAppName || "",
+ "purchaseAppIcon" : purchase_arguments_t.purchaseAppIcon || "",
+ "extraParameters" : purchase_arguments_t.extraParameters || "" };
+
+ onCreateSuccess = successCallback;
+ onCreateFail = failCallback;
+ window.webworks.event.once(_ID, "payment.purchase.callback", webworksPurchaseCallback);
@jeffheifetz
jeffheifetz added a note Dec 12, 2012

Its better to create this callback within here than to use the same one. That way you can have more than one purchase request at once in theory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeffheifetz jeffheifetz and 1 other commented on an outdated diff Dec 12, 2012
ext/Payment/index.js
+module.exports = {
+ purchase: function (args) {
+ var purchase_arguments_t = { "digitalGoodID" : JSON.parse(decodeURIComponent(args.digitalGoodID)),
+ "digitalGoodSKU" : JSON.parse(decodeURIComponent(args.digitalGoodSKU)),
+ "digitalGoodName" : JSON.parse(decodeURIComponent(args.digitalGoodName)),
+ "metaData" : JSON.parse(decodeURIComponent(args.metaData)),
+ "purchaseAppName" : JSON.parse(decodeURIComponent(args.purchaseAppName)),
+ "purchaseAppIcon" : JSON.parse(decodeURIComponent(args.purchaseAppIcon)),
+ "extraParameters" : JSON.parse(decodeURIComponent(args.extraParameters)) };
+
+ try {
+ //success(paymentJNext.purchase(purchase_arguments_t));
+ paymentJNext.purchase(purchase_arguments_t);
+ } catch (e) {
+ paymentJNext.stopEvents();
+ throw "Purchase Failed with error: " + e;
@jeffheifetz
jeffheifetz added a note Dec 12, 2012

Don't throw an error, call fail.

@shamim1
shamim1 added a note Dec 12, 2012

I will pass fail(-1, e);

@jeffheifetz
jeffheifetz added a note Dec 12, 2012

Probably better just to send the error message, if you want to log the trace it should be fine to do so if it may be necessary for debugging issues. @nukulb to confirm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeffheifetz jeffheifetz and 1 other commented on an outdated diff Dec 12, 2012
ext/Payment/index.js
+_event = require("../../lib/event"),
+_exports = {};
+
+module.exports = {
+ purchase: function (args) {
+ var purchase_arguments_t = { "digitalGoodID" : JSON.parse(decodeURIComponent(args.digitalGoodID)),
+ "digitalGoodSKU" : JSON.parse(decodeURIComponent(args.digitalGoodSKU)),
+ "digitalGoodName" : JSON.parse(decodeURIComponent(args.digitalGoodName)),
+ "metaData" : JSON.parse(decodeURIComponent(args.metaData)),
+ "purchaseAppName" : JSON.parse(decodeURIComponent(args.purchaseAppName)),
+ "purchaseAppIcon" : JSON.parse(decodeURIComponent(args.purchaseAppIcon)),
+ "extraParameters" : JSON.parse(decodeURIComponent(args.extraParameters)) };
+
+ try {
+ //success(paymentJNext.purchase(purchase_arguments_t));
+ paymentJNext.purchase(purchase_arguments_t);
@jeffheifetz
jeffheifetz added a note Dec 12, 2012

Call success if it was successfull.

@shamim1
shamim1 added a note Dec 12, 2012

If the "purchase" method is successful, we then need to listen for event. and after that we can call "success" callback.

@shamim1
shamim1 added a note Dec 12, 2012

I will call success() here as you specified. The callback will happen later once the "payment.purchase.callback" event is triggered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeffheifetz jeffheifetz and 1 other commented on an outdated diff Dec 12, 2012
ext/Payment/index.js
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+var paymentJNext,
+_methods = ["purchase"],
+_event = require("../../lib/event"),
+_exports = {};
+
+module.exports = {
+ purchase: function (args) {
@jeffheifetz
jeffheifetz added a note Dec 12, 2012

This is the wrong signature, there should be a success and fail as well.

@shamim1
shamim1 added a note Dec 12, 2012

Looks like I need to fix code here. From Client.js I call "return window.webworks.execSync(_ID, "purchase", args);"

SO, that probably looks for

purchase: function (success, fail, args) {

in index.js

I will fix the function signature and fix the code with

        try {
            paymentJNext.purchase(purchase_arguments_t);
            success();
        } catch (e) {
            paymentJNext.stopEvents();
            //throw "Purchase Failed with error: " + e;
            fail(-1, e);
        }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shamim1
shamim1 commented Dec 12, 2012

In general the way we expected the flow to work is -

The client will call purchase with callback for success and fail.

Webworks will call the "PaymentService" purchase method. The method may return success or failure. In case of success, we will wait for the "PURCHASE_RESPONSE" event. Once the event is fired then we will get all the values and pass to index.js. index.js will trigger to the client using "_event.trigger("payment.purchase.callback", JSON.parse(args));"

In case purchase method returns failure then we call
"_event.trigger("payment.purchase.callback", JSON.parse("BPS_FAILURE" + " ;;;; " + "Purchase Failed. Payment Service Error."));}". That will go to the client.js and it will let the user know that the attempt to purchase failed.

@rwmtse
rwmtse commented Dec 12, 2012

@shamim1
For all your C++ code, please make sure you follow this format for all if or if-else statements:

    if (something) {
        /* Do some stuff here. */
    } else {
        /* Do something else. */
    }

Notice that open brace and close brace are on the same line.

But for methods, the open brace and close brace are on the next line, like:

int doSomething(char *opt)
{
    /* Do stuff */
}
@shamim1
shamim1 commented Dec 12, 2012

cool. thanks :)

@pagey pagey and 1 other commented on an outdated diff Dec 12, 2012
ext/Payment/index.js
+ return false;
+ }
+
+ JNEXT.registerEvents(self);
+ };
+
+ self.onEvent = function (strData) {
+ var arData = strData.split(" "),
+ strEventId = arData[0],
+ args = arData[1],
+ info = {};
+
+ // Trigger the event handler of specific Payment events
+ if (strEventId === "payment.purchase.callback") {
+ self.stopEvents();
+ _event.trigger("payment.purchase.callback", JSON.parse(args));
@pagey
pagey added a note Dec 12, 2012

I don't think this is going to work, if your JSON object has spaces in it, which it probably does this will never be parable, you need to use Array.slice to combine everything into a single string again

@shamim1
shamim1 added a note Dec 12, 2012

You pointed out in a good place. I wonder if I can call
_event.trigger("payment.purchase.callback", args);

The args content call be like "SUCCESS ;;;; JSON Object". So, JSON.parse won't work. But, if I don't have to call JSON.parse then it should be fine.

Then, the callback should go back to
function webworksPurchaseCallback(result) {

in client.js. I split JSON value there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 12, 2012
ext/Payment/native/payment_bps.cpp
+ //onGetExistingPurchasesSuccess(event);
+ }
+ else if (CANCEL_SUBSCRIPTION_RESPONSE == bps_event_get_code(event)) {
+ //onGetExistingPurchasesSuccess(event);
+ }
+ }
+ else {
+ onFailureCommon(event);
+ }
+ }
+ }
+ }
+ }
+
+ return (status == BPS_SUCCESS) ? 0 : 1;
+}// End Method
@pagey
pagey added a note Dec 12, 2012

don't need this comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 12, 2012
ext/Payment/native/payment_bps.cpp
+ int extra_parameter_count = paymentservice_event_get_extra_parameter_count(event, 0);
+ if (extra_parameter_count)
+ {
+ for (int i = 0; i < extra_parameter_count; i++)
+ {
+ const char* key = paymentservice_event_get_extra_parameter_key_at_index(event, i, 0);
+ std::string key_Str(key);
+ const char* value = paymentservice_event_get_extra_parameter_value_at_index(event, i, 0);
+ extraParameter[key_Str] =Json::Value(value);
+ }
+ }
+ purchasedItem["extraParameters"]= extraParameter;
+
+ ss << "SUCCESS";
+ ss << separator;
+ ss << writer.write(purchasedItem);
@pagey
pagey added a note Dec 12, 2012

use Json::FastWriter().write instead, it's cheaper than constructing an object for a one time use

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 12, 2012
ext/Payment/native/payment_bps.cpp
+
+ int error_id = paymentservice_event_get_error_id(event);
+ const char* error_text = paymentservice_event_get_error_text(event);
+
+ Json::Value error;
+ error["errorID"] = Json::Value(error_id);
+ error["errorText"] = Json::Value(error_text);
+
+ //fprintf(stderr, "Purchase Failed.
+ // error_id,
+ // error_text ? error_text : "N/A",
+
+ // Convert to string
+ ss << "FAILURE";
+ ss << separator;
+ ss << Json::FastWriter().write(error);
@pagey
pagey added a note Dec 12, 2012

use Json::FastWriter().write

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 12, 2012
ext/Payment/native/payment_bps.cpp
+ ss << writer.write(purchasedItem);
+
+ std::string result_str = ss.str();
+ fprintf(stderr, "stop: Failed to write to pipe\n");
+ //fprintf(stderr, result_str);
+
+ m_parent->NotifyEvent("payment.purchase.callback", ss.str());
+}
+
+void PaymentBPS::onFailureCommon(bps_event_t *event)
+{
+ std::stringstream ss;
+ std::string separator= " ;;;; ";
+ Json::FastWriter writer;
+
+ int error_id = paymentservice_event_get_error_id(event);
@pagey
pagey added a note Dec 12, 2012

this should probably be const

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 12, 2012
ext/payment/native/arm/o.le-v7/Makefile
@@ -0,0 +1 @@
+include ../../common.mk
@pagey
pagey added a note Dec 12, 2012

unless you plan on writing native unit tests for this extension, you can remove this folder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 12, 2012
ext/payment/native/x86/o/Makefile
@@ -0,0 +1,2 @@
+UNITTEST=yes
+include ../../common.mk
@pagey
pagey added a note Dec 12, 2012

and this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Dec 12, 2012
ext/Payment/native/payment_bps.cpp
+
+ ss << "SUCCESS";
+ ss << separator;
+ ss << writer.write(purchasedItem);
+
+ std::string result_str = ss.str();
+ fprintf(stderr, "stop: Failed to write to pipe\n");
+ //fprintf(stderr, result_str);
+
+ m_parent->NotifyEvent("payment.purchase.callback", ss.str());
+}
+
+void PaymentBPS::onFailureCommon(bps_event_t *event)
+{
+ std::stringstream ss;
+ std::string separator= " ;;;; ";
@pagey
pagey added a note Dec 12, 2012

this should be const

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shamim1
shamim1 commented Dec 12, 2012

how many lint error you are getting now. I am a bit curious.

@rwmtse
rwmtse commented Dec 12, 2012

@shamim1 have you pushed your changes?

@shamim1
shamim1 commented Dec 12, 2012

ya, CODE AMENDED (6) should be the one.

Pending changes (will check-in) I have is

_event.trigger("payment.purchase.callback", JSON.parse(args));
to
_event.trigger("payment.purchase.callback", args);

And some small .cpp change you mentioned in the review (constant and Json::FastWriter().write)

@shamim1
shamim1 commented Dec 12, 2012

Ok, I am not sure if

_event.trigger("payment.purchase.callback", JSON.parse(args));
to
_event.trigger("payment.purchase.callback", args);

will work. I may have to bring a JSON array from the .cpp file and in client.js I may need to change some code. If you are sure that it won't work, I will need to start tricking the code.

@shamim1
shamim1 commented Dec 12, 2012

I wonder if I can get a comment on if "_event.trigger("payment.purchase.callback", args);" would work. If I have to convert everything to JSON to call "_event.trigger" method then I can do that though it will take some time.

@rwmtse rwmtse and 1 other commented on an outdated diff Dec 12, 2012
ext/Payment/index.js
+ var self = this,
+ hasInstance = false;
+
+ self.stopEvents = function () {
+ JNEXT.invoke(self.m_id, "stopEvents");
+ };
+
+ self.purchase = function (args) {
+ var val = JNEXT.invoke(self.m_id, "purchase " + JSON.stringify(args));
+ //----------------------------------------------------------------------------------------------------------
+ //if (val === "BPS_FAILURE") {
+ //----------------------------------------------------------------------------------------------------------
+ if (val === "-1") {
+ //window.webworks.event.remove(_self._ID, "payment.purchase.callback", webworksPurchaseCallback);
+ self.stopEvents();
+ _event.trigger("payment.purchase.callback", JSON.parse("BPS_FAILURE" + " ;;;; " + "Purchase Failed. Payment Service Error."));
@rwmtse
rwmtse added a note Dec 12, 2012

When you call event.trigger, the object you send must be a valid JavaScript object, e.g.

_event.trigger("fontchanged", {'fontFamily': fontFamily, 'fontSize': fontSize});

Your current code at line 62 for sure will not work because the string you pass to JSON.parse is not valid JSON.

@shamim1
shamim1 added a note Dec 12, 2012

yep. So, basically it need to pass Object (can't be string). I already started making some changes as I looked in the event.js and figured that it will need to be JSON object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shamim1
shamim1 commented Dec 13, 2012

converted everything to object coming from .cpp file. Hopefully that will fix the bigger question. Since it is getting delayed quite a bit, I will probably give a call next time if you find any thing to fix now. That should make it go faster.

@rwmtse
rwmtse commented Dec 13, 2012

@shamim1 I strongly suggest you to take a look at other Web works extension. Your Java script is not following our coding convention. And you still need to add Java script unit test and functional test. A few extensions for your reference: sensors, BBM, pim contacts, pim calendar.

@shamim1
shamim1 commented Dec 13, 2012

The thing is when I looked into extensions, I found that ours is unique. Our uses BPS and we have callback method for success or failure. And that callback gets active after a API call to BPS is successful. The 2 extension that I followed most is "Connection" from (BB10-WebWorks-Framework-1.0.2.9) and "Push".

May be we need to talk about how I implemented and how you want me to implement. Basically that is the reason I initiated code review first day of the week. I wanted to make sure that the code makes sense to you and I don't have to do any structural change.

At this moment I have spent quite a bit of time and wondering what else you find that I should fix. I guess we should talk.

@rwmtse rwmtse commented on an outdated diff Dec 13, 2012
ext/Payment/manifest.json
@@ -0,0 +1,5 @@
+{
+ "global": false,
+ "namespace": "blackberry.payment",
+ "dependencies": []
@rwmtse
rwmtse added a note Dec 13, 2012

I think this should be:

"dependencies": ["utils"]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shamim1
shamim1 commented Dec 13, 2012

I am trying to run "jake" And every time I get the error

C:/bbndk/host_127_0_1_891/win32/x86/usr/bin/rm -f C:/GitRepo/blackberry/BB10-WebWorks-Framework/ext/sensors/native/x86/so/libsensors.so
C:/bbndk/host_127_0_1_891/win32/x86/usr/bin/qcc -Vgcc_ntox86 -lang-c++ -shared -Wl,-hlibsensors.so.1 -Wl,-rpath,./app/native/plugins/jnext,-z,re
-L C:/bbndk/target_127_0_1_3072/qnx6/x86/lib -L C:/bbndk/target_127_0_1_3072/qnx6/x86/usr/lib -Wl,--rpath-link . -Wl,--rpath-link C:/bbndk/targ
C:\bbndk\host_127_0_1_891\win32\x86\usr\bin\ntox86-ld: cannot find -lsensor
cc: C:/bbndk/host_127_0_1_891/win32/x86/usr/bin/ntox86-ld caught signal 1
make[5]: *** [C:/GitRepo/blackberry/BB10-WebWorks-Framework/ext/sensors/native/x86/so/libsensors.so] Error 1


I wonder if you experienced the same error or not and how to fix it.

@rwmtse
@shamim1
shamim1 commented Dec 13, 2012

ok

@shamim1
shamim1 commented Dec 14, 2012

wondering anybody checked last check-in. I made some change to handle the error call back on the .cpp side.

I am working on creating a client webworks app. I am running in different problem now (my device is not getting detected by the computer). But, I will let you know the result once I am able to run the sample app on the device.

@rwmtse
rwmtse commented Dec 14, 2012

@shamim1 I could build your branch and here are the lint errors:

> jake lint
path.existsSync is now called `fs.existsSync`.
EXECUTING jshint . --reporter build/lint/reporter.js --show-non-errors
path.existsSync is now called `fs.existsSync`.
ext/Payment/index.js :
    Unused Variables:
        info(91), 

test/unit/ext/ui.cover/client.js :
    Unused Variables:
        coverSize(59),
EXECUTING python /Users/rtse/bb10/BB10-WebWorks-Framework/build/../dependencies/cpplint/cpplint.py --R --filter=-whitespace/line_length,-whitespace/comments,-whitespace/labels,-whitespace/braces,-readability/streams ext/app ext/bbm.platform ext/card ext/connection ext/event ext/identity ext/invoke ext/invoked ext/io ext/io.filetransfer ext/notification ext/Payment ext/pim.calendar ext/pim.contacts ext/push ext/sensors ext/system ext/ui.contextmenu ext/ui.cover ext/ui.dialog ext/ui.toast ext/utils
ext/Payment/native/payment_bps.cpp:20:  Found C system header after C++ system header. Should be: payment_bps.h, c system, c++ system, other.  [build/include_order] [4]
ext/Payment/native/payment_bps.cpp:31:  Missing space before ( in if(  [whitespace/parens] [5]
ext/Payment/native/payment_bps.hpp:40:  Add #include <string> for string  [build/include_what_you_use] [4]
@shamim1
shamim1 commented Dec 14, 2012

ext/Payment/native/payment_bps.hpp:40: Add #include for string [build/include_what_you_use] [4]

Line 40 says
std::string eventNameToCallback;

I don't understand why I need to #include for std::string

ext/Payment/native/payment_bps.cpp:20: Found C system header after C++ system header. Should be: payment_bps.h, c system, c++ system, other. [build/include_order] [4]

I couldn't figure out how it fix it. I need help on that. I changed order of include (stdio.h seems the only C header).


Other then that can you please review the code change as well.

Thanks

@rwmtse
rwmtse commented Dec 14, 2012

Any header that ends with .h is a C header, so this is a C header:
#include <json/writer.h>

Also, can I ask that you don't use --amend every time? If you use git commit --amend, I cannot tell the code changes you add for each commit, making review incredibly difficult.

@shamim1
shamim1 commented Dec 14, 2012

Ok, if you want I can "commit" instead of "amend". I just fear that it will create a new code review. But, if it does not, I can surely "commit". Very sorry to make your task harder.

Thanks for explaining C header. I am going home now. But, I really appreciate your help. Last few days I learned quite a lot from you.

@rwmtse
rwmtse commented Dec 14, 2012

No problem. "commit" will not create a new pull request. Eventually when the review is completed and there is no more changes required, we'd ask you to squash all commits into one prior to us merging it to next. But that would be the very last step. For now, you can keep every commit separate, this way we can tell the change you made each time.

@jeffheifetz

I know this is a nitpick, but can you re-style your files (or at least your JS). We use a 4 space tab and not 2.

@shamim1
shamim1 commented Dec 17, 2012

I am stuck with “jake deploy-tests”

The error I am getting is –

Calling jake package['../target/zip/','test/test-app/wwTest.zip','-d']
jake aborted.


One note is that I am using windows.

@rwmtse
rwmtse commented Dec 17, 2012

@shamim1 can you paste the full error?

If this is blocking you, I don't think you need to do jake deploy-tests at this point. You can package and deploy your own app without worrying about our test app.

@shamim1
shamim1 commented Dec 17, 2012

What would be the command? would it be something like that

#192

@jeffheifetz

@shamim1 You have failed to provide the path to a packager. Please run jake -T for full instructions

@shamim1
shamim1 commented Dec 17, 2012

Ok, I am trying to "pagkage" against my test app fist and "deploy" the test app.

While packaging I get this error

C:\GitRepo\blackberry-webworks\BB10-WebWorks-Framework>jake package[C:\GitRepo\blackberry-webworks\BB10-WebWorks-Framework,C:_Payment\Webworks\myPaymentEvent\myPaymentEvent.zip,-d]
node.js:201
throw e; // process.nextTick error, or 'error' event on first tick
^
Error: Cannot find module 'C:\GitRepo\blackberry-webworks\BB10-WebWorks-Framework\lib\bbwp.js'
at Function._resolveFilename (module.js:332:11)
at Function._load (module.js:279:25)
at Array.0 (module.js:479:10)
at EventEmitter._tickCallback (node.js:192:40)


The deploy code works fine -
jake deploy[C:_Payment\Webworks\myPaymentEvent\device\myPaymentEvent.bar,169.254.0.1,qaqa]

But, most probably "blackberry.payment.purchase" is undefined and I get that error since the package didn't work.

@jeffheifetz

If you run jake -T, you'll see that the jake package command takes the path to a paclager (repo or installed version) and NOT a framework repo.

@rwmtse
rwmtse commented Dec 17, 2012

@shamim1 Please clone this packager repo:
https://github.com/blackberry-webworks/BB10-WebWorks-Packager/

Take a look at README on that page too, "Building an application" in particular

@shamim1
shamim1 commented Dec 18, 2012

First of all thanks "rwmtse" for unblocking me.

I am doing some testing with different logging. What I have now is - I get
"Error User agent not set" when calling
var val = JNEXT.invoke(self.m_id, "purchase " + JSON.stringify(args))

I am assuming there is somewhere I need to set that value. Wondering where ?

@shamim1
shamim1 commented Dec 18, 2012

I had no luck with the new packager. I also enabled “web inspector” in the browser trying to fix it. That didn’t do anything either.
But, there are some error that I can point to (from slog2info) –
Dec 18 16:00:00.511 webkit_launcher.41996509 0 -----ONLINE-----
Dec 18 16:00:00.511 webkit_launcher.41996509 webkit* 0 JS Console: local:///chrome/index.html:5: Viewport target-densitydpi is not supported.
Dec 18 16:00:00.888 wpa_pps.3190852 wlan_mgr* 0 Process msg::rssi_update
Dec 18 16:00:00.888 wpa_pps.3190852 wlan_mgr 0 msg::rssi_update dat::off
Dec 18 16:00:01.217 PPS.1880096 log 3 pps: WARNING: msgpid 6316185: devmode: `?nopersist' option without write
Dec 18 16:00:01.415 webkit_launcher.41996509 webkit 0 Unable to get group: Operation not permitted (1).
Dec 18 16:00:01.974 webkit_launcher.41996509 webkit 0 JS Console: undefined:1: ReferenceError: Can't find variable: Created
Dec 18 16:00:01.975 webkit_launcher.41996509 webkit 0 JS Console: undefined:1: ReferenceError: Can't find variable: Created
Dec 18 16:00:02.604 input_service.5369982 default* 0 [info @ EventQueue.cpp:388 (tid:7-imf_screen_focus_change_listener)]: New
foreground process pid 41996509, pgid 41996509, uid 100, gid 901

@pagey
pagey commented Jan 2, 2013

for BPS you need to set the channel to be the same as the thread you want to listen to events on, i'm not sure what the payment API does internally but it's possible that your events are happening but not being received by the right thread. If it's a problem to change the payment API to work like that you can move your call to paymentservice_purchase_request_with_arguments to the same thread as your event code and it should work for you that way

@shamim1
shamim1 commented Jan 4, 2013

With single thread the purchase seems to work fine. The only thing remaining is, on BPS_FAILURE I want to throw an exception to the javaScript side and expect the client app will receive it on catch section. That is probably not happening as expected.

@pagey
pagey commented Jan 4, 2013

In order to do that you will need to setup on event with event.once() on the client side and use event.trigger() on the JNEXT javascript side of things when your event is called. This can get complicated and we don't normally use exceptions in our code. It is much easier to use onSuccess and onError callbacks for your function.

@shamim1
shamim1 commented Jan 4, 2013

ok, I will skip that for now. At this moment we are trying to have "LocalMode" variable changed by the client. I am not exactly sure how. We are calling
blackberry.payment.developmentMode = true;
from client app. and in client.js we have
Object.defineProperty(_self, "developmentMode", {"value": false});

We don't have getter and setting yet in client.js yet. Do we have to have setter in client.js to give the user ability to change the developmentMode value?

@pagey
pagey commented Jan 4, 2013

If you want to have a property in your class you need to use Object.defineProperty and include a getter and setter for the field. In your getter/setter you then need to call webworks.execSync to do something on the index side if you need to interact with your plugin

@nukulb
nukulb commented Jan 8, 2013

where is this now? @pagey and @shamim1 - please comment on the status

@pagey
pagey commented Jan 8, 2013

waiting for new code to review

@shamim1
shamim1 commented Jan 9, 2013

we are almost done with the API. There are few questions that need help. We are testing the output on different scenario using the client app.

First question: We were able to get and set Payment.developmentMode variable by using Object.defineProperty(). Now, how to assign a default value?

Second Question: we need to find a way for the user to set development mode only once (for the scope of each application). We are trying to find out how to efficiently do it.

@pagey
pagey commented Jan 9, 2013

If you want to assign a value to your property on runtime then you can just do it in the client.js, put a line that assigns the value and it will run when the extension is first loaded.

I'm not sure what you mean by set it only once? Do you want development mode to stay on when you exit and restart the application?

@shamim1
shamim1 commented Jan 9, 2013

I am calling

Object.defineProperty(_self, "developmentMode", {
get: function () {
console.log("developmentMode get called");
return getFieldValue("developmentMode");
},
set: function (value) {
try {
console.log("developmentMode set called");
window.webworks.execSync(_ID, "developmentMode", {"developmentMode": value});
} catch (e) {
console.error(e);
}
}
});

_self.developmentMode.set(false);

Wonder why "_self.developmentMode.set" is called undefined.

@pagey
pagey commented Jan 9, 2013

You defined a property, you don't call set/get directly. You call _self.developmentMode = false;

@shamim1
shamim1 commented Jan 9, 2013

Ok that worked. Looks like things are working relatively well. For some input I am not getting expected result. But, I can fix them and working on them now.

@shamim1
shamim1 commented Jan 9, 2013

One remaining question I have is, from the webworks side we call

try{
blackberry.payment.checkAppSubscription(checkAppSubscription_success, checkAppSubscription_failure);
}catch (e){
console.log(e);
}

Now, for BPS_FAILURE, it shouldn't go to the callbacks. It should rather go to the catch exception. From the client.js after all the value is populated and callback happens, I have something like

else if (isSuccessful === "BPS_FAILURE") {
//window.webworks.event.remove(_self._ID, "payment.purchase.callback", webworksPurchaseCallback);
throw JSON.stringify(result.errorObject.errorText);
}

I wonder if it is the right way to throw exception.

@pagey
pagey commented Jan 9, 2013

it doesn't make sense to provide an onError callback and then throw an exception, you should be handling everything through the onError callback, what exactly does BPS_FAILURE mean?

@shamim1
shamim1 commented Jan 9, 2013

BPS_FAILURE means that we called a Payment Service API. But, something wrong happened and we won't get any further event. Had it returned BPS_SUCCESS, the API call may still fail, but will return nice error with description.

Now, if it is OK, I would rather call the fail_Callback and pass the message saying something like "getExistingPurchases Failed. Payment Service Error.". User will see this message when they call "blackberry.payment.getExistingPurchases" from webworks side and BPS_FAILURE happens. We will just replace "getExistingPurchases" method name with other method name for other API call.

That way I don't have to throw as I already understood that we should not throw anything.

@shamim1
shamim1 commented Jan 9, 2013

Just making sure I understand, we have code in client.js

_self.purchase = function (purchase_arguments_t, successCallback, failCallback) {

if (!purchase_arguments_t || typeof purchase_arguments_t !== "object") {
    throw new Error("Purchase argument is not provided or is not a object.");
}

I am not sure where in client will the error be caught. Would it be in the "catch" section. Is the implementation right?

@pagey
pagey commented Jan 9, 2013

I'm saying don't use exceptions if you have an error call the error callback, thats where a developer is going to expect the errors to be, they aren't going to catch an exception and expect a callback to be called, it's too much work for them, all the errors should be going to one place to be handled

@shamim1
shamim1 commented Jan 9, 2013

ok

@shamim1
shamim1 commented Jan 10, 2013

We did bunch of testing yesterday. Things seem to be working fine from Payment Service point of view. Today, I will make sure that any error goes to error call back. And then start code review.

My initial code review will have logging, but once it is accepted by everybody, I will remove logging and fix all lint problem for the final code.

Sometime I have to squash all check-in or something. I am not totally familiar on that and will need assistance.

@shamim1
shamim1 commented Jan 10, 2013

The code is ready for code review.

@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/index.js
+ console.log("windowGroup parsed " + decodeURIComponent(window.qnx.webplatform.getController().windowGroup));
+ console.log("_webview developmentMode is " + _webview.getDevelopmentMode());
+ purchase_arguments_t = { "digitalGoodID" : JSON.parse(decodeURIComponent(args.digitalGoodID)),
+ "digitalGoodSKU" : JSON.parse(decodeURIComponent(args.digitalGoodSKU)),
+ "digitalGoodName" : JSON.parse(decodeURIComponent(args.digitalGoodName)),
+ "metaData" : JSON.parse(decodeURIComponent(args.metaData)),
+ "purchaseAppName" : JSON.parse(decodeURIComponent(args.purchaseAppName)),
+ "purchaseAppIcon" : JSON.parse(decodeURIComponent(args.purchaseAppIcon)),
+ "extraParameters" : JSON.parse(decodeURIComponent(args.extraParameters)),
+ "windowGroup" : decodeURIComponent(window.qnx.webplatform.getController().windowGroup),
+ "developmentMode" : JSON.parse(_webview.getDevelopmentMode()) };
+ } catch (e) {
+ console.log("purchase_arguments_t data parsing failed.");
+ }
+
+ try {
@pagey
pagey added a note Jan 10, 2013

unless you are trying to catch exceptions you are throwing yourself you don't need to put everything in try/catch blocks it slows things down and is not needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey and 1 other commented on an outdated diff Jan 10, 2013
ext/Payment/index.js
+ }
+
+ try {
+ console.log("called paymentJNext.purchase method.");
+ paymentJNext.getInstance().purchase(purchase_arguments_t);
+ success();
+ } catch (e) {
+ console.log("paymentJNext.purchase caught exception.");
+ fail(-1, e);
+ }
+ },
+ cancelSubscription: function (success, fail, args) {
+ var cancelSubscription_arguments_t;
+ console.log("cancelSubscription args: " + args);
+ try {
+ cancelSubscription_arguments_t = { "transactionID" : JSON.parse(decodeURIComponent(args.transactionID)),
@pagey
pagey added a note Jan 10, 2013

you don't need try/catch parsing should never fail because you are sending it yourself from the client side

@shamim1
shamim1 added a note Jan 10, 2013

ok.... I will remove all those try catch once you are happy with other things. I guess it should look good on how it is implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey and 1 other commented on an outdated diff Jan 10, 2013
ext/Payment/index.js
+ result.errorObject = errorObject;
+
+ return result;
+ };
+
+ self.bpsInilitize = function () {
+ JNEXT.invoke(self.m_id, "bpsInilitize");
+ };
+
+ self.purchase = function (args) {
+ console.log("Called JNEXT.invoke with args: " + args);
+ var val = JNEXT.invoke(self.m_id, "purchase " + JSON.stringify(args)), result = {};
+ console.log("Passed JNEXT.invoke method with value: " + val);
+ if (val === "-1") {
+ result = self.getErrorObject("BPS_FAILURE", "-1", "Purchase Failed. Payment Service Error.");
+ _event.trigger("payment.purchase.callback", result);
@pagey
pagey added a note Jan 10, 2013

this doesn't seem to be an async function you don't need to use event.trigger here, you can just return the value right away

@shamim1
shamim1 added a note Jan 10, 2013

We are calling
window.webworks.event.once(_ID, "payment.purchase.callback", webworksPurchaseCallback);
from client.js. So instead of "cancelling the event listening and returning to the user" I just used the same callback method.

Besides BPS_FAILURE is very rare case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shamim1
shamim1 commented Jan 10, 2013

ok.... I will remove all those try catch once you are happy with other things. I guess it should look good on how it is implemented.

@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_bps.cpp
+ unsigned int requestID;
+ return paymentservice_get_price(someSKU, someSKU, group_id, &requestID);
+}
+
+int PaymentBPS::CheckExisting(const char *sku, const char* group_id) {
+ unsigned int requestID;
+ return paymentservice_check_existing(sku, sku, group_id, &requestID);
+}
+
+int PaymentBPS::WaitForEvents()
+{
+ fprintf(stderr, "%s", "WaitForEvents starts after BPS_SUCCESS.\n");
+ for (;;) {
+ bps_event_t *event = NULL;
+ bps_get_event(&event, -1); // Blocking
+ if (event) {
@pagey
pagey added a note Jan 10, 2013

this code is not indented properly, you need to run jake lint on it to see the cpplint results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_bps.cpp
+ } else {
+ return Json::Value(value);
+ }
+ } else {
+ return null_value;
+ }
+}
+
+int PaymentBPS::onPurchaseSuccess(bps_event_t *event)
+{
+ if (event == NULL) {
+ fprintf(stderr, "Invalid event.\n");
+ return 0;
+ }
+
+ try{
@pagey
pagey added a note Jan 10, 2013

do not use exceptions in native code, they will not be supported soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_bps.cpp
+ } catch (...) {
+ fprintf(stderr, "Caught Unrecognized exception \n");
+ throw;
+ }
+
+ return 1;
+}
+
+int PaymentBPS::onGetExistingPurchasesSuccess(bps_event_t *event)
+{
+ if (event == NULL) {
+ fprintf(stderr, "Invalid event.\n");
+ return 0;
+ }
+
+ try{
@pagey
pagey added a note Jan 10, 2013

no exceptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_bps.hpp
+
+namespace webworks {
+
+class PaymentBPS {
+public:
+ explicit PaymentBPS(Payment *parent = NULL);
+ ~PaymentBPS();
+ int InitializeEvents();
+ int WaitForEvents();
+ //Method call from JavaScript side
+ BPS_API int Purchase(purchase_arguments_t *purchase_arguments);
+ BPS_API int getExistingPurchases(bool allow_refresh, const char* group_id, unsigned* request_id);
+ int getPrice(const char* someSKU, const char* group_id);
+ int CheckExisting(const char *sku, const char *windowGroup);
+ int CancelSubscription(const char *someSKU, const char *windowGroup);
+ std::string result_str;
@pagey
pagey added a note Jan 10, 2013

this should be private

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_js.cpp
+ bool parse = reader.parse(jsonObject, obj);
+ if (!parse) {
+ fprintf(stderr, "%s", "error parsing\n");
+ return "Cannot parse JSON object";
+ }
+ }
+
+ if (strCommand == "purchase") {
+ fprintf(stderr, "%s", "Native purchase method called\n");
+ bool developmentMode = obj["developmentMode"].asBool();
+ fprintf(stderr, "Json.Value developmentMode %s \n", developmentMode?"true":"false");
+ fprintf(stderr, "Json.Value %s \n", obj["windowGroup"].asCString());
+
+ purchase_arguments_t* args = NULL;
+ std::stringstream ss;
+ paymentservice_purchase_arguments_create(&args);
@pagey
pagey added a note Jan 10, 2013

your jnext class implementation should contain no extension logic aside from parsing variables and calling your bps class. we are moving away from jnext and leaving all this code here will make it difficult to transition the code, please put all your extension code inside your bps class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey and 3 others commented on an outdated diff Jan 10, 2013
lib/webview.js
@@ -171,6 +171,14 @@ webview =
return _webviewObj.setFileSystemSandbox;
},
+ setDevelopmentMode: function (developmentMode) {
@pagey
pagey added a note Jan 10, 2013

these should be called something different than setDevelopmentMode as it has nothing to do with the actual webworks development mode and only related to payment, what were you trying to do here? I don't think this file should be modified by any extensions

@shamim1
shamim1 added a note Jan 10, 2013

We needed getter and setter of our (payment service) DevelopmentMode property. I will change the name to paymentServiceDevelopmentMode.

@pagey
pagey added a note Jan 10, 2013

I still don't think it belongs here, why does it need to be part of the webview API?

@rwmtse
rwmtse added a note Jan 10, 2013

I think @shamim1 follows blackberry.io sandbox getter/setter which needs to set something in webview. But that totally DOES NOT apply for this extension!!!!!

@shamim1
shamim1 added a note Jan 10, 2013

We then need a way to have getter and setter implemented then. We don't know how.

@pagey
pagey added a note Jan 10, 2013

why do you need it? what are you using it for?

@shamim1
shamim1 added a note Jan 10, 2013

Client will call DevelopmentMode before calling any PaymentAPI from webworks. The subsequent call need to remember what client did set. Now since payment_js has same life cycle as the app, we can set DevelopmentMode in payment_js.

Is it ok to do the new way then?

@scdowney
scdowney added a note Jan 10, 2013

Just to clarify, the payment sdk's support a "local mode" in which the payment library (in this case bps paymentservice) does not actually contact the paymentsystem (and subsequently the server), for development testing purposes. The default value is false, but the ability needs to be exposed to set development mode to true. We simply need to have a way to tie the setting of the developmentMode property to making a paymentservice_set_connection_mode() call in bps.

It sounds to me like setting it through payment_js should work fine.

@rwmtse
rwmtse added a note Jan 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey and 1 other commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_js.cpp
+ return ss.str();
+ } else {
+ fprintf(stderr, "checkExisting:: paymentResponse is 0.");
+ int return_value = payment->WaitForEvents();// this would be blocking
+ if(!developmentMode) {
+ paymentservice_stop_events(0);
+ }
+ if (return_value == 1) {
+ ss.str("");
+ ss << payment->result_str;
+ delete payment;
+ return ss.str();
+ }
+ }
+ }
+ else if (strCommand == "bpsInilitize") {
@pagey
pagey added a note Jan 10, 2013

this should be in your constructor, not a separate call

@shamim1
shamim1 added a note Jan 10, 2013

We are calling bpsInilitize only once (in client.js we call _self.bpsInilitize())

My feeling is payment_js gets initialized and dies for every single webworks API call.

@pagey
pagey added a note Jan 10, 2013

no it stays active until your application exists

@shamim1
shamim1 added a note Jan 10, 2013

Ok, then we can call bps_initialize() in the constructor and call bps_shutdown() in the destructor.

In between every call don't need to care about bps_initialize or bps_shutdown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rwmtse rwmtse commented on an outdated diff Jan 10, 2013
ext/Payment/index.js
+ paymentJNext.getInstance().checkExisting(check_existing_args);
+ success();
+ } catch (e) {
+ console.log("paymentJNext.checkExisting caught exception.");
+ fail(-1, e);
+ }
+ },
+ developmentMode: function (success, fail, args, env) {
+ var value;
+ console.log("developmentMode args: " + args);
+ _webview = _util.requireWebview();
+
+ if (args && args["developmentMode"]) {
+ console.log("developmentMode args is valid");
+ value = JSON.parse(decodeURIComponent(args["developmentMode"]));
+ _webview.setDevelopmentMode(JSON.parse(value));
@rwmtse
rwmtse added a note Jan 10, 2013

You should really be saving the state of the development mode (or local mode, whatever you call it) somewhere, maybe as a static variable in the native class? It has absolutely nothing to do with webview.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shamim1
shamim1 commented Jan 10, 2013

Checked in new changes based on code review. Also removed logging and exception handling. This should be close to actual product.

@pagey
pagey commented Jan 10, 2013

your functions in client.js are all sync but you are using event.once() which is used for async callbacks, this doesn't seem to make sense, if your function is sync you should return the result or call the callback that was passed in immediately

@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_js.cpp
+ delete payment;
+ return ss.str();
+ }
+ else {
+ int return_value = payment->WaitForEvents(developmentMode);// this would be blocking
+ //all the callback returns 1
+ if (return_value == 1) {
+ ss.str("");
+ ss << payment->getResult_str();
+ delete payment;
+ return ss.str();
+ }
+ }
+ } else if (strCommand == "getExistingPurchases") {
+ std::stringstream ss;
+ webworks::PaymentBPS *payment = new webworks::PaymentBPS();
@pagey
pagey added a note Jan 10, 2013

is there a reason why this class needs to be created every time you call a function? can you make it work with a single instance that is shared by all the functions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_js.cpp
+ */
+
+#include <json/reader.h>
+#include <stdio.h>
+#include <webworks_utils.hpp>
+#include <string>
+#include <vector>
+#include "payment_js.hpp"
+#include "payment_bps.hpp"
+
+bool eventsInitialized = false;
+
+Payment::Payment(const std::string& id) : m_id(id)
+{
+ developmentMode = false;
+ bps_initialize();
@pagey
pagey added a note Jan 10, 2013

you should be calling this inside payment_bps.cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 10, 2013
lib/webview.js
@@ -170,7 +170,7 @@ webview =
getSandbox: function () {
return _webviewObj.setFileSystemSandbox;
},
-
+
@pagey
pagey added a note Jan 10, 2013

can you fix this file so no changes are being made to it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_js.cpp
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <json/reader.h>
+#include <stdio.h>
+#include <webworks_utils.hpp>
+#include <string>
+#include <vector>
+#include "payment_js.hpp"
+#include "payment_bps.hpp"
+
+bool eventsInitialized = false;
@pagey
pagey added a note Jan 10, 2013

this variable is not used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_js.hpp
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef PAYMENT_JS_H_
+#define PAYMENT_JS_H_
+
+#include <plugin.h>
+#include <sstream>
+#include <string>
+
+void* PaymentEventThread(void *args);
@pagey
pagey added a note Jan 10, 2013

this function is not used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 10, 2013
ext/Payment/native/payment_bps.cpp
+ extraParameter[key_Str] =Json::Value(value);
+ }
+ purchasedItem["extraParameters"]= extraParameter;
+ }
+
+ result["successState"] = successState;
+ result["purchasedItem"] = purchasedItem;
+ ss << Json::FastWriter().write(result);
+ result_str = ss.str();
+
+ return 1;
+}
+
+int PaymentBPS::onGetExistingPurchasesSuccess(bps_event_t *event)
+{
+ if (event == NULL) {
@pagey
pagey added a note Jan 10, 2013

you have some weird indentation here, this needs to be fixed, all lines should have 4 spaces of indentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scdowney

FYI, I still need to get the test app packaged to make sure these changes didn't break anything. I just wanted to get the updates in while I work on that, to keep the ball rolling.

@scdowney

I'm trying to get a test app re-packaged to test these changes, and I'm having some issues (previously only Ash was 100% committed to working on this, so I hadn't taken the time to get my environment fully working like he had, and he's on vacation today).

I'm on windows, and when I run jake package from my Framework directory, pointed at the Packager directory I cloned from blackberry-webworks (on next branch I believe), I get:

module.js:340
throw err;
^
Error: Cannot find module 'wrench'
at Function.Module._resolveFilename (module.js:338:15)
at Function.Module._load (module.js:280:25)
at Module.require (module.js:362:17)
at require (module.js:378:17)
at Object. (c:\webworks\BB10-Webworks-Packager\third_party\wrench\wrench.js:21:14)
at Module._compile (module.js:449:26)
at Object.Module._extensions..js (module.js:467:10)
at Module.load (module.js:356:32)
at Function.Module._load (module.js:312:12)
at Module.require (module.js:362:17)

When I first ran jake from Frameworks dir, I got the same issue with wrench, so I just did an npm install and it worked. Any ideas why it can't find it from the Packager directory? I even tried doing another npm install for wrench from the Packager directory, to no avail. I have also re-opened any cmd prompts, to make sure any env changes got picked up.

Any ideas?

@scdowney

Nevermind, I played around with npm a bit and was able to figure it out.

@jeffheifetz jeffheifetz and 1 other commented on an outdated diff Jan 14, 2013
ext/Payment/index.js
+ _methods = ["purchase", "cancelSubscription", "getExistingPurchases", "getPrice", "checkExisting"],
+ _event = require("../../lib/event"),
+ _exports = {},
+ _util = require("./../../lib/utils");
+
+module.exports = {
+ purchase: function (success, fail, args) {
+ var purchase_arguments_t;
+ purchase_arguments_t = { "digitalGoodID" : JSON.parse(decodeURIComponent(args.digitalGoodID)),
+ "digitalGoodSKU" : JSON.parse(decodeURIComponent(args.digitalGoodSKU)),
+ "digitalGoodName" : JSON.parse(decodeURIComponent(args.digitalGoodName)),
+ "metaData" : JSON.parse(decodeURIComponent(args.metaData)),
+ "purchaseAppName" : JSON.parse(decodeURIComponent(args.purchaseAppName)),
+ "purchaseAppIcon" : JSON.parse(decodeURIComponent(args.purchaseAppIcon)),
+ "extraParameters" : JSON.parse(decodeURIComponent(args.extraParameters)),
+ "windowGroup" : decodeURIComponent(window.qnx.webplatform.getController().windowGroup) };
@jeffheifetz
jeffheifetz added a note Jan 14, 2013

This does not need to be decoded

@shamim1
shamim1 added a note Jan 14, 2013

do we have to remove decodeURIComponent from every line in index.js? Since we are always passing a object
[
var args = { VALUE }
]

We probably don't need to decodeURIComponent.

@jeffheifetz
jeffheifetz added a note Jan 18, 2013

DecodeURIComponent is necessary for any data coming across from the client file, but is not for anything else. The only unnecessary call I see is for windowGroup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeffheifetz jeffheifetz and 1 other commented on an outdated diff Jan 14, 2013
ext/Payment/index.js
+ _util = require("./../../lib/utils");
+
+module.exports = {
+ purchase: function (success, fail, args) {
+ var purchase_arguments_t;
+ purchase_arguments_t = { "digitalGoodID" : JSON.parse(decodeURIComponent(args.digitalGoodID)),
+ "digitalGoodSKU" : JSON.parse(decodeURIComponent(args.digitalGoodSKU)),
+ "digitalGoodName" : JSON.parse(decodeURIComponent(args.digitalGoodName)),
+ "metaData" : JSON.parse(decodeURIComponent(args.metaData)),
+ "purchaseAppName" : JSON.parse(decodeURIComponent(args.purchaseAppName)),
+ "purchaseAppIcon" : JSON.parse(decodeURIComponent(args.purchaseAppIcon)),
+ "extraParameters" : JSON.parse(decodeURIComponent(args.extraParameters)),
+ "windowGroup" : decodeURIComponent(window.qnx.webplatform.getController().windowGroup) };
+
+ try {
+ paymentJNext.getInstance().purchase(purchase_arguments_t);
@jeffheifetz
jeffheifetz added a note Jan 14, 2013

You have not done any argument validation on your arguments. In the client file you have simply passed through anything the user sent in. If they send in something invalid (say numbers where a string is expected) do you want to handle this before making the call?

@shamim1
shamim1 added a note Jan 14, 2013

We probably can change the code in client.js with

if (!purchase_arguments_t || typeof purchase_arguments_t !== "object") {
    throw new Error("Purchase argument is not provided or is not a object.");
}
else
{
    if (purchase_arguments_t.digitalGoodID)
    {
        if (typeof purchase_arguments_t.digitalGoodID !== "string") {
            throw new Error("Purchase argument digitalGoodID is not provided or is not a string.");
        }
    }
    // all string validation
    if (purchase_arguments_t.extraParameters)
    {
       if (typeof purchase_arguments_t.digitalGoodID !== "object") {
       throw new Error("Purchase argument extraParameters is not provided or is not a object.");
    }
}

That probably will work for you.

@jeffheifetz
jeffheifetz added a note Jan 15, 2013

You only have to add type checking if the underlying implementation cares, if it doesn't you can disregard this.

You should probably actually end up calling on the onFail rather than throw exceptions.

Additionally you should be doing type checking in the index.js because developers may access your API using URI.

@jeffheifetz
jeffheifetz added a note Jan 18, 2013

Just re-iterating my understanding of your comments below that the underlying implementation will be fine with incorrect data or will throw an easy to understand error message through the onError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeffheifetz jeffheifetz and 1 other commented on an outdated diff Jan 14, 2013
ext/Payment/client.js
+_self.purchase = function (purchase_arguments_t, successCallback, failCallback) {
+
+ if (!purchase_arguments_t || typeof purchase_arguments_t !== "object") {
+ throw new Error("Purchase argument is not provided or is not a object.");
+ }
+ var args = { "digitalGoodID" : purchase_arguments_t.digitalGoodID || "",
+ "digitalGoodSKU" : purchase_arguments_t.digitalGoodSKU || "",
+ "digitalGoodName" : purchase_arguments_t.digitalGoodName || "",
+ "metaData" : purchase_arguments_t.metaData || "",
+ "purchaseAppName" : purchase_arguments_t.purchaseAppName || "",
+ "purchaseAppIcon" : purchase_arguments_t.purchaseAppIcon || "",
+ "extraParameters" : purchase_arguments_t.extraParameters || "" };
+
+ onPurchaseSuccess = successCallback;
+ onPurchaseFail = failCallback;
+ window.webworks.event.once(_ID, "payment.purchase.callback", webworksPurchaseCallback);
@jeffheifetz
jeffheifetz added a note Jan 14, 2013

Currently your callback function is invoked asynchronously but only supports a single set of callbacks (onPurchaseSuccess and onPurchaseFail). I believe there may be an issue if someone calls purchase twice in a row since you will overwrite the callbacks from the first invocation with the second.

@jeffheifetz
jeffheifetz added a note Jan 14, 2013

Nvm, this can't happen atm since everything actually happens synchronously. However this issue exists for any underlying native that is not synchronous.

@shamim1
shamim1 added a note Jan 14, 2013

Ok...

From client.js we call
...

return window.webworks.execSync(_ID, "purchase", args);

And that calls (in index.js)

purchase: function (success, fail, args) {
...

So, I guess technically we can pass onPurchaseSuccess and onPurchaseFail as argument to purchase function in index.js. The new method signature will be -

purchase: function (success, fail, args, onPurchaseSuccess, onPurchaseFail ) {

and the function will return
success(onPurchaseSuccess(result));

[I am not sure if I have to say success(return onPurchaseSuccess(result));]

That should make things work synchronously and we all can be happy. I am up for improving the code as long as there is some defined way. I just didn't see callback method passed like that in the API codes that I was following.

@jeffheifetz
jeffheifetz added a note Jan 15, 2013

Probably the easier solution is to make the callbacks all local to the scope of the function and that way not be limited to only storing one.

Again this is not truly necessary for APIs that are actually synchronous, but if there are ones that are asynchronous it is necessary

@jeffheifetz
jeffheifetz added a note Jan 18, 2013

I see that none of your APIs are asynchronous, so technically this is not necessary, however I still feel like it is a better implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shamim1
shamim1 commented Jan 15, 2013

I did not get a comment on the fix that I described. If that sounds fine, I will fix the code and submit for review.

@jeffheifetz

@shamim1 I think the confusing part of all this to me is that you have an API designed as asynchronous (using onSuccess and onFail) when the underlying API is actually synchronous. Is there a compelling reason to do this?

@shamim1
shamim1 commented Jan 15, 2013

There was some problem with multiple thread and payment dialogue. We initially put the event listening code in separate thread. But, we ran into problem. So, we needed to implement is synchronously.

@pagey
pagey commented Jan 17, 2013

what is the status on this?

@shamim1
shamim1 commented Jan 18, 2013

I think we can finish code review. I explained why we didn't make the native call asynchronous. For the input validation, we are passing default value from client.js for most of the fields. So, if a parameter which is required isn't passed on (or the empty default value isn't accepted) then we should expect failure callback.

The only remaining this is "decodeURIComponent". That from my knowledge can stay for first version as it doesn't really do any harm.

Please let me know if my understanding is wrong.

@pagey
pagey commented Jan 18, 2013

needs unit tests, also Payment directory name should be all lowercase

@shamim1
shamim1 commented Jan 21, 2013

folder rename is done.

@pagey
pagey commented Jan 21, 2013

are you working on unit tests? we can't accept this extension without them

@scdowney

Is it possible to get the working code integrated, in the interest in of getting payments available to webworks developers ASAP, but with unit tests being added in the (near) future? I think the concern was just regarding the learning curve for the unit test framework being used (ash mentioned being unfamiliar with it, I honestly haven't even had a chance to investigate) vs. the desire to have payments available to developers as soon as possible.

@shamim1
shamim1 commented Jan 21, 2013

Yes, right now the problem is time. I have other tasks in my plate now and this task isn't even part of my sprint task yet. It probably won't take too long. But, looking into how others done and trying to come up with "unit test" code probably will take a bit of time and there will be few back and forth.

I think, depending how urgent it is and when it need to be done, we can decide the next step. If I can manage time, surely I wouldn't mind doing it.

@rwmtse
rwmtse commented Jan 22, 2013

@pagey @jeffheifetz I've added JS unit tests and a test app (provided by payment team). Also did a bit of clean-up. Lint, unit tests and the test app seems to all run fine. Please review again.

@pagey pagey and 2 others commented on an outdated diff Jan 22, 2013
ext/payment/index.js
+ };
+
+ self.purchase = function (args) {
+ var val = JNEXT.invoke(self.m_id, "purchase " + JSON.stringify(args)),
+ result = {};
+
+ if (val === "-1") {
+ result = self.getErrorObject("BPS_FAILURE", "-1", "Purchase Failed. Payment Service Error.");
+ _event.trigger("payment.purchase.callback", result);
+ } else {
+ console.log("BPS_SUCCESS.");
+ _event.trigger("payment.purchase.callback", JSON.parse(val));
+ }
+ };
+
+ self.getExistingPurchases = function (args) {
@pagey
pagey added a note Jan 22, 2013

I still think this is really weird to have async callbacks for a sync function

@rwmtse
rwmtse added a note Jan 22, 2013

I thought this has been raised before? It has to be sync for a reason right?

@pagey
pagey added a note Jan 22, 2013

I mean it's calling JNEXT and getting a value back right away, why is it using event.trigger?

@scdowney
scdowney added a note Jan 22, 2013

I wasn't intimate with the first go of designing this, but is my interpretation correct (from what I remember when doing little code edits here and there), that JNEXT.invoke() appeared to block, at which point the user would interact with the various dialogs (if necessary for the particular call), and only once that bps event cycle was completed would JNEXT.invoke return val, and this call would continue?

Given that the calling app will essentially be blocked while the dialogs are on screen, is there a need for async? again, just trying to catch up on the design...i believe the overarching concern was under which circumstances the calling app would get blocked etc. There are a few paymentservice calls, for example, that would potentially take some time (such as going to the server to get purchase history) but require no dialogs, so the calling app could potentially continue while waiting for the callback, ideally. I am not sure of all the implications but I believe this was the initial reasoning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 22, 2013
ext/payment/native/payment_bps.cpp
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <stdio.h>
+#include <string>
+#include <vector>
+#include <sstream>
+//#include <json/writer.h>
@pagey
pagey added a note Jan 22, 2013

remove commented line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey
pagey commented Jan 22, 2013

docs?

@rwmtse
rwmtse commented Jan 22, 2013

@pagey In progress, will start another pull request when ready

@scdowney

@rwmtse, did Ash point you to the next-BB10-payments branch in WebWorks-API-Docs? I had the docs basically complete there, but the initial pull request was canceled a while back because the implementation was a little way off

@rwmtse
rwmtse commented Jan 22, 2013

@scdowney I see. Ash sent me a docs JS file. Didn't know there's a branch. I'd check that out.

@scdowney

@rwmtse The only change that I THINK needs to be made is, as the last commit, we need to remove getChildSkus and getPPID, as they are not implemented yet on the back-end, so we cannot include them in this release. Nukul suggested we do it this way, so that we can just re-add them in one go, just by re-adding that SHA.

@pagey pagey commented on an outdated diff Jan 22, 2013
ext/payment/native/payment_bps.cpp
+ if (!developmentMode) {
+ paymentservice_stop_events(0);
+ }
+ return onFailureCommon(event);
+ }
+ }
+ }
+ }
+
+ return 0;
+}
+
+Json::Value PaymentBPS::getJsonValue(BPS_API const char* value)
+{
+ Json::Value null_value = "NOT SPECIFIED";
+ if (value){
@pagey
pagey added a note Jan 22, 2013

space between ) and {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 22, 2013
ext/payment/native/payment_bps.cpp
+ if (event == NULL) {
+ return 0;
+ }
+
+ std::stringstream ss;
+ Json::Value result;
+ Json::Value dataItem;
+ Json::Value successState;
+ successState["state"] = "SUCCESS";
+
+ dataItem["price"] = Json::Value(getJsonValue(paymentservice_event_get_price(event)));
+ const char* initialPeriod = paymentservice_event_get_initial_period(event);
+ const char* renewalPrice = paymentservice_event_get_renewal_price(event);
+ const char* renewalPeriod = paymentservice_event_get_renewal_period(event);
+
+ if (initialPeriod){
@pagey
pagey added a note Jan 22, 2013

space between ) and {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey and 1 other commented on an outdated diff Jan 22, 2013
ext/payment/native/payment_js.cpp
+
+ // Parse JSON object
+ Json::Value obj;
+
+ if (command.length() > index) {
+ std::string jsonObject = command.substr(index + 1, command.length());
+
+ bool parse = Json::Reader().parse(jsonObject, obj);
+ if (!parse) {
+ return "Cannot parse JSON object";
+ }
+ }
+
+ if (strCommand == "purchase") {
+ std::stringstream ss;
+ payment->InitializeEvents();
@pagey
pagey added a note Jan 22, 2013

i'm a little confused as to why the events are always being initialized with every command, they should ideally only be once, also where are they shut down?

@scdowney
scdowney added a note Jan 22, 2013

The initialization and stopping of events is what facilitates paymentsystem on the back end, which is bslaunched on the device, being able to shut down when no work is present. By requesting and stopping paymentservice events for each call, paymentsystem can go away if there is a long wait between calls from the calling webworks app. To answer the other part, the events get stopped after the call(s) are completed, in waitForEvents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 22, 2013
ext/payment/native/payment_js.cpp
+ //all the callback returns 1
+ if (return_value == 1) {
+ ss.str("");
+ ss << payment->getResult_str();
+ return ss.str();
+ }
+ }
+ } else if (strCommand == "getExistingPurchases") {
+ std::stringstream ss;
+ payment->InitializeEvents();
+ BPS_API int paymentResponse = payment->getExistingPurchases(obj, developmentMode);
+ ss << webworks::Utils::intToStr(paymentResponse);
+ if (paymentResponse == -1){
+ return ss.str();
+ } else {
+ int return_value = payment->WaitForEvents(developmentMode);// this would be blocking
@pagey
pagey added a note Jan 22, 2013

this block of code is repeated many times, can you rewrite it so be more modular?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 22, 2013
ext/payment/client.js
+ "metaData" : purchase_arguments_t.metaData || "",
+ "purchaseAppName" : purchase_arguments_t.purchaseAppName || "",
+ "purchaseAppIcon" : purchase_arguments_t.purchaseAppIcon || "",
+ "extraParameters" : purchase_arguments_t.extraParameters || ""
+ };
+
+ _onPurchaseSuccess = successCallback;
+ _onPurchaseFail = failCallback;
+ window.webworks.event.once(_ID, "payment.purchase.callback", webworksPurchaseCallback);
+
+ return window.webworks.execSync(_ID, "purchase", args);
+};
+
+_self.getExistingPurchases = function (refresh, successCallback, failCallback) {
+ if (typeof refresh !== "boolean") {
+ throw new Error("Refresh argument is not provided or is not a boolean value.");
@pagey
pagey added a note Jan 22, 2013

we shouldn't be throwing any errors if we have an error callback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rwmtse
rwmtse commented Jan 22, 2013

@pagey Please review again. Refactored to get rid of redundant code. Invoke error callback instead of throwing errors.

@rwmtse
rwmtse commented Jan 24, 2013

@pagey @jeffheifetz I've removed all event trigger code. And here's the docs pull request: blackberry/WebWorks-API-Docs#447

@rwmtse
rwmtse commented Jan 25, 2013

@pagey @jeffheifetz Please resume review. @nukulb is hoping to fold ASAP
@tracyli Can you please grab this CI build and test? You can find a test app in test-suite/Apps/Payment
http://wwci:8080/view/BB10-WebWorks-Framework/view/Jobs/job/BB10-WebWorks-Framework-next-payment-sdk/27/

@shamim1
shamim1 commented Jan 25, 2013

Our SV&V testing team uncovered a possible issue around purchase (while working on the extended test app) . We are going to investigate it today and find a solution. I grabbed the latest code from github. If any change are made, it will be around input validation.

@pagey pagey commented on an outdated diff Jan 25, 2013
ext/payment/native/payment_bps.cpp
+ }
+ purchases[current_purchase_index] =Json::Value(purchasedItem);
+ }
+ result["purchases"] = purchases;
+ } else {
+ result["purchases"] = null_arr_value;
+ }
+
+ result["successState"] = successState;
+ ss << Json::FastWriter().write(result);
+ result_str = ss.str();
+
+ return 1;
+}
+
+int PaymentBPS::onCancelSubscriptionSuccess(bps_event_t *event) {
@pagey
pagey added a note Jan 25, 2013

{ on next line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 25, 2013
ext/payment/native/payment_bps.cpp
+ std::stringstream ss;
+ Json::Value result;
+ Json::Value dataItem;
+ Json::Value successState;
+ successState["state"] = "SUCCESS";
+
+ dataItem["subscriptionCancelled"] = Json::Value(paymentservice_event_get_cancelled(event));
+ result["dataItem"] = dataItem;
+ result["successState"] = successState;
+
+ ss << Json::FastWriter().write(result);
+ result_str = ss.str();
+ return 1;
+}
+
+int PaymentBPS::onGetPriceSuccess(bps_event_t *event) {
@pagey
pagey added a note Jan 25, 2013

{ on next line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 25, 2013
ext/payment/native/payment_bps.cpp
+ } else {
+ //NOT A SUBSCRIPTION
+ }
+ } else {
+ //NOT A SUBSCRIPTION
+ }
+
+ result["dataItem"] = dataItem;
+ result["successState"] = successState;
+
+ ss << Json::FastWriter().write(result);
+ result_str = ss.str();
+ return 1;
+}
+
+int PaymentBPS::onCheckExistingSuccess(bps_event_t *event) {
@pagey
pagey added a note Jan 25, 2013

{ on next line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey pagey commented on an outdated diff Jan 25, 2013
ext/payment/native/payment_bps.hpp
+#include <bps/paymentservice.h>
+#include <json/writer.h>
+#include <string>
+
+class Payment;
+
+namespace webworks {
+
+class PaymentBPS {
+public:
+ explicit PaymentBPS();
+ ~PaymentBPS();
+ int InitializeEvents();
+ int WaitForEvents(bool developmentMode);
+ //Method call from JavaScript side
+ BPS_API int Purchase(Json::Value obj, bool developmentMode);
@pagey
pagey added a note Jan 25, 2013

parameters should be const

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pagey
pagey commented Jan 25, 2013

everything looks good after comments

@rwmtse
rwmtse commented Jan 25, 2013

@shamim1 Please keep us posted regarding the issues found by your SV&V. This change needs to go in soon.

@shamim1
shamim1 commented Jan 25, 2013

While running jake unfortunately I still get the error. I got proper NDK and ran “C:\bbndk\bbndk-env_10_0_9_2270.bat” to make sure the proper NDK is used.

But, I just wonder where is “existsSync” method. It is called in bundler.js:93:17. It could be npm error. I may have to reinstall “fs” module (I guess). Here is the error –

TypeError: Object # has no method 'existsSync'
at Object.bundle (C:\GitRepo\blackberry-webworks\BB10-WebWorks-Framework\build\build\bundler.js:93:17)
at C:\GitRepo\blackberry-webworks\BB10-WebWorks-Framework\build\build\pack.js:94:26
at Object.pass (C:\GitRepo\blackberry-webworks\BB10-WebWorks-Framework\node_modules\jWorkflow\lib\jWorkflow.js:41:52)
at C:\GitRepo\blackberry-webworks\BB10-WebWorks-Framework\build\build\build-native.js:39:23
at Object.pass (C:\GitRepo\blackberry-webworks\BB10-WebWorks-Framework\node_modules\jWorkflow\lib\jWorkflow.js:41:52)
at C:\GitRepo\blackberry-webworks\BB10-WebWorks-Framework\build\build\utils.js:101:27
at ChildProcess.exithandler (child_process.js:281:7)
at ChildProcess.emit (events.js:70:17)
at maybeExit (child_process.js:361:16)
at Process.onexit (child_process.js:397:5)

@rwmtse
rwmtse commented Jan 25, 2013

I suspect this might actually have to do with node. Which version of node are you running?

@shamim1
shamim1 commented Jan 25, 2013

6.10

@shamim1
shamim1 commented Jan 25, 2013

v0.6.10

@rwmtse
rwmtse commented Jan 25, 2013

@shamim1 I think we have upgraded to the 0.8.14, please upgrade to latest node

@shamim1
shamim1 commented Jan 25, 2013

O, ok.

@shamim1
shamim1 commented Jan 25, 2013

I am in the process of uninstalling the old node.js and installing the new one. I am following the post below (for my windows XP OS)

http://stackoverflow.com/questions/11177954/how-do-i-completely-uninstall-node-js-and-reinstall-from-beginning-mac-os-x

Unfortunately I have only 1 hour today. But, I will make sure that on Monday this is my first priority and will make sure that the specific defect is fixed before I go home on Monday. Hopefully that won't be too late for you.

@tracyli
tracyli commented Jan 28, 2013

tested in 10.0.10.225 with packager build http://wwci:8080/view/BB10-WebWorks-Packager/view/Jobs/job/BB10-Webworks-Packager-next/26/ and framework build http://wwci:8080/view/BB10-WebWorks-Framework/view/Jobs/job/BB10-WebWorks-Framework-next-payment-sdk/28/:
1. automated functional test got passed except for one known issue of pim.calendar
2.test app as suggested worked well
some questions:
1.there's no test suite for blackberry.payment in automatic test
2.all error seems returning the same error id: eg, 'User Cancelled' or "Payment System Busy" both returns 0
3.is it better for us to get special BBID and an account in Vendor Portal to do payment transaction with the server?

@rwmtse
rwmtse commented Jan 28, 2013

@tracyli There is no automated test because purchase would show a dialog, can't automate it w/o automation. As for the error value, @shamim1 can you confirm if the error id will always be 0 when we use developmentMode, the docs says so for one of the functions, if that's the case I'd add the note to all functions. Also, @shamim1 please update us about the findings from your team's SV&V ASAP.

@scdowney

There was an issue in previous bps libraries where error id was always 0 on development mode, but this should have been fixed a while back. Ash can investigate to see if this is still happening for some reason, or if there is just an issue in the webworks implementation.

@shamim1
shamim1 commented Jan 28, 2013

I fixed the problem that we found on Friday. It was a easy fix. :)

@scdowney

On further inspection, this is a known issue on release9 builds. The fix was only targeted for 10.1+.

As for the BBID question, we have a test environment which the devices can be configured to use, with test payment instruments etc, to facilitate this. See our wiki page (http://wikis.rim.net/display/bbaw/BB10+Payment+Service+Configuration+Guide)

Ultimately, our "proper" test app will be made available on eval and production servers, to allow the "full" testing of an app downloaded from AW, being used for WW purchases. This app is being completed by our SVV devs as we speak, as I understand.

@shamim1
shamim1 commented Jan 28, 2013

The time taken was to setup the environment and testing with few different variance.

@rwmtse
rwmtse commented Jan 28, 2013

@scdowney so can you confirm if this note in getExistingPurchases error callback param: "Note: The actual values may be different when developmentMode equals true. should be added to all functions? since the fix isn't in R9 builds yet?

@scdowney
@rwmtse
rwmtse commented Jan 28, 2013

I've fixed the docs to move the note from getExistingPurchases to purchase. As far as I understand, this is now good to go? Anyone who has concern please voice out ASAP.

@scdowney

@rwmtse has that change been pushed somewhere? I can't seem to find it...

I'd like to just take a scan over the "final" docs, since it sounds like this is wrapping up

@rwmtse
rwmtse commented Jan 28, 2013

@scdowney you can find the latest built docs here
The github pull request for the docs is here: blackberry/WebWorks-API-Docs#447

@rwmtse rwmtse Implemented blackberry.payment for BB10
Reviewed by: Eric Pearson <epearson@rim.com>, Jeffrey Heifetz <jheifetz@rim.com>
Tested by: Tracy Li <tli@rim.com>
b50ca0b
@jeffheifetz

Merged Manually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.