[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