Test failure:

 - webbrowser_app.tests.test_bookmark_options.TestBookmarkOptions.test_set_bookmark_title reliably fails when run on krillin (with the latest vivid image). In fact the this is a functional issue, as the popover is partially scrolled out of view when the OSK slides in.


Functional issues:

 - Pressing Ctrl+D to bookmark a page should bring up the popover too.

 - When the popover is visible, pressing ESC should dismiss it and remove the bookmark, Return should dismiss it and save the bookmark (similar to desktop chromium’s behaviour).


UX issue, which I discussed with James earlier today:

 - Clicking the star or pressing Ctrl+D for a page that is already bookmarked should allow editing the bookmark, instead of removing it. This requires further design work though, so let’s keep it as it is for now.


Tests coverage:

 - bookmarks-folderlist-model.cpp has only 70.6% coverage, this should be improved.
 - bookmarks-model.cpp has 93.8% coverage, which is ok, but we’re not testing initially populating a model with existing folders. This should be tested.


Code remarks:

 - In BookmarkOptions.qml:
   * It doesn’t look like selectorDelegate is used in several places, perhaps it doesn’t need to be enclosed in a separate component, but rather be set directly as folderOptionSelector.delegate?
   * Can you add translator comments for the "Name" and "Save in" strings? Without context it’s not necessarily obvious how to translate those.
   * In titleTextField (and other places), please define anchors instead of setting width to parent.width.
   * Not sure why newFolderButton and okButton are inside a Row, but if you want to ensure that one is anchored to the left and the other one to the right, that’s not the right solution.
   * In newFolderDialog.saveButton, is there a bug report we could link to for the issue with onClicked not being triggered?

 - In BookmarksFolderDelegate.qml:
   * objectName doesn’t need to include the folderName. Instead, in autopilot tests, you can filter both on 'objectName' ("bookmarkFolderDelegate") and on the 'folderName' property.
   * There are two references to bookmarksFolderListViewItem, which isn’t defined in the scope of this component, this breaks encapsulation. In fact it doesn’t look like BookmarksFolderDelegate really needs to be a separate QML file (it’s not re-used anywhere else, is it?), so I think it would be easier to just inline it in BookmarksFolderListView.

 - In BookmarksFolderListView.qml:
   * The BookmarksFolderDelegate instance that serves as a delegate doesn’t need to be explicitly enclosed in a Component, QML does that automatically.

 - In Browser.qml:
   * In the BookmarkOptions instance, the binding of bookmarkTitle to browser.currentWebview.title has undesired side effects if the page updates the title periodically (test e.g. with http://htmlmarquee.com/title.html). Instead, the title needs to be retrieved on the current page and set in Component.onCompleted.

 - In bookmarks-folderlist-model.cpp:
   * The documentation says that each item has three roles, but they have two.
   * BookmarksFolderListModel::data() should probably call checkValidFolderIndex(index.row()) to ensure that the the row is valid.

 - In bookmarks-model.cpp:
   * In the while loop in BookmarksModel::populateFromDatabase(), you could use a local variable to store the value of populateFolderQuery.value(1).toString(), instead of querying it twice.
   * In BookmarksModel::addFolder(), what does "if (newFolderId)" check? Can the boolean test be made explicit, i.e. "if (newFolderId != 0)" ?
   * The above also applies to the if statement in BookmarksModel::updateExistingEntryInDatabase().