Last Comment Bug 773383 - [Web Activities] Activities filtering heuristic does not support arrays in filters
: [Web Activities] Activities filtering heuristic does not support arrays in fi...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: [:fabrice] Fabrice Desré
: Jason Smith [:jsmith]
Mentors:
: 782755 (view as bug list)
Depends on:
Blocks: 715814 776027
  Show dependency treegraph
 
Reported: 2012-07-12 12:13 PDT by Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
Modified: 2012-09-14 18:22 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.27 KB, patch)
2012-08-15 11:26 PDT, [:fabrice] Fabrice Desré
21: review+
Details | Diff | Splinter Review

Description Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-12 12:13:03 PDT

    
Comment 1 [:fabrice] Fabrice Desré 2012-08-14 14:08:07 PDT
*** Bug 782755 has been marked as a duplicate of this bug. ***
Comment 2 [:fabrice] Fabrice Desré 2012-08-15 11:26:47 PDT
Created attachment 652156 [details] [diff] [review]
patch

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.
Comment 3 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-08-15 14:49:57 PDT
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.
Comment 4 [:fabrice] Fabrice Desré 2012-08-15 15:07:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/31036bc023a8
Comment 5 Ed Morley [:emorley] 2012-08-16 06:18:51 PDT
https://hg.mozilla.org/mozilla-central/rev/31036bc023a8
Comment 6 Mounir Lamouri (:mounir) 2012-08-16 06:23:16 PDT
Fabrice, can you open a follow-up to support regexps, if we don't have a bug on file yet?
Comment 7 David Flanagan [:djf] 2012-08-16 07:11:53 PDT
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?
Comment 8 Mounir Lamouri (:mounir) 2012-08-16 07:58:20 PDT
(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.
Comment 9 [:fabrice] Fabrice Desré 2012-08-16 07:59:32 PDT
(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?
Comment 10 Mounir Lamouri (:mounir) 2012-08-24 12:59:45 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.