In order to compile the branch you need GFBGraph, which haven't a release already, but you can get it from the gnome git: https://git.gnome.org/browse/libgfbgraph

Now that gnome-online-miners is part of the OSTree images (see bug 706122), we need to add libgfbgraph to OSTree before we can merge wip/facebook. Otherwise we will break the build.

I filed bug 706472 against OSTree.

Thank you Álvaro for finishing the wip/facebook branch. A few questions:

- /* TODO: Check updated time to avoid updating the photo if has not been modified since our last run */
  How much work would it require to do this? It is not a blocker for merging, but would be nice to have.

- I see that you are using "facebook:photos:<id>" instead of "facebook:<id>". Is that because the "id" of a photo can collide with the something else? I am asking because we usually use "flickr:<id>", "owncloud:<id>"

Some minor comments:

- I do not think there is a need to check the value returned by gfbgraph_goa_authorizer_new with GFBGRAPH_IS_GOA_AUTHORIZER. It appears to be a classic case of g_object_new (GFBGRAPH_TYPE_GOA_AUTHORIZER, ...)

- I do not think we need to claim an extra reference on the authorizer after creating it. Where you following the example of the Flickr miner?

- I regenerated MINER_IDENTIFIER using uuidgen(1). From the look of it, I assumed that you had generated it using something else. Does not really matter, but I still did for the sake of consistency.

- I do not think there is a need to print the error using g_warning if it is fatal enough for us to fail the operation. eg., in query_facebook, we will fail the whole query if gfbgraph_user_get_me fails. There is no need to use a g_warning because the GomMiner base class will print out a warning anyway.

- You might be leaking the error in query_facebook, because you are passing it to account_miner_job_lookup_album in a loop but not doing anything if the account_miner_job_lookup_album failed. I am guessing that you want to continue looking up the albums even if one failed. This is where I think you should use a g_warning to print the error before continuing.

- Same in account_miner_job_lookup_album

- I do not think you need to check for NULL before using g_list_free_full.

I am planning to clean up the minor details myself, but I still thought that it might be useful to put my comments here for the sake of transparency.

Created attachment 263032 [details] [review]
src: add a Facebook miner based on GFBGraph

Created attachment 263033 [details] [review]
data: add a DBus service file for org.gnome.OnlineMiners.Facebook

(In reply to comment #4)
> Thank you Álvaro for finishing the wip/facebook branch. A few questions:
> 
> - /* TODO: Check updated time to avoid updating the photo if has not been
> modified since our last run */
>   How much work would it require to do this? It is not a blocker for merging,
> but would be nice to have.

Not too much, I hope to do it today or along this weekend.

> 
> - I see that you are using "facebook:photos:<id>" instead of "facebook:<id>".
> Is that because the "id" of a photo can collide with the something else? I am
> asking because we usually use "flickr:<id>", "owncloud:<id>"

No, the ID of a photo is an unique ID in Facebook Graph, all the IDs in FB Graph are unique.
No problem in remove the ":photos:".

> 
> Some minor comments:
> 
> - I do not think there is a need to check the value returned by
> gfbgraph_goa_authorizer_new with GFBGRAPH_IS_GOA_AUTHORIZER. It appears to be a
> classic case of g_object_new (GFBGRAPH_TYPE_GOA_AUTHORIZER, ...)

You are right.

> 
> - I do not think we need to claim an extra reference on the authorizer after
> creating it. Where you following the example of the Flickr miner?

Yes, a mistake.

> 
> - I regenerated MINER_IDENTIFIER using uuidgen(1). From the look of it, I
> assumed that you had generated it using something else. Does not really matter,
> but I still did for the sake of consistency.

I don't remember how I generated the miner identifier. No problem in generate it again.

> 
> - I do not think there is a need to print the error using g_warning if it is
> fatal enough for us to fail the operation. eg., in query_facebook, we will fail
> the whole query if gfbgraph_user_get_me fails. There is no need to use a
> g_warning because the GomMiner base class will print out a warning anyway.

Ok.

> 
> - You might be leaking the error in query_facebook, because you are passing it
> to account_miner_job_lookup_album in a loop but not doing anything if the
> account_miner_job_lookup_album failed. I am guessing that you want to continue
> looking up the albums even if one failed. This is where I think you should use
> a g_warning to print the error before continuing.
> 
> - Same in account_miner_job_lookup_album
> 
> - I do not think you need to check for NULL before using g_list_free_full.
> 
> I am planning to clean up the minor details myself, but I still thought that it
> might be useful to put my comments here for the sake of transparency.

Ok to all! Thanks for your review!

Review of attachment 263032 [details] [review]:

::: src/gom-facebook-miner.c
@@ +277,3 @@
+    {
+      GFBGraphPhoto *photo = GFBGRAPH_PHOTO (l->data);
+      account_miner_job_process_photo (job, photo, resource, creator, error);

We can not pass error directly here. If one instance of account_miner_job_process_photo throws an error, then we can not pass the same error to the next call.

Review of attachment 263033 [details] [review]:

Looks good to me.

Created attachment 263113 [details] [review]
src: add a Facebook miner based on GFBGraph

Thank you Álvaro for your efforts. I am really glad to have this feature.