URL Dispatcher

Merge lp://staging/~ted/url-dispatcher/more-sophisticated-urls into lp://staging/url-dispatcher/rtm-14.09

  1. more-sophisticated-urls
  2. Merge into trunk.rtm-14.09
Proposed by Ted Gould
Status: Merged
Approved by: Charles Kerr
Approved revision: 82
Merged at revision: 75
Proposed branch: lp://staging/~ted/url-dispatcher/more-sophisticated-urls
Merge into: lp://staging/url-dispatcher/rtm-14.09
Diff against target: 76 lines (+43/-1)
2 files modified
service/dispatcher.c (+2/-1)
tests/dispatcher-test.cc (+41/-0)
To merge this branch: bzr merge lp://staging/~ted/url-dispatcher/more-sophisticated-urls
Reviewer Review Type Date Requested Status
Charles Kerr (community) Approve
PS Jenkins bot continuous-integration Pending
Review via email: mp+240006@code.staging.launchpad.net

This proposal supersedes a proposal from 2014-10-23.

Commit message

Making generic regular expression more robust

Description of the change

Turns out some people need some more flexibility in our URLs. Let's give it to them.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:81
http://jenkins.qa.ubuntu.com/job/url-dispatcher-ci/149/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/url-dispatcher-utopic-amd64-ci/43
    SUCCESS: http://jenkins.qa.ubuntu.com/job/url-dispatcher-utopic-armhf-ci/41
    SUCCESS: http://jenkins.qa.ubuntu.com/job/url-dispatcher-utopic-i386-ci/41

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/url-dispatcher-ci/149/rebuild

review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote : Posted in a previous version of this proposal

Not sure about magnet links, but for DialerTest, shouldn’t we also test that a URL of the form "tel:///+442031485000" (i.e. with slashes) is valid? I don’t know how frequent this kind of link is around the web, but given that both forms are theoretically valid, we should assume we’ll encounter both.

Revision history for this message
Charles Kerr (charlesk) wrote : Posted in a previous version of this proposal

The patch LGTM.

I don't know how common tel:/// is and at first glance it doesn't seem to conform to RFC 3966... unless there's at least some anecdotal evidence that tel:/// is frequent in the wild, I'd vote against it.

Ted, looks like this MP predates the branching your scripts did last week. Does this need to be submitted against /14.10 as well?

review: Approve
Revision history for this message
Charles Kerr (charlesk) :
review: Approve
Revision history for this message
Olivier Tilloy (osomon) wrote :

It seems that "tel:///" is not correct indeed. Although I couldn’t find proper anecdotal evidence, this question on stackoverflow (http://stackoverflow.com/questions/22704662/using-slashes-in-tel-uris) seems to imply that there are tel links in the wild with two slashes (which is definitely incorrect).

As I understand it, the generic RE is used for matching "http" URLs as well as "tel", so unless we introduce a more specific RE for matching "tel" URLs, the generic RE will accept "tel" links with slashes, right?

I don’t think that’s a big deal though, the issue at hand (handling "tel:" links) is fixed, the logic can probably be refined later with a lower priority.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches