Closed Bug 849988 Opened 9 years ago Closed 9 years ago

Implement support for a removable property for preinstalled apps to define if the app can be uninstalled or not

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:tef+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: jsmith, Assigned: fabrice)

References

Details

(Whiteboard: [Apps Watch List] QARegressExclude)

Attachments

(2 files)

Currently, our preinstalled app framework does not support specifying a "removable" property in the metadata.json file to indicate if the app can be removed or not. There some back and forth debate on whether we should have the ability to support non-removable apps, but let's handle the actual definition of using it in a different bug. This bug focuses on implementing the support for a removable property to allow for an app to be uninstalled or not.

If no removable property is specified, then the default should be that the preinstalled app is removable.
Duplicate of this bug: 848432
Blocks: 839001
partner is waiting on a patch for this. anyone can take this?
tef? as this is needed to make 3rd party apps removable by default
blocking-b2g: --- → tef?
(In reply to Joe Cheng [:jcheng] from comment #2)
> partner is waiting on a patch for this. anyone can take this?

Probably someone who knows the build system piece for apps, potentially the core dom:apps pieces might a person to hunt for. Fabrice would be a good person to ask.

I was experimenting around with the source here:

https://github.com/mozilla-b2g/gaia/blob/master/build/webapp-manifests.js

Where I actually noticed the odd behavior that we:

1. Never read any property called removable from metadata.json
2. Default to removable to true on a packaged app and undefined on a hosted app
(tef- for now, as it's not clear why do we must support removing preinstalled apps?  how does that work with the device "Reset" functionality in settings for example?  For v1.0.1, I think we should treat all preloads as fixed just like the build-in core apps given that we are mostly out of time.)
blocking-b2g: tef? → -
(In reply to Michael Vines [:m1] [:evilmachines] from comment #5)
> (tef- for now, as it's not clear why do we must support removing
> preinstalled apps? 

I believe this was part of the partner customization requirements for specific locale builds for any app that was not deemed core. For core apps, they would not be non-removable.

> how does that work with the device "Reset" functionality
> in settings for example?

Good question. I actually don't know this answer. Need info on product to clarify.

> For v1.0.1, I think we should treat all preloads
> as fixed just like the build-in core apps given that we are mostly out of
> time.)

Well, that's not the current behavior either. We currently do the following:

* If the app is a hosted preinstalled app, it's non-removable by default
* If the app is preinstalled packaged app, it's removable by default

Thoughts? The behavior is a tad awkward in it's current state, so we really do need to pick one or the other.
Flags: needinfo?(mvines)
Flags: needinfo?(ffos-product)
Attached patch gaia patchSplinter Review
Assignee: nobody → fabrice
Attachment #725076 - Flags: review?(ferjmoreno)
Attached patch gecko patchSplinter Review
On the gecko side, we remove any use of the 'removable' property as an indicator of a non-system app. Instead, we check the basePath and compare it to the core apps path. I also removed the hardcoded "removable=true" when preinstalling 3rd party packaged apps. Gaia now sets the removable flag.

On the gaia side, the metadata.json now supports a "removable" field that defaults to true. I turned it to false for the maps app, and this will haunt me for the next 100 years.
Attachment #725079 - Flags: review?(ferjmoreno)
Comment on attachment 725079 [details] [diff] [review]
gecko patch

Review of attachment 725079 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Fabrice! Looks good. r=me

::: dom/apps/src/Webapps.jsm
@@ +1994,5 @@
>        return;
>      }
>  
>      // the manifest file used to be named manifest.json, so fallback on this.
> +    //let baseDir = (this.webapps[id].removable ? DIRECTORY_NAME : "coreAppsDir");

Remove the comment, please
Attachment #725079 - Flags: review?(ferjmoreno) → review+
(In reply to Michael Vines [:m1] [:evilmachines] from comment #5)
> (tef- for now, as it's not clear why do we must support removing
> preinstalled apps?  how does that work with the device "Reset" functionality
> in settings for example?  For v1.0.1, I think we should treat all preloads
> as fixed just like the build-in core apps given that we are mostly out of
> time.)

from Bug 840891 comment 53 and on, you should see more background on it. Basically all preloaded 3rd party apps are expected to be removed by end users
blocking-b2g: - → tef?
Comment on attachment 725076 [details] [diff] [review]
gaia patch

Review of attachment 725076 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/webapp-manifests.js
@@ +133,5 @@
>      webapp.manifestFile.copyTo(webappTargetDir, 'manifest.webapp');
>      origin = webapp.metaData.origin;
> +    installOrigin = webapp.metaData.installOrigin || webapp.metaData.origin;
> +    manifestURL = webapp.metaData.manifestURL ||
> +                    (webapp.metaData.origin + 'manifest.webapp');

nit: alignment
Attachment #725076 - Flags: review?(ferjmoreno) → review+
Whiteboard: [Apps Watch List]
Gaia part pushed to master: https://github.com/mozilla-b2g/gaia/commit/718b76a95d2bc8ecf928d56a308b36cd8b69b3bc

Don't mark as fixed until I land the gecko patch (inbound is closed).
(In reply to Joe Cheng [:jcheng] from comment #10)
> from Bug 840891 comment 53 and on, you should see more background on it.

I can't access this bug.
Flags: needinfo?(mvines)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/16ac88f4d5b6 for timing out test_bug_779982.html.
And also that bunch of localstorage tests in mochitest-chrome that always bust when webapp tests are busted.
Pushed to try with a small fix to make sure I cover all non-b2g platforms:
https://tbpl.mozilla.org/?tree=Try&rev=71e5fcee49cf
This commit rendered the whole of Gaia unbootable for me. Can we back it out?
(In reply to Ben Francis [:benfrancis] from comment #19)
> This commit rendered the whole of Gaia unbootable for me. Can we back it out?

Yes, let's back out then.
Whiteboard: [Apps Watch List] → [Apps Watch List][backout needed]
Need info on fabrice to address comment 19.
Flags: needinfo?(ffos-product) → needinfo?(fabrice)
I have backed out the gaia part in:
https://github.com/mozilla-b2g/gaia/commit/532a60eabbf3a97f0df6a274614dec3d2d0d8ec7

because it caused the regression reported in https://groups.google.com/d/msg/mozilla.dev.gaia/C1Cqw4Z0CLE/9McijzGUgWAJ which causes gaia to fail to start up.

It seems that |make production| still worked, but a |make install-gaia| or |make reset-gaia| would result in a gaia which couldn't be started.
That broke the eng builds. I'm looking at what's happening for those. My bad for testing only with a production setting.
Flags: needinfo?(fabrice)
Why was this landed without a test? Please back out the Gecko part and re-land with a test.
(semi-blind tef+ purely based on comment 3, as I can't see the full justification as per comment 13)
blocking-b2g: tef? → tef+
https://hg.mozilla.org/mozilla-central/rev/a63b7a4c03f8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Reopening for the fact the Gaia part was backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [Apps Watch List][backout needed] → [Apps Watch List]
Keywords: checkin-needed
Setting status-firefox22 back to affected - we've only landed the gecko changes here. The Gaia changes was backed out.
Fabrice, the gaia patch was backed out.  Do you have time to investigate or should we find someone else to take it from here?
Flags: needinfo?(fabrice)
I need to re-test now that the gecko patch has been uplifted, and reland.
Flags: needinfo?(fabrice)
Re-landed: https://github.com/mozilla-b2g/gaia/commit/99267614620b8c09a82711d4ded14a87211d263b
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Keywords: verifyme
QA Contact: jsmith
Does not make sense to create a regression issue.
Flags: in-moztrap-
Uplifted commit 99267614620b8c09a82711d4ded14a87211d263b as:
v1-train: deabd66c2c5b555d83795284bc372b1c77af413f
v1.0.1: 4b3b2b98ca0659dbc9883f3675215e0b0c6c4a0f
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
(In reply to croesch from comment #35)
> Can you please provide steps to verify this fix - as we will blackbox test
> from the UI?

The maps app should be by default be non-removable as a result of this patch. Already checked and it is okay. That's one blackbox flow.

There's also more testing at the build system level, but that's on my end. I'd ignore this bug for your testing you guys are doing.
Whiteboard: [Apps Watch List] → [Apps Watch List] QARegressExclude
Being left to Jason Smith per Comment 36.
Verified sanity-wise by seeing the build generated for maps that is non-removable and not being able to uninstall it.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.