Closed
Bug 1144848
Opened 10 years ago
Closed 10 years ago
[Add-On Manager] Add activity for detail view
Categories
(Firefox OS Graveyard :: Gaia, defect, P1)
Tracking
(b2g-master fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
b2g-master | --- | fixed |
People
(Reporter: justindarc, Assigned: yzen)
References
Details
(Whiteboard: [spark])
Attachments
(2 files, 1 obsolete file)
The Add-On Manager should support an activity for taking the user directly to the detail view for a specific add-on.
Reporter | ||
Updated•10 years ago
|
Updated•10 years ago
|
Assignee: jdarcangelo → yzenevich
Updated•10 years ago
|
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 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•10 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?
Comment 5•10 years ago
|
||
(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•10 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•10 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 8•10 years ago
|
||
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+
Comment 9•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8584692 -
Flags: review?(arthur.chen)
Comment 10•10 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,
> 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 11•10 years ago
|
||
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•10 years ago
|
Attachment #8583922 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 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)
Comment 13•10 years ago
|
||
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•10 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 15•10 years ago
|
||
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•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
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•10 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 21•10 years ago
|
||
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•10 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+
Comment 23•10 years ago
|
||
Thanks EJ and Yura!
master: 42fcc4b3ebe60872fd9c8c157570f6627f4570f5
Keywords: checkin-needed
Updated•10 years ago
|
Keywords: checkin-needed
Comment 24•10 years ago
|
||
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•10 years ago
|
||
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.
Updated•10 years ago
|
Whiteboard: [lightsaber] → [ignite]
Updated•10 years ago
|
Whiteboard: [ignite] → [spark]
You need to log in
before you can comment on or make changes to this bug.
Description
•