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

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

Previously we always opened an offline page, if there was one. This CL
simply opens the URL for non-download snippets and then Offline Pages
decide whether to open its offline page. For Downloads nothing changed.

BUG=668485
Committed: https://crrev.com/6f538029f3659bf667470deca0b22abdcd7cafbd
Cr-Commit-Position: refs/heads/master@{#434445}

[vitaliii, 2016-11-24 13:33:37]

vitaliii@chromium.org changed reviewers:
+ dgn@chromium.org

[vitaliii, 2016-11-24 13:33:38]

Hi dgn@,

Would you mind having a look?

[dgn, 2016-11-24 15:49:35]

lgtm

https://codereview.chromium.org/2525323003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2525323003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:385: if
(article.isDownload() && !article.mIsAssetDownload) {
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]

I ack'ed your nit. No need to look.

https://codereview.chromium.org/2525323003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/2525323003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:385: if
(article.isDownload() && !article.mIsAssetDownload) {
On 2016/11/24 15:49:34, dgn wrote:
> 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?

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.

[vitaliii, 2016-11-25 06:51:14]

The CQ bit was checked by vitaliii@chromium.org

[commit-bot: I haz the power, 2016-11-25 06:51:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-25 07:22:54]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1480056674285640, "parent_rev":
"ae192f2ff81fce75e7e175eacba9f290cce845cc", "commit_rev":
"f818bf481411e3b2a46503f20d2e62d01bab3962"}

[commit-bot: I haz the power, 2016-11-25 07:23:17]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-25 07:24:36]

Description was changed from

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

Previously we always opened an offline page, if there was one. This CL
simply opens the URL for non-download snippets and then Offline Pages
decide whether to open its offline page. For Downloads nothing changed.

BUG=668485
==========

to

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

Previously we always opened an offline page, if there was one. This CL
simply opens the URL for non-download snippets and then Offline Pages
decide whether to open its offline page. For Downloads nothing changed.

BUG=668485

Committed: https://crrev.com/6f538029f3659bf667470deca0b22abdcd7cafbd
Cr-Commit-Position: refs/heads/master@{#434445}
==========

[commit-bot: I haz the power, 2016-11-25 07:24:37]

Patchset 1 (id:??) landed as
https://crrev.com/6f538029f3659bf667470deca0b22abdcd7cafbd
Cr-Commit-Position: refs/heads/master@{#434445}