853019 - Web Activities' regexp should have a behaviour closer to HTML's pattern

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

See:
https://hacks.mozilla.org/2013/01/introducing-web-activities/comment-page-1/#comment-1971716

I agree that it would have been a good idea to stay close to the pattern attribute. I think we could even rename 'regexp' to 'pattern' (to help understanding the behaviour).

Do we really need to keep the flags attribute? I actually wonder if that would be very useful? Why would that be more useful here than in HTML's pattern attribute? Maybe HTML should be fixed? or should we wait until we find some use cases?
FWIW, the only possible use case I see would be with 'i' because if you want to filter on 'http?://mywebsite.com' you might want to allow MYWEBSITE.com too. It's still possible to do 'http?://[mM][yY][wW][eE][sS][iI][tT][eE].[cC][oO][mM]' but this is quite unfriendly and fairly unreadable.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree that switching to a pattern-like behavior would be a good idea. However I definitely think that we need to enable setting flags as well. The fact that HTML doesn't is simply a bug.

Something like

  ...  
  pattern: "...",
  pattern-flags: "im"
}

Might work?

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch

I don't know how many apps are actually using regexp filters.
A prefer to have patternFlag instead patter-flag. patter-flag is not a single word in JS...

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 729608 [details] [diff] [review]
patch

Review of attachment 729608 [details] [diff] [review]:
-----------------------------------------------------------------

Please rename 'patternFlag' to 'patternFlags'.

Generally speaking, we should find a convention regarding the naming in the manifest file. We currently are using underscores, Jonas proposed using dash and you used camelcase. It might look like complete bikeshedding but we can't have multiple different naming conventions in the same document. I'm fine with keeping this name as camelcase but we should figure out a common style. I guess I should send a message to dev-webapi.

Also, Andrea, could you make sure dev-gaia is aware of that change?

::: dom/activities/src/ActivitiesServiceFilter.jsm
@@ +40,1 @@
>          return re.test(aValue);

Can't you do:
return new RegExp('...', patternFlags).test(aValue);

::: dom/activities/tests/unit/test_activityFilters.js
@@ -146,5 @@
> -                                              {a: { required: true, regexp: '/foo/i'}}));
> -  do_check_true(ActivitiesServiceFilter.match({a: 'aafoobarasdsad'},
> -                                              {a: { required: true, regexp: '/foo/'}}));
> -  do_check_false(ActivitiesServiceFilter.match({a: 'aaFOOsdsad'},
> -                                               {a: { required: true, regexp: '/foo/'}}));

Could you add back the tests you removed and check that they actually don't match the string they could have been expected to match (and were matching before) and maybe test the string they should be matching?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cbb678c722d

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7cbb678c722d

Comment 8

[Julien Wajsberg [:julienw]]

Even if we decide to not support "regexp" at all anymore, it would be useful to have at least a warning if an app uses "regexp" without using "pattern".