Closed Bug 805292 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
blocking-basecamp -
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
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.
Assignee: nobody → amarchesini
blocking-basecamp: --- → ?
Sorry I meant it's needed for 801635
Blocks: 801635
blocking-basecamp: ? → ---
This blocks a blocker which blocks a crasher blocker.  Nomming.
blocking-basecamp: --- → ?
Actually, the bug it blocks isn't a blocker so marking this blocking-.
blocking-basecamp: ? → -
Attached patch patch 1 (obsolete) — Splinter Review
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
Attachment #675512 - Flags: feedback?(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."
Attachment #675512 - Flags: feedback?(mounir) → feedback+
Attached patch patch 2 (obsolete) — Splinter Review
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.
Attachment #675512 - Attachment is obsolete: true
Attachment #675538 - Flags: review?(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 [...]"
Attachment #675538 - Flags: review?(mounir) → review+
CCing Vivien and David Flanagan so they are aware of that change.
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Component: General → DOM: Mozilla Extensions
Product: Boot2Gecko → Core
Attached patch patchSplinter Review
Attachment #675538 - Attachment is obsolete: true
Attachment #675575 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Version: unspecified → Trunk
(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.
> 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.
(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.
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.
Andrea, this bug is not a blocker. We should not spend time on it at this point.
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 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.
Attachment #675575 - Flags: approval-mozilla-beta?
Attachment #675575 - Flags: approval-mozilla-aurora?
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/3ec8fba88e7b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Attachment #675575 - Flags: approval-mozilla-beta?
Attachment #675575 - Flags: approval-mozilla-beta+
Attachment #675575 - Flags: approval-mozilla-aurora?
Attachment #675575 - Flags: approval-mozilla-aurora+
(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.
I merged the pull request. Ryan is going to land the Gecko patch to m-a and m-b.
Blocks: 832923
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.