Merge lp://staging/~jdstrand/webapps-core/lp1296170 into lp://staging/webapps-core

Proposed by Jamie Strandboge
Status: Merged
Merged at revision: 38
Proposed branch: lp://staging/~jdstrand/webapps-core/lp1296170
Merge into: lp://staging/webapps-core
Diff against target: 12 lines (+1/-1)
1 file modified
webapp-facebook/webapp-facebook.desktop (+1/-1)
To merge this branch: bzr merge lp://staging/~jdstrand/webapps-core/lp1296170
Reviewer Review Type Date Requested Status
Alexandre Abreu Pending
David Barth Pending
Review via email: mp+213538@code.staging.launchpad.net

This proposal supersedes a proposal from 2014-03-31.

Description of the change

webapp-facebook/webapp-facebook.desktop: adjust --webAppUrlPatterns to use --webappUrlPatterns=https?://[a-z]+.facebook.com/* (LP: #1296170)

To post a comment you must log in.
Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote : Posted in a previous version of this proposal

Is there a specific reason why you have something like [a-z]+ and not e.g. [a-z][a-z0-9]+ ?

Also, the url patterns specified at this level (specified in --webappUrlPatterns) are actually a weak versions of wildcard patterns (with '?' as an extra pattern char). So full blown regexp should not really used (although not strictly forbidden). In this context maybe 'https?://*.facebook.com/*' would match your needs a bit more (it'll be translated to 'https?://[^\\./]*.facebook.com/*' ?

review: Needs Information
Revision history for this message
Jamie Strandboge (jdstrand) wrote : Posted in a previous version of this proposal

I just didn't want '*' in there and they seem to be using one letter hostnames, so I went with that. Note this:
  [a-z][a-z0-9]+

requires at least a two-character hostname and would not resolve the issue for me.

I forgot about the translation of 'https?://*.facebook.com/*' to 'https?://[^\\./]*.facebook.com/*'. I would be fine with: 'https?://*.facebook.com/*'. Should I resubmit?

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote : Posted in a previous version of this proposal

> I just didn't want '*' in there and they seem to be using one letter
> hostnames, so I went with that. Note this:
> [a-z][a-z0-9]+
>
> requires at least a two-character hostname and would not resolve the issue for
> me.

right,

>
> I forgot about the translation of 'https?://*.facebook.com/*' to
> 'https?://[^\\./]*.facebook.com/*'. I would be fine with:
> 'https?://*.facebook.com/*'. Should I resubmit?

I'd say yes, just to avoid too much heterogeneous approaches when we can,

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Updated to use '--webappUrlPatterns=https?://*.facebook.com/*' per feedback.

Revision history for this message
Alexandre Abreu (abreu-alexandre) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
David Barth (dbarth) wrote : Posted in a previous version of this proposal

Works

review: Approve

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

to all changes: