🏠 Extract the ContentSuggestionManager interface from NTP

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

+// Copyright 2017 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.suggestions;
+
+import android.support.annotation.Nullable;
+
+import org.chromium.base.Callback;
+import org.chromium.chrome.browser.favicon.FaviconHelper.FaviconImageCallback;
+import org.chromium.chrome.browser.favicon.FaviconHelper.IconAvailabilityCallback;
+import org.chromium.chrome.browser.favicon.LargeIconBridge.LargeIconCallback;
+import org.chromium.chrome.browser.ntp.NewTabPage.DestructionObserver;
+import org.chromium.chrome.browser.ntp.snippets.SnippetArticle;
+import org.chromium.chrome.browser.ntp.snippets.SuggestionsSource;
+import org.chromium.content_public.browser.LoadUrlParams;
+
+import java.util.Set;
+
+/**
+ * Interface between the suggestion surface and the rest of the browser.
+ */
+public interface ContentSuggestionsManager {

[tschumann, 2017/01/13 09:05:54]

drive-by: I have the feeling we're putting too much functionality into this
interface.
I like the idea to have a proper wrapper around the snippetsbridge hiding the
lower-level bridge.

However, a lot of other functionality creeps in here that actually has nothing
to do with managing content suggestions(e.g. offline URLs, navigation, state
checks). It's IMO better to have clear and easy-to use interfaces for these
tasks as well (we can put them next to each other in one package to make
discoverability easier).

I'd also recommend against introducing another generic name like manager -- it's
really unclear what behind it.

Please don't mix these

[Bernhard Bauer, 2017/01/13 09:30:23]

I think this is mostly because of the confusing name :) I think I would go with
something like [Content]SuggestionsUiDelegate, as this allow the UI to delegate
anything that isn't UI.

Navigations, offline URLs etc are in fact things that are necessary for showing
a suggestions _UI_ -- we already have an interface for handling just suggestions
with the SuggestionsSource. This is quite similar to the model that //content/
uses, where the ContentClient interface contains a lot of different methods and
ChromeContentClient implements them (although I wouldn't necessarily use the
term "client" for this interface either).

That being said, this CL is necessary right now to make a lot of the
functionality in the standalone suggestions UI work; if we want to split the
interface up into smaller ones, we can still do that at a later point.

[tschumann, 2017/01/13 09:38:37]

A different name would certainly help. If all clients/consumers of this
interface need the whole functionality set, then keeping it all in one interface
might make sense. However, if it turns out that some parts only need certain
pieces, I'd vote for splitting it up.
It's a trade-off between convenience and clear, sharp interfaces in the end.

Leaving the discussion and timing up to you. If you can clarify the role with a
better name, then I'm certainly less concerned.
However, I wonder if some functionality should be carved out in classes
corresponding to SnippetsBridge (e.g. the Navigation part) and turning this
interface more into an access point delegating most functionality). But like I
said, just my 2 cents :-)

+    // Dependency injection
+
+    /**
+     * Returns the SuggestionsSource or null if it doesn't exist. The SuggestionsSource is
+     * invalidated (has destroy() called) when the NewTabPage is destroyed so use this method
+     * instead of keeping your own reference.
+     */
+    @Nullable
+    SuggestionsSource getSuggestionsSource();
+
+    // Favicons
+
+    /**
+     * Checks if an icon with the given URL is available. If not,
+     * downloads it and stores it as a favicon/large icon for the given {@code pageUrl}.
+     * @param pageUrl The URL of the site whose icon is being requested.
+     * @param iconUrl The URL of the favicon/large icon.
+     * @param isLargeIcon Whether the {@code iconUrl} represents a large icon or favicon.
+     * @param callback The callback to be notified when the favicon has been checked.
+     */
+    void ensureIconIsAvailable(String pageUrl, String iconUrl, boolean isLargeIcon,
+            boolean isTemporary, IconAvailabilityCallback callback);
+
+    /**
+     * Gets the large icon (e.g. favicon or touch icon) for a given URL.
+     * @param url The URL of the site whose icon is being requested.
+     * @param size The desired size of the icon in pixels.
+     * @param callback The callback to be notified when the icon is available.
+     */
+    void getLargeIconForUrl(String url, int size, LargeIconCallback callback);
+
+    /**
+     * Gets the favicon image for a given URL.
+     * @param url The URL of the site whose favicon is being requested.
+     * @param size The desired size of the favicon in pixels.
+     * @param faviconCallback The callback to be notified when the favicon is available.
+     */
+    void getLocalFaviconImageForURL(String url, int size, FaviconImageCallback faviconCallback);
+
+    // Feature/State checks
+
+    /** @return Whether context menus should allow the option to open a link in incognito. */
+    boolean isOpenInIncognitoEnabled();
+
+    /** @return Whether context menus should allow the option to open a link in a new window. */
+    boolean isOpenInNewWindowEnabled();
+
+    /**
+     * Registers a {@link DestructionObserver}, notified when the New Tab Page goes away.
+     */
+    void addDestructionObserver(DestructionObserver destructionObserver);
+
+    // Navigation
+
+    /** Opens the bookmarks page in the current tab. */
+    void navigateToBookmarks();
+
+    /** Opens the Download Manager UI in the current tab. */
+    void navigateToDownloadManager();
+
+    /** Opens the recent tabs page in the current tab. */
+    void navigateToRecentTabs();
+
+    /**
+     * Handles clicks on the "learn more" link in the footer.
+     */
+    void onLearnMoreClicked();
+
+    /**
+     * Opens a content suggestion and records related metrics.
+     * @param windowOpenDisposition How to open (current tab, new tab, new window etc).
+     * @param article The content suggestion to open.
+     */
+    void openSnippet(int windowOpenDisposition, SnippetArticle article);
+
+    /** Opens an url with the desired disposition. */
+    void openUrl(int windowOpenDisposition, LoadUrlParams loadUrlParams);
+
+    // Offline
+
+    /**
+     * Checks if the pages with the given URLs are available offline.
+     * @param pageUrls The URLs of the sites whose offline availability is requested.
+     * @param callback Fired when the results are available.
+     */
+    void getUrlsAvailableOffline(Set<String> pageUrls, Callback<Set<String>> callback);
+
+    // UMA
+
+    /**
+     * Tracks per-page-load metrics for content suggestions.
+     * @param categories The categories of content suggestions.
+     * @param suggestionsPerCategory The number of content suggestions in each category.
+     */
+    void trackSnippetsPageImpression(int[] categories, int[] suggestionsPerCategory);
+
+    /**
+     * Tracks impression metrics for a content suggestion.
+     * @param article The content suggestion that was shown to the user.
+     */
+    void trackSnippetImpression(SnippetArticle article);
+
+    /**
+     * Tracks impression metrics for the long-press menu for a content suggestion.
+     * @param article The content suggestion for which the long-press menu was opened.
+     */
+    void trackSnippetMenuOpened(SnippetArticle article);
+
+    /**
+     * Tracks impression metrics for a category's action button ("More").
+     * @param category The category for which the action button was shown.
+     * @param position The position of the action button within the category.
+     */
+    void trackSnippetCategoryActionImpression(int category, int position);
+
+    /**
+     * Tracks click metrics for a category's action button ("More").
+     * @param category The category for which the action button was clicked.
+     * @param position The position of the action button within the category.
+     */
+    void trackSnippetCategoryActionClick(int category, int position);
+}