Code review comment for lp:~uriboni/oxide/find-in-page

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

The API looks good to me (I’ll let Chris comment on the actual implementation). Here are a few comments, most of them on the unit tests:

 - In test_case_insensitive(), the second test should be on a different string, otherwise the count might not have been updated yet.

 - In test_new_text_resets_count() and in test_navigation_does_not_reset(), next() should be called after verifying that the count is 2.

 - In test_find_on_invalid_page(), waiting for a successful load doesn’t look right. Shouldn’t you use webView.waitForLoadFailed() ?

 - In test_navigation_does_not_reset(), the comparison of text to "dolor" doesn’t need a tryCompare(), it should be a simple compare().

 - In OxideQQuickWebViewFindInPage’s constructor, the explicit initialization of text_ is useless.

 - In the implementation of WebView::findInPage() and other methods, please avoid the use of a return statement on the same line as the condition, and always use curly braces, even for one-liner if blocks.

« Back to merge proposal