[Merge] lp:~abreu-alexandre/webbrowser-app/add-uninstall-click-hook-to-webapps into lp:webbrowser-app

Alexandre Abreu alexandre.abreu at canonical.com
Tue Nov 18 15:49:14 UTC 2014


> I didn't review it thoroughly, but I'd like to suggest two changes:
> 
> 1) Of course, this is about the style :-) Make it consistent, in some places
> you are using "type& var", in some others "type &var".

done :)

> 2) I think that the OptionalData class (and the Fallible template) can be
> removed and the code simplified a bit. The Data() default constructor already
> does the right thing (setting the optional fields to "false") and it doesn't
> look like you really need to distinguish the case when the hook file is absent
> or if it's present and the keys are set to false.

I thought a bit about this one, I agree that it would simplify the code a bit
and as things are now would still make things work, but I find really annoying
(not to say bad) that there is no way (unless you complexify the function code a bit
and add cumbersome return arguments) to tell if a data is a valid data or not
(obtained by way of a valid click hook file). It adds a bit of overload on the data
structure which should also make sure that its default values produce no-ops when
there is a failure of any sort ... 

This is a rather clean solution to sparkling those logic bits around the code (return
function arguments, etc.),


-- 
https://code.launchpad.net/~abreu-alexandre/webbrowser-app/add-uninstall-click-hook-to-webapps/+merge/241584
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.



More information about the Ubuntu-reviews mailing list