627386 - Add more wildcards to match-pattern

Status: RESOLVED FIXED

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

Description

[Hernán Rodriguez Colmeiro (:peregrino)]

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

Comment 1

[Matěj Cepl]

Alternative would be to allow just plain RegExp.test() to serve as discriminator.

Comment 2

[Matěj Cepl]

Pull request https://github.com/mozilla/addon-sdk/pull/114

Test is failing ... see comment in the pull request.

Comment 3

[Matěj Cepl]

new version of the pull request passes tests.

Comment 4

[Alexandre Poirot [:ochameau]]

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.

Comment 5

[Matěj Cepl]

(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.

Comment 6

[Matěj Cepl]

Updated my pull request with patches from alex.

Comment 7

[Alexandre Poirot [:ochameau]]

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

Comment 8

[Matěj Cepl]

Just to record this somewhere:

https://developer.mozilla.org/En/Developer_Guide/Coding_Style
https://wiki.mozilla.org/index.php?title=Labs/Jetpack/Reboot/Style_Guide&oldid=202247

Comment 9

[Matěj Cepl]

Attachment: cleaned up patch

Comment 10

[Matěj Cepl]

    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.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 13

[Matěj Cepl]

(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.

Comment 14

[Matěj Cepl]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/114

Pointer to Github pull-request

Comment 15

[Matěj Cepl]

(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.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 17

[Matěj Cepl]

(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?

Comment 18

[Myk Melez [:myk] [@mykmelez]]

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.