[NTP] Do not force offline version when opening non-download snippets.

chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
index b4d9b05569ede8d787bd36134efead5a60818e31..28df04efb4d269aebefa5e3c2a600a33ae12395b 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
@@ -379,10 +379,14 @@ public class NewTabPage
             }
 
             LoadUrlParams loadUrlParams;
-            // The snippet's offline page is ignored if the snippet is saved or opened in incognito.
-            if (article.getOfflinePageOfflineId() != null
-                    && windowOpenDisposition != WindowOpenDisposition.SAVE_TO_DISK
-                    && windowOpenDisposition != WindowOpenDisposition.OFF_THE_RECORD) {
+            // We explicitly open an offline page only for offline page downloads. For all other
+            // sections the URL is opened and it is up to Offline Pages whether to open its offline
+            // page (e.g. when offline).
+            if (article.isDownload() && !article.mIsAssetDownload) {

[dgn, 2016/11/24 15:49:34]

nit: the mIsAssetDownload is not supposed to be useful, as we would have
excluded this possibility from the check at line 359. Maybe just assert?

[vitaliii, 2016/11/25 06:32:18]

Ack.
Marc and I discussed this. We agreed that the dependency between two |if|s may
be unclear, so having an assert makes it easier to miss a bug introduced when
changing the first |if|. Moreover, I feel like having |article.mIsAssetDownload|
makes it more obvious that the |if| is about offline pages. 

+                assert article.getOfflinePageOfflineId() != null;
+                assert windowOpenDisposition == WindowOpenDisposition.CURRENT_TAB
+                        || windowOpenDisposition == WindowOpenDisposition.NEW_WINDOW
+                        || windowOpenDisposition == WindowOpenDisposition.NEW_FOREGROUND_TAB;
                 loadUrlParams = OfflinePageUtils.getLoadUrlParamsForOpeningOfflineVersion(
                         article.mUrl, article.getOfflinePageOfflineId());
                 // Extra headers are not read in loadUrl, but verbatim headers are.