Closed Bug 957057 Opened 7 years ago Closed 7 years ago
figure out why Browser
App needs to import Apps Utils
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
(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.
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.
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+
revised patch based on feedback
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.
Fixed import indentation nit
Attachment #8363561 - Flags: review?(wjohnston) → review+
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.