[NTP::PhysicalWeb] Do not show published time.

[NTP::PhysicalWeb] Do not show published time.

In this CL:
1) published time for Physical Web suggestions is not provided anymore;
2) UI is changed not to show published time at all if it is equal to
   NULL time.

BUG=674568
Committed: https://crrev.com/3ac0bb8bf760c4a783fdbe2f176711585b1a070e
Cr-Commit-Position: refs/heads/master@{#440407}

[vitaliii, 2016-12-20 10:06:09]

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

[commit-bot: I haz the power, 2016-12-20 10:06:22]

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 10:06:57]

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

[vitaliii, 2016-12-20 10:06:57]

Hi jkrcal@,

could you have a look at C++ bit of the change?

[vitaliii, 2016-12-20 10:09:59]

vitaliii@chromium.org changed reviewers:
+ jkrcal@google.com, peconn@chromium.org
- jkrcal@chromium.org

[vitaliii, 2016-12-20 10:10:00]

Hi peconn@,

could you have a look at UI bit of the change?

I had to change UI, because otherwise 1st of January 1970 was shown in case of
an empty time stamp.

[PEConn, 2016-12-20 10:20:54]

On 2016/12/20 10:10:00, vitaliii wrote:
> Hi peconn@,
> 
> could you have a look at UI bit of the change?
> 
> I had to change UI, because otherwise 1st of January 1970 was shown in case of
> an empty time stamp.

Could you check that this leaves everything working when:
- Device is in RTL mode.
- The language (and RTL/LTR mode) of the device is different from that of the
publisher (see https://crbug.com/608680).

[jkrcal, 2016-12-20 10:26:32]

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

[jkrcal, 2016-12-20 10:26:33]

physical_web_page_suggestions_provider.cc lgtm

[vitaliii, 2016-12-20 10:49:36]

On 2016/12/20 10:20:54, PEConn wrote:
> On 2016/12/20 10:10:00, vitaliii wrote:
> > Hi peconn@,
> > 
> > could you have a look at UI bit of the change?
> > 
> > I had to change UI, because otherwise 1st of January 1970 was shown in case
of
> > an empty time stamp.
> 
> Could you check that this leaves everything working when:
> - Device is in RTL mode.
> - The language (and RTL/LTR mode) of the device is different from that of the
> publisher (see https://crbug.com/608680).

Could you give a hint how to
1) enable RTL mode;
2) change the language in Android Chrome?

I cannot find any way.

[PEConn, 2016-12-20 11:01:27]

On 2016/12/20 10:49:36, vitaliii wrote:
> On 2016/12/20 10:20:54, PEConn wrote:
> > On 2016/12/20 10:10:00, vitaliii wrote:
> > > Hi peconn@,
> > > 
> > > could you have a look at UI bit of the change?
> > > 
> > > I had to change UI, because otherwise 1st of January 1970 was shown in
case
> of
> > > an empty time stamp.
> > 
> > Could you check that this leaves everything working when:
> > - Device is in RTL mode.
> > - The language (and RTL/LTR mode) of the device is different from that of
the
> > publisher (see https://crbug.com/608680).
> 
> Could you give a hint how to
> 1) enable RTL mode;
> 2) change the language in Android Chrome?
> 
> I cannot find any way.

They're both Android settings.
For RTL, go Android Settings -> Developer options -> Force RTL layout direction.
For the language, check with both:
- Your device in English and the publisher string being in Arabic/Hebrew (if you
use Eclipse, that handles RTL strings well).
- Your device in Arabic/Hebrew and the publisher string being in English (eg,
"The Times").
To change your device language, Android Settings -> Languages & Input ->
Languages, Add a language then drag it to the top of the list.
For the publisher strings, just hardcode them.

If you set your device to Arabic/Hebrew it will automatically go to RTL layout,
the Developer options flag is just handy to see the RTL layout while being in a
language you are comfortable with.

[dgn, 2016-12-20 11:15:39]

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

[dgn, 2016-12-20 11:15:39]

https://codereview.chromium.org/2591673002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2591673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:53:
private static final String PUBLISHER_FORMAT_STRING = "%s%s";
that's kind of icky... how about something like this instead:


String getAttributionString(SnippetArticle article) {
  if (article.mPublishTimestampMs == 0) return article.mPublisher;
  
  // Then try/finally with strict mode stuff to get the relative timestring

  // then bi-di formatting, since when you only have the publisher you don't
need that
}

mPublisherTextView.setText(getAttributionString(mArticle));

https://codereview.chromium.org/2591673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:224:
} finally {
nit: why is the try/finally that big? shouldn't it only wrap the
getRelativeTimespan String?

[PEConn, 2016-12-20 11:24:07]

https://codereview.chromium.org/2591673002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2591673002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:224:
} finally {
On 2016/12/20 11:15:39, dgn wrote:
> nit: why is the try/finally that big? shouldn't it only wrap the
> getRelativeTimespan String?

That is the only part that can fail, but if it does then there's no sense
executing the rest of the try block. I guess we could do:

String relativeTimeSpan = "";
try {
  relativeTimeSpan = DateUtils.getRelativeTimeSpanString(...)
} finally {
  ...
}

// use relativeTimeSpan

[commit-bot: I haz the power, 2016-12-20 11:41:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 11:41:04]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2016-12-22 10:05:01]

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

[commit-bot: I haz the power, 2016-12-22 10:05: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-22 10:06:25]

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

[vitaliii, 2016-12-22 10:06:25]

Hi mvanouwerkerk@,

could you have a look at the UI bit since everyone else is OOO?

The screenshots for RTL related checks are in the bug, they seem ok to me, but I
am not experienced with RTL.

[commit-bot: I haz the power, 2016-12-22 10:47:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 10:47:07]

Dry run: This issue passed the CQ dry run.

[Michael van Ouwerkerk, 2016-12-22 10:55:28]

https://codereview.chromium.org/2591673002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2591673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:188:
public String getAttributionString(SnippetArticle article) {
Can this method be private?

https://codereview.chromium.org/2591673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:206:
RecordHistogram.recordTimesHistogram("Android.StrictMode.SnippetUIBuildTime",
Moving this histogram recording further down may affect the data it logs, let's
keep it where it was, immediately below the DateUtils call.

[vitaliii, 2016-12-22 12:38:26]

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

[commit-bot: I haz the power, 2016-12-22 12:38:32]

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-22 12:39:01]

Hi mvanouwerkerk@,

I addressed your comments,
please have a look.

https://codereview.chromium.org/2591673002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
(right):

https://codereview.chromium.org/2591673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:188:
public String getAttributionString(SnippetArticle article) {
On 2016/12/22 10:55:28, Michael van Ouwerkerk wrote:
> Can this method be private?

Done.

private static

https://codereview.chromium.org/2591673002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java:206:
RecordHistogram.recordTimesHistogram("Android.StrictMode.SnippetUIBuildTime",
On 2016/12/22 10:55:28, Michael van Ouwerkerk wrote:
> Moving this histogram recording further down may affect the data it logs,
let's
> keep it where it was, immediately below the DateUtils call.

Done.

[commit-bot: I haz the power, 2016-12-22 13:18:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 13:18:54]

Dry run: This issue passed the CQ dry run.

[Michael van Ouwerkerk, 2016-12-22 13:59:32]

Thanks! lgtm

[vitaliii, 2016-12-22 14:03:19]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2016-12-22 14:03:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2591673002/#ps60001
(title: "mvanouwerkerk@ comments.")

[commit-bot: I haz the power, 2016-12-22 14:03:42]

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-22 14:06:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482415399426750,
"parent_rev": "89974c156619c20aea911fbc72a9820575d61940", "commit_rev":
"3cd71715060ce6c33db03a38243c4629fbf87f7d"}

[commit-bot: I haz the power, 2016-12-22 14:08:37]

Description was changed from

==========
[NTP::PhysicalWeb] Do not show published time.

In this CL:
1) published time for Physical Web suggestions is not provided anymore;
2) UI is changed not to show published time at all if it is equal to
   NULL time.

BUG=674568
==========

to

==========
[NTP::PhysicalWeb] Do not show published time.

In this CL:
1) published time for Physical Web suggestions is not provided anymore;
2) UI is changed not to show published time at all if it is equal to
   NULL time.

BUG=674568

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

[commit-bot: I haz the power, 2016-12-22 14:08:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-22 14:13:19]

Description was changed from

==========
[NTP::PhysicalWeb] Do not show published time.

In this CL:
1) published time for Physical Web suggestions is not provided anymore;
2) UI is changed not to show published time at all if it is equal to
   NULL time.

BUG=674568

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

to

==========
[NTP::PhysicalWeb] Do not show published time.

In this CL:
1) published time for Physical Web suggestions is not provided anymore;
2) UI is changed not to show published time at all if it is equal to
   NULL time.

BUG=674568

Committed: https://crrev.com/3ac0bb8bf760c4a783fdbe2f176711585b1a070e
Cr-Commit-Position: refs/heads/master@{#440407}
==========

[commit-bot: I haz the power, 2016-12-22 14:13:20]

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