[Merge] lp:~mardy/webbrowser-app/refactoring into lp:webbrowser-app
Olivier Tilloy
olivier.tilloy at canonical.com
Tue May 19 12:30:51 UTC 2015
That’s a huge changeset and I’m not sure I understand all the implications, so given that it doesn’t touch any shared code, I’ll trust that David, Alex and yourself test it thoroughly.
One thing that would greatly increase our confidence when doing major reworks like this one would be to have autopilot tests for the online accounts integration. Is that something that was ever considered? Not saying it should be done as part of this MR, but I think it would be very valuable to have, to validate the functionality as well as future changes.
Aside from that general suggestion, a couple of minor remarks from taking a quick glance at the code:
- For all localized strings that contain placeholders, please add a TRANSLATORS comment to explain the meaning of each placeholder.
- Indentation in src/app/webcontainer/AccountChooserDialog.qml is busted, can you fix it?
--
https://code.launchpad.net/~mardy/webbrowser-app/refactoring/+merge/258074
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~mardy/webbrowser-app/refactoring into lp:webbrowser-app.
More information about the Ubuntu-reviews
mailing list