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

Revision history for this message
Alexandre Abreu (abreu-alexandre) 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 test_default_scheme_filter, shouldn’t the test filter use
> encodeURIComponent() instead of r.path.replace(…) ?

all updated,

> 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?

Last time I looked no app uses a default filter for intents, the default one
does the work already,

« Back to merge proposal