Closed Bug 965152 Opened 10 years ago Closed 6 years ago

Support for same activity type with different entry points in an application

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Gonk (Firefox OS)
defect
Not set
major

Tracking

(tracking-b2g:backlog)

RESOLVED WONTFIX
tracking-b2g backlog

People

(Reporter: sharaf.tir, Assigned: sharaf.tir)

References

Details

Attachments

(2 files, 1 obsolete file)

As per the comment #2 in Bug 819170, we can add activity property to any entry points. 

It should be possible for us to define same activity type with different entry points in an application.

For example, the "Communication" application in Gaia defines entry points like "Dialer" and "Contacts". It should be possible for them to define "pick" activities individually so that Dialer pick activity will allow other application to select call log entries, and contact pick activity will allow selection of contacts.
Right now we can add same activity type with different entry points, but only the last activity is considered by the system.
Blocks: 958307
Adding alive also to CC as it will require modification in system app also.
Blocks: 963043
Fabrice, can you comment on what the best way to achieve the use-cases in comment #0 would be?
Flags: needinfo?(fabrice)
AFAIK we are going to split communications SOON, and deprecate entry point. It brought us some troubles :(
Do we still need this if communications are different apps?
Discussed with communication engineer, and as per him the splitting of communication app wont be done for 1.4.

I feel that this is a bug in the current implementation as we already support activities associated with entry points. So it should be possible to define same activity type with different entry points. Please correct me if I am wrong.

I checked the code, and the issue is because we are considering only manifest and app name while generating activity Ids. So if we consider entry point name also for generating the Id hash, it will work. Please comment.

I have a working patch for this issue. If all agree to this issue, assign the issue to me, and I can upload it for feedback.
blocking-b2g: --- → 1.4?
Yep, that should work. Please upload your patch.
Assignee: nobody → sharaf.tir
Flags: needinfo?(fabrice)
Attached patch Gecko modifications (obsolete) — Splinter Review
Gecko side modifications for this issue based on comment #5
Attachment #8368534 - Flags: feedback?(fabrice)
Gaia side modification. This is mainly for localizing entry point names and showing to the user.
Attachment #8368536 - Flags: feedback?(alive)
Comment on attachment 8368534 [details] [diff] [review]
Gecko modifications

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

Nice! I'm a bit worried about the behavior when updating apps though. We'll try to unregister activities that are not in the db, but may let some in that we should remove.

::: b2g/components/ActivitiesGlue.js
@@ +24,4 @@
>  
>      let choices = [];
>      activity.list.forEach(function(item) {
> +      choices.push({ manifest: item.manifest, icon: item.icon, entryPoint: item.entryPoint });

nit: is that < 80 chars long?

::: dom/apps/src/AppsUtils.jsm
@@ +627,5 @@
>    },
>  
> +  entryPointIconURLForSize: function(aEntryPoint, aSize) {
> +    let entryPoints = this._localeProp("entry_points");
> +    if (!entryPoints)

nit: braces even for single line ifs

@@ +641,5 @@
> +        icon = this._origin.resolve(icons[size]);
> +        dist = Math.abs(iSize - aSize);
> +      }
> +    }
> +    return icon;

that should be refactored to not replicate code from iconURLForSize()
Attachment #8368534 - Flags: feedback?(fabrice) → feedback+
Attachment #8368536 - Flags: feedback?(alive) → feedback+
How will the different activities be presented to the user? What name will we see?
(In reply to Fabrice Desré [:fabrice] from comment #9)
> Comment on attachment 8368534 [details] [diff] [review]
> Gecko modifications
> 
> Review of attachment 8368534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! I'm a bit worried about the behavior when updating apps though. We'll
> try to unregister activities that are not in the db, but may let some in
> that we should remove.

I had verified all the cases before. My understanding is that whenever we update an app, all the activities from old manifest is unregistered and new manifest activities are registered. I am referring to "updateAppHandlers" function. Please correct me if I missed anything here :-)

Thank you for your review comments, I will modify the code based on your comments.

adding ni? to fabrice just to make sure that he sees this.
Flags: needinfo?(fabrice)
(In reply to Anthony Ricaud (:rik) from comment #10)
> How will the different activities be presented to the user? What name will
> we see?

In my patch I am using the entry point name and entry point Icon while presenting to the user.
(In reply to Sharaf from comment #11)
> (In reply to Fabrice Desré [:fabrice] from comment #9)
> > Comment on attachment 8368534 [details] [diff] [review]
> > Gecko modifications
> > 
> > Review of attachment 8368534 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Nice! I'm a bit worried about the behavior when updating apps though. We'll
> > try to unregister activities that are not in the db, but may let some in
> > that we should remove.
> 
> I had verified all the cases before. My understanding is that whenever we
> update an app, all the activities from old manifest is unregistered and new
> manifest activities are registered. I am referring to "updateAppHandlers"
> function. Please correct me if I missed anything here :-)
> 
> Thank you for your review comments, I will modify the code based on your
> comments.
> 
> adding ni? to fabrice just to make sure that he sees this.

The issue I see is that any application installed *before* this patch that will get an update *after* this patch will not have its activities properly removed, because the updateAppHandlers code has no idea when we registered the activities, and thus which key was used.

That would also be the case with OS upgrades, and that would be really bad. So we need a solution here...
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #13)
> The issue I see is that any application installed *before* this patch that
> will get an update *after* this patch will not have its activities properly
> removed, because the updateAppHandlers code has no idea when we registered
> the activities, and thus which key was used.
> 
> That would also be the case with OS upgrades, and that would be really bad.
> So we need a solution here...

Okey Great.. I got it.. :-) to solve this how about adding a fallback mechanism in activity service remove part? first we will try generating id with the tuple manifest, name, entry point. If its failing, try with tuple manifest, name?

ni? to fabrice
Flags: needinfo?(fabrice)
Yes, that could work. Go ahead! ;)
Flags: needinfo?(fabrice)
Hi Fabrice,

I have modified the patch based on the comments.

Request you to please review it. :-)
Attachment #8368534 - Attachment is obsolete: true
Attachment #8382974 - Flags: review?(fabrice)
Not blocking - this doesn't hit the QC FC blocking list or DSDS feature blocking list.
blocking-b2g: 1.4? → backlog
Fabrice,

