Code review comment for lp:~uriboni/webbrowser-app/keyboard-navigation

Revision history for this message
Olivier Tilloy (osomon) wrote :

We’re getting there nicely. A few more minor remarks:

288 + if (tabsModel.count >= 0) {

It should be (tabsModel.count > 0), there is no point in trying to close the current tab if there isn’t any.

680 + def setUp(self, url="/test1"):

That argument would better be named 'path', rather than 'url'.

986 + self.assertThat(suggestions.count, Eventually(NotEquals(0)))

Although functionally equivalent, GreaterThan would be more appropriate than NotEquals.

996 + def test_keyboard_movement(self):

Can test_keyboard_movement be renamed test_keyboard_navigation ?

1085 + QCOMPARE(item.count(), 0);

Can I suggest QVERIFY(item.isEmpty()) ?

review: Needs Fixing

« Back to merge proposal