Closed Bug 957057 Opened 8 years ago Closed 7 years ago

figure out why BrowserApp needs to import AppsUtils


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



(Not tracked)

Firefox 29


(Reporter: myk, Assigned: mhaigh)




(1 file, 2 obsolete files)

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.
ManifestHelper object in AppUtils is used in the doInstall function
Assignee: nobody → mhaigh
(In reply to Martyn Haigh (:mhaigh) from comment #1)
> ManifestHelper object in AppUtils is used in the doInstall function
> 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.
Priority: -- → P3
Removing the import creates an error in WebAppRT.js on line 70 ( : 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.
Attached patch 0001-957057-move-import.patch (obsolete) — Splinter Review
Moved the import as both ifdef and ifndef were importing
Attachment #8361110 - Flags: feedback?(myk)
Comment on attachment 8361110 [details] [diff] [review]

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:

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
Attachment #8361110 - Flags: feedback?(myk) → feedback+
Attached patch 0001-957057.patch (obsolete) — Splinter Review
revised patch based on feedback
Attachment #8361110 - Attachment is obsolete: true
Attachment #8361761 - Flags: feedback?(myk)
Comment on attachment 8361761 [details] [diff] [review]

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");
> +    Cu.import("resource://gre/modules/AppsUtils.jsm");

Nit: in general, we don't indent statements relative to a preprocessor directive.
Attachment #8361761 - Flags: review?(wjohnston)
Attachment #8361761 - Flags: feedback?(myk)
Attachment #8361761 - Flags: feedback+
Fixed import indentation nit
Attachment #8361761 - Attachment is obsolete: true
Attachment #8361761 - Flags: review?(wjohnston)
Attachment #8363561 - Flags: review?(wjohnston)
Attachment #8363561 - Flags: review?(wjohnston) → review+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.