Closed Bug 1272102 Opened 4 years ago Closed 4 years ago

[FlyWeb] Review Android changes for landing in mozilla-central

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: djvj, Assigned: justindarc)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch flyweb-android.patch (obsolete) — Splinter Review
Sub-bug for reviewing Android changes for landing in m-c.
Android code, let's keep filed under the Android component.
Component: Networking → General
Product: Core → Firefox for Android
Comment on attachment 8751442 [details] [diff] [review]
flyweb-android.patch

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

::: mobile/android/base/java/org/mozilla/gecko/home/HomeConfig.java
@@ +41,5 @@
>      @RobocopTarget
>      public static enum PanelType implements Parcelable {
>          TOP_SITES("top_sites", TopSitesPanel.class),
>          BOOKMARKS("bookmarks", BookmarksPanel.class),
> +        FLYWEB("flyweb", FlyWebPanel.class),

You're not planning to land this home panel, right? My understanding is the plan was to create a system add-on that builds this UI.

I really don't want us to create another home panel.
Attached patch bug1272102.patch (obsolete) — Splinter Review
Margaret, here's the final FlyWeb Fennec UI patch to be landed on m-c.
Assignee: nobody → jdarcangelo
Attachment #8751442 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8759295 - Flags: review?(margaret.leibovic)
Attached patch bug1272102.patch (obsolete) — Splinter Review
Made correction to follow updated FlyWeb API naming convention.
Attachment #8759295 - Attachment is obsolete: true
Attachment #8759295 - Flags: review?(margaret.leibovic)
Attachment #8759513 - Flags: review?(margaret.leibovic)
Comment on attachment 8759295 [details] [diff] [review]
bug1272102.patch

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

Drive-by review of the Java bits.. :)

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +763,5 @@
>                        .run();
>          }
>  
> +        // Check if this is a new installation or if the app has been updated since the last time the
> +        // bundled system add-ons were extracted from the APK.

Nice, I wanted to build such a service for generic tasks in the past ("Run this code on app update"). But so far this hasn't happened.

@@ +764,5 @@
>          }
>  
> +        // Check if this is a new installation or if the app has been updated since the last time the
> +        // bundled system add-ons were extracted from the APK.
> +        if (!AppConstants.MOZ_APP_BUILDID.equals(prefs.getString(GeckoPreferences.PREFS_EXTENSIONS_SYSTEM_ADDONS_LAST_BUILD_ID, null))) {

I was a bit afraid that the build id might always change for local builds and that this code runs all the time while developing locally. But it seems like the gradle build does not update this build id, so this shouldn't be a problem.

@@ +3081,5 @@
>  
> +    /**
> +     * Copies the /assets/features folder out of the APK and into the app's data directory.
> +     */
> +    private void copyFeaturesFromAPK() {

We recently introduced BrowserAppDelegate[1] to move code that depends on the lifecycle of BrowserApp into separate classes (BrowserApp is already huge and this patch adds even more code) This here could be moved into a separate class implementing BrowserAppDelegate too. See [2] and [3] for some examples. Register your class here: [4].

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/delegates/BrowserAppDelegate.java
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/delegates/BookmarkStateChangeDelegate.java
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/promotion/AddToHomeScreenPromotion.java
[4] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#307-313

@@ +3090,5 @@
> +        final String fullPrefix = assetsPrefix + "features/";
> +
> +        final SharedPreferences prefs = GeckoSharedPrefs.forApp(this);
> +
> +        ThreadUtils.postToBackgroundThread(new Runnable() {

This is on a background thread but it might still affect app startup because there's so much other stuff happening (especially with Gecko starting up).

If possible start this after receiving the the Gecko:DelayedStartup event:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1867

BrowserAppDelegate doesn't have a callback method for delayed startup yet [1]. But it should be trivial to add.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/delegates/BrowserAppDelegate.java

@@ +3096,5 @@
> +            public void run() {
> +                Log.d(LOGTAG, "Copying system add-ons from APK to dataDir");
> +
> +                try {
> +                    final ZipFile zip = new ZipFile(applicationPackage);

This is reading the whole APK as zip file and searching for the assets. I assume this could be easier by calling getAssets() [1] and using list() / open() on the AssetManager [2].

[1] https://developer.android.com/reference/android/content/Context.html#getAssets()
[2] https://developer.android.com/reference/android/content/res/AssetManager.html

@@ +3098,5 @@
> +
> +                try {
> +                    final ZipFile zip = new ZipFile(applicationPackage);
> +                    final Enumeration<? extends ZipEntry> zipEntries = zip.entries();
> +                    

nit: whitespaces

@@ +3131,5 @@
> +                        }
> +                    }
> +
> +                    zip.close();
> +                } catch (Exception e) {

I'm not really a fan of catching the generic 'Exception'. If this breaks then no one will easily notice because we just catch everything here. I guess we only really need to catch the expected exceptions here (IOException?)

@@ +3149,5 @@
> +     * @return null if the parents could not be created.
> +     */
> +    private File getDataFile(final String name) {
> +        final String dataDir = getApplicationInfo().dataDir;
> +        

nit: whitespaces

@@ +3164,5 @@
> +
> +        return outFile;
> +    }
> +
> +    private void writeStream(InputStream fileStream, File outFile, final long modifiedTime, byte[] buffer)

IOUtils and/or FileUtils has helper methods for such tasks.
Attachment #8759295 - Flags: feedback+
Attached patch bug1272102.patch (obsolete) — Splinter Review
Addressed :sebastian's comments on the Java parts.
Attachment #8759513 - Attachment is obsolete: true
Attachment #8759513 - Flags: review?(margaret.leibovic)
Attachment #8760424 - Flags: review?(margaret.leibovic)
Comment on attachment 8760424 [details] [diff] [review]
bug1272102.patch

Redirecting this to Sebastian, since he already started :)
Attachment #8760424 - Flags: review?(margaret.leibovic) → review?(s.kaspari)
Comment on attachment 8760424 [details] [diff] [review]
bug1272102.patch

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

::: mobile/android/base/java/org/mozilla/gecko/updater/PostUpdateHandler.java
@@ +34,5 @@
> +public class PostUpdateHandler extends BrowserAppDelegateWithReference {
> +    private static final String LOGTAG = "PostUpdateHandler";
> +
> +    @Override
> +    public void onStart(BrowserApp browserApp) {

Still wondering: Could this be called when we receive the DelayedStartup event? With that this won't interfere with app startup and run after the app/gecko is fully initialized. If this isn't a problem then I can add the necessary callbacks to BrowserAppDelegate in a follow-up bug.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1882

@@ +53,5 @@
> +    private void copyFeaturesFromAPK() {
> +        final BrowserApp browserApp = getBrowserApp();
> +        if (browserApp == null) {
> +            return;
> +        }

onStart() gets the BrowserApp object passed in. You could just pass it to this method and avoid the null check here.

@@ +64,5 @@
> +        final String fullPrefix = assetsPrefix + "features/";
> +
> +        final SharedPreferences prefs = GeckoSharedPrefs.forApp(browserApp);
> +
> +        ThreadUtils.postToBackgroundThread(new Runnable() {

I'd move everything to a background thread much earlier, right after onStart() has been called. There's no reason to do anything on the main thread here.

@@ +70,5 @@
> +            public void run() {
> +                Log.d(LOGTAG, "Copying system add-ons from APK to dataDir");
> +
> +                try {
> +                    final ZipFile zip = new ZipFile(applicationPackage);

I still think it's unnecessary to go through the whole APK if we are only interested in some files in the assets folder and the system has methods for doing that. Can we at least file a follow-up bug for this? I can look into writing the patch for that if you want to.

@@ +72,5 @@
> +
> +                try {
> +                    final ZipFile zip = new ZipFile(applicationPackage);
> +                    final Enumeration<? extends ZipEntry> zipEntries = zip.entries();
> +                    

nit: trailing whitespaces

@@ +104,5 @@
> +                            fileStream.close();
> +                        }
> +                    }
> +
> +                    zip.close();

This should be closed in a 'finally' block so that it is guaranteed to always get closed.
Attachment #8760424 - Flags: review?(s.kaspari) → review+
Attached patch bug1272102.patchSplinter Review
Addressed comments. Carrying R+ from :sebastian
Attachment #8760424 - Attachment is obsolete: true
Attachment #8761365 - Flags: review+
Kannan, I just pulled down m-i and it looks like you pushed the previous version of the patch before I addressed :sebastian's comments. Can you please revert and push the latest version posted to this bug when you get a chance? Thanks!
Flags: needinfo?(kvijayan)
https://hg.mozilla.org/mozilla-central/rev/8b658fb9b510
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(In reply to Justin D'Arcangelo [:justindarc] from comment #11)
> Kannan, I just pulled down m-i and it looks like you pushed the previous
> version of the patch before I addressed :sebastian's comments. Can you
> please revert and push the latest version posted to this bug when you get a
> chance? Thanks!

I just noticed this too. I'll reopen the bug. :)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by kvijayan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee4fbc79fba
FlyWeb Android UI delta patch - last push was not final version of patch. r=sebastian
Depends on: 1279330
https://hg.mozilla.org/mozilla-central/rev/7ee4fbc79fba
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Clearing needinfo since the request was addressed and this has been fixed for a while now.
Flags: needinfo?(kvijayan)
You need to log in before you can comment on or make changes to this bug.