[Merge] lp:~abreu-alexandre/webbrowser-app/intent into lp:webbrowser-app
Olivier Tilloy
olivier.tilloy at canonical.com
Wed Jan 28 13:05:33 UTC 2015
Review: Needs Fixing
> > Why not make the parameter of parseIntentUri(…) a QUrl ?
> > This would make it much easier to parse the URI, without the need for
> > regexpes. E.g.:
> >
> > QUrl url("intent://…");
> >
> > qDebug() << "URL:" << url;
> > qDebug() << "scheme:" << url.scheme();
> > qDebug() << "host:" << url.host();
> > qDebug() << "path:" << url.path();
> > qDebug() << "query:" << url.query();
> > qDebug() << "fragment:" << url.fragment();
> >
> > QStringList fragments = url.fragment().split(";");
> > assert(fragments.takeFirst() == "Intent");
> > assert(fragments.takeLast() == "end");
> > QMap<QString, QString> tokens;
> > Q_FOREACH(const QString& fragment, fragments) {
> > QStringList token = fragment.split("=");
> > assert(token.size() == 2);
> > tokens.insert(token[0], token[1]);
> > }
> > qDebug() << "tokens:" << tokens;
>
> I'd agree, except that it also brings extra logic to reconstruct
> the intent URI parts:
> - path + query (if any),
> - host (if any) and path,
> etc.
> plus some other things,
>
> which in the end makes it also a bit convoluted imo,
Fair enough (I disagree but let’s agree to disagree). A couple of comments on the logic though:
- the intentRe regexp should end with the ";end;" token
- the assignment of result.host and result.uriPath will fail if s has more than 2 items (e.g. if the path is several levels deep, like example.org/example/path/to/some/resource)
--
https://code.launchpad.net/~abreu-alexandre/webbrowser-app/intent/+merge/247421
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.
More information about the Ubuntu-reviews
mailing list