Code review comment for lp:~dobey/unity-scope-click/clicksnap

On Sat, 2016-09-03 at 01:11 +0000, Charles Kerr wrote:
> (optional) when a function reaches two bool arguments it's a
> candidate for an options bitfield a la GBusNameOwnerFlags

This is somewhat temporary. When we only support snaps, this will get
factored back out, as we won't need to know if something is a snap or
not at that point, since we'll only support snaps.

> I get that this isn't a red or green line, but out of curiosity,
> what's the rationale for calling details_callback() twice like this?
> So that showing the rest of the information won't block on the
> screenshot download?

I think maybe you were tired when you read this? Those are two
different branches of an if statement. :)

> 1. Each of the new tests looks at something unique, but they also
> look at get_architecture and get_available_framemworks in exactly the
> same way without any apparent benefit from the duplication. Maybe
> that should be moved into its own test; or if it's necessary for each
> of these tests, at least extracted into a fixture method that all of
> these can call

Refactored this into two different fixture classes.

> (needinfo) Is this correct? Shouldn't we be checking pkg.name or
> pkg.alias for empty() instead of pkg.snap_id?

Did it this way because we specifically only want to use alias for
snaps, and I think there isn't quite any guarantee that alias will
always we empty for clicks. However, this is also quite temporary, as
when we only support snaps, we'll only use what is appropriate for
that, and not worry about supporting clicks.

> (needinfo) Capturing 'this' in lambdas-inside-lambdas is always
> dicey. Are we certain that the lifetime of 'this' is safe s.t. it can
> be used this way?

I think this is fine here, as the outer call returns a pointer which we
store in this's private data, and which when destroyed aborts the
network requests, should prevent the callbacks being called, as the
requests get aborted. There is theoretically a possible race here, but
this will also be reduced back to single level when we stop supporting
clicks.