816730 - [Activities] filters do not work anymore

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

https://bugzilla.mozilla.org/show_bug.cgi?id=805292 seems to have completely altered the way the system decides which apps to list as handlers for an activity request.

The purpose of that bug was to enable a generic 'pick' activity that would allow the user to select a file of any type (to implement <input type=file>, I think). 

The way filtering worked before is that activity handlers could require certain parameters so they would not be invoked unless the request was for a certain file type or met other criteria. Now, it appears that filters only apply if the initiating app actually includes that field in the activity request. If the initiating app omits a field, it becomes a wildcard that matches any filter any receiving app sets up.

The wallpaper app, for example, included filters on width and height so that it would only appear as a choice when the activity was requesting a 320x480 image.  This meant that when the user wants to pick an image from the contacts app, they would see Gallery and Camera as choices, but not Wallpaper.  But now filtering is broken. If the contacts app does not specify a width and height then the system will not filter on those, even though the wallpaper app explicitly lists those as filters in its manifest.webapp file. So users of the contacts app are shown the Wallpaper app as one of their choices.

In general, it seens wrong and possibly very bad to take filtering power away from activity handlers.  Those filters are an app's way of declaring its preconditions. If the system ignores the filters, the app will likely be invoked with payloads that it does not know how to handle. Imagine an image upload app that implements the share activity an does a filter type:[image/jpeg, image/png]. It used to be that this would guarantee that it would only be invoked by apps that were passing (or claiming to pass) an image to it.  But now, an app that initiates a share activity without specifying a type field would display this image upload app as a choice to the user and if the user selects that choice, then some arbitrary payload will be passed to the image uploader, which will probably not be able to handle it.

I think we should just revert https://bugzilla.mozilla.org/show_bug.cgi?id=805292

But failing that, we should discuss alternatives here (like maybe allow initiating apps to explicitly specify a wildcard for certain fields) instead of just omitting them.

And, once we have decided what to do, we should set the dev-doc-needed flag, so that this gets documented.

Comment 1

[David Flanagan [:djf]]

Cc'ing everyone who was cc'ed on #805292

I'll also add that I think it is okay to revert #805292 because it wouldn't have worked as a file chooser anyway, because the pick activity returns blobs, not files. The gallery app, for example, crops the image you choose and returns it to you without ever saving it.  The way to do a file chooser app is to leave activities alone and write a file chooser app that implements a "choose-file" activity (or something like that). Give it device-storage:sdcard permission and have it display the contents of the /sdcard to the user.

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to David Flanagan [:djf] from comment #1)
> Cc'ing everyone who was cc'ed on #805292
> 
> I'll also add that I think it is okay to revert #805292 because it wouldn't
> have worked as a file chooser anyway, because the pick activity returns
> blobs, not files.

You forgot that there is some chrome code in-between which can create a temporary file from the Blob and use that file.

> The way to do a file chooser
> app is to leave activities alone and write a file chooser app that
> implements a "choose-file" activity (or something like that). Give it
> device-storage:sdcard permission and have it display the contents of the
> /sdcard to the user.

I do not think that would be a good idea.

Comment 3

[Andrea Marchesini [:baku]]

The current implementation "should" work in this way:

App: { type: 'foo', probA: ['a', 'b', 'c'], probB: 42 }
Request: {}
Status: Accepted - because no props are sent

App: { type: 'foo', probA: ['a', 'b', 'c'], probB: 42 }
Request: { type: 'foo' }
Status: Accepted - because type matches

App: { type: 'foo', probA: ['a', 'b', 'c'], probB: 42 }
Request: { propA: 'a' }
Status: Accepted - because propA matches at least 1 value

App: { type: 'foo', probA: ['a', 'b', 'c'], probB: 42 }
Request: { propA: ['a','d'] }
Status: Accepted - because propA matches at least 1 value

App: { type: 'foo', probA: ['a', 'b', 'c'], probB: 42 }
Request: { propC: 'foobar' }
Status: Accept

App: { type: 'foo', probA: ['a', 'b', 'c'], probB: 42 }
Request: { type: [ 'foo', 'foobar'] }
Status: Accept - because type includes 1 value of request.type

App: { type: 'foo', probA: ['a', 'b', 'c'], probB: 42 }
Request: { type: [ 'foobar', 'foobar'] }
Status: Reject - no match of prop type

But maybe I miss some use-case. Can we try to find all the possible combinations?

An alternative can be to propose a different approach:

"filters": {
  "number": { type: "string", required: true },
  "type": { type: "fixed", value: "sms", required: false }
}

Where any property has:

. a type - string, fixed (with a value), number, etc.
. required - true/false
. min/max if number
. other parameters.

Comment 4

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

> The wallpaper app, for example, included filters on width and height so that
> it would only appear as a choice when the activity was requesting a 320x480
> image.  This meant that when the user wants to pick an image from the
> contacts app, they would see Gallery and Camera as choices, but not
> Wallpaper.  But now filtering is broken. If the contacts app does not
> specify a width and height then the system will not filter on those, even
> though the wallpaper app explicitly lists those as filters in its
> manifest.webapp file.

That's quite a hack though. First off, why should the wallpaper app be able to provide pictures? It doesn't seem common to ever be in a situation when you'd want to get a list of applications that can provide a picture and want to choose the wallpaper as that picture. Most likely the wallpaper is going to also be stored in gallery and so you could just pick it from there.

Second, in what cases would an application ask for exactly a 320x480 picture? Especially considering that different devices have different sized screens.

Why isn't the wallpaper app only acting as a consumer of images, rather than a producer and consumer?

> In general, it seens wrong and possibly very bad to take filtering power
> away from activity handlers.  Those filters are an app's way of declaring
> its preconditions. If the system ignores the filters, the app will likely be
> invoked with payloads that it does not know how to handle. Imagine an image
> upload app that implements the share activity an does a filter
> type:[image/jpeg, image/png].

I agree it's a problem. But the way things previously worked, where it was impossible to get all activities that can provide files of a certain type, or all activities that can provide files.

I'd love to hear ideas for how we can satisfy those types of usecases too.

> But failing that, we should discuss alternatives here (like maybe allow
> initiating apps to explicitly specify a wildcard for certain fields) instead
> of just omitting them.
> 
> And, once we have decided what to do, we should set the dev-doc-needed flag,
> so that this gets documented.

Sounds good.

So my first question is, other than the wallpaper problem, which seems fixable by simply making the wallpaper app not respond to "pick" activities at all, what usecases are currently broken?

Comment 5

[David Flanagan [:djf]]