Can you still review Attachment #8382974 [details] [diff] nonetheless? 
Also in case the splitting of apps doesn't happen soon, would this be beneficial for FxOS to land into 1.5/2.0? (thinking nomination)

Thanks!
Flags: needinfo?(fabrice)
Wayne & Sharaf, sorry for taking so long to review. I'll do it next week.
Flags: needinfo?(fabrice)
Do you have any update for this patch?
Flags: needinfo?(fabrice)
Gecko patch is not reviewed yet.
Could you review the patch?
Comment on attachment 8382974 [details] [diff] [review]
Gecko modifications

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

Looks good overall, thanks! Can you update the patch, I promise I'll review and test faster...

::: dom/activities/src/ActivitiesService.jsm
@@ +116,3 @@
>          };
>          debug("Going to remove " + JSON.stringify(object));
> +        var idToRemove = this.createId(object);

nit: s/var/let

::: dom/apps/src/AppsUtils.jsm
@@ +642,4 @@
>      return icon;
>    },
>  
> +  entryPointIconURLForSize: function(aEntryPoint, aSize) {

I would rather reorder the parameters as (aSize, aEntryPoint), and switch to what iconURLForSize() is doing when aEntryPoint is falsy.

@@ +642,5 @@
>      return icon;
>    },
>  
> +  entryPointIconURLForSize: function(aEntryPoint, aSize) {
> +    let entryPoints = this._localeProp('entry_points');

nit: we use double quotes for strings in this file.

@@ +651,5 @@
> +    return this._extractIconURLForSize(icons, aSize);
> +  },
> +
> +  iconURLForSize: function(aSize) {
> +    let icons = this._localeProp('icons');

here too.
Attachment #8382974 - Flags: review?(fabrice) → feedback+
Flags: needinfo?(fabrice)
blocking-b2g: backlog → ---
Product: Core → Core Graveyard
Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: