Code review comment for lp:~uriboni/webbrowser-app/new-tab-wide-format

Why is there a new "selectedIndexNewTabViewWide" setting? If, as I suspect, this is because the UX spec mandates that the state being retained, then it shouldn’t be a setting. Instead, the StateSaver should be used for that.

Favicons appear to be too large compared to the visual spec (hint: the Favicon component already has a built-in default size, you should probably not change it).

Folder names in bookmarks (left panel) shouldn’t be black when not selected, in the visual spec they appear to be darkGrey.

The spec doesn’t consider the case where a bookmark folder is empty. Can you check with design whether we should display some sort of informative message in this case, or if having an empty right panel is expected?

Can the failing unit test be temporarily skipped (http://doc.qt.io/qt-5/qml-qttest-testcase.html#skip-method) with a reference to bug #1481233 ?

If I swipe a bookmark to the right to delete it, the red delete action covers the left panel.

Bookmark dnd doesn’t seem to work here (I’m not seeing any icon appear when hovering over a bookmark). Might be due to me running a locally-built (and not installed) version of the UITK staging branch.

43 + Binding { target: newTabViewLoader.item; property: "focus"; value: newTabViewLoader.focus }
Is this really needed? A Loader is a FocusScope, so I would expect that always setting focus to true in the component being loaded would be enough to have focus handling just work.

Why was asynchronous loading removed from newTabViewLoader?

There’s now two strings for top sites: "Top Sites" and "Top sites". Can this be made consistent?

In bookmarks-folderlist-model.[h|cpp], I don’t think the count() method is necessary, you can simply define the getter for the count property to be rowCount(). And countChanged() doesn’t appear to ever be emitted, that seems wrong.