Closed Bug 773383 Opened 12 years ago Closed 12 years ago

[Web Activities] Activities filtering heuristic does not support arrays in filters

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: vingtetun, Assigned: fabrice)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
This patch provides support for arrays. I'm not sure we can do regexps since they are not serializable in the json manifest (I very well may be wrong on this).

djf tested the patch with the gallery app.
Assignee: nobody → fabrice
Attachment #652156 - Flags: review?(21)
Comment on attachment 652156 [details] [diff] [review]
patch

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

::: dom/activities/src/ActivitiesService.jsm
@@ +254,5 @@
> +        if (Array.isArray(aResult.description.filters[prop])) {
> +          if (aResult.description.filters[prop].indexOf(aMsg.options.data[prop]) == -1) {
> +            return false;
> +          }
> +        } else if (aMsg.options.data[prop] !== aResult.description.filters[prop]) {

nit: For consistency I would invert the last expression so you can read it the same order as the indexOf expression.
Attachment #652156 - Flags: review?(21) → review+
https://hg.mozilla.org/mozilla-central/rev/31036bc023a8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Fabrice, can you open a follow-up to support regexps, if we don't have a bug on file yet?
Summary: [Web Activities] Activities filtering heuristic does not support arrays/regexp in filters → [Web Activities] Activities filtering heuristic does not support arrays in filters
I think Fabrice is right in comment #2.  You can't use regexp literals in JSON.  So you'll need to pick a syntax to distinguish a regexp string from just a regular literal string.

Could you get away with using ordinary wildcards?

pattern = new RegExp(s.replace('*', '\S*'))

Or something like that?
QA Contact: jsmith
Whiteboard: [qa+]
(In reply to David Flanagan [:djf] from comment #7)
> I think Fabrice is right in comment #2.  You can't use regexp literals in
> JSON.  So you'll need to pick a syntax to distinguish a regexp string from
> just a regular literal string.
> 
> Could you get away with using ordinary wildcards?
> 
> pattern = new RegExp(s.replace('*', '\S*'))
> 
> Or something like that?

We should not use JSON but structured clones.
(In reply to Mounir Lamouri (:mounir) from comment #8)
> 
> We should not use JSON but structured clones.

We still need a way to write that in the manifest. How do structured clones help here?
(In reply to Fabrice Desré [:fabrice] from comment #9)
> (In reply to Mounir Lamouri (:mounir) from comment #8)
> > 
> > We should not use JSON but structured clones.
> 
> We still need a way to write that in the manifest. How do structured clones
> help here?

Indeed. I don't have any answer regarding this for the moment.
Keywords: verifyme
Whiteboard: [qa+]
Keywords: verifyme
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: