Merge lp://staging/~alecu/unity-lens-music/ubuntuone-webservices into lp://staging/unity-lens-music

56 + public unowned string sign_url2(string url, string? postargs,

Looking at the docs, it says that you need to free the return, so s/unowned//, plus the postargs should be "out string postargs" as it's also a return (out params are nullable by default).

118 + internal Soup.SessionAsync http_session;

"internal" is a special keyword used when building libraries in vala (internal symbols are callable by other parts of the library, but not by the consumers), pls use private/public as appropriate.

177 + public string nickname {
178 + get { return _nickname; }
179 + }
+ other instances

No need for the extra private variable, use "public string x { get; private set; }"

243 + credentials_management.find_credentials ();
244 + yield;

Wouldn't it be better if find_credentials() itself was async? Then you wouldn't need the signals and you could just return the credentials / throw an error in the async method.

275 + internal virtual async void fetch_account () throws PurchaseError

Nothing wrong here, nice async method ;)

285 + var result = (string) message.response_body.data;

278 + queue_signed_webservice_call ("GET", ACCOUNT_URI, (session, message) => {

You're using this multiple times, perhaps add a helper async method that takes method + uri and returns flattened Soup.MessageBody?

.data could be null, no? Or does the flatten() cause possible null to become ""?

576 + return 0;

You should return result of Test.run ()

852 + MainLoop mainloop = new MainLoop ();
853 + purchase_service.refetch_payment_info.begin((obj, res) => {
854 + mainloop.quit ();
855 + try {
856 + purchase_service.refetch_payment_info.end (res);
857 + } catch (PurchaseError e) {
858 + error ("Can't fetch payment info: %s", e.message);
859 + }
860 + });
861 + mainloop.run ();

I remember you mentioned on IRC that you weren't sure how to do async tests with vala, so yep, this is the way, usually we just have a wrapper for the mainloop.run() which also adds a timeout, so the test fails eventually if there's no response. Usually using "assert (run_with_timeout (mainloop));"

Thanks for your through review!

> 56 + public unowned string sign_url2(string url, string? postargs,
>
> Looking at the docs, it says that you need to free the return, so s/unowned//,
> plus the postargs should be "out string postargs" as it's also a return (out
> params are nullable by default).

Fixed as requested.

> 118 + internal Soup.SessionAsync http_session;
>
> "internal" is a special keyword used when building libraries in vala (internal
> symbols are callable by other parts of the library, but not by the consumers),
> pls use private/public as appropriate.

Fixed all unnecessary uses of "internal". A few remain, as needed in some tests.

> 177 + public string nickname {
> 178 + get { return _nickname; }
> 179 + }
> + other instances
>
> No need for the extra private variable, use "public string x { get; private
> set; }"

Fixed as requested.

> 243 + credentials_management.find_credentials ();
> 244 + yield;
>
> Wouldn't it be better if find_credentials() itself was async? Then you
> wouldn't need the signals and you could just return the credentials / throw an
> error in the async method.

find_credentials is defined in lp:ubuntu-sso-client, and used via dbus. We found some timeout problems in the python dbus bindings that were solved by using signals instead, so most of the client code in Ubuntu SSO Client and Ubuntu One already uses this pattern. I agree that it would simplify things, but too much code depends on this dbus API as is, and it's not easy to change just for this case.

> 278 + queue_signed_webservice_call ("GET", ACCOUNT_URI,
> (session, message) => {
>
> You're using this multiple times, perhaps add a helper async method that takes
> method + uri and returns flattened Soup.MessageBody?

As suggested, I created a helper async method for the repeated instances. There are still at least two places which did not follow the same pattern (one used different http object and error codes, the other was not async), so I kept the existing code for those.

> 285 + var result = (string) message.response_body.data;
>
> .data could be null, no? Or does the flatten() cause possible null to become
> ""?

I just checked, and if .data is null, flatten() does the right thing and turns it into an empty string.

> 576 + return 0;
>
> You should return result of Test.run ()

Fixed.

> 852 + MainLoop mainloop = new MainLoop ();
> 853 + purchase_service.refetch_payment_info.begin((obj, res) => {
> 854 + mainloop.quit ();
> 855 + try {
> 856 + purchase_service.refetch_payment_info.end (res);
> 857 + } catch (PurchaseError e) {
> 858 + error ("Can't fetch payment info: %s", e.message);
> 859 + }
> 860 + });
> 861 + mainloop.run ();
>
> I remember you mentioned on IRC that you weren't sure how to do async tests
> with vala, so yep, this is the way, usually we just have a wrapper for the
> mainloop.run() which also adds a timeout, so the test fails eventually if
> there's no response. Usuall...

Awesome, /me happy now :)