[Add-On Manager] Add activity for detail view

RESOLVED FIXED

Status

Firefox OS
Gaia
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: justindarc, Assigned: yzen)

Tracking

(Blocks: 1 bug)

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(b2g-master fixed)

Details

(Whiteboard: [spark])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
The Add-On Manager should support an activity for taking the user directly to the detail view for a specific add-on.
(Reporter)

Updated

3 years ago
Assignee: nobody → jdarcangelo
Blocks: 1133990, 1144843
Priority: -- → P1
Whiteboard: [lightsaber]
(Reporter)

Updated

3 years ago
Depends on: 1144849
Assignee: jdarcangelo → yzenevich
This can also land on master.
status-b2g-master: --- → affected
No longer depends on: 1144849
See Also: → bug 1144849
Created attachment 8583922 [details] [review]
[gaia] yzen:bug-1144848-2 > mozilla-b2g:lightsaber
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Comment 4

3 years ago
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)
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
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+
Created attachment 8584692 [details] [review]
[gaia] yzen:bug-1144848 > mozilla-b2g:lightsaber
(Assignee)

Updated

3 years ago
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)
(Assignee)

Updated

3 years ago
Attachment #8583922 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
(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)
(Assignee)

Comment 14

3 years ago
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+
(Assignee)

Comment 16

3 years ago
https://github.com/mozilla-b2g/gaia/commit/4b6bd3004d4d91d6abea6aa57881402813e34236
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Created attachment 8591593 [details] [review]
[gaia] crh0716:1144848_master > mozilla-b2g:master
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)
(Assignee)

Comment 19

3 years ago
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)
(Assignee)

Comment 22

3 years ago
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
status-b2g-master: affected → fixed
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
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.