Code review comment for lp:~jonas-drange/ubuntu-system-settings/security-allow-acceptance-of-here-terms-fixes-1375322

Revision history for this message
Ken VanDine (ken-vandine) wrote :

> On Fri, Oct 17, 2014 at 04:31:28PM -0000, Ken VanDine wrote:
> > Review: Approve
> >
> > Tested this on krillin, worked well. We should look at refactoring this if
> we ever have more location providers. We'll need to make this more more
> dynamic, and hopefully share the logic with the wizard.
>
> This is copy and pasted code, so it should be refactored *anyway*.
>
> Also I made some specific suggestions for how the implementation could
> be improved, and I still think those are valid.
>
> (not intended to block this MP, but you have my opinion)

Understood, I captured your comments into bug 1382662

>
> --
> Iain Lane [ <email address hidden> ]
> Debian Developer [ <email address hidden> ]
> Ubuntu Developer [ <email address hidden> ]

« Back to merge proposal