Vertical home screen should use homescreen-webapps-manage permission

RESOLVED FIXED

Status

Firefox OS
Gaia::Homescreen
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: tedders1, Assigned: tedders1)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe][p=2])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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

4 years ago
Assignee: nobody → tclancy
Blocks: 1000305
No longer blocks: 899994, 934289, 980768, 993266, 994858, 898330, 987753
No longer depends on: 1000305, 1000313, 1000315, 912340
(Assignee)

Comment 1

4 years ago
Created attachment 8462748 [details] [diff] [review]
bug-1042807.patch

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

4 years ago
*if I get a chance
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)
(Assignee)

Updated

4 years ago
Blocks: 1092726
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
Last Resolved: 4 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?
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.
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
Blocks: 1134659
Ted, can you rebase this patch?
Flags: needinfo?(tclancy)
(Assignee)

Comment 10

3 years ago
I'm sorry for not doing this yet. I should be able to do it next week.
(Assignee)

Comment 11

3 years ago
Created attachment 8664754 [details] [diff] [review]
bug-1042807-rebased.patch

Rebased.
Attachment #8462748 - Attachment is obsolete: true
Flags: needinfo?(tclancy)
(Assignee)

Comment 12

3 years ago
Fabrice, do you want me to land this?
(Assignee)

Updated

3 years ago
Flags: needinfo?(fabrice)
I think that would not hurt, but I defer the review to kgrandon.
Flags: needinfo?(fabrice)
Attachment #8664754 - Flags: review?(kevingrandon)
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+
Created attachment 8665046 [details] [review]
[gaia] tedders1:bug-1042807-fix > mozilla-b2g:master
(Assignee)

Updated

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

Comment 16

3 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

3 years ago
Attachment #8665046 - Flags: review?(kevingrandon)
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

3 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

3 years ago
Attachment #8665046 - Flags: review?(kevingrandon)
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

3 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)
It varied, I will try to repro again soon.
Flags: needinfo?(kevingrandon)
Created attachment 8669431 [details]
Screenshot - missing icons after flash

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

3 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)
(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)
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
Last Resolved: 4 years ago3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Depends on: 1220311
You need to log in before you can comment on or make changes to this bug.