Adding CrHome-specific implementation for home page tile.

chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java

Index: chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java
index 78dacb10c4fbd1fb09c033f3f10fc6f0c0a647dc..71d95c875e8c99c2c9fd337eee7d84335c4f4d38 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java
@@ -4,14 +4,20 @@
 
 package org.chromium.chrome.browser.suggestions;
 
+import static org.chromium.base.ContextUtils.getApplicationContext;

[dgn, 2017/05/25 10:33:33]

just use a regular import here, and call ContextUtils.getApplicationContext.

For tests static imports are fine to make things less verbose, but it just looks
confusing in production code.

[fhorschig, 2017/05/30 10:25:21]

Done.

+
 import org.chromium.base.annotations.JNIAdditionalImport;
+import org.chromium.chrome.browser.ChromeFeatureList;
+import org.chromium.chrome.browser.UrlConstants;
+import org.chromium.chrome.browser.partnercustomizations.HomepageManager;
 import org.chromium.chrome.browser.profiles.Profile;
 
 /**
  * Methods to bridge into native history to provide most recent urls, titles and thumbnails.
  */
 @JNIAdditionalImport(MostVisitedSites.class) // Needed for the Observer usage in the native calls.
-public class MostVisitedSitesBridge implements MostVisitedSites {
+public class MostVisitedSitesBridge
+        implements MostVisitedSites, HomepageManager.HomepageStateListener {
     private long mNativeMostVisitedSitesBridge;
 
     /**
@@ -21,6 +27,11 @@ public class MostVisitedSitesBridge implements MostVisitedSites {
      */
     public MostVisitedSitesBridge(Profile profile) {
         mNativeMostVisitedSitesBridge = nativeInit(profile);
+        // The first tile replaces the home page button (only) in Chrome Home.
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.CHROME_HOME)) {
+            initializeClient();
+            HomepageManager.getInstance(getApplicationContext()).addListener(this);

[dgn, 2017/05/25 10:33:33]

when should the listener be removed? destroy() ?

[fhorschig, 2017/05/30 10:25:21]

Yes. Done.

+        }
     }
 
     /**
@@ -76,10 +87,43 @@ public class MostVisitedSitesBridge implements MostVisitedSites {
         nativeRecordOpenedMostVisitedItem(mNativeMostVisitedSitesBridge, index, type, source);
     }
 
+    @Override
+    public void onHomepageStateUpdated() {
+        if (mNativeMostVisitedSitesBridge == 0) {

[dgn, 2017/05/25 10:33:33]

can it happen in the normal functioning of the class? just assert otherwise?
That's what we do in the other functions.

[fhorschig, 2017/05/30 10:25:21]

Changed to assert.
This can not happen anymore (because the listener is always removed before
destroy()-ing the mNativeBridge; and initialized after native initialization).

+            return; // Calling the blacklist only works if the native side of the bridge is alive.
+        }
+        // Ensure the home page is set as first tile if (re-)enabled - even if blacklisted before.

[dgn, 2017/05/25 10:33:33]

how does that set the tile as first? to me it only looks like it just removes it
from the backlist. Isn't the order implementation detail thus not useful as a
comment here?

[fhorschig, 2017/05/30 10:25:21]

Absolutely. Gone.

+        if (HomepageManager.isHomepageEnabled(getApplicationContext())) {
+            removeBlacklistedUrl(HomepageManager.getHomepageUri(getApplicationContext()));
+        }
+    }
+
+    private void initializeClient() {
+        nativeSetClient(mNativeMostVisitedSitesBridge, new Client() {
+            @Override
+            public boolean isHomePageEnabled() {
+                return HomepageManager.isHomepageEnabled(getApplicationContext());
+            }
+
+            @Override
+            public boolean isNewTabPageUsedAsHomePage() {
+                return HomepageManager.getHomepageUri(getApplicationContext())

[dgn, 2017/05/25 10:33:33]

compare strings with TextUtils.equals(s1, s2), the home page URI being null here
would cause a crash. Also, why not just use getHomePageUrl here?

[fhorschig, 2017/05/30 10:25:21]

Thanks, done and used.
(This code used to work because isHomePageEnabled() is checked before anything
else but it's of course better to avoid the pitfalls for future changes.)

+                        .equals(UrlConstants.NTP_URL);
+            }
+
+            @Override
+            public String getHomePageUrl() {

[dgn, 2017/05/25 10:33:33]

this can return null, mark @Nullable? also, does the backend properly handle
that?

[fhorschig, 2017/05/30 10:25:21]

@Nullable added.
The backend handles null values now. (Previously, it relied on checking whether
the feature is enabled. But setting an empty page would have crashed.)

+                return HomepageManager.getHomepageUri(getApplicationContext());
+            }
+        });
+    }
+
     private native long nativeInit(Profile profile);
     private native void nativeDestroy(long nativeMostVisitedSitesBridge);
     private native void nativeSetObserver(
             long nativeMostVisitedSitesBridge, MostVisitedSites.Observer observer, int numSites);
+    private native void nativeSetClient(
+            long nativeMostVisitedSitesBridge, MostVisitedSites.Client client);
     private native void nativeAddOrRemoveBlacklistedUrl(
             long nativeMostVisitedSitesBridge, String url, boolean addUrl);
     private native void nativeRecordPageImpression(