805292 - A "pick" activity without type prop, doesn't work

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

I think there are a couple of problems in the current filter match for activities.

1. empty type doesn't match apps.
let a = new MozActivity({ name: "pick", data: {}});
This should be the most generic 'pick' activity but actually it doesn't show any app as such as gallery, camera or wallpaper.

2. If a property is not set, the app is not shown.
Example: wallpaper requires width and height properties, and this:

let a = new MozActivity({ name: "pick", data: { type: "image/png" }});

is not enough to have it in the list of apps.

Comment 1

[Gene Lian (I already quit Mozilla)]

Hi Andrea,

Does this one block any V1 features for now? I might be able to look into this one if it's urgent. Please let me know.

Comment 2

[Andrea Marchesini [:baku]]

I'm working on a patch. It's needed for 805292.
Without this, the file picker will not able to handle a generic <input type="file"/> because the 'pick' activity doesn't show any usable application for pick a file.

Comment 3

[Andrea Marchesini [:baku]]

Sorry I meant it's needed for 801635

Comment 4

[Andrew Overholt [:overholt]]

This blocks a blocker which blocks a crasher blocker.  Nomming.

Comment 5

[Andrew Overholt [:overholt]]

Actually, the bug it blocks isn't a blocker so marking this blocking-.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch 1

Mounir can you give me a feedback about this patch?
By example this patch should work in this wait:

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: Reject - propC is unknown

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

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 675512 [details] [diff] [review]
patch 1

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

Thank you for working on that Andrea! \o/

