> This looks like useful functionality.
> 
> Were you aware the lp_save will detect races where the value has changed since
> being read?  If so, why did you choose to forgo that protection by doing a raw
> patch?
> 

I didn't know that. I based that bit of the code on what was already done in 
lp/app/javascript/picker.js, which uses client.patch().
I'll look at changing over to lp_save as see how that works out.

> How are logged-out users handled?  Usually, we show them the edit icon, and
> require them to log in when they click it.
>

Yes, that's what happens here too.
 
> Did you consider accepting "linkify" as a boolean parameter rather than
> "attribute_type"?  Seems simpler, and it's always hard to predict future
> extensions.  You might even be able to eliminate "attribute_type" by calling
> canonical_url unconditionally and handling exceptions.
> 

I considered it but thought it was too restrictive. I agree it's hard to predict the future, but passing in a "type" value defers the decision to what to do with it to the implementing code. It allows other possible behaviours based on attribute type to be transparently implemented. Having a linkify boolean exposes a specific implementation detail to the caller and precludes future work without requiring an api change. 

The second idea - to call canonical_url unconditionally and handle exceptions - I also considered and it was 50/50 which approach I went with. I tend to steer away from using exceptions in this way though. To me they are to communicate errors not probe for functionality etc. I settled for the attribute_type parameter to allow for future flexibility but if you feel its a case of YAGNI, I'll happily use the exception approach. I do think the attribute_type approach is a bit ugly but it was the lesser of 2 evils for me. 

> I think you could shorten your code by sticking your json attributes in a
> single dict, and accepting such a dict as the input to addMultiCheckboxPatcher
> would be perfectly consistent with YUI3 parameter style.
> 

I used the same approach as already used for InlineEditPickerWidget in going with the individual parameters. Note that one of those parameters passed to addMultiCheckboxPatcher() and also addPickerPatcher() in the InlineEditPickerWidget case is actually a dict of config values. I'll find out the rational there and consider applying any improvements to both widgets so they are implemented consistently.

> Usually, we indent by four spaces, but there are a bunch of places where
> you've done more, including: 94, 133, 194, 413, 514-517, 527, 535-537,
> 539-540, 652-653, 781, 792.
>

Yeah, it's a personal preference - I prefer to try and keep the method parameters close to the method name instead of all the way over to the left. I'll fix them.

 
> I don't know if we have a style recommendation for how many blank lines to
> spaces to use at the top level, but if we don't, I suggest using the python
> style of two blank lines.
>

Ok.
 
> According to deryck, who just tested on my behalf, function names are scoped
> to the closure by default, so I think "foo function()" is better than "var foo
> = function()".  Is there a reason why you're using the latter?
> 

Depends on what existing code I was grepping to provide guidance on what to do. I tended to follow what had gone before. I'll change to the style you suggest.

> You seem a bit inconsistent about spaces when using "+" for string
> concatenation.  I suggest using a space, e.g. "foo" + "bar", not "foo"+"bar".
> This is more like python style, and I find it more readable.
> 

Ok.

> On a related note, CHECKBOX_TEMPLATE should use the [str1, str2, ...].join("")
> form per https://dev.launchpad.net/JavaScriptReviewNotes
> 

Thanks. Will do.

> The expected_item generation in test_items_for_field_vocabulary,
> test_items_for_custom_vocabulary and test_items_for_custom_vocabulary_name
> looks duplicated, and should be extracted to a helper.

Fair point. Will fix.