(In reply to Mounir Lamouri (:mounir) from comment #2)
> (In reply to David Flanagan [:djf] from comment #1)
> > Cc'ing everyone who was cc'ed on #805292
> > 
> > I'll also add that I think it is okay to revert #805292 because it wouldn't
> > have worked as a file chooser anyway, because the pick activity returns
> > blobs, not files.
> 
> You forgot that there is some chrome code in-between which can create a
> temporary file from the Blob and use that file.

I wasn't talking about temporary files. I meant that to me a file chooser should exactly represent the state of the filesystem. The contents shown by a <input type=file> element should correspond to the files seen when the user does as USB mass storage session.

If you base a file chooser on pick activities, there will be some files that just won't be visible (because no app implements pick for them).  And there will be some things (like cropped images) that have no name and no permanent representation in the filesystem.

> > The way to do a file chooser
> > app is to leave activities alone and write a file chooser app that
> > implements a "choose-file" activity (or something like that). Give it
> > device-storage:sdcard permission and have it display the contents of the
> > /sdcard to the user.
> 
> I do not think that would be a good idea.

I suppose I can't debate it with you if you don't say why.  I'll note that our current device storage api would actually make this inefficient to do because it has no way to enumerate directories, only files.  So the picker would have to enumerate the entire sdcard at once, rather than just a directory at a time.

Comment 6

[David Flanagan [:djf]]

(In reply to Jonas Sicking (:sicking) from comment #4)

> That's quite a hack though. First off, why should the wallpaper app be able
> to provide pictures? It doesn't seem common to ever be in a situation when
> you'd want to get a list of applications that can provide a picture and want
> to choose the wallpaper as that picture. Most likely the wallpaper is going
> to also be stored in gallery and so you could just pick it from there.

I don't think you understand what the wallpaper app is.  Wallpaper is a hidden app that exists only to service Pick and Share activities.  The gallery app (by design, apparently) does not support folders: all images are in one big list, so mixing wallpapers in with photos does not make sense.  We need a way for the user to pick pre-defined system wallpapers.  The wallpaper app does that. If you press and hold on the homescreen to set the wallpaper, you initiate a pick activity.  And you are given options: Gallery, Camera, Wallpaper.

> Second, in what cases would an application ask for exactly a 320x480
> picture? Especially considering that different devices have different sized
> screens.

The Contacts app also uses the pick activity to get an image. I believe that users of the contacts app do not want to see wallpapers as a choice when they're trying to pick an image for a new contact. So I added the width and height filters on the wallpaper's pick activity, so that wallpaper only shows up when the requesting app explicitly requests an image that is the same size as the screen. Maybe it is a hack, but I think it is kind of elegant.  Note, however, that the width and height parameters are not just for this purpose. The contacts app wants a square image, so the gallery app accepts width and height parameters as part of the pick request and honors the requested aspect ratio when allowing the user to crop the image.

I agree that the hardcoded screen size is problematic, but I punted on it for v1.  One solution is that the wallpaper app could filter width to [320,480] and filter height to [480,800].  Another solution I considered as to make these properties strings and use CSS-style px and % values.


> Why isn't the wallpaper app only acting as a consumer of images, rather than
> a producer and consumer?

The wallpaper app also handles image share activities so if you share an image from Gallery, one of the choices you will have is setting it as your wallpaper.

> 
> > In general, it seens wrong and possibly very bad to take filtering power
> > away from activity handlers.  Those filters are an app's way of declaring
> > its preconditions. If the system ignores the filters, the app will likely be
> > invoked with payloads that it does not know how to handle. Imagine an image
> > upload app that implements the share activity an does a filter
> > type:[image/jpeg, image/png].
> 
> I agree it's a problem. But the way things previously worked, where it was
> impossible to get all activities that can provide files of a certain type,
> or all activities that can provide files.

First, I question whether that should even be a design goal of activities. Is not how I understood the system and in some ways it seems to invert the original design.

Second: I just want to be clear that we don't have any activities that "provide files".  We have various pick activities, and in general, these return objects to the requesting app.  Sometimes those objects are Blobs, and sometimes those Blobs have a permanent representation in the filesystem.  But sometimes they are cropped images that are not in the file system. And sometimes they're things like phone numbers and email addresses.

You're proposing that it is important to be able to use the Web Activities API to be able to make requests like: "Hey! I want to pick something. Please show me all the things I can pick". Maybe there is an argument to be made for adding an introspection API to activities so that we can ask for details about all pick handlers, but I don't see what it is.  Using a pick activity to implement a file chooser will fail in interesting ways when the user ends up picking a phone number instead of a file, for example.  And even it there is a need to be able to enumerate handlers, I don't see why we should try to force that into the MozActivity() constructor and significantly change the behavior of filtering.


> I'd love to hear ideas for how we can satisfy those types of usecases too.

I disagree with the need for the usecase. But if we're going to try to satisfy it, I'd suggest that we allow fields in the activity request to include wildcards. I believe that Andrea's patch makes an omitted field into a wildcard, and I don't like that. But if the wildcards were explicit, I wouldn't mind so much.
 
> > But failing that, we should discuss alternatives here (like maybe allow
> > initiating apps to explicitly specify a wildcard for certain fields) instead
> > of just omitting them.
> > 
> > And, once we have decided what to do, we should set the dev-doc-needed flag,
> > so that this gets documented.
> 
> Sounds good.
> 
> So my first question is, other than the wallpaper problem, which seems
> fixable by simply making the wallpaper app not respond to "pick" activities
> at all, what usecases are currently broken?

So given that you misunderstood the wallpaper app, this doesn't make sense...  Right now the wallpaper app works fine for picking wallpaper.  The contacts app specifies its desired width and height, and since that doesn't match the wallpaper's numbers, I think that wallpaper won't show up in the list of choices in the contacts app.  But any app that tries to pick an image without specifying a size will see wallpaper as a choice.  That is not what I intended, but it is not terrible.

My fundamental objection is (if I'm understanding correctly) that this change takes filtering power away from the privileged activity handler apps, and gives handler selection power to the less privileged activity requesting apps.  

Imagine a 3rd party app that initiates a Share activity without specifying a type property for it.  Under this new system, all share handlers, regardless of their type filter, will be presented to the user, even though most of them won't know how to handle the data being shared. Suppose the 3rd party app is a GPS tracker and it wants to share its saved routes in some proprietary binary format.  What is the wallpaper app going to do with that blob? (It will fail is some way. I didn't write it to defend against this sort of thing, so I'm not sure what it will do.) (Note that this is a footgun issue, not a security issue: a 3rd party app could of course send bad data with a purposely invalid type property.)  

Both the email app and the bluetooth transfer app should implement share activities that do not filter on the type. They can handle share requests for any type of data, so they would respond even to our hypothetical GPS app that omitted the type property. (I think the email app currently only supports image attachments, but that is a temporary restriction.)  But it doesn't make sense for the user to be given the option of sharing this mystery data with apps that have declared the type of data that they can handle.

Comment 7

[David Flanagan [:djf]]

(In reply to Andrea Marchesini (:baku) from comment #3)
> The current implementation "should" work in this way:
> 
> App: { type: 'foo', probA: ['a', 'b', 'c'], probB: 42 }
> Request: {}
> Status: Accepted - because no props are sent
> 

When you say App:, I assume that this is the list of filters specified in the manifest.webapp file, right?

This is really what I object to: that the requesting app can by pass the handler app's filters just by leaving out a property.  If that's the case its not a filter. I believe that almost all of the match examples you give would have not matched prior to your patch. So this isn't a minor tweak: it is a major change in the behavior of Web Activities, very late in the game, made with no consultation with the Gaia developers who are actually using the features.

I understand the motivation for the change: to try to implement a file picker with the pick activity.  (I don't think that file picker would have worked well, but I understand what the goal was.)  As I argue above, however, this completely breaks down when the data transfer goes the other way in a share activity.  If you ignore the filter like this, you can end up sending data to apps that don't know how to handle it.

> App: { type: 'foo', probA: ['a', 'b', 'c'], probB: 42 }
> Request: { propA: ['a','d'] }
> Status: Accepted - because propA matches at least 1 value

I've never seen arrays used in the request. I didn't know we could do that. Is that a new feature that you implemented?
 
If you're already allowing arrays in the request and are implementing matching based on those, it probably wouldn't be hard to allow wildcards in the request and write a match algorithm that handled wildcards. I still don't think we should do that, but it would be better than the current state in which just omitting a property makes it behave like a wildcard.

Comment 8

[Andrea Marchesini [:baku]]

> I don't think you understand what the wallpaper app is.  Wallpaper is a
> hidden app that exists only to service Pick and Share activities. 

If this is what the wallpaper app is, I think it's wrong to use the image size for filter it out.
We should use a different approach, but this is not related to the topic of this bug IMHO.

I see your point but I disagree with your approach.

The filter cannot be use to prevent misbehaviours of 3th party apps or to increase the security.
A 'filter' for me is a configuration, a settings, something that an application can handle or not.
Let's go by examples: wallpaper app has these filters:

        "type": ["image/jpeg", "image/png"],
        "width": 320,
        "height": 480

I see just 1 meaning: The this app is able to pick images (jpeg or png) size 320x480. So if you want to use this app for something else, this app cannot be used.
So if I specify a different width/height, this app will not be selected, but if I don't, or if I specify 320x480, then this app will be selectable.

I think this is the right way to read filters. Just because, what about if tomorrow I write an app that can pick images with these filters:

        "type": ["image/jpeg", "image/png"],
        "width": 300,
        "height": 300

If we revert the patch we cannot use this new app for selecting the background of the desktop because the mozActivity must contain the width/height for the wallpaper or for this new app. This seems wrong to me.

Several times I proposed a more complex configuration where the filters have attributes as such as 'required', 'type', etc.
Maybe we can speak about this too.

I would like to move the conversation out of bugzilla. What about a meeting?

Comment 9

[David Flanagan [:djf]]

Andrea,

Your argument makes sense for pick activities, but if you turn it around and think about share activitis, it does not work.  Imagine an app that wants to accept shared images, but (for whatever reason) it can only handle 320x480 images.  So it declares filters for width and height on its share activity.  This means "don't invoke me unless the image you are passing me is 320x480".  But under your new filtering rules the system will just ignore that if the requesting app does not specify those parameters.

Your new filtering rules arguably work better for pick, but they work worse for share. The wallpaper app was relying on the old filtering rules, and was intentionally using them to prevent the wallpaper app from being listed in the pick menu except when the homescreen or settings app invoked the pick with that specific image size.  I think it was an elegant way to finesse the existing filtering rules.  Now my finesse doesn't work anymore.

The wallpaper app isn't that big a deal, and I don't want this bug to be about that.

I can see the argument (though I disagree) that the new rules improve the pick activity. But I think they harm the share activity.

(In reply to Andrea Marchesini (:baku) from comment #8)

> 
> Several times I proposed a more complex configuration where the filters have
> attributes as such as 'required', 'type', etc.
> Maybe we can speak about this too.

What do you think of my idea to revert to the old rules but allow '*' as a value in the request to do the kind of wildcard matching you want?

> 
> I would like to move the conversation out of bugzilla. What about a meeting?

Andrew has tried to schedule one.  It will only make sense if Mounir and Jonas can attend, since I don't think you or I have any decision making power over this API. Frankly, though, I really don't have time to think about the design at this point. I just want a stable API.

Comment 10

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

What we need for both pick and share activities is for the consumer of the data to be able to select what type of data can be consumed. The reason the two types has different requirements here is that data flows in opposite directions for the two types of activities.

For the share activity the handler is the consumer and the caller is the producer. Here the handler needs to be able to apply strict filters indicating which types the handler can consume.

For she pick activity the handler is the producer and the caller is the consumer. Here the caller needs to be able to apply strict filters indicating which types the caller can consume.


Neither the method that we used to have, or the method that we currently has, is able to solve both scenarios. Can we agree on that and try to come up with a mechanism that can handle both directions of data flow?

Comment 11

[David Flanagan [:djf]]

(In reply to Jonas Sicking (:sicking) from comment #10)
> What we need for both pick and share activities is for the consumer of the
> data to be able to select what type of data can be consumed. The reason the
> two types has different requirements here is that data flows in opposite
> directions for the two types of activities.
> 
> For the share activity the handler is the consumer and the caller is the
> producer. Here the handler needs to be able to apply strict filters
> indicating which types the handler can consume.

And the handler does that with the declaration in manifest.webapp. It worked with the old filtering system, and is broken in the new system.

> 
> For she pick activity the handler is the producer and the caller is the
> consumer. Here the caller needs to be able to apply strict filters
> indicating which types the caller can consume.

We don't have strict filtering here. When the contacts app picks and image, it includes its desired width and height dimensions. But in both the new and the old filtering schemes it is entirely optional for the handler to honor those dimensions. If I understand you correctly, Jonas, I think you're saying that the contacts app ought to be able to limit this activity to handlers that have declared in their manifest that they will honor the width and height activities.

So no, we don't have that, in either the old or the new filtering system. (And I don't think that anyone has actually requeested it either.)

IIRC, the new filtering system is all about making the filtering less strict, and its justifying use case is the ability to create a file chooser with a pick activity that has no type specified. I think that satisfying that use case is not actually a reasonable design goal for Web Activities.

> 
> Neither the method that we used to have, or the method that we currently
> has, is able to solve both scenarios. Can we agree on that and try to come
> up with a mechanism that can handle both directions of data flow?

I worry a little that a fully general bi-direcitonal filtering scheme would end up being complex and hard to understand.  I've been as involved as any gaia developer in working with activities, and I'm not convinced that there is a need for anything more than what we had.

I'm also very, very concerned about the schedule. Jonas, is this something you think we can and should solve for v1?  I don't see myself having time to work on in in that time frame.

Given that the file chooser that was the justifying use case for the new filtering system has been blocking minused, I suggest we just revert to the old system and they try a redesign in the v2 timeframe.

Comment 12

[Kai-Chih Hu [:khu]]

B2G v1.0 won't provide file chooser. It should be not a blocker.

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to David Flanagan [:djf] from comment #5)
> (In reply to Mounir Lamouri (:mounir) from comment #2)
> > (In reply to David Flanagan [:djf] from comment #1)
> > > The way to do a file chooser
> > > app is to leave activities alone and write a file chooser app that
> > > implements a "choose-file" activity (or something like that). Give it
> > > device-storage:sdcard permission and have it display the contents of the
> > > /sdcard to the user.
> > 
> > I do not think that would be a good idea.
> 
> I suppose I can't debate it with you if you don't say why.

Sorry, I should have been more verbose here. I like what is usually done on Android and I think it is working pretty well. It is using 'pick' intent extensively to show the best list of activities. For a regular file picker, it will be 'pick' with no type specified and if you have things like <input type='file' accept='image/*'>, it will be a pick for images only. This can even be extended to add some activities like taking a picture or recording a sound/video. I do not think mobile phones usually have a file browser and using such thing, I believe, would not be quite as good.

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to khu from comment #12)
> B2G v1.0 won't provide file chooser. It should be not a blocker.

Kevin, did you read what this bug was about before removing the bb+? This is not related to the file chooser. There was a change in bug 805292 that changed how the filtering is handled and this seems to have changed some activities behaviour.

Jonas, David, Andrea and I had a chat on Monday regarding this issue and we found a solution that the four of us think is doable. Making this bug no longer blocking means that no priority should be given in fixing any issue Gaia is currently having with activities filtering and generally speaking, keep a poor filter mechanism for all apps in the wild.

Comment 15

[Kai-Chih Hu [:khu]]

(In reply to Mounir Lamouri (:mounir) from comment #14)
> (In reply to khu from comment #12)
> > B2G v1.0 won't provide file chooser. It should be not a blocker.
> 
> Kevin, did you read what this bug was about before removing the bb+? This is
> not related to the file chooser. There was a change in bug 805292 that
> changed how the filtering is handled and this seems to have changed some
> activities behaviour.
> 
> Jonas, David, Andrea and I had a chat on Monday regarding this issue and we
> found a solution that the four of us think is doable. Making this bug no
> longer blocking means that no priority should be given in fixing any issue
> Gaia is currently having with activities filtering and generally speaking,
> keep a poor filter mechanism for all apps in the wild.

Hi, Mounir, sorry for the confusion. I discussed this with Thinker, Swu and Keven Kuo. Thinker told me it's related to file chooser. So, I think, we misunderstood here. Since you have better view than us, I will nominate this back as bb+.

Comment 16

[Alex Keybl [:akeybl]]

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch

This patch implements the filter how we discussed. I created a separated module in order to make easier the implementation of unit tests.
Please check if all the scenarios are covered.

Comment 18

[David Flanagan [:djf]]

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

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

ActivityServiceFilter.jsm looks mostly good to me, but see my individual comments on that file; I think there are a few minor changes you should make.  One open question: do we want to support regexp flags?

I think there are a couple more tests you should add to test multiple incoming values.

I would really like to see documentation of the filter rules, though I wouldn't withold r+ for that.

I'm not a gecko engineer, and am not qualified to review all the Makefiles and xpcshell.ini files.

I'm also not currently able to build gecko. My tree is badly broken somehow and I don't have time to figure it out.  So I can't test this patch.  So before I give r+, please describe how you have tested it.  In particular, have you verified that it does not break any of the activities that are used in Gaia?  (I think the plan is to modify Gaia to use the improved filters once this has landed, but I hope we can land it without breaking any existing activities.)

::: dom/activities/src/ActivitiesServiceFilter.jsm
@@ +22,5 @@
> +      if (('rangeMax' in aFilterObj) && aFilterObj.rangeMax < aValue) {
> +        return false;
> +      }
> +
> +      // The last check is the comparation.

I'm not sure what you're trying to say with this comment.
English nit: comparison

Same comment for the next function, too.

@@ +33,5 @@
> +
> +    function matchValueString(aValue, aFilter, aFilterObj) {
> +      // Regexp.
> +      if (('regexp' in aFilterObj) && aFilterObj.regexp) {
> +        var re = new RegExp(aFilter);

Do we need a way to specify regexp flags like i for case-insensitive?

@@ +34,5 @@
> +    function matchValueString(aValue, aFilter, aFilterObj) {
> +      // Regexp.
> +      if (('regexp' in aFilterObj) && aFilterObj.regexp) {
> +        var re = new RegExp(aFilter);
> +        return !!re.exec(aValue);

I think re.test(value) would be better here

@@ +66,5 @@
> +    // this function returns true if the value matches with the filter object
> +    function matchObject(aValue, aFilterObj) {
> +
> +      if (!('value' in aFilterObj)) {
> +        aFilterObj.value = null;

Is this a private copy of the filter object? Is it okay to modify it here?  I don't know where this match function is being invoked from and if the caller cares whether the filter objects are modified like this.

How about just doing

let filtervalue = ('value' in aFilterObj) ? aFilterObj.value : null;

And then below using filtervalue.  Would that work without modifying the object?

@@ +77,5 @@
> +      let ret = false;
> +      filters.forEach(function(filter) {
> +        values.forEach(function(value) {
> +          if (matchValue(value, filter, aFilterObj)) {
> +            ret = true;

If you use for loops instead of forEach(), then you could just return immediately here and skip any remaining interations and tests.

::: dom/activities/tests/test_activityFilters.js
@@ +114,5 @@
> +  do_check_eq(ActivitiesServiceFilter.match({a: 'aafoobar'},
> +                                            {a: { required: true, value: '^foobar', regexp: true}}), false);
> +  do_check_eq(ActivitiesServiceFilter.match({a: 'aafoobarasdsad'},
> +                                            {a: { required: true, value: 'foo', regexp: true}}), true);
> +}

You don't have any tests that test multiple input values. The first argument is always an object with one property.

Comment 19

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

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

I'd like to get Mounir's review on this too

Comment 20

[Andrea Marchesini [:baku]]

> Do we need a way to specify regexp flags like i for case-insensitive?

What about:
{a: { required: true, regexp: 'foo', regexpFlags: 'i'}})

> You don't have any tests that test multiple input values. The first argument
> is always an object with one property.

What do you mean? My idea is that a property can have an array of values. E.g.:

  do_check_eq(ActivitiesServiceFilter.match({a: [4, 'foobar']},
                                            {a: 'foobar', b: [1, 2, 3], c: 42}), true);

Comment 21

[Andrea Marchesini [:baku]]

Attachment: patch b

Comment 22

[David Flanagan [:djf]]

(In reply to Andrea Marchesini (:baku) from comment #20)
> > Do we need a way to specify regexp flags like i for case-insensitive?
> 
> What about:
> {a: { required: true, regexp: 'foo', regexpFlags: 'i'}})

That sounds good to me.

> > You don't have any tests that test multiple input values. The first argument
> > is always an object with one property.
> 
> What do you mean? My idea is that a property can have an array of values.
> E.g.:
> 
>   do_check_eq(ActivitiesServiceFilter.match({a: [4, 'foobar']},
>                                             {a: 'foobar', b: [1, 2, 3], c:
> 42}), true);

I mean you don't have tests where the first argument is something like:

{a: 'foobar', b: 2, c: true}

Comment 23

[Andrea Marchesini [:baku]]

Attachment: patch c

new tests added

Comment 24

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

How does filtering on sub-properties work with this patch? I.e. can I write a filter that says that the "pet.type property must have the value 'cat'"?

I *think* that works in the current implementation, so it would be nice to not lose that ability. If the syntax is

filter: {
  "pet.type": { required: true, value: "cat" }
}

or

filter: {
  pet: {
    type: { required: true, value: "cat" }
  }
}

I care less about.

Comment 25

[Andrea Marchesini [:baku]]

> filter: {
>   "pet.type": { required: true, value: "cat" }
> }

This works because 'pet.type' is a string and this string is used as property of the filter object.

Comment 26

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

The task is to match an object like:

{
  ...
  pet: {
    name: "mittens",
    superpower: "cuteness",
    battlecry: "meow",
    type: "cat"
  }
  ...
}


Would a filter like:

filter: {
  "pet.type": { required: true, value: "cat" }
}

match that?

Comment 27

[Andrea Marchesini [:baku]]

> I *think* that works in the current implementation, so it would be nice to
> not lose that ability. If the syntax is

This is not supported by my patch but I don't think it works in the current implementation either. Mounir, feedback?

Comment 28

[Dietrich Ayala (:dietrich)]

No response from Mounir, who else can review?

Comment 29

[Dietrich Ayala (:dietrich)]

Also, can you please summarize the user impact of this bug?

Comment 30

[Andrea Marchesini [:baku]]

(In reply to Dietrich Ayala (:dietrich) from comment #29)
> Also, can you please summarize the user impact of this bug?

This patch should be retro-compatible.
Maybe Jonas can review it?

Comment 31

[Mounir Lamouri (:mounir)]

Comment on attachment 691414 [details] [diff] [review]
patch c

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

Sorry for the delay. I was on vacation and fully offline last week. I got back online yesterday and had to deal with my inboxes before processing my bugmails.

First thing, thank you for writing a test :) This is very appreciated :) However, you should move the xpcshell tests to test/unit/, this is the convention.

I reviewed the tests as much as the code because usually tests tell a lot about the code behaviour. Sometimes, it is clearer than the actual code. So, please, read as carefully the comments on the tests and on the code.

Unfortunately, there are a lot of things that I would like to see fixed and a few that might need to be discussed.

Could you update your patch and re-ask for a review?

::: dom/activities/Makefile.in
@@ +13,5 @@
>  PARALLEL_DIRS = interfaces src
>  
> +ifdef ENABLE_TESTS
> +XPCSHELL_TESTS = tests
> +endif

Don't add XPCSHELL_TESTS here but:
TEST_DIRS += test

::: dom/activities/src/ActivitiesServiceFilter.jsm
@@ +8,5 @@
> +
> +this.ActivitiesServiceFilter = {
> +  match: function(aValues, aFilters) {
> +    function matchValueBoolean(aValue, aFilter, aFilterObj) {
> +      return typeof(aFilter) == 'boolean' && aValue == !!aFilter;

|... && aValue == aFilter| should be enough given that we know that aValue and aFilter are of type boolean.

Actually, you could simply do:
  return aValue === aFilter;
I believe.

@@ +24,5 @@
> +      }
> +
> +      // Comparing values only if the type matches.
> +      if (typeof(aFilter) == 'number') {
> +        return aValue == aFilter;

That is very unclear IMO. If there was no tests, I would have told you this can't work.

I think a better way to do this would be to, at the beginning of the function, do:
  // If a filter 'value' is specified, we should just compare it to the given value.
  if (aFilter !== null) {
    return aFilter === aValue);
  }

@@ +27,5 @@
> +      if (typeof(aFilter) == 'number') {
> +        return aValue == aFilter;
> +      }
> +
> +      return (aFilter == null);

That should be:
  return true;

@@ +40,5 @@
> +      }
> +
> +      // Comparing values only if the type matches.
> +      if (typeof(aFilter) == 'string') {
> +        return aValue == aFilter;

Can't you do:
  return aValue === aFilter;

Without the condition.

@@ +59,5 @@
> +      case 'string':
> +        return matchValueString(aValue, aFilter, aFilterObj);
> +
> +      default: // not supported
> +        return false;

nit: the coding style of this method looks weird but I don't really know our JS coding style so I will not bother you :)

@@ +69,5 @@
> +
> +      // Let's consider anything an array.
> +      let filters = ('value' in aFilterObj) ?
> +                      (Array.isArray(aFilterObj.value) ? aFilterObj.value : [aFilterObj.value]) :
> +                      [ null ];

Argh, this is hard to read.
What about:
      let filters = ('value' in aFilterObj)
                      ? (Array.isArray(aFilterObj.value) ? aFilterObj.value
                                                         : [aFilterObj.value])
                      : [ null ];
Given that bugzilla's font is very likely going to break the identation, note that the idea is to have ? and : aligned vertically so you know at a glance what matches with what.

::: dom/activities/tests/Makefile.in
@@ +1,2 @@
> +# vim: noexpandtab ts=8 sw=8
> +#

Please, remove those two lines.

@@ +7,5 @@
> +DEPTH		= @DEPTH@
> +topsrcdir	= @top_srcdir@
> +srcdir		= @srcdir@
> +VPATH		= @srcdir@
> +relativesrcdir = @relativesrcdir@

nit: align the '='.

@@ +8,5 @@
> +topsrcdir	= @top_srcdir@
> +srcdir		= @srcdir@
> +VPATH		= @srcdir@
> +relativesrcdir = @relativesrcdir@
> +FAIL_ON_WARNINGS := 1

nit: FAIL_ON_WARNINGS in a test directory isn't incredibly useful but doesn't hurt too...

@@ +13,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +DIRS = \
> +  $(NULL)

You do not need that.

@@ +16,5 @@
> +DIRS = \
> +  $(NULL)
> +
> +MOCHITEST_FILES = \
> +	$(NULL)

I doubt you need that.

However, you need:
XPCSHELL_TESTS = unit

And create a "unit" directly with the xpcshell.ini file and the test file (which is going to require an update of testing/xpcshell/xpcshell.ini).

::: dom/activities/tests/test_activityFilters.js
@@ +7,5 @@
> +  do_check_true(!!ActivitiesServiceFilter);
> +
> +  // No input, no filters:
> +  do_check_eq(ActivitiesServiceFilter.match(null, null), true);
> +  do_check_eq(ActivitiesServiceFilter.match({}, {}), true);

In the entire file, you use:
do_check_eq(..., true);
do_check_eq(..., false);

Instead, you should use:
do_check_true(...);
do_check_false(...);

It is way more readable.

@@ +12,5 @@
> +
> +  // No filters:
> +  do_check_eq(ActivitiesServiceFilter.match({foobar: 42}, null), true);
> +
> +  // Empty request:

nit: you call this "request" here. "input" a few lines earlier and "data" in the jsm file. Clear and consistent naming is important.

@@ +33,5 @@
> +                                            {a: 'foobar', b: [1, 2, 3], c: 42}), true);
> +  do_check_eq(ActivitiesServiceFilter.match({b: 4},
> +                                            {a: 'foobar', b: [1, 2, 3], c: 42}), false);
> +  do_check_eq(ActivitiesServiceFilter.match({b: [2, 4]},
> +                                            {a: 'foobar', b: [1, 2, 3], c: 42}), true);

That's interesting. [2, 4] matches [1, 2, 3] because 2 matches 2. I don't know if that is what we want or not. I don't really have an opinion but I would be interested to know why you chose that.

@@ +39,5 @@
> +                                            {a: 'foobar', b: [1, 2, 3], c: 42}), false);
> +  do_check_eq(ActivitiesServiceFilter.match({a: [4, 'foobar2']},
> +                                            {a: 'foobar', b: [1, 2, 3], c: 42}), false);
> +  do_check_eq(ActivitiesServiceFilter.match({a: [4, 'foobar']},
> +                                            {a: 'foobar', b: [1, 2, 3], c: 42}), true);

Same here.

@@ +63,5 @@
> +                                            {a: { required: true, value: ['a', 'b', 'foobar']}}), true);
> +  do_check_eq(ActivitiesServiceFilter.match({a: ['k', 'z', 'foobar2']},
> +                                            {a: { required: true, value: ['a', 'b', 'foobar']}}), false);
> +
> +  // Empty

The comment doesn't really match the two tests. Only the second one.

@@ +75,5 @@
> +                                            {a: { required: true, value: false}}), true);
> +  do_check_eq(ActivitiesServiceFilter.match({a: true},
> +                                            {a: { required: true, value: false}}), false);
> +  do_check_eq(ActivitiesServiceFilter.match({a: [false, true]},
> +                                            {a: { required: true, value: false}}), true);

... here, it is even weird.

@@ +103,5 @@
> +                                            {a: { required: true, rangeMin: 1, rangeMax: 2}}), true);
> +  do_check_eq(ActivitiesServiceFilter.match({a: 2},
> +                                            {a: { required: true, rangeMin: 2, rangeMax: 2}}), true);
> +  do_check_eq(ActivitiesServiceFilter.match({a: 2},
> +                                            {a: { required: true, value: 'foo'}}), false);

Could you add a test where rangeMin > rangeMax. What should we do in that case? I think we could ignore the rangeMin/rangeMax restrictions.

Also, there is no reason a user should pass:
{ a: 2 }
and not:
{ a: '2' }
This is too restrictive.

Could you add tests with rangeMin and rangeMax that take numbers into string?
You could simply convert the string to a number in JS in the code handling those cases.

@@ +109,5 @@
> +  // String
> +  do_check_eq(ActivitiesServiceFilter.match({a: 'foo'},
> +                                            {a: { required: true, value: 'foo'}}), true);
> +  do_check_eq(ActivitiesServiceFilter.match({a: 'foo2'},
> +                                            {a: { required: true, value: 'foo'}}), false);

Could you add tests where rangeMin and rangeMax are used with random string like 'foo' or 'bar' and also one with rangeMin=0, rangeMax=255 and a being a single character?

@@ +119,5 @@
> +                                            {a: { required: true, regexp: '^foobar'}}), false);
> +  do_check_eq(ActivitiesServiceFilter.match({a: 'aafoobarasdsad'},
> +                                            {a: { required: true, regexp: 'foo'}}), true);
> +  do_check_eq(ActivitiesServiceFilter.match({a: 'aaFOOsdsad'},
> +                                            {a: { required: true, regexp: 'foo', regexpFlags: 'i'}}), true);

A few comments regarding regexps:
- what if 'value' is also present?
- what about requesting the regexp on the form "/pattern/flag"?

I wonder if having regexp with the following format wouldn't be simpler:
a: { regexp: true, value: '/foo/i' }

If the leading and ending '/' are not present, we can just add them when creating the regexp. IOW, that:
a: { regexp: true, value: 'foo' }
would be equivalent to:
a: { regexp: true, value: '/foo/' }

Comment 32

[Andrea Marchesini [:baku]]

> Could you update your patch and re-ask for a review?

Sure.

> > +this.ActivitiesServiceFilter = {
> > +  match: function(aValues, aFilters) {
> > +    function matchValueBoolean(aValue, aFilter, aFilterObj) {
> > +      return typeof(aFilter) == 'boolean' && aValue == !!aFilter;
> 
> |... && aValue == aFilter| should be enough given that we know that aValue
> and aFilter are of type boolean.

This is not true:

1. aFilter can be null if the request is: { foo: { required: true }}
   This would mean: the 'foo' property is required, but it doesn't
   matter the value.

2. we can have a value that doesn't match with the filter type:
   Request: { a: true }
   Filter: { a: { value: 'foobar' } }

> @@ +24,5 @@
> > +      }
> > +
> > +      // Comparing values only if the type matches.
> > +      if (typeof(aFilter) == 'number') {
> > +        return aValue == aFilter;
> 
> That is very unclear IMO. If there was no tests, I would have told you this
> can't work.

This is what I meant with the point 2:

   Request: { a: 'foobar' }
   Filter: { a: { value: 42 } }

> > +      return (aFilter == null);
> 
> That should be:
>   return true;

I don't think so. Example:

   Request: { a: 'foobar' }
   Filter: { a: { value: 42 } }

=> false;

we should return true only if the filter value is missing:

   Request: { a: 'foobar' }
   Filter: { a: {} }

=> true;

> > +  do_check_eq(ActivitiesServiceFilter.match({b: [2, 4]},
> > +                                            {a: 'foobar', b: [1, 2, 3], c: 42}), true);
> 
> That's interesting. [2, 4] matches [1, 2, 3] because 2 matches 2. I don't
> know if that is what we want or not. I don't really have an opinion but I
> would be interested to know why you chose that.

Yes. I see what you mean, but take a look of this example:

1. an app has a filter for image/png, image/jpeg
   { type: [ 'image/png', 'image/jpeg' ]}

2. we send a request for having image/jpeg or image/svg
   { type: [ 'image/jpeg', 'image/svg' ]}

To me, this matches because there is at least 1 type matching the request with the filter.
David, what do you think about?

> I wonder if having regexp with the following format wouldn't be simpler:
> a: { regexp: true, value: '/foo/i' }

Unfortunately if I do:
var re = new RegExp('/foo/i');
console.log(re); # this prints //foo/i/

I'm going to upload a new version.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: patch d

Comment 34

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

I suspect the best way to do regexps is to do something like:

{ required: true, regexp: "^foopy", regexpFlags: "mi" }

Comment 35

[Andrea Marchesini [:baku]]

(In reply to Jonas Sicking (:sicking) from comment #34)
> I suspect the best way to do regexps is to do something like:
> 
> { required: true, regexp: "^foopy", regexpFlags: "mi" }

What about if the filter is:

{ required: true, regexp: "^foopy", regexpFlags: "mi", value: "foopyfoo" } ?

Should we ignore the value?

Comment 36

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

Ideally we should flag that as an invalid manifest.

But yes, let's ignore the value (or the regexp, I don't really care which), for now.

Comment 37

[Mounir Lamouri (:mounir)]

(In reply to Andrea Marchesini (:baku) from comment #32)
> > > +this.ActivitiesServiceFilter = {
> > > +  match: function(aValues, aFilters) {
> > > +    function matchValueBoolean(aValue, aFilter, aFilterObj) {
> > > +      return typeof(aFilter) == 'boolean' && aValue == !!aFilter;
> > 
> > |... && aValue == aFilter| should be enough given that we know that aValue
> > and aFilter are of type boolean.
> 
> This is not true:
> 
> 1. aFilter can be null if the request is: { foo: { required: true }}
>    This would mean: the 'foo' property is required, but it doesn't
>    matter the value.
> 
> 2. we can have a value that doesn't match with the filter type:
>    Request: { a: true }
>    Filter: { a: { value: 'foobar' } }

Argh. It's very confusing that aFilter can be null.
What about:
  return aFilter === null || aFilter == aValue;

> > @@ +24,5 @@
> > > +      }
> > > +
> > > +      // Comparing values only if the type matches.
> > > +      if (typeof(aFilter) == 'number') {
> > > +        return aValue == aFilter;
> > 
> > That is very unclear IMO. If there was no tests, I would have told you this
> > can't work.
> 
> This is what I meant with the point 2:
> 
>    Request: { a: 'foobar' }
>    Filter: { a: { value: 42 } }
> 
> > > +      return (aFilter == null);
> > 
> > That should be:
> >   return true;
> 
> I don't think so. Example:
> 
>    Request: { a: 'foobar' }
>    Filter: { a: { value: 42 } }
> 
> => false;
>
> we should return true only if the filter value is missing:
> 
>    Request: { a: 'foobar' }
>    Filter: { a: {} }
> 
> => true;

I think what I propose would work. It is basically:
  if (aFilter !== null) {
    return aFilter === aValue;
  }

  // range check do early |return false;|

  return true

> > I wonder if having regexp with the following format wouldn't be simpler:
> > a: { regexp: true, value: '/foo/i' }
> 
> Unfortunately if I do:
> var re = new RegExp('/foo/i');
> console.log(re); # this prints //foo/i/

Indeed. You can't implement that that way but that doesn't make this syntax not implementable. I still think it would be better: it is more intuitive and would prevent the ambiguity of having a regexp and a value that would put us in an unpredictable state.
Unless the syntax seems worse than the one we have, I think we should do that.

Comment 38

[Mounir Lamouri (:mounir)]

Comment on attachment 693461 [details] [diff] [review]
patch d

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

Those comments are mostly nits but you didn't attach the test and some behaviour still need to be defined.

::: dom/activities/Makefile.in
@@ +10,4 @@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
> +PARALLEL_DIRS = interfaces src tests

Use TEST_DIRS, please.

::: dom/activities/src/ActivitiesServiceFilter.jsm
@@ +8,5 @@
> +
> +this.ActivitiesServiceFilter = {
> +  match: function(aValues, aFilters) {
> +    function matchValueBoolean(aValue, aFilter, aFilterObj) {
> +      return typeof(aFilter) == 'boolean' && aValue == !!aFilter;

I think that would be more understandable:
  return aFilter === null || aFilter === aValue;

@@ +12,5 @@
> +      return typeof(aFilter) == 'boolean' && aValue == !!aFilter;
> +    }
> +
> +    function matchValueNumber(aValue, aFilter, aFilterObj) {
> +      // Validation of the rangeMin/Max.

Before checking the range, I think you could do:
if (aFilter !== null) {
  return aFilter === aValue;
}

@@ +33,5 @@
> +        }
> +      }
> +
> +      // Comparing values only if the type matches.
> +      if (typeof(aFilter) == 'number') {

... then, instead of that, just:
return true;

@@ +42,5 @@
> +    }
> +
> +    function matchValueString(aValue, aFilter, aFilterObj) {
> +      // Regexp.
> +      if (aFilter && ('regexp' in aFilterObj) && aFilterObj.regexp) {

With the current implementation, you could simply do:
if (aFilter === null) {
  return true;
}

@@ +50,5 @@
> +      }
> +
> +      // Comparing values only if the type matches.
> +      if (typeof(aFilter) == 'string') {
> +        return aValue == aFilter;

... then:
return aFilter === aValue;

Comment 39

[Andrea Marchesini [:baku]]

Attachment: patch e

Comment 40

[Mounir Lamouri (:mounir)]

Comment on attachment 693849 [details] [diff] [review]
patch e

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

The code is okay but you ignored the comments related to the general behaviour:
- I think regexp should be declared this way:
{ regexp: true, value='/foo/i' } or if there is no '/' (like in |value=foo|), we should consider that the value has a leading and a trailing slash (previous example would give |value=/foo/|). In my opinion, doing so would prevent any ambiguity regarding having a regexp and value property;
- I think we shouldn't put too much constraint in the type. IOW, those three data/filter tuples should always match:
{ a='42' }, { a: { value=42 } }
{ a=42 }, { a: { value='42' } }
{ a=42 }, { a; { min='1' max=50 } }

BTW, why do you use rangeMin/rangeMax and not min/max? We should use the later unless there is a reason.

::: dom/activities/Makefile.in
@@ +11,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  
>  PARALLEL_DIRS = interfaces src
>  
> +TEST_DIRS = tests

TEST_DIRS += test, see comment 31

::: dom/activities/src/ActivitiesServiceFilter.jsm
@@ +52,5 @@
> +                            ('regexpFlags' in aFilterObj ? aFilterObj.regexpFlags : ''));
> +        return re.test(aValue);
> +      }
> +
> +      return (aFilter === aValue);

nit: parenthesis are not needed.

::: dom/activities/tests/unit/test_activityFilters.js
@@ +38,5 @@
> +                                               {a: 'foobar', b: [1, 2, 3], c: 42}));
> +  do_check_false(ActivitiesServiceFilter.match({a: [4, 'foobar2']},
> +                                               {a: 'foobar', b: [1, 2, 3], c: 42}));
> +  do_check_true(ActivitiesServiceFilter.match({a: [4, 'foobar']},
> +                                              {a: 'foobar', b: [1, 2, 3], c: 42}));

Could you add a this (that might require changing the comment on top of this block):
do_check_true(ActivitiesServiceFilter.match({a: ['foo', 'bar']},
                                            {a: 'foo'}));

Comment 41

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

I prefer

foo: { regexp: "foopy" },

over

foo: { regexp: true, value: "/foopy/" }


Pros with using the first syntax:
* Less to type.
* It is in my opinion more clear to have a separate parameter when doing a
  separate operation, than having having "mode" parameters. Having "mode"
  parameters requires seeing and understanding multiple parameters. If you miss
  the "mode" parameter in the second syntax, or don't understand it, you'll
  interpret the filter as having a completely different meaning. If you miss,
  or don't understand the regexp parameter in the first syntax, it's clear that
  you need to read up on how it works.
* It requires less parsing since we don't have to strip the slashes.

Pros with using the latter syntax:
* There are fewer ways of expressing invalid rules.

I agree that it is a real downside that the former syntax can express invalid rules in more ways, but I think the advantages are bigger.

Comment 42

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

I agree that we shouldn't pay too much attention to types though. Javascript tends to treat "42" and 42 fairly equivalent. And they are treated entirely equivalent when doing greater-than/less-than compares.

If we want strict type checking then I think that should be an opt-in thing. And something we should do later if needed.

Comment 43

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #41)
> I prefer
> 
> foo: { regexp: "foopy" },
> 
> over
> 
> foo: { regexp: true, value: "/foopy/" }
> 
> 
> Pros with using the first syntax:
> * Less to type.
> * It is in my opinion more clear to have a separate parameter when doing a
>   separate operation, than having having "mode" parameters. Having "mode"
>   parameters requires seeing and understanding multiple parameters. If you
> miss
>   the "mode" parameter in the second syntax, or don't understand it, you'll
>   interpret the filter as having a completely different meaning. If you miss,
>   or don't understand the regexp parameter in the first syntax, it's clear
> that
>   you need to read up on how it works.
> * It requires less parsing since we don't have to strip the slashes.
> 
> Pros with using the latter syntax:
> * There are fewer ways of expressing invalid rules.
> 
> I agree that it is a real downside that the former syntax can express
> invalid rules in more ways, but I think the advantages are bigger.

You are not taking into account the flags. My proposal makes adding flag easy because you can simply do: value = "/foopy/i" while the former proposal forces us to add a regexpFlag property.

Anyway, I'm not fighting for that. I am fine with having you doing the referee so we don't waste too much time.

Andrea, could you do the two other changes, that means change range{Min,Max} to {min,max} and makes the type less strict? :)

Comment 44

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

We could still do the "/foo/i" thing even if we use only a regexp property. That would remove the last advantage of the ones I listed, but the other two would still remain.

But given how tricky regexp's are to parse, I think I'd prefer not to do it. Even though we technically wouldn't have to actually parse the regexp part itself, just the flags. But I don't feel particularly strongly about that.

So either { regexp: "foo", regexpFlags: "i" } or { regexp: "/foo/i/" } are fine with me.

Comment 45

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

Err.. that should be:

Either { regexp: "foo", regexpFlags: "i" } or { regexp: "/foo/i" } are fine with me.

Comment 46

[Andrea Marchesini [:baku]]

Attachment: patch f

last review, please?

Comment 47

[Mounir Lamouri (:mounir)]

Comment on attachment 694375 [details] [diff] [review]
patch f

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

The way you decide to call matchNumber or matchString seems a bit conplex. What about doing this:
- if typeof() is number, just call matchNumber.
- if typeof() is string but parseInt(aValue) != NaN, call matchNumber with parseInt(aValue) instead of aValue. Otherwise call matchString.
- in matchString, if aFilter !== null, do: aFilter = parseInt(aFilter). If aFilter === NaN , return false. Otherwise continue as usual.

Regarding regexp, I'm not a big fan of allowing:
{ foo: { regexp: "/foo/ie", value: "foobar", regexFlags: "blah" } }
That seems a lot of ways to have unexpected behaviour (value + regexp ; regexp + regexpFlags).
However, I will let Jonas decide.

Could you attach a patch with the parseInt() usage changed as described above? It should get r+.

::: dom/activities/src/ActivitiesServiceFilter.jsm
@@ +19,5 @@
> +        return aFilter === aValue;
> +      }
> +
> +      // Validation of the min/Max.
> +      if (!('min' in aFilterObj) ||

Add a comment saying that it is magically working with parseInt(min) or parseInt(max) == NaN.

::: dom/activities/tests/unit/test_activityFilters.js
@@ +108,5 @@
> +                                               {a: { required: true, value: 'foo'}}));
> +  do_check_true(ActivitiesServiceFilter.match({a: 2},
> +                                              {a: { required: true, min: 100, max: 0}}));
> +  do_check_true(ActivitiesServiceFilter.match({a: 2},
> +                                              {a: { required: true, min: 'a', max: 'b'}}));

Add those tests, please:
{a: '2'}, {a: 2} => true
{a: 2}, {a: '2'} => true
{a: 2}, {a: {min: '4'}} => false
{a: 2}, {a: {max: '0'}} => false

@@ +134,5 @@
> +                                              {a: { required: true, regexp: 'foo'}}));
> +  do_check_true(ActivitiesServiceFilter.match({a: 'aaFOOsdsad'},
> +                                              {a: { required: true, regexp: 'foo', regexpFlags: 'i'}}));
> +  do_check_true(ActivitiesServiceFilter.match({a: 'aaFOOsdsad'},
> +                                              {a: { required: true, regexp: '/foo/i'}}));

Could you test:
do_check_false(ActivitiesServiceFilter.match({a: 'aaFOOsdsad'},
                                             {a: { required: true, regexp: '/foo/'}}));

Comment 48

[Mounir Lamouri (:mounir)]

(In reply to Mounir Lamouri (:mounir) from comment #47)
> Comment on attachment 694375 [details] [diff] [review]
> patch f
> 
> Review of attachment 694375 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The way you decide to call matchNumber or matchString seems a bit conplex.
> What about doing this:
> - if typeof() is number, just call matchNumber.
> - if typeof() is string but parseInt(aValue) != NaN, call matchNumber with
> parseInt(aValue) instead of aValue. Otherwise call matchString.
> - in matchString, if aFilter !== null, do: aFilter = parseInt(aFilter). If
> aFilter === NaN , return false. Otherwise continue as usual.

You should actually use parseFloat()...

Comment 49

[Andrea Marchesini [:baku]]

Attachment: patch g

Comment 50

[Andrea Marchesini [:baku]]

Attachment: patch h

Ok, here a completely different approach: if the manifest contains '42' this is a string, 42 is a number. The code is much simpler and we don't have to do conversions.
We 'cast' the value to the type of the filter if this is not null, then we compare. If the filter is null, we use regexp or min/max.

Comment 51

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

Comment on attachment 694496 [details] [diff] [review]
patch h

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

sr=me with the below fixed.

::: dom/activities/src/ActivitiesServiceFilter.jsm
@@ +16,5 @@
> +        case 'boolean':
> +          return aValue === aFilter;
> +
> +        case 'number':
> +          return parseFloat(aValue) === aFilter;

This should be |Number(aValue) === aFilter;|. Otherwise you end up first converting to a string and then to a number, since parseFloat takes a string.

@@ +37,5 @@
> +          regexpFlags = aFilterObj.regexpFlags;
> +
> +        // otherwise a simple parser for /foo/i.
> +        } else if (regexp[0] == '/') {
> +          var tmp = regexp.substr(1);

I don't think that we should support two ways of specifying the regexp flags. One is simpler.

And I also don't think we should support regexps either on the form of "foopy" *or* on the form of "/foopy/". If nothing else to keep things simpler, but it also introduces fewer ambiguities.

how about simply doing:

var regexp = String(aFilterObj.regexp);
if (regexp[0] != "/")
  return false;

var pos = tmp.lastIndexOf("/");
if (pos == 0)
  return false;

re = new RegExp(regexp.substring(1, pos), regexp.substr(pos + 1));
return re.test(aValue);

@@ +52,5 @@
> +
> +      // Validation of the min/Max.
> +      if (!('min' in aFilterObj) ||
> +          !('max' in aFilterObj) ||
> +          aFilterObj.min <= aFilterObj.max) {

This seems strange. If min is greater than max it will mean that the filter will match anything. Elsewhere for invalid filters you make it not match anything which seems like a better way to flag to the developer that something is wrong.

So how about simply making this:

if (('min' in aFilterObj) || ('max' in aFilterObj)) { ...

If min and max are inverted the tests below will ensure that we always return false.

@@ +67,5 @@
> +          return false;
> +        }
> +      }
> +
> +      return true;

Returning false here seems better since it'll make it easier for the developer to catch that something is wrong..

Comment 52

[Mounir Lamouri (:mounir)]

Comment on attachment 694496 [details] [diff] [review]
patch h

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

r=me with:
- the test added;
- the regexpFlags property VS specifying the flags in the regexp property discussion happening.

::: dom/activities/src/ActivitiesServiceFilter.jsm
@@ +37,5 @@
> +          regexpFlags = aFilterObj.regexpFlags;
> +
> +        // otherwise a simple parser for /foo/i.
> +        } else if (regexp[0] == '/') {
> +          var tmp = regexp.substr(1);

Jonas, you do not mention which ways you want to keep for the regexp flags. If we make the leading and trailing slashes mandatory, maybe we can simply have the flags listed after the last slash and trash the rgexpFlags property? How does that sound Andrea?

@@ +52,5 @@
> +
> +      // Validation of the min/Max.
> +      if (!('min' in aFilterObj) ||
> +          !('max' in aFilterObj) ||
> +          aFilterObj.min <= aFilterObj.max) {

I think that was my idea to automatically fail if min > max. I'm fine to have it always pass. The important thing is that we do something on purpose and we have tests that check it.

@@ +67,5 @@
> +          return false;
> +        }
> +      }
> +
> +      return true;

I think we should keep |return true;| here otherwise this will not match:
{ a: 2 } and { a: { required: true } }

::: dom/activities/tests/unit/test_activityFilters.js
@@ +62,5 @@
> +                                              {a: { required: true, value: ['a', 'b', 'foobar']}}));
> +  do_check_true(ActivitiesServiceFilter.match({a: ['k', 'z', 'foobar']},
> +                                              {a: { required: true, value: ['a', 'b', 'foobar']}}));
> +  do_check_false(ActivitiesServiceFilter.match({a: ['k', 'z', 'foobar2']},
> +                                               {a: { required: true, value: ['a', 'b', 'foobar']}}));

Could you add a case where there is a requirement without a value, like:
do_check_true(ActivitiesServiceFilter.match({a: 2},
                                            {a: { required: true }};

Comment 53

[Andrea Marchesini [:baku]]

> Jonas, you do not mention which ways you want to keep for the regexp flags.
> If we make the leading and trailing slashes mandatory, maybe we can simply
> have the flags listed after the last slash and trash the rgexpFlags
> property? How does that sound Andrea?

if you think that the parser of the regexp is good enough, we can remove regexpFlags:

        // otherwise a simple parser for /foo/i.
        } else if (regexp[0] == '/') {
          var tmp = regexp.substr(1);
          var pos = tmp.lastIndexOf('/');
          if (pos != -1) {
            regexpFlags = tmp.substr(pos + 1);
            regexp = tmp.substr(0, pos);
          }
        }

I prefer to keep both: regexpFlags + this parser. Jonas?


> I think that was my idea to automatically fail if min > max. I'm fine to
> have it always pass. The important thing is that we do something on purpose
> and we have tests that check it.

test added.

> I think we should keep |return true;| here otherwise this will not match:
> { a: 2 } and { a: { required: true } }

yep. We have a couple of tests for this:

  do_check_false(ActivitiesServiceFilter.match({},
                                               {a: { required: true}}));
  do_check_true(ActivitiesServiceFilter.match({a: 42},
                                              {a: { required: true}}));

Comment 54

[Mounir Lamouri (:mounir)]

Given that Jonas said:
> I don't think that we should support two ways of specifying the regexp flags. One is simpler.
And also asked to have only the "/foo/" format kept instead of "foo". I think keeping the flags in the regexp property would be the right way to go.

Comment 55

[Andrea Marchesini [:baku]]

Actually I prefer to use the same pattern of RegExp object: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/RegExp

This is much much easier to be used because we don't have to implement the parser.
For instance, the current parser fails with: regexp: '/aaa\/'

Probably we can implement /foo/ in a follow up bug but the parser cannot so simple as what I implemented.

Comment 56

[Mounir Lamouri (:mounir)]

Using the regexp property directly to put the flags will follow this syntax:
  var re = /foo/ig

Comment 57

[Andrea Marchesini [:baku]]

Attachment: patch i

Comment 58

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

Yay! Looks great! Ship it!

Comment 59

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

What is remaining to check this in? Can we just add the checkin-needed keyword?

Comment 61

[Andrea Marchesini [:baku]]

(In reply to Jonas Sicking (:sicking) from comment #59)
> What is remaining to check this in? Can we just add the checkin-needed
> keyword?

I spent few minutes to test it again... I don't like regressions :)
I think now it's ready to be checked in.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c5b23292ca

In the future, commit messages that describe what the patch is actually doing are appreciated :)

Comment 63

[David Flanagan [:djf]]

I've filed bug 825020 for any gaia-side changes we want to make once these new filters land.

We can use these new features to ensure that we don't show wallpapers unless the app that initiates the pick activity explicitly requests them, for example.

Are there other changes that should be made as well?  If so, please comment in bug 825020.

For old-style filters, I forget whether this patch treats them as required:true or required:false.  In either case, are there some activities where we want to alter the app manifests to explicitly make the filter required or optional? For pick activities do we want to make the type filter optional to enable file pickers, or for share activities, do we want to make the type filter required to prevent accidental abuse?

Comment 64

[Andrea Marchesini [:baku]]

the default behaviour is {required: false}
I think the manifests should be used in order to know which properties are required or optional.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f9c5b23292ca

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7baa3ef5fe23
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d88f09add84c