webbrowser-app

Merge lp:~dbarth/webbrowser-app/short-url-patterns into lp:webbrowser-app

short-url-patterns
Merge into trunk

Proposed by David Barth on 2014-08-18

Status:

Merged

Approved by:

David Barth on 2014-09-10

Approved revision:

677

Merged at revision:

709

Proposed branch:

lp:~dbarth/webbrowser-app/short-url-patterns

Merge into:

lp:webbrowser-app

Diff against target:

49 lines (+22/-1)

2 files modified

src/app/webcontainer/url-pattern-utils.cpp (+14/-1)
tests/unittests/container-url-patterns/tst_ContainerUrlPatternsTests.cpp (+8/-0)

To merge this branch:

bzr merge lp:~dbarth/webbrowser-app/short-url-patterns

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2014-09-07
Alexandre Abreu (community)		2014-08-26	Approve on 2014-09-06
Ubuntu Phablet Team		2014-08-18	Pending
Review via email: mp+231233@code.launchpad.net

Commit message

This branch adds support for url patterns without subdomains, ie of the form: https?://mydomain.com/*

Description of the change

This branch adds support for url patterns without subdomains, ie of the form: https?://mydomain.com/*

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-18:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1044/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-08-26:

I wonder if there was a good reason for not considering short URLs in pattern filtering in the first place, or if it was simply overlooked. Alex, could you comment?

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-08-26:

The change looks good to me.

It looks like the block under "else if (isValidShortUrlPattern(pattern))" is exactly the same as the one under "if (isValidWebappUrlPattern(pattern))", maybe this could be factored out? I.e., either:

    if (isValidWebappUrlPattern(pattern) || isValidShortUrlPattern(pattern)) {
        …
    }

or factor out the contents of the block into a "transformValidWebappUrlPattern(…)" function.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-08-26:

I verified that the change fixes bug #1358399 indeed with the untappd app.

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2014-09-03:

I tried to look up the discussions the we initially had w/ Jamie about it, but I cannot recall why it was left on the side (although it was done on purpose at the time). I cannot come up with a reasonable objection to this at this point,

See a few comments below, minor points though,

review: Approve

lp:~dbarth/webbrowser-app/short-url-patterns updated on 2014-09-05

677. By David Barth on 2014-09-05: factor code and add test for strict domain matching

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-09-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1073/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Alexandre Abreu (abreu-alexandre) on 2014-09-06:

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-09-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1074/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-09-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/1078/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

David Barth

Ubuntu Phablet Team

to status/vote changes:

Rachel Liu

 === modified file 'src/app/webcontainer/url-pattern-utils.cpp'
 --- src/app/webcontainer/url-pattern-utils.cpp	2014-04-01 18:45:08 +0000
 +++ src/app/webcontainer/url-pattern-utils.cpp	2014-09-05 20:15:11 +0000
@@ -77,12 +77,25 @@
      return grammar.match(pattern).hasMatch();
+ }
++/* A strict URL pattern has a strict domain name with no wildcard pattern,
++ * and possibly regular path and protocol globs.
++ *
++ * @param pattern pattern that is to be tested for validity
++ * @return true if the url is valid, false otherwise
++ */
++static bool isValidStrictUrlPattern(const QString& pattern)
++{
++    static QRegularExpression grammar("^http(s|s\\?)?://[^\\.\\*\\?]+\\.[^\\.\\*\\?]+(\\.[^\\.\\*\\?/]+)*/.*$");
++    return grammar.match(pattern).hasMatch();
++}
++
  QString UrlPatternUtils::transformWebappSearchPatternToSafePattern(const QString& pattern)
+ {
      QString transformedPattern;
--    if (isValidWebappUrlPattern(pattern)) {
++    if (isValidWebappUrlPattern(pattern) ||
++        isValidStrictUrlPattern(pattern)) {
          QRegularExpression urlRe("(.+://)([^/]+)(.+)");
          QRegularExpressionMatch match = urlRe.match(pattern);
 === modified file 'tests/unittests/container-url-patterns/tst_ContainerUrlPatternsTests.cpp'
 --- tests/unittests/container-url-patterns/tst_ContainerUrlPatternsTests.cpp	2014-04-30 12:03:55 +0000
 +++ tests/unittests/container-url-patterns/tst_ContainerUrlPatternsTests.cpp	2014-09-05 20:15:11 +0000
@@ -40,6 +40,14 @@
                  << "https?://*.mydomain.com/*"
                  << "https?://[^\\./]*.mydomain.com/[^\\s]*";
++        QTest::newRow("Valid pattern - short url")
++                << "https?://mydomain.com/*"
++                << "https?://mydomain.com/[^\\s]*";
++
++        QTest::newRow("Valid pattern - strict url")
++                << "https?://www.mydomain.com/*"
++                << "https?://www.mydomain.com/[^\\s]*";
++
  #define WEBAPP_INVALID_URL_PATTERN_TEST(id,invalid_url_pattern) \
          QTest::newRow("Invalid pattern " #id) \
                  << invalid_url_pattern \