::: dom/activities/src/ActivitiesService.jsm
@@ +259,5 @@
>  
>      let matchFunc = function matchFunc(aResult) {
> +
> +      function matchFuncValue(aValue, aFilter) {
> +        // Bug 773383: arrays of strings / regexp.

nit: you should update that comment. Bug 773383 has been fixed and was only about arrays. We need to point to the regexp bug.

@@ +274,5 @@
> +
> +        // If the filter is an array, the value must be included:
> +        } else if (Array.isArray(aFilter)) {
> +          return (aFilter.indexOf(aValue) != -1);
> +        }

The recursivity makes this method quite painful to get.
What about:

let values = Array.isArray(aValue) ? aValue : [aValue];
let filters = Array.isArray(aValue) ? aFilter : [aFilter];

values.forEach(function(value) {
  filters.forEach(function(filter) {
    if (value === filter) {
      return true;
    }
  }
}

return false;

@@ +276,5 @@
> +        } else if (Array.isArray(aFilter)) {
> +          return (aFilter.indexOf(aValue) != -1);
> +        }
> +
> +        // If the filter is a string, the value must equal:

nit: s/:/./

@@ +280,5 @@
> +        // If the filter is a string, the value must equal:
> +        return (aFilter === aValue);
> +      }
> +
> +      // for any incoming property:

nit: "// For any incoming property."

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch 2

If you like this patch, probably we have to change here and there some MozActivity in gaia. For instance, apps/homescreen/js/wallpaper.js is:

    var a = new MozActivity({
      name: 'pick',
      data: {
        type: 'image/jpeg',
        width: 320,
        height: 480
      }
    });

but, applying this patch this will be:

    var a = new MozActivity({
      name: 'pick',
      data: { type: 'image/jpeg' }
    });

because gallery and camera do not have width/height properties.

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 675538 [details] [diff] [review]
patch 2

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

::: dom/activities/src/ActivitiesService.jsm
@@ +259,5 @@
>  
>      let matchFunc = function matchFunc(aResult) {
> +
> +      function matchFuncValue(aValue, aFilter) {
> +        // Bug 773383: arrays of strings.

nit: can you check if there is a regexp bug open? If not, open one and put the bug number here.
Bug 773383 is fixed AFAICT. No need to mention it here.

@@ +275,5 @@
> +
> +        return ret;
> +      }
> +
> +      // for any incoming property:

nit: "// For any incoming property."

@@ +283,5 @@
> +        if (!(prop in aResult.description.filters)) {
> +          return false;
> +        }
> +
> +        // otherwise, let's check the value against the filter.

nit: "// Otherwise [...]"

Comment 10

[Mounir Lamouri (:mounir)]

CCing Vivien and David Flanagan so they are aware of that change.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 12

[[:fabrice] Fabrice Desré]

(In reply to Andrea Marchesini (:baku) from comment #8)
> Created attachment 675538 [details] [diff] [review]
> patch 2
> 
> If you like this patch, probably we have to change here and there some
> MozActivity in gaia. For instance, apps/homescreen/js/wallpaper.js is:
> 
>     var a = new MozActivity({
>       name: 'pick',
>       data: {
>         type: 'image/jpeg',
>         width: 320,
>         height: 480
>       }
>     });
> 
> but, applying this patch this will be:
> 
>     var a = new MozActivity({
>       name: 'pick',
>       data: { type: 'image/jpeg' }
>     });
> 
> because gallery and camera do not have width/height properties.

And that's fine... how do you want to *send* data from the caller if you don't add that? The "data" field is not just a filter, it's the actual payload.

Comment 13

[Andrea Marchesini [:baku]]

> And that's fine... how do you want to *send* data from the caller if you
> don't add that? The "data" field is not just a filter, it's the actual
> payload.

If you want to sent data, check the examples at comment #6 and let me know if you see a case that is not covered. If I understood correctly, you want this:

App: { type: 'foo', probA: ['a', 'b', 'c'] }
Request: { type: 'foo', propA: 'a' }

and this is supported.

Actually I think can be nice to have this: if an app has a filter property which value is null, it means that any value is accepted. Example:

App: { type: 'image/png', width: null, height: null }
Request: { type: 'image/png', width: 42, height: 42 }

This can be nice, because it shows the properties accepted by an app, without forcing a value.

Comment 14

[[:fabrice] Fabrice Desré]

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

> Actually I think can be nice to have this: if an app has a filter property
> which value is null, it means that any value is accepted. Example:
> 
> App: { type: 'image/png', width: null, height: null }
> Request: { type: 'image/png', width: 42, height: 42 }
> 
> This can be nice, because it shows the properties accepted by an app,
> without forcing a value.

That should work, but then you need to check all the activities usage in gaia if we want to take this for v1.

Comment 15

[Andrea Marchesini [:baku]]

I would love it. Can I submit a patch here about this?
So we can land them together if the review process succeeds?

> That should work, but then you need to check all the activities usage in
> gaia if we want to take this for v1.

Comment 16

[[:fabrice] Fabrice Desré]

Andrea, this bug is not a blocker. We should not spend time on it at this point.

Comment 17

[Mounir Lamouri (:mounir)]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec8fba88e7b

I spoke with Vivien and he said that now is a good moment to land this. I assume it's because most activities are already quite broken so if there is one or two breakage because of that patch, they might just be fixed for free.

Andrea, could you open a bug and do the pull request for the Gaia patch you said you had?

Comment 18

[Andrea Marchesini [:baku]]

https://github.com/mozilla-b2g/gaia/pull/6530 here the pull request.

Comment 19

[Mounir Lamouri (:mounir)]

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

[Approval Request Comment]
This is a B2G patch affecting only B2G.
See the last comment for the rational why it is a good idea to land that now.

FWIW, with this patch, we would be very close from an implementation of <input type='file'> which, FWIW, is something users a complaining a lot about.

Comment 20

[Alex Keybl [:akeybl]]

This isn't a blocker, the end-user impact is not clear at this point, and no risk assessment was provided. Sounds like a developer would just need to define the necessary properties. Please help clear things up for triage.

Comment 21

[Mounir Lamouri (:mounir)]

(In reply to Alex Keybl [:akeybl] from comment #20)
> This isn't a blocker, the end-user impact is not clear at this point, and no
> risk assessment was provided. Sounds like a developer would just need to
> define the necessary properties. Please help clear things up for triage.

End user impact? <input type='file'> isn't working. Though, we need to fix bug 801635 too for that.
Risk is pretty low. It's a b2g only feature and according to Andrea, there is only one regression that should be fixed by the pull request mentioned in comment 18. In addition, a lot of activities are already broken because of bug 794407. Pushing this now should be relatively safe compared to the possible win.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ec8fba88e7b

Comment 23

[Alex Keybl [:akeybl]]

(In reply to Mounir Lamouri (:mounir) from comment #21)
> (In reply to Alex Keybl [:akeybl] from comment #20)
> > This isn't a blocker, the end-user impact is not clear at this point, and no
> > risk assessment was provided. Sounds like a developer would just need to
> > define the necessary properties. Please help clear things up for triage.
> 
> End user impact? <input type='file'> isn't working. Though, we need to fix
> bug 801635 too for that.
> Risk is pretty low. It's a b2g only feature and according to Andrea, there
> is only one regression that should be fixed by the pull request mentioned in
> comment 18. In addition, a lot of activities are already broken because of
> bug 794407. Pushing this now should be relatively safe compared to the
> possible win.

Please do not land until the associated Gaia change has also landed. We should not be introducing any new regressions.

Comment 24

[Mounir Lamouri (:mounir)]

I merged the pull request. Ryan is going to land the Gecko patch to m-a and m-b.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f719a9c96ea9
https://hg.mozilla.org/releases/mozilla-beta/rev/92f99fc77dbc