[Add-On Manager] Add activity for detail view

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: justindarc, Assigned: yzen)

Tracking

unspecified
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(b2g-master fixed)

Details

(Whiteboard: [spark])

Attachments

(2 attachments, 1 obsolete attachment)

The Add-On Manager should support an activity for taking the user directly to the detail view for a specific add-on.
Assignee: nobody → jdarcangelo
Blocks: 1133990, 1144843
Priority: -- → P1
Whiteboard: [lightsaber]
Depends on: 1144849
Assignee: jdarcangelo → yzenevich
This can also land on master.
No longer depends on: 1144849
See Also: → 1144849
Comment on attachment 8583922 [details] [review]
[gaia] yzen:bug-1144848-2 > mozilla-b2g:lightsaber

Hi Arthur, I have a wip of settings handling addon-details activity, I just wanted to get your feedback if the direction is right. The intention is to create an activity with the following options:
{
  name: 'configure', 
  data: {
    target: 'device', 
    section: 'addon-details', 
    options: {
      addon: /* Addon goes here */
    }
  }
}
Attachment #8583922 - Flags: feedback?(arthur.chen)
If this is the right direction, I was wondering if instead of passing an addon itself, its origin would be sufficient, and then we could look up the addon object with the addon manager?
(In reply to Yura Zenevich [:yzen] from comment #4)
> If this is the right direction, I was wondering if instead of passing an
> addon itself, its origin would be sufficient, and then we could look up the
> addon object with the addon manager?

We uniquely identify apps using the `manifestURL` property everywhere else, so we might want to pass that instead of the full `App` object. The disadvantage of doing this is that it would require handlers to look up that app.

Fabrice, what do you think? I'm not sure what the precedent here is, but I would think that sending a full `App` object is a lot for an activity.
Flags: needinfo?(fabrice)
(In reply to Doug Sherk (:drs) (use needinfo?) from comment #5)
> (In reply to Yura Zenevich [:yzen] from comment #4)
> > If this is the right direction, I was wondering if instead of passing an
> > addon itself, its origin would be sufficient, and then we could look up the
> > addon object with the addon manager?
> 
> We uniquely identify apps using the `manifestURL` property everywhere else,

Ahh good to know, manifestURL would work just fine.
It should now be working with moz activities of this form:
{
  name: 'configure', 
  data: {
    target: 'device', 
    section: 'addon-details', 
    options: {
      manifestURL: "app://customizer.gaiamobile.org/manifest.webapp"
    }
  }
}
Comment on attachment 8583922 [details] [review]
[gaia] yzen:bug-1144848-2 > mozilla-b2g:lightsaber

Looking good to me!
Attachment #8583922 - Flags: feedback?(arthur.chen) → feedback+
Attachment #8584692 - Flags: review?(arthur.chen)
(In reply to Doug Sherk (:drs) (use needinfo?) from comment #5)
> (In reply to Yura Zenevich [:yzen] from comment #4)
> > If this is the right direction, I was wondering if instead of passing an
> > addon itself, its origin would be sufficient, and then we could look up the
> > addon object with the addon manager?
> 
> We uniquely identify apps using the `manifestURL` property everywhere else,
> so we might want to pass that instead of the full `App` object. The
> disadvantage of doing this is that it would require handlers to look up that
> app.
> 
> Fabrice, what do you think? I'm not sure what the precedent here is, but I
> would think that sending a full `App` object is a lot for an activity.

You can't even pass a full App object through an activity (they are DOM objects that we can't serialize to go through the IPC). Sending the manifestURL is the way to go.
Flags: needinfo?(fabrice)
Comment on attachment 8584692 [details] [review]
[gaia] yzen:bug-1144848 > mozilla-b2g:lightsaber

There is a comment in github that needs to be addressed before merging, thanks.
Attachment #8584692 - Flags: review?(arthur.chen)
Attachment #8583922 - Attachment is obsolete: true
(In reply to Arthur Chen [:arthurcc] from comment #11)
> Comment on attachment 8584692 [details] [review]
> [gaia] yzen:bug-1144848 > mozilla-b2g:lightsaber
> 
> There is a comment in github that needs to be addressed before merging,
> thanks.

I can't seem to find them in the actual PR, if you are referring to the comment in the obsolete PR about checking for initial options only once, afaik it should be addressed.
Flags: needinfo?(arthur.chen)
I couldn't find them either. But it doesn't matter as it seems already being fixed.

For the latest PR, I was wondering why it touches `apps/settings/test/unit/panels/addons/addons_manager_test.js`?
Flags: needinfo?(arthur.chen) → needinfo?(yzenevich)
Comment on attachment 8584692 [details] [review]
[gaia] yzen:bug-1144848 > mozilla-b2g:lightsaber

I was thinking that that panel was the same as the addon manager in the modules folder. Undid those changes, hopefully looks good now.
Flags: needinfo?(yzenevich)
Attachment #8584692 - Flags: review?(arthur.chen)
Comment on attachment 8584692 [details] [review]
[gaia] yzen:bug-1144848 > mozilla-b2g:lightsaber

The patch is looking good, thanks. r=me.
Attachment #8584692 - Flags: review?(arthur.chen) → review+
https://github.com/mozilla-b2g/gaia/commit/4b6bd3004d4d91d6abea6aa57881402813e34236
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8591593 [details] [review]
[gaia] crh0716:1144848_master > mozilla-b2g:master

EJ, the patch was proposed against master based on the one from yura. It adds support for accepting options when invoking an activity. The options then get passed to the add-on detail panel.

Yura, the only difference is that I return a promise in `onBeforeShow` so the panel is displayed only after it is rendered.

Would you mind review the patch? Thanks!
Attachment #8591593 - Flags: review?(ejchen)
Attachment #8591593 - Flags: feedback?(yzenevich)
Comment on attachment 8591593 [details] [review]
[gaia] crh0716:1144848_master > mozilla-b2g:master

It looks good, though there's still visible un-rendered panel when opening details from activity, until the addon is found. I think this is why I had to only display the panel when the addon is loaded in lightsaber.
Attachment #8591593 - Flags: feedback?(yzenevich)
Comment on attachment 8591593 [details] [review]
[gaia] crh0716:1144848_master > mozilla-b2g:master

Looks nice !

I left some messages on GitHub and you can see whether you are going to address that or not. 

Thanks Arthur, r+ btw.
Attachment #8591593 - Flags: review?(ejchen) → review+
Comment on attachment 8591593 [details] [review]
[gaia] crh0716:1144848_master > mozilla-b2g:master

Just realized that returning a promise won't work if it is the first panel at launch. Switching back to the original solution. Yura, please help take a look at it again when you get a chance, thanks!
Attachment #8591593 - Flags: feedback?(yzenevich)
Comment on attachment 8591593 [details] [review]
[gaia] crh0716:1144848_master > mozilla-b2g:master

The updated version works really well. Cool!
Attachment #8591593 - Flags: feedback?(yzenevich) → feedback+
Thanks EJ and Yura!

master: 42fcc4b3ebe60872fd9c8c157570f6627f4570f5
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.
Whiteboard: [lightsaber] → [ignite]
Whiteboard: [ignite] → [spark]
You need to log in before you can comment on or make changes to this bug.