figure out why BrowserApp needs to import AppsUtils

RESOLVED FIXED in Firefox 29

Status

()

defect
P3
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: myk, Assigned: mhaigh)

Tracking

unspecified
Firefox 29
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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

https://github.com/mykmelez/gecko-dev/commit/5fe4a168b2e48a717568c7865d44e15eb5a02899
Assignee: nobody → mhaigh
(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.
Priority: -- → P3
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.
Posted 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]
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
Attachment #8361110 - Flags: feedback?(myk) → feedback+
Posted 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]
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.
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+
https://hg.mozilla.org/mozilla-central/rev/d3382f9254a8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.