Code review comment for lp:~artmello/webbrowser-app/webbrowser-app-bookmarks_view

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

Comments on revision 1224:
 - In BookmarksModelUtils.js:
   * Is there really a use case for model==null? It looks like that check was in BookmarksFoldersViewWide.qml before, but it was not in BookmarksFoldersView.qml, so it doesn’t make sense to factor that bit out.
   * The docstring for the function isn’t correct, it’s not a temporary model (it would be temporary if it was instantiated, used and then deleted within the same scope). And there shouldn’t be a check for homeEntry==undefined. Calling that function with no home entry is useless and needlessly expensive, as it does a deep-copy of the already existing model.
   * Can we find a more expressive name for createUrlsListModel()? Maybe prependHomepageToBookmarks()?
 - As pointed out above, in BookmarksFoldersView.qml and BookmarksFoldersViewWide.qml, calling createUrlsListModel() without a homepage entry is useless, you should simply return the existing model.

review: Needs Fixing

« Back to merge proposal