Code review comment for lp:~uriboni/webbrowser-app/tab-context-menu

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

In src/app/webbrowser/TabsBar.qml:

    var menu = PopupUtils.open(contextualOptionsComponent, tabDelegate)
    menu.targetIndex = index

  'targetIndex' should be set at instantiation time, passing properties to PopupUtils.open()

    if (tab.url.toString().length > 0) tab.webview.reload()

  This won’t work if the webview hasn’t been instantiated (they aren’t until the tabs are first activated). You’ll need to check whether the webview is not null before calling reload().

In src/app/webbrowser/tabs-model.cpp:

    if (index < 0) index == 0;

  Looks like an assignment was meant, but it’s a silent comparison instead.

    In TabsModel::insert(), the call to setCurrentIndexNoCheck, in case a tab is inserted in the model before the current tab, will fire currentIndexChanged and currentTabChanged, but this is incorrect as only the current index changed, not the current tab.

In tests/unittests/qml/qml_tree_helpers.js:

    function qmlType() can be simplified, by always returning String(item).split("_")[0]
    findChildrenByType() should probably be renamed findDescendantsByType, as it’s recursive

In tests/unittests/qml/tst_TabsBar.qml:

    it looks like bug #1205144 was fixed in the UITK, can the tests be simplified now?

In tests/unittests/tabs-model/tst_TabsModelTests.cpp:

    there are inconsistent indentations (4 spaces mixed with 2 spaces)

review: Needs Fixing

« Back to merge proposal