Closed Bug 627386 Opened 12 years ago Closed 11 years ago

Add more wildcards to match-pattern

Categories

(Add-on SDK Graveyard :: General, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: peregrino, Assigned: mcepl)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [papercuts])

Attachments

(2 files)

If you want to match:

sub1.page.com/page.com/*
sub2.page.com/page.com/*
sub3.page.com/page.com/*

...

subN.page.com/page.com/*

N pattern-matchs are needed., as *.page.com/* doesn't work.

This is a pretty common scenario, and chrome extensions support it, by the way: http://code.google.com/chrome/extensions/match_patterns.html
Alternative would be to allow just plain RegExp.test() to serve as discriminator.
Pull request https://github.com/mozilla/addon-sdk/pull/114

Test is failing ... see comment in the pull request.
new version of the pull request passes tests.
Hi Matej,
Thanks for working on this patch and trying to find workaround on current regexp/sanboxes bug!

I don't have review/commit capabilities so I can only drop some comments.
You may try to pass RegExp objects by using RegExp.test function instead of String.match. (test is working fine across sanboxes)
So it would be clearer between string pattern and regexp.
(In reply to comment #4)
> I don't have review/commit capabilities so I can only drop some comments.
> You may try to pass RegExp objects by using RegExp.test function instead of
> String.match. (test is working fine across sanboxes)
> So it would be clearer between string pattern and regexp.

What else I am doing on line 93 of https://github.com/mozilla/addon-sdk/pull/114/files?

+    if (this.regexp && this.regexp.test(urlStr))
+      return true;

I don't understand what you mean.
Updated my pull request with patches from alex.
I think except some code guidelines errors: 
cuddle the else on line 47 of packages/api-utils/lib/match-pattern.js,
there seems to be a global indentation problem when we look at pull request diff on github.

You may ask for review to :myk
Attached patch cleaned up patchSplinter Review
Attachment #520896 - Flags: review?(myk)
    This is actually very tiny patch:
    
         exports.MatchPattern = MatchPattern;
    
         function MatchPattern(pattern) {
        +  if (typeof pattern.test == "function") {
        +      this.regexp = pattern;
        +  }
        +  else {
           let firstWildcardPosition = pattern.indexOf("*");
           let lastWildcardPosition = pattern.lastIndexOf("*");
           if (firstWildcardPosition != lastWildcardPosition)
    
        -----------------------------------------------------------
               return false;
             }
    
        +    if (this.regexp && this.regexp.test(urlStr))
        +      return true;
             if (this.anyWebPage && /^(https?|ftp)$/.test(url.scheme))
               return true;
             if (this.exactURL && this.exactURL == urlStr)
    
    (plus test and some missing braces), but given that whole rest of the
    constructor is now in the else branch, it makes a massive changes in the
    indentation.
Duplicate of this bug: 631738
Comment on attachment 520896 [details] [diff] [review]
cleaned up patch

I'm ok with adding the ability to use regular expressions as match patterns, modulo the outcome of the conversation in bug 634764 about the best way to implement style modification, since the user stylesheet approach would constrain the match pattern syntax to the existing API.

However, if the outcome of that conversation is that we have syntax flexibility, then I'd still really like to see a simple match pattern enhancement like the one that Chrome Extensions provides to address this use case, since although regexes are a powerful tool for pattern matching, they are also complex and potentially dangerous.


>+  ok(/http[s]?:\/\/bugzilla\..*\/.*/, "https://bugzilla.redhat.com/show_bug.cgi?id=569753");
>+  ok(/http[s]?:\/\/bugzilla\..*\/.*/, "https://bugzilla.mozilla.org/show_bug.cgi?id=627386");

Nit: to make it more obvious that this is testing to make sure a single regex matches two different URLs, it would be handy to assign the regex to a variable that gets passed into the ok function, i.e.:

  let regex = /http[s]?:\/\/bugzilla\..*\/.*/;
  ok(regex, "https://bugzilla.redhat.com/show_bug.cgi?id=569753");
  ok(regex, "https://bugzilla.mozilla.org/show_bug.cgi?id=627386");


Otherwise looks good, r=myk on the code, but setting a feedback request to keep this on my radar, as we need to hold off checking it in until we finish the conversation in bug 634764.
Attachment #520896 - Flags: review?(myk)
Attachment #520896 - Flags: review+
Attachment #520896 - Flags: feedback?(myk)
Depends on: 634764
(In reply to comment #12)
> However, if the outcome of that conversation is that we have syntax
> flexibility, then I'd still really like to see a simple match pattern
> enhancement like the one that Chrome Extensions provides to address this use
> case, since although regexes are a powerful tool for pattern matching, they
> are also complex and potentially dangerous.

I am not sure I like the Google match patterns. First of all, they are too limiting (so for example, they couldn't be used for the test I use, which I don’t is a unreasonable one).

Second, I know there are fears about too complex RE language (“You wanted to solve your problem with RE, and you have two problems.”), but RE is part of Ecmascript, so authors could be expected to just learn its basics, there is wealth of documentation, tutorials, etc. Also, it is part of every JavaScript interpreter, so it is not bringing any additional burden on the platform (and no additional maintenance inside of Add-on SDK).

So, even if you decide to introduce full Google match patterns, I would vote for keeping REs alongside (not instead of) them.

>   let regex = /http[s]?:\/\/bugzilla\..*\/.*/;
>   ok(regex, "https://bugzilla.redhat.com/show_bug.cgi?id=569753");
>   ok(regex, "https://bugzilla.mozilla.org/show_bug.cgi?id=627386");

Sure, good point, pull request updated.
OS: Mac OS X → All
Pointer to Github pull-request
(In reply to comment #14)
> Created attachment 522837 [details]
> Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/114
> 
> Pointer to Github pull-request

Sorry, stupid clicking on the new acquired button.
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → 1.0
Whiteboard: [papercuts]
Comment on attachment 520896 [details] [diff] [review]
cleaned up patch

The outcome of conversation in bug 634764 is that we're going to implement style mods using user stylesheets.  But we can implement support for regex-based site matching into user stylesheets, so the current limitations of user styleesheets do not require us to forgo landing this patch.  Thus it's ready to be checked in.
Attachment #520896 - Flags: feedback?(myk) → feedback+
(In reply to comment #16)
> Thus it's ready to be checked in.

Is there something I am supposed to do to make this happen?
Assignee: nobody → mcepl
Nope, checking it in is the responsibility of the committers.  Landed:

https://github.com/mozilla/addon-sdk/commit/45505a8c67a3b337715746b7340a6b578b8feb08

Thanks for the fix!

Marking this dev-doc-needed, as we also need to document this.  The documentation should emphasize that regexes are powerful but dangerous and should be avoided unless the simpler wildcard syntax is insufficient.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.