957057 - figure out why BrowserApp needs to import AppsUtils

Status: RESOLVED FIXED

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

Description

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

BrowserApp imports AppsUtils and appears to need it in order to install and/or run apps via synthetic APKs, but it isn't clear why it needs it, so we should figure that out and add a code comment explaining the reason.

Comment 1

[Martyn Haigh (:mhaigh)]

ManifestHelper object in AppUtils is used in the doInstall function

https://github.com/mykmelez/gecko-dev/commit/5fe4a168b2e48a717568c7865d44e15eb5a02899

Comment 2

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

(In reply to Martyn Haigh (:mhaigh) from comment #1)
> ManifestHelper object in AppUtils is used in the doInstall function
> 
> https://github.com/mykmelez/gecko-dev/commit/
> 5fe4a168b2e48a717568c7865d44e15eb5a02899

The line in question is #ifdef MOZ_ANDROID_SYNTHAPKS, while WebappsUI.doInstall is #ifndef MOZ_ANDROID_SYNTHAPKS (and gets AppsUtils from WebappsUI.init), so that isn't it.

Comment 3

[Martyn Haigh (:mhaigh)]

Removing the import creates an error in WebAppRT.js on line 70 (https://github.com/mykmelez/gecko-dev/blob/apks/mobile/android/chrome/content/WebAppRT.js#L70) : let manifest = new ManifestHelper(app.manifest, app.origin);

WebAppRT is called in browser.js in _initRuntime only.  Moving the import to be included within that function seems to return behavior to normal although I'm not sure this hasn't cause other side effects which I haven't managed to test.

On closer inspection I think the import is in a #ifdef MOZ_ANDROID_SYNTHAPKS block as there is another import for the same file within a #ifndef MOZ_ANDROID_SYNTHAPKS block.  We should remove the import from any processed block as it seems that both paths need it.

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: 0001-957057-move-import.patch

Moved the import as both ifdef and ifndef were importing

Comment 5

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

Comment on attachment 8361110 [details] [diff] [review]
0001-957057-move-import.patch

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

This is ok, but as I mentioned this morning, I'd rather we import AppsUtils in WebappRT.js in the MOZ_ANDROID_SYNTHAPKS case, so the import happens in the script that uses the imported module.

We can still also import it in browser.js for the non-MOZ_ANDROID_SYNTHAPKS case, even if it means importing it twice (since the second import will just return the cached singleton).  But I would leave that import in the WebappsUI.init method, which may be lazier than doing it in the main body of the script.


Also, note that this patch appears to be a patch against the previous version of your patch rather than master, so it doesn't apply to master.  To generate a patch relative to master, you'll need to squash the two commits, perhaps on a throwaway branch.

Some of the ways to do that are described in this Stack Overflow thread:

  http://stackoverflow.com/questions/616556/how-do-you-squash-commits-into-one-patch-with-git-format-patch

But perhaps the simplest way, given a <branch> containing your changes and a <commit message> describing them, is:

git checkout -b temp master              # create temporary branch based on master
git diff master..apk-name | git apply -  # apply changes between master and <branch>
git commit -a -m"<commit message>"       # commit changes with appropriate message
git format-patch -1 HEAD                 # generate patch
git checkout master                      # change back to master branch
git branch -D temp                       # delete temp branch

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: 0001-957057.patch

revised patch based on feedback

Comment 7

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

Comment on attachment 8361761 [details] [diff] [review]
0001-957057.patch

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

Besides the nit, this looks good!  Let's get WesJ to review it.

::: mobile/android/chrome/content/WebAppRT.js
@@ +9,5 @@
>  Cu.import("resource://gre/modules/FileUtils.jsm");
>  Cu.import("resource://gre/modules/NetUtil.jsm");
>  Cu.import("resource://gre/modules/PermissionsInstaller.jsm");
> +#ifdef MOZ_ANDROID_SYNTHAPKS
> +    Cu.import("resource://gre/modules/AppsUtils.jsm");

Nit: in general, we don't indent statements relative to a preprocessor directive.

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: Refactor import in browser.js to make code easier to understand

Fixed import indentation nit

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3382f9254a8

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d3382f9254a8