Code review comment for lp:~michael-sheldon/webbrowser-app/implement-download-folder

Can the autopilot tests detect at run time whether the platform supports downloads, and be skipped where relevant (typically a desktop for udm is not installed)?

What (and how) does the test [typeof(webapp) == "undefined"] check exactly?

In src/app/ContentPickerDialog.qml, this breaks encapsulation:
    var downloadPage = browser.showDownloadsPage()
Can’t we have the dialog emit a signal with the necessary parameters, which WebViewImpl would connect to and forward, and Browser would connect to that signal from the webview and act accordingly?
The same applies to Downloader.qml.

In mimeTypeRegexForContentType(), shouldn’t the regexp for vcards also match "text/vcard" ?
And for documents, shouldn’t it accept the mimetypes listed at https://bazaar.launchpad.net/~ubuntu-docviewer-dev/ubuntu-docviewer-app/lo-viewer/view/head:/src/plugin/file-qml-plugin/docviewerutils.cpp#L52 ?

In MimeDatabase::iconForMimetype() and MimeDatabase::nameForMimetype(), why instantiate a new QMimeDatabase instead of reusing m_database ?

In MimeDatabase::iconForMimetype(), please wrap "save" in a call to QStringLiteral(). On a related note, given that MimeDatabase is a generic helper (not necessarily used only for downloads), wouldn’t it make more sense to return an empty icon, and on the caller’s end replace it by "save" if empty?

In Browser.qml, version 1.0 of Ubuntu.Content is imported. There are other places in the code where version 0.1 is being imported. Can this be made consistent?

On a related note, if Browser.qml imports Ubuntu.Content, then a runtime dependency on qtdeclarative5-ubuntu-content1 needs to be added to the packaging information for webbrowser-app. But qtdeclarative5-ubuntu-content1 is in universe, so we can’t have that dependency. This needs to be decoupled somehow, by having the code that refers to Ubuntu.Content dynamically loaded. Otherwise the browser cannot be started without qtdeclarative5-ubuntu-content1.

DownloadDelegate.qml appears to be breaking encapsulation by referring to 'downloadManager'. The entire Component.onCompleted block can be moved to where this DownloadDelegate is being instantiated, that will probably make it easier to not break encapsulation.

In the file picker when uploading a file, the "select all" button should be disable/not visible if multiple files are not allowed. And how do I actually pick a file to upload? I’m seeing the selection mode, I can tick the checkbox for one given file, but how do I validate (there’s only the selectAll and delete actions in the header)? If there isn’t one already, an autopilot test to validate this use case would be good to have.

Could the DownloadsMimetypeModel be re-implemented purely in QML using a SortFilterModel (https://developer.ubuntu.com/api/apps/qml/sdk-15.04/Ubuntu.Components.SortFilterModel/), by any chance? If not, in DownloadsMimetypeModel::filterAcceptsRow() instead of re-instantiating a QRegExp at each call, it should be cached as a member attribute of the model.

Ideally, the DownloadsModel class should be unit tested, with a decent test coverage. Let’s not block on this, but let’s also plan on adding the missing tests soon. Or as a temporary trade-off, can you add the structure for unit tests for the model, with maybe one or two test functions, and plan on improving the coverage later on?