1144848 - [Add-On Manager] Add activity for detail view

Status: RESOLVED FIXED

Description

[Justin D'Arcangelo [:justindarc]]

The Add-On Manager should support an activity for taking the user directly to the detail view for a specific add-on.

Comment 1

[Doug Sherk (:drs) (inactive)]

This can also land on master.

Comment 2

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1144848-2 > mozilla-b2g:lightsaber

Comment 3

[Yura Zenevich [:yzen]]

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 */
    }
  }
}

Comment 4

[Yura Zenevich [:yzen]]

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?

Comment 5

[Doug Sherk (:drs) (inactive)]

(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.

Comment 6

[Yura Zenevich [:yzen]]

(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.

Comment 7

[Yura Zenevich [:yzen]]

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 8

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8583922 [details] [review]
[gaia] yzen:bug-1144848-2 > mozilla-b2g:lightsaber

Looking good to me!

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1144848 > mozilla-b2g:lightsaber

Comment 10

[[:fabrice] Fabrice Desré]

(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.

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 12

[Yura Zenevich [:yzen]]

(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.

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

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`?

Comment 14

[Yura Zenevich [:yzen]]

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.

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8584692 [details] [review]
[gaia] yzen:bug-1144848 > mozilla-b2g:lightsaber

The patch is looking good, thanks. r=me.

Comment 16

[Yura Zenevich [:yzen]]

https://github.com/mozilla-b2g/gaia/commit/4b6bd3004d4d91d6abea6aa57881402813e34236

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] crh0716:1144848_master > mozilla-b2g:master

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

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!

Comment 19

[Yura Zenevich [:yzen]]

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.

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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.

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

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!

Comment 22

[Yura Zenevich [:yzen]]

Comment on attachment 8591593 [details] [review]
[gaia] crh0716:1144848_master > mozilla-b2g:master

The updated version works really well. Cool!

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Thanks EJ and Yura!

master: 42fcc4b3ebe60872fd9c8c157570f6627f4570f5

Comment 24

[GitHub Autolander Bot]

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.

Comment 25

[GitHub Autolander Bot]

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.