1021342 - Eliminate non-synthetic web app code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[Richard Newman [:rnewman]]

Starting with removing omg.WebappImpl, and eliminating conditionals elsewhere.

I'll tackle this prior to Bug 1016611.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Eliminate non-synthetic web app code. v1

Haven't built this yet, but should be everything. Gonna clobber to test.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8435362 [details] [diff] [review]
Eliminate non-synthetic web app code. v1

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

This builds, and there's no trace of the var left in tree or objdir, so I think it's ready for review.

I'm interested if there's anything more we should delete.

Myk, do you have a good way to validate this change?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8435362 [details] [diff] [review]
Eliminate non-synthetic web app code. v1

Myk should have final review on this. It looks like you covered the necessary bits and I am good to have the build flag removed at this point.

Myk might know if we have any other patches in the works that will be bitrotted by this. We should ping the devs for a heads up. Marco might have something in the works somewhere. He usually does.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8435362 [details] [diff] [review]
Eliminate non-synthetic web app code. v1

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

> I'm interested if there's anything more we should delete.

The only other remnants of pre-synthapk runtime are the aboutApps.uninstall and aboutApps.addToHomescreen strings in mobile/android/locales/en-US/chrome/aboutApps.dtd.

But I want to keep those around for now (at least the "uninstall" string), as I'm going to re-enable the uninstall context menu item over in bug 1019054, and I might want to uplift that patch to Aurora afterward, for which we'd need to leave the strings in the tree.


> Myk, do you have a good way to validate this change?

Unfortunately, we don't have unit test coverage for the synthapk-based runtime (bug 1009954 is the first step in that direction).

But it's worth using tryserver to run the mochitest-chrome, mochitest-content, and xpcshell tests in dom/apps/, dom/tests/mochitest/webapps/, and toolkit/webapps/, since those'll at least confirm that we haven't broken the cross-runtime code in dom/apps/ for anyone else.

::: dom/apps/src/AppsUtils.jsm
@@ +74,1 @@
>    aObj.apkPackageName = aApp.apkPackageName;

This should still be restricted to the Android runtime, since the property isn't defined on other platforms.  Use |#ifdef MOZ_WIDGET_ANDROID| to do that.  (There's also ANDROID, but that one is defined for both Fennec and B2G, so we can't use it here.)

(Note that JavaScript would happily assign the "undefined" value to aObj.apkPackageName if aApp has no property named "apkPackageName".  But then aObj would have that property, even if its value is undefined; so |"apkPackageName" in aObj| would be true even though |"apkPackageName" in aApp| is false.  That's unlikely to cause a problem, but I'd rather keep this Fennec-only while we continue to look for better patterns for sharing dom/apps/ code across runtimes.)

::: dom/apps/src/Webapps.jsm
@@ +1090,1 @@
>          Services.obs.notifyObservers(mm, "webapps-runtime-install", JSON.stringify(msg));

Here (and elsewhere in Webapps.jsm) we still need to restrict this to |#ifdef MOZ_WIDGET_ANDROID| and call this.doInstall directly if that Make variable isn't defined.  Sorry, I know this is sucky!  We're working on better hooks for runtime-specific functionality over in bug 910473.

::: mobile/android/base/moz.build
@@ +403,5 @@
>      'webapp/InstallListener.java',
>      'webapp/TaskKiller.java',
>      'webapp/UninstallListener.java',
>      'webapp/WebappImpl.java',
>      'WebappAllocator.java',

Also remove WebappAllocator.java!  It's only used by the old implementation (the synthapk allocator is at webapp/Allocator.java).

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Myk might know if we have any other patches in the works that will be
> bitrotted by this. We should ping the devs for a heads up. Marco might have
> something in the works somewhere. He usually does.

Yes, Marco's work in bug 910473 and its dependencies will be horked, as will tedders1's work in bug 1000315 and my work in bug 1019054.  There're probably others.  cc:ing those folks along with Martyn and James!

Comment 6

[Marco Castelluccio [:marco]]

I'll need to rebase my patch in bug 1007402 but I'm happy to do it for a greater good :D

Comment 7

[Richard Newman [:rnewman]]

Attachment: Eliminate non-synthetic web app code. v2

Try:

https://tbpl.mozilla.org/?tree=Try&rev=ea74e0603ba1

I switched everything in Webapps.jsm to be MOZ_WIDGET_ANDROID rather than eliminating clauses, and addressed other comments.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8436183 [details] [diff] [review]
Eliminate non-synthetic web app code. v2

Try build looks green (but please make sure it ran everything you expected it to).

Comment 9

[Marco Castelluccio [:marco]]

You could also remove some webapp-related strings from browser.properties.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8436183 [details] [diff] [review]
Eliminate non-synthetic web app code. v2

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

(In reply to Marco Castelluccio [:marco] from comment #9)
> You could also remove some webapp-related strings from browser.properties.

Indeed!  The new implementation's strings are in webapp.properties, so we should remove the strings from lines 245 to 253 of browser.properties:

https://github.com/mozilla/gecko-dev/blob/967b66a647369f14a33cb2bce3cfb2b11926f9c9/mobile/android/locales/en-US/chrome/browser.properties#L245-L253

Also, note that my patch in bug 1019054 is racing this patch.  I'm pretty sure I'm going to lose this one. :-)

::: dom/apps/src/Webapps.jsm
@@ +2510,3 @@
>        // In that case, we would already have the manifest, not just the update
>        // manifest.
> +#ifdef MOZ_WIDGET_ANDROID

Nice catch!  That's a better ordering of comment and directive.

@@ +2514,5 @@
>  #else
> +      if (aData.app.localInstallPath) {    
> +        dontNeedNetwork = true;   
> +        jsonManifest.package_path = "file://" + aData.app.localInstallPath;   
> +      }   

Nit: you've inadvertently introduced four occurrences of trailing whitespace here.

Comment 11

[Richard Newman [:rnewman]]

Pushed with comments addressed (including string removal).

Thanks, folks!

https://hg.mozilla.org/integration/fx-team/rev/729d275bcb9d

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/729d275bcb9d