[Enhanced Bookmark]Rewrite initialization logic

chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java

Index: chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java b/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java
index 938d83805bff5c36a4eb809e5d1f1062863c7ecf..7864583baceb5b4f276e97314253c98d7b4361a4 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkUtils.java
@@ -11,7 +11,9 @@ import android.graphics.Bitmap;
 import android.net.Uri;
 import android.os.AsyncTask;
 import android.os.Bundle;
+import android.preference.PreferenceManager;
 import android.provider.Browser;
+import android.text.TextUtils;
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.metrics.RecordUserAction;
@@ -38,6 +40,7 @@ import org.chromium.chrome.browser.util.FeatureUtilities;
 import org.chromium.chrome.browser.util.MathUtils;
 import org.chromium.components.bookmarks.BookmarkId;
 import org.chromium.components.bookmarks.BookmarkType;
+import org.chromium.components.variations.VariationsAssociatedData;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.ui.base.DeviceFormFactor;
 
@@ -45,8 +48,11 @@ import org.chromium.ui.base.DeviceFormFactor;
  * A class holding static util functions for enhanced bookmark.
  */
 public class EnhancedBookmarkUtils {
-
+    private static final String FIELD_TRIAL_NAME = "EnhancedBookmarks";
+    private static final String DEFAULT_FOLDER = "default_folder";
+    private static final String PREF_LAST_USED_URL = "enhanced_bookmark_last_used_url";
     private static final String BOOKMARK_SAVE_NAME = "SaveBookmark";
+    private static final String URI_PERSIST_QUERY_NAME = "should_persist";
     private static final int[] DEFAULT_BACKGROUND_COLORS = {
             0xFFE64A19,
             0xFFF09300,
@@ -255,6 +261,19 @@ public class EnhancedBookmarkUtils {
     }
 
     /**
+     * Gets whether bookmark manager should load offline page initially.
+     */
+    public static boolean shouldShowOfflinePageAtFirst(EnhancedBookmarksModel model,

[Kibeom Kim (inactive), 2015/11/19 19:18:19]

private?

[Ian Wen, 2015/12/01 09:17:14]

Done.

+            Context context) {
+        OfflinePageBridge bridge = model.getOfflinePageBridge();
+        if (bridge == null || bridge.getAllPages().isEmpty()
+                || OfflinePageUtils.isConnected(context)) {
+            return false;
+        }
+        return true;
+    }
+
+    /**
      * Shows enhanced bookmark main UI, if it is turned on. Does nothing if it is turned off.
      * @return True if enhanced bookmark is on, false otherwise.
      */
@@ -262,15 +281,102 @@ public class EnhancedBookmarkUtils {
         if (!isEnhancedBookmarkEnabled()) {
             return false;
         }
+
+        String url = getFirstUrlToLoad(activity);
+
         if (DeviceFormFactor.isTablet(activity)) {
-            openBookmark(activity, UrlConstants.BOOKMARKS_URL);
+            openBookmark(activity, url);
         } else {
-            activity.startActivity(new Intent(activity, EnhancedBookmarkActivity.class));
+            Intent intent = new Intent(activity, EnhancedBookmarkActivity.class);
+            intent.setData(Uri.parse(url));
+            activity.startActivity(intent);
         }
         return true;
     }
 
     /**
+     * The initial url the bookmark manager shows depends on offline page status and some
+     * experiments we run.
+     */
+    private static String getFirstUrlToLoad(Activity activity) {
+        EnhancedBookmarksModel model = new EnhancedBookmarksModel();
+        try {
+            if (shouldShowOfflinePageAtFirst(model, activity.getApplicationContext())) {

[newt (away), 2015/11/19 23:29:28]

Why getApplicationContext()? Activity is a Context too.

[Ian Wen, 2015/12/01 09:17:15]

I copied the implementation that the offline page folks wrote. And in terms of
implementation details an activity is indeed sufficient here. Done. 

+                return encodeUrl(UrlConstants.BOOKMARKS_FILTER_URL,
+                        EnhancedBookmarkFilter.OFFLINE_PAGES.toString(), false);
+            }
+            String lastUsedUrl = getLastUsedUrl(activity);
+            if (!TextUtils.isEmpty(lastUsedUrl)) return lastUsedUrl;
+            if (model.isBookmarkModelLoaded() && VariationsAssociatedData
+                    .getVariationParamValue(FIELD_TRIAL_NAME, DEFAULT_FOLDER)
+                    .equals("mobile")) {

[Kibeom Kim (inactive), 2015/11/19 19:18:19]

I reverted mobile experiment, let's exclude from this patch.

[Ian Wen, 2015/12/01 09:17:15]

Done.

+                return encodeUrl(UrlConstants.BOOKMARKS_FOLDER_URL,
+                        model.getMobileFolderId().toString());
+            }
+            return UrlConstants.BOOKMARKS_URL;
+        } finally {
+            model.destroy();
+        }
+    }
+
+    /**
+     * Saves the last used url to preference. The saved url will be later queried by
+     * {@link #getLastUsedUrl(Context)}
+     */
+    public static void setLastUsedUrl(Context context, String url) {

[newt (away), 2015/11/19 23:29:29]

Make all of these private unless they need to be public. Or make them package
private and mark with @VisibleForTesting is they're used in tests.

[Ian Wen, 2015/12/01 09:17:15]

Done.

+        PreferenceManager.getDefaultSharedPreferences(context).edit()
+                .putString(PREF_LAST_USED_URL, url).apply();
+    }
+
+    /**
+     * Fetches url representing the user's state last time they close the bookmark manager.
+     */
+    public static String getLastUsedUrl(Context context) {
+        return PreferenceManager.getDefaultSharedPreferences(context).getString(
+                PREF_LAST_USED_URL, UrlConstants.BOOKMARKS_URL);
+    }
+
+    /**
+     * Encodes the suffix and append it to the prefix. This is equivalent to
+     * {@link #encodeUrl(String, String, true)}
+     */
+    public static String encodeUrl(String prefix, String suffix) {
+        Uri.Builder builder = Uri.parse(prefix).buildUpon();

[newt (away), 2015/11/19 23:29:28]

This should just forward to the other encodeUrl() method, e.g.

  encodeUrl(prefix, suffix, true);

[Ian Wen, 2015/12/01 09:17:15]

Done.

+        builder.appendPath(suffix);
+        return builder.build().toString();
+    }
+
+    /**
+     * Encodes the suffix as path and append it to the prefix.

[newt (away), 2015/11/19 23:29:28]

This comment doesn't make any sense. Higher level: why does this method exist?
Why might someone call it? What does it return?

[Ian Wen, 2015/12/01 09:17:15]

Rewrote it and added the purpose of the function. Done.

+     * @param shouldPersist Whether this url should be saved to preferences as user's last location.
+     */
+    public static String encodeUrl(String prefix, String suffix, Boolean shouldPersist) {

[Kibeom Kim (inactive), 2015/11/19 19:18:19]

I guess we can use String.valueOf(boolean) at #356 to avoid Boolean object here?

[Ian Wen, 2015/12/01 09:17:15]

Did it in newt's way.

+        Uri.Builder builder = Uri.parse(prefix).buildUpon();
+        builder.appendPath(suffix);
+        builder.appendQueryParameter(URI_PERSIST_QUERY_NAME, shouldPersist.toString());

[newt (away), 2015/11/19 23:29:29]

if (!shouldPersist) {
  builder.appendQueryParameter(URI_PERSIST_QUERY_NAME, "0");
}

[Ian Wen, 2015/12/01 09:17:15]

Done.

+        return builder.build().toString();
+    }
+
+    /**
+     * Gets the suffix of a url and decode it.

[newt (away), 2015/11/19 23:29:28]

Also confused by this comment

[Ian Wen, 2015/12/01 09:17:15]

Rewrote it. Hope it is now better.

+     */
+    public static String decodePath(String url) {

[newt (away), 2015/11/19 23:29:28]

maybe call this "getPathFromUrl()" or even "getLastPathSegment()" because that's
what it's really doing.

[Ian Wen, 2015/12/01 09:17:15]

Done.

+        return Uri.parse(url).getLastPathSegment();
+    }
+
+    /**
+     * Checks if the url contains a query specifying whether the url should be persisted. If no
+     * query is present in the given url, returns true. Otherwise return the value stated by the
+     * query.
+     */
+    public static boolean getShouldPersistFromUrl(String url) {
+        Uri uri = Uri.parse(url);
+        String booleanString = uri.getQueryParameter(URI_PERSIST_QUERY_NAME);
+        if (booleanString == null) return true;
+        return Boolean.parseBoolean(booleanString);

[newt (away), 2015/11/19 23:29:28]

This will crash if it's not parsable. I'd instead check for equality against the
expected value:

  String shouldPersist = uri.getQueryParameter(URI_PERSIST_QUERY_NAME);
  return !("0".equals(shouldPersist));

[Ian Wen, 2015/12/01 09:17:15]

Done.

+    }
+
+    /**
      * Starts an {@link EnhancedBookmarkEditActivity} for the given {@link BookmarkId}.
      */
     public static void startEditActivity(