Ntp: use AMP urls for content suggestions when available.

Ntp: use AMP urls for content suggestions when available.

Controlled by the NTPPreferAmpUrls feature, enabled by default.

BUG=668095
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/ee93200f104433b9637d0f5688ba46ec21439098
Cr-Commit-Position: refs/heads/master@{#439838}

[Michael van Ouwerkerk, 2016-12-20 15:29:55]

Description was changed from

==========
Ntp: use AMP urls for content suggestions when available.

Controlled by the NTPPreferAmpUrls feature, enabled by default.

BUG=668095
==========

to

==========
Ntp: use AMP urls for content suggestions when available.

Controlled by the NTPPreferAmpUrls feature, enabled by default.

BUG=668095
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Michael van Ouwerkerk, 2016-12-20 15:30:52]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 15:31:11]

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

[Michael van Ouwerkerk, 2016-12-20 15:33:07]

mvanouwerkerk@chromium.org changed reviewers:
+ bauerb@chromium.org

[Michael van Ouwerkerk, 2016-12-20 15:33:08]

Bernhard, could you take a look please?

[Bernhard Bauer, 2016-12-20 15:42:49]

Nice, LGTM!

+Marc FYI

https://codereview.chromium.org/2593573003/diff/1/components/ntp_snippets/fea...
File components/ntp_snippets/features.cc (right):

https://codereview.chromium.org/2593573003/diff/1/components/ntp_snippets/fea...
components/ntp_snippets/features.cc:44: const base::Feature
kPreferAmpUrlsFeature{"NTPPreferAmpUrls",
Nit: If we capitalize the full acronym in NTP, we should do the same thing for
AMP, or do it for neither (but that would require changing the other instances
as well, for consistency).

https://codereview.chromium.org/2593573003/diff/1/components/ntp_snippets/fea...
File components/ntp_snippets/features.h (right):

https://codereview.chromium.org/2593573003/diff/1/components/ntp_snippets/fea...
components/ntp_snippets/features.h:41: // Feature to prefer amp urls over
regular urls when available.
Nit: AMP instead of amp.

[Michael van Ouwerkerk, 2016-12-20 15:43:41]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 15:44:01]

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

[Michael van Ouwerkerk, 2016-12-20 15:51:00]

https://codereview.chromium.org/2593573003/diff/1/components/ntp_snippets/fea...
File components/ntp_snippets/features.cc (right):

https://codereview.chromium.org/2593573003/diff/1/components/ntp_snippets/fea...
components/ntp_snippets/features.cc:44: const base::Feature
kPreferAmpUrlsFeature{"NTPPreferAmpUrls",
On 2016/12/20 15:42:49, Bernhard Bauer wrote:
> Nit: If we capitalize the full acronym in NTP, we should do the same thing for
> AMP, or do it for neither (but that would require changing the other instances
> as well, for consistency).

I think "NTP" is the anomaly here, and we should keep it for consistency with
existing code. I would not want to write these as "kPreferAMPURLsFeature" and
"NTPPreferAMPURLs".

https://codereview.chromium.org/2593573003/diff/1/components/ntp_snippets/fea...
File components/ntp_snippets/features.h (right):

https://codereview.chromium.org/2593573003/diff/1/components/ntp_snippets/fea...
components/ntp_snippets/features.h:41: // Feature to prefer amp urls over
regular urls when available.
On 2016/12/20 15:42:49, Bernhard Bauer wrote:
> Nit: AMP instead of amp.

Done.

[Michael van Ouwerkerk, 2016-12-20 15:51:34]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-12-20 15:51:34]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2593573003/#ps40001
(title: "Address review comments from bauerb.")

[vitaliii, 2016-12-20 15:51:45]

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

[vitaliii, 2016-12-20 15:51:46]

Drive-by nits.

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(left):

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:31:
public final String mUrl;
I would add a comment "May be AMP.".

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:54:
/** To be run when the offline status of the article or AMP article changes. */
This comment is confusing. 
I understand what you mean only because I know this class, otherwise I would be
confused by "AMP article", because there is no other mention of AMP in entire
file.

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:55:
private Runnable mOfflineStatusChangeRunnable;
Have you tested offline pages support of AMP URLs? E.g. can you offline such a
page and then open?

https://codereview.chromium.org/2593573003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2593573003/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:86: // be accessed.
I would say that this may be an AMP URL.
Also "normal content URL" seems strange.

[commit-bot: I haz the power, 2016-12-20 15:51:51]

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

[Michael van Ouwerkerk, 2016-12-20 15:52:03]

The CQ bit was unchecked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-12-20 16:12:48]

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(left):

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:31:
public final String mUrl;
On 2016/12/20 15:51:46, vitaliii wrote:
> I would add a comment "May be AMP.".

Done.

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:54:
/** To be run when the offline status of the article or AMP article changes. */
On 2016/12/20 15:51:46, vitaliii wrote:
> This comment is confusing. 
> I understand what you mean only because I know this class, otherwise I would
be
> confused by "AMP article", because there is no other mention of AMP in entire
> file. 

I don't understand, could you have misread the diff? I deleted the "or AMP
article" part, not added.

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:55:
private Runnable mOfflineStatusChangeRunnable;
On 2016/12/20 15:51:46, vitaliii wrote:
> Have you tested offline pages support of AMP URLs? E.g. can you offline such a
> page and then open?

Yes, that works as well as regular pages. Do you have reason to suspect it
wouldn't work?

https://codereview.chromium.org/2593573003/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestion.h (right):

https://codereview.chromium.org/2593573003/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestion.h:86: // be accessed.
On 2016/12/20 15:51:46, vitaliii wrote:
> I would say that this may be an AMP URL.
> Also "normal content URL" seems strange.

Done.

[Michael van Ouwerkerk, 2016-12-20 16:12:53]

The CQ bit was checked by mvanouwerkerk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 16:13:10]

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

[vitaliii, 2016-12-20 16:35:12]

LGTM.

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java
(left):

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:54:
/** To be run when the offline status of the article or AMP article changes. */
On 2016/12/20 16:12:48, Michael van Ouwerkerk wrote:
> On 2016/12/20 15:51:46, vitaliii wrote:
> > This comment is confusing. 
> > I understand what you mean only because I know this class, otherwise I would
> be
> > confused by "AMP article", because there is no other mention of AMP in
entire
> > file. 
> 
> I don't understand, could you have misread the diff? I deleted the "or AMP
> article" part, not added.

Indeed, I misread the diff. Sorry.

https://codereview.chromium.org/2593573003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticle.java:55:
private Runnable mOfflineStatusChangeRunnable;
On 2016/12/20 16:12:48, Michael van Ouwerkerk wrote:
> On 2016/12/20 15:51:46, vitaliii wrote:
> > Have you tested offline pages support of AMP URLs? E.g. can you offline such
a
> > page and then open?
> 
> Yes, that works as well as regular pages. Do you have reason to suspect it
> wouldn't work?

I am not familiar with AMP, so just wanted to make sure.

[Michael van Ouwerkerk, 2016-12-20 16:53:45]

The CQ bit was unchecked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-12-20 16:53:59]

The CQ bit was checked by mvanouwerkerk@chromium.org

[Michael van Ouwerkerk, 2016-12-20 16:54:00]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2593573003/#ps60001
(title: "Address review comments by vitalii.")

[commit-bot: I haz the power, 2016-12-20 16:54:03]

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-12-20 17:41:53]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482252839942640,
"parent_rev": "729260231539a78627a19dfd052e87c43f3846ba", "commit_rev":
"ac7d89043184919dece338bdf181d8ba0f93ead3"}

[commit-bot: I haz the power, 2016-12-20 17:42:24]

Description was changed from

==========
Ntp: use AMP urls for content suggestions when available.

Controlled by the NTPPreferAmpUrls feature, enabled by default.

BUG=668095
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Ntp: use AMP urls for content suggestions when available.

Controlled by the NTPPreferAmpUrls feature, enabled by default.

BUG=668095
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2593573003
==========

[commit-bot: I haz the power, 2016-12-20 17:42:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-20 17:44:38]

Description was changed from

==========
Ntp: use AMP urls for content suggestions when available.

Controlled by the NTPPreferAmpUrls feature, enabled by default.

BUG=668095
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2593573003
==========

to

==========
Ntp: use AMP urls for content suggestions when available.

Controlled by the NTPPreferAmpUrls feature, enabled by default.

BUG=668095
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/ee93200f104433b9637d0f5688ba46ec21439098
Cr-Commit-Position: refs/heads/master@{#439838}
==========

[commit-bot: I haz the power, 2016-12-20 17:44:39]

Patchset 4 (id:??) landed as
https://crrev.com/ee93200f104433b9637d0f5688ba46ec21439098
Cr-Commit-Position: refs/heads/master@{#439838}

[jkrcal, 2016-12-21 07:17:14]

On 2016/12/20 17:44:39, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/ee93200f104433b9637d0f5688ba46ec21439098
> Cr-Commit-Position: refs/heads/master@{#439838}

I am bit frustrated. Was it not mentioned in a recent meeting that this breaks
favicon fetching for amp articles?
 (which still happens on java side, based on the url of the article)

My CLs to move it to the c++ side are in the works, hope to land it before M57
BP but I am not sure :|

[Michael van Ouwerkerk, 2016-12-21 10:29:09]

On 2016/12/21 07:17:14, jkrcal wrote:
> On 2016/12/20 17:44:39, commit-bot: I haz the power wrote:
> > Patchset 4 (id:??) landed as
> > https://crrev.com/ee93200f104433b9637d0f5688ba46ec21439098
> > Cr-Commit-Position: refs/heads/master@{#439838}
> 
> I am bit frustrated. Was it not mentioned in a recent meeting that this breaks
> favicon fetching for amp articles?
>  (which still happens on java side, based on the url of the article)
> 
> My CLs to move it to the c++ side are in the works, hope to land it before M57
> BP but I am not sure :|

Sorry Jan. I remember you mentioning it, and I did check both the code path and
the final user experience. I saw no failures and no technical reason for why it
would fail, though this may depend on the data. Basically, all the AMP urls I
saw just used the regular publisher domain, and they had a favicon. Let's talk
about what might be broken, and what we might do about it.