Add timestamp to infobar for stale Previews

Add timestamp to infobar for stale Previews

Add a feature to enable a timestamp on the Previews infobar. This 
timestamp should be shown when the previews_freshness is greater 
than the finch controlled "min_staleness_in_minutes" and less than
"max_staleness_in_minutes".

BUG=718643
Review-Url: https://codereview.chromium.org/2940083003
Cr-Commit-Position: refs/heads/master@{#481018}
Committed: https://chromium.googlesource.com/chromium/src/+/fa7b3da220ade97ad23b41c56be59fb76c96691b

[megjablon, 2017-06-15 23:30:59]

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

[commit-bot: I haz the power, 2017-06-15 23:31:20]

Dry run: CQ is trying da patch.

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

[megjablon, 2017-06-15 23:31:55]

Description was changed from

==========
Add timestamp to infobar for stale Previews

Add feature to enable a timestamp on the Previews infobar. This timestamp
should be shown when the previews_freshness is greater than the finch
controlled "min_staleness_in_minutes" and less than
"max_staleness_in_minutes".

BUG=718643
==========

to

==========
Add timestamp to infobar for stale Previews

Add a feature to enable a timestamp on the Previews infobar. This 
timestamp should be shown when the previews_freshness is greater 
than the finch controlled "min_staleness_in_minutes" and less than
"max_staleness_in_minutes".

BUG=718643
==========

[commit-bot: I haz the power, 2017-06-15 23:39:46]

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

[commit-bot: I haz the power, 2017-06-15 23:39:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[megjablon, 2017-06-17 01:21:24]

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

[commit-bot: I haz the power, 2017-06-17 01:21:50]

Dry run: 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, 2017-06-17 01:53:56]

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

[commit-bot: I haz the power, 2017-06-17 01:53:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[megjablon, 2017-06-19 19:08:50]

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

[commit-bot: I haz the power, 2017-06-19 19:09:09]

Dry run: CQ is trying da patch.

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

[megjablon, 2017-06-19 20:02:06]

Patchset #2 (id:20001) has been deleted

[megjablon, 2017-06-19 20:02:36]

megjablon@chromium.org changed reviewers:
+ tbansal@chromium.org

[megjablon, 2017-06-19 20:02:36]

PTAL, thanks!

[commit-bot: I haz the power, 2017-06-19 20:12:56]

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

[commit-bot: I haz the power, 2017-06-19 20:12:57]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-19 20:52:29]

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.cc:198: base::string16
timestamp = GetTimestampText();
Why is the implementation different for Android vs. non-Android?

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.h:72: base::Time
previews_freshness_;
Can this be a const variable?

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.h:72: base::Time
previews_freshness_;
Add a variable comment?
Seems like this is the time when preview was created?
Also, it might be useful to comment that a value of 0 implies that creation time
is unknown.

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right):

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate_unittest.cc:624:
infobar->GetTimestampText());
For my own understanding:
Should not infobar->GetTimestampText() also include the |message_text_|? If yes,
how are these two equal?

[megjablon, 2017-06-20 18:42:12]

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.cc:198: base::string16
timestamp = GetTimestampText();
On 2017/06/19 20:52:28, tbansal1 wrote:
> Why is the implementation different for Android vs. non-Android?

https://chromiumcodereview.appspot.com/2860083003/
In that cl I added the path that calls GetTimestampText() for a custom Android
InfoBar that adds the timestamp in a description subtext. We'll only be
launching the infobar for Android. For desktop this is simply for our own
debugging purposes, which has been very useful in the past, so I'm just
appending the string since it would require either changing
ConfirmInfoBarDelegate and ConfirmInfoBar or creating our own Delegates and
Infobar, which is a lot of overhead for our own debugging purposes.

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.h:72: base::Time
previews_freshness_;
On 2017/06/19 20:52:29, tbansal1 wrote:
> Add a variable comment?
> Seems like this is the time when preview was created?
> Also, it might be useful to comment that a value of 0 implies that creation
time
> is unknown.

Done.

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right):

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate_unittest.cc:624:
infobar->GetTimestampText());
On 2017/06/19 20:52:29, tbansal1 wrote:
> For my own understanding:
> Should not infobar->GetTimestampText() also include the |message_text_|? If
yes,
> how are these two equal?

No, GetTimestampText() only returns the timestamp since on Android this is added
in a separate view and shouldn't be shown with message_text_. See the cl linked
above and the bug for mocks.

[tbansal1, 2017-06-20 21:07:15]

lgtm % comment below.

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.cc:198: base::string16
timestamp = GetTimestampText();
On 2017/06/20 18:42:12, megjablon wrote:
> On 2017/06/19 20:52:28, tbansal1 wrote:
> > Why is the implementation different for Android vs. non-Android?
> 
>
https://chromiumcodereview.appspot.com/2860083003/?_ga=2.191048873.2081889127...
> In that cl I added the path that calls GetTimestampText() for a custom Android
> InfoBar that adds the timestamp in a description subtext. We'll only be
> launching the infobar for Android. For desktop this is simply for our own
> debugging purposes, which has been very useful in the past, so I'm just
> appending the string since it would require either changing
> ConfirmInfoBarDelegate and ConfirmInfoBar or creating our own Delegates and
> Infobar, which is a lot of overhead for our own debugging purposes.

Can we add a comment here to explain what's going on?

[megjablon, 2017-06-20 21:27:03]

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

[commit-bot: I haz the power, 2017-06-20 21:27:25]

Dry run: CQ is trying da patch.

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

[megjablon, 2017-06-20 21:27:26]

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://codereview.chromium.org/2940083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.cc:198: base::string16
timestamp = GetTimestampText();
On 2017/06/20 21:07:14, tbansal1 wrote:
> On 2017/06/20 18:42:12, megjablon wrote:
> > On 2017/06/19 20:52:28, tbansal1 wrote:
> > > Why is the implementation different for Android vs. non-Android?
> > 
> >
>
https://chromiumcodereview.appspot.com/2860083003/?_ga=2.191048873.2081889127...
> > In that cl I added the path that calls GetTimestampText() for a custom
Android
> > InfoBar that adds the timestamp in a description subtext. We'll only be
> > launching the infobar for Android. For desktop this is simply for our own
> > debugging purposes, which has been very useful in the past, so I'm just
> > appending the string since it would require either changing
> > ConfirmInfoBarDelegate and ConfirmInfoBar or creating our own Delegates and
> > Infobar, which is a lot of overhead for our own debugging purposes.
> 
> Can we add a comment here to explain what's going on?

Done.

[commit-bot: I haz the power, 2017-06-20 22:49:11]

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

[commit-bot: I haz the power, 2017-06-20 22:49:12]

Dry run: This issue passed the CQ dry run.

[megjablon, 2017-06-20 23:09:05]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2017-06-20 23:09:06]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2940083003/#ps80001
(title: "add GetMessageText comment")

[commit-bot: I haz the power, 2017-06-20 23:09:35]

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, 2017-06-20 23:14:11]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498000145804420,
"parent_rev": "d26c5cab136d7fe27046e09d759dc930712c8175", "commit_rev":
"2399ff2f97fc0fb4a7463ea0dc8547b280c3b27b"}

[commit-bot: I haz the power, 2017-06-20 23:14:26]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498000145804420,
"parent_rev": "783f0ba8e99665eb08f13f7b32e75a64a90d6509", "commit_rev":
"fa7b3da220ade97ad23b41c56be59fb76c96691b"}

[commit-bot: I haz the power, 2017-06-20 23:14:40]

Description was changed from

==========
Add timestamp to infobar for stale Previews

Add a feature to enable a timestamp on the Previews infobar. This 
timestamp should be shown when the previews_freshness is greater 
than the finch controlled "min_staleness_in_minutes" and less than
"max_staleness_in_minutes".

BUG=718643
==========

to

==========
Add timestamp to infobar for stale Previews

Add a feature to enable a timestamp on the Previews infobar. This 
timestamp should be shown when the previews_freshness is greater 
than the finch controlled "min_staleness_in_minutes" and less than
"max_staleness_in_minutes".

BUG=718643

Review-Url: https://codereview.chromium.org/2940083003
Cr-Commit-Position: refs/heads/master@{#481018}
Committed:
https://chromium.googlesource.com/chromium/src/+/fa7b3da220ade97ad23b41c56be5...
==========

[commit-bot: I haz the power, 2017-06-20 23:14:41]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/fa7b3da220ade97ad23b41c56be5...