webapps: introduce helper class to store extended set of data

chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappDataStorage.java

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.webapps;
+
+import android.content.Context;
+import android.content.SharedPreferences;
+import android.graphics.Bitmap;
+import android.graphics.BitmapFactory;
+import android.os.AsyncTask;
+import android.text.TextUtils;
+import android.util.Base64;
+
+import java.io.ByteArrayOutputStream;
+
+/**
+ * This is a class used to store data about an installed webapp.
+ */
+public class WebappDataStorage {
+
+    private final SharedPreferences mPreferences;
+
+    private WebappDataStorage(Context context, String webappId) {
+        mPreferences = context.getSharedPreferences("webapp_" + webappId,
+            Context.MODE_PRIVATE);
+        updateLastAccessedDate();
+    }
+
+    public static WebappDataStorage open(Context context, String webappId) {
+        return new WebappDataStorage(context.getApplicationContext(), webappId);
+    }
+
+    public void getSplashIcon(final Callback<Bitmap> callback) {
+        new FetchTask<Bitmap>(callback) {
+            @Override
+            protected Bitmap fetchSharedPreference() {
+                String icon = mPreferences.getString("splash_icon", "");

[gone, 2015/08/24 21:15:16]

Pull these keys into private static final Strings.

[Lalit Maganti, 2015/08/25 13:35:17]

Done.

+                Bitmap decodedIcon = null;
+                if (!TextUtils.isEmpty(icon)) {
+                    byte[] decoded = Base64.decode(icon, Base64.DEFAULT);
+                    decodedIcon = BitmapFactory.decodeByteArray(decoded, 0, decoded.length);
+                }
+                return decodedIcon;
+            }
+        }.execute();
+    }
+
+    public WebappDataStorage putSplashIcon(final Bitmap icon) {
+        new PutTask() {
+            @Override
+            protected void putSharedPreference() {
+                ByteArrayOutputStream output = new ByteArrayOutputStream();
+                icon.compress(Bitmap.CompressFormat.PNG, 100, output);
+                String splash_icon_base64 = Base64.encodeToString(
+                        output.toByteArray(), Base64.DEFAULT);
+                mPreferences.edit()
+                    .putString("splash_icon", splash_icon_base64)
+                    .commit();
+            }
+        }.execute();
+        return this;
+    }
+
+    public WebappDataStorage updateLastAccessedDate() {
+        new PutTask() {
+            @Override
+            protected void putSharedPreference() {
+                mPreferences.edit()
+                    .putLong("last_accessed", System.currentTimeMillis())
+                    .commit();
+            }
+        }.execute();
+        return this;
+    }
+
+    /**
+     * Callback class used when items are requested from the storage.
+     */
+    public interface Callback<T> {
+        public void run(T readObject);
+    }
+
+    private abstract static class FetchTask<T> extends AsyncTask<Void, Void, T> {
+
+        private final Callback<T> mCallback;
+
+        public FetchTask(Callback<T> callback) {
+            mCallback = callback;
+        }
+
+        @Override
+        protected final T doInBackground(Void... nothing) {
+            return fetchSharedPreference();

[gone, 2015/08/24 20:22:46]

Is there any reason why you set these AsyncTasks this way (with abstract
methods)?

Instead of abstracting away the AsyncTasks with new classes that don't do much,
you should just use AsyncTask directly and put the code into doInBackground().

[Lalit Maganti, 2015/08/24 20:34:23]

Right now this doesn't make any sense to add this indirection but with the
eventual aim of making this class hold all data for webapps, it makes much more
sense to extract as much code (especially the callback code in FetchTask) as
possible.

[gone, 2015/08/24 20:37:33]

I'd suggest dealing with abstracting it away when you actually have other data
to deal with.  Right now it just makes the code look more complicated than it
really is.  PutTask (in its current state) just adds another layer between the
task and doInBackground() since it doesn't even have a callback to worry about.

[Lalit Maganti, 2015/08/24 20:43:20]

The eternal struggle between abstraction and complexity :P

OK for Put I agree there isn't much benefit. I personally feel that it's a good
idea to keep Fetch because I'm almost certain it will be used and the usecase
for it. If you feel otherwise though I will remove.

[gone, 2015/08/24 20:57:36]

Do you have a concrete second use case for abstracting FetchTask?  If your
second use case is also an icon I'd suggest changing FetchTask to FetchIconTask,
which takes in the preference name -- the logic for dealing with empty encoded
bitmap strings will likely be shared.  If you're going to make the FetchTask
retrieve generic strings, I imagine you'd want a different class for that,
anyway, since the AsyncTask signatures would be different.

[Lalit Maganti, 2015/08/24 21:00:16]

I'd imagine we'd use this for name/short name as well as the recents icon yes.
The signature difference is accounted for in the generic T.

[gone, 2015/08/24 21:15:16]

I'd still suggest separating the image and string fetches out into different
AsyncTask types.  The icon saving/loading and encoding/decoding logic will have
to be shared eventually; they may as well be done in the AsyncTask subclass
itself rather than having effectively duplicated fetchSharedPreference() calls. 
I guess an alternative is to have a generic WebappDataStorage.fetchBitmap() call
that takes in a generic preference name and then abstracts away the decoding,
but that'd still be more complicated than just putting it into the AsyncTask
itself.

[Lalit Maganti, 2015/08/25 13:35:17]

OK I've split off the icon saving/loading logic into their own classes. For now,
the benefit of doing the same for the string classes is negligible so I'd leave
that till when the code for new values are added.

+        }
+
+        @Override
+        protected final void onPostExecute(T result) {
+            mCallback.run(result);
+        }
+
+        protected abstract T fetchSharedPreference();
+    }
+
+    private abstract static class PutTask extends AsyncTask<Void, Void, Void> {
+
+        @Override
+        protected final Void doInBackground(Void... nothing) {
+            putSharedPreference();
+            return null;
+        }
+
+        protected abstract void putSharedPreference();
+    }
+}