Closed
Bug 1042807
Opened 10 years ago
Closed 9 years ago
Vertical home screen should use homescreen-webapps-manage permission
Categories
(Firefox OS Graveyard :: Gaia::Homescreen, defect)
Firefox OS Graveyard
Gaia::Homescreen
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: tedders1, Assigned: tedders1)
References
Details
(Whiteboard: [systemsfe][p=2])
Attachments
(2 files, 2 obsolete files)
Under the new permission model, vertical homescreen can't access the collection icons because they live in a different app. We need to put the icons somewhere that vertical homescreen can access them. +++ This bug was initially created as a clone of Bug #1000305 +++
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
This patch contains changes that make the vertical homescreen work with the new icon API and the new permission model. With this patch, the collections app copies the collection icons into a datastore as a blob, and the vertical homescreen extracts them from the datastore. However, I've noticed this makes the vertical homescreen app start up slower, so this solution might be unsatisfactory. I want to investigate a different solution if I get a change.
Attachment #8462748 -
Flags: review?(kgrandon)
Assignee | ||
Comment 2•10 years ago
|
||
*if I get a chance
Comment 3•10 years ago
|
||
Comment on attachment 8462748 [details] [diff] [review] bug-1042807.patch Review of attachment 8462748 [details] [diff] [review]: ----------------------------------------------------------------- Hey Ted, This looks good, but my main question is why do we need a new property on the collection object? It looks like we already have a decoratedIconBlob property - can we reuse that? Also do you know where the slowdown on startup is coming from? Is it due to the get getIcon() method? Startup concerns are super important for 2.0 now, so if this regresses startup time we are going to have a bad time =/
Attachment #8462748 -
Flags: review?(kgrandon)
Comment 4•10 years ago
|
||
Since bug 1092726 has been marked as a WONTFIX for now, I think we can also keep this as a wontfix? Feel free to re-open if necessary.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
3rd party homescreens still can't access content from other apps since they aren't certified. But once bug 1000305 is fixed, 3rd party homescreens can access icons from other apps. Is that enough for what we need here?
Comment 6•10 years ago
|
||
If we want to convert the vertical home screen to use homescreen-webapps-manage, then we would still want the patch here. Not really something that I think we need to do right now, but I could be convinced. The main benefit I can think of is third party developers looking at our code as an example.
I definitely think that we should limit our homescreen to privileged APIs (and capabilities) so that we can be sure that 3rd party homescreens can be as good as ours.
Comment 8•10 years ago
|
||
Sounds good to me then. Let's keep this around to track the porting to the homescreen-webapps-manage permission.
Status: RESOLVED → REOPENED
Component: DOM: Apps → Gaia::Homescreen
Product: Core → Firefox OS
Resolution: WONTFIX → ---
Summary: Vertical Homescreen can't access icons for collections → Vertical home screen should use homescreen-webapps-manage permission
Version: Trunk → unspecified
Assignee | ||
Comment 10•9 years ago
|
||
I'm sorry for not doing this yet. I should be able to do it next week.
Assignee | ||
Comment 11•9 years ago
|
||
Rebased.
Attachment #8462748 -
Attachment is obsolete: true
Flags: needinfo?(tclancy)
Assignee | ||
Comment 12•9 years ago
|
||
Fabrice, do you want me to land this?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(fabrice)
Comment 13•9 years ago
|
||
I think that would not hurt, but I defer the review to kgrandon.
Flags: needinfo?(fabrice)
Updated•9 years ago
|
Attachment #8664754 -
Flags: review?(kevingrandon)
Comment 14•9 years ago
|
||
Comment on attachment 8664754 [details] [diff] [review] bug-1042807-rebased.patch Review of attachment 8664754 [details] [diff] [review]: ----------------------------------------------------------------- I am fine with doing something like this, but shouldn't we remove the webapps-manage permission if so? Please let me know why or why not and flag me again, thanks.
Attachment #8664754 -
Flags: review?(kevingrandon) → feedback+
Comment 15•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8664754 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #14) > Comment on attachment 8664754 [details] [diff] [review] > shouldn't we remove the webapps-manage permission if so? Quite right.
Assignee | ||
Updated•9 years ago
|
Attachment #8665046 -
Flags: review?(kevingrandon)
Comment 17•9 years ago
|
||
Comment on attachment 8665046 [details] [review] [gaia] tedders1:bug-1042807-fix > mozilla-b2g:master Ted - I gave this a shot on the device and all app icons were defaulted to the rocketship - I'm not entirely sure why. Was this working for you after a `make reset-gaia`? I tried using the latest gecko build, did all necessary gecko patches land?
Flags: needinfo?(tclancy)
Attachment #8665046 -
Flags: review?(kevingrandon)
Assignee | ||
Comment 18•9 years ago
|
||
Oops. I did check on a device, but I guess I didn't flash the phone properly. (And it passed the automated tests.) New version pushed.
Flags: needinfo?(tclancy)
Assignee | ||
Updated•9 years ago
|
Attachment #8665046 -
Flags: review?(kevingrandon)
Comment 19•9 years ago
|
||
Comment on attachment 8665046 [details] [review] [gaia] tedders1:bug-1042807-fix > mozilla-b2g:master I really want to land this patch, but for some reason every other time I flash this an icon or two is missing from the home page. I'm running the latest gecko and am testing with a flame. Ted - it feels like there's a race condition somewhere, and I'm not sure if it's in the home screen code, or in gecko. Any ideas?
Flags: needinfo?(tclancy)
Attachment #8665046 -
Flags: review?(kevingrandon) → feedback+
Assignee | ||
Comment 20•9 years ago
|
||
> but for some reason every other time I flash this an icon or two is missing from the home page.
How weird. I just flashed my phone 5 times, and not once did I see any icons missing.
I'm also using a flame, but I'm not sure I have the latest gecko.
Can you tell me what icon is missing? Is it a Marketplace app, or a pre-installed webapp? Maybe knowing that might help me reproduce the problem.
Flags: needinfo?(tclancy)
Comment 22•9 years ago
|
||
(In reply to Ted Clancy [:tedders1] from comment #20) > Can you tell me what icon is missing? Is it a Marketplace app, or a > pre-installed webapp? Maybe knowing that might help me reproduce the problem. It appears that it can happen to any installed app after a reset-gaia. It's totally random and sometimes it's 0 icons missing, or sometimes 4. Here's a screenshot with 4 missing icons. (Typically they will return after a restart, but my hunch is that's because we're rendering some cached icons at that point).
Flags: needinfo?(kevingrandon)
Comment 23•9 years ago
|
||
This also happens with the new homescreen so it seems there might be a problem with the api/platform: https://bugzilla.mozilla.org/show_bug.cgi?id=1211266
Comment 24•9 years ago
|
||
I've also never experienced this, but I've only been testing on an aries device. This blocks new homescreen, which blocks pin-the-web. Is there anyone actively looking at it?
Flags: needinfo?(tclancy)
Assignee | ||
Comment 25•9 years ago
|
||
Kevin, if that's a separate issue from this ticket, can this ticket land? Or is that blocking this ticket?
Flags: needinfo?(tclancy) → needinfo?(kevingrandon)
Comment 26•9 years ago
|
||
(In reply to Ted Clancy [:tedders1] from comment #25) > Kevin, if that's a separate issue from this ticket, can this ticket land? Or > is that blocking this ticket? It's a separate issue technically. I filed bug 1211266 to better capture the issue, so maybe we should track it there, or re-purpose that one to specifically fix the platform issue.
Flags: needinfo?(kevingrandon)
Comment 27•9 years ago
|
||
Since this is no longer the default, the platform issue is being fixed in bug 1211266, and qa wants a privileged home screen - may as well land to close this out so we can ship it as a privileged one if we want. Thanks Ted! In master: https://github.com/mozilla-b2g/gaia/commit/2e89362de40a6c9c36525d36317fa1ae8e67e143
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•