Code review comment for lp:~abreu-alexandre/webbrowser-app/add-scheme-filter-handler

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

Docstrings in scheme-filter.h are outdated, and to be honest not very useful as they don’t contain any actual documentation. Can they be either updated, or removed?

On a related note, some minimal documentation at the class-level to explain the general purpose of the class and how it is used in conjunction with other constructs by the webapp container would be very helpful, especially for our future selves.

intent-parser.h has some unused includes (QObject, QVariantMap). The include for QUrl could be made a forward declaration.

intent-parser.cpp has some unused includes (QJSEngine, QJSValue).

The copyright notice for the two new files should add 2015 (it’s not new code, but those are new files).

In webapp-container.cpp, won’t the renaming of the default filename for the scheme filter (LOCAL_SCHEME_FILTER_FILENAME) break existing filters for already published apps?

In test_default_scheme_filter, shouldn’t the test filter use encodeURIComponent() instead of r.path.replace(…) ?

« Back to merge proposal