Add timestamp support to the Previews infobar

Add timestamp support to the Previews infobar

Add a PreviewsInfobar so that when Previews are served, a timestamp
of how old the Preview is can be added from the PreviewsInfoBarDelegate.

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

[megjablon, 2017-05-04 23:12:40]

Description was changed from

==========
Add Previews infobar timestamp support

Add a PreviewsInfobar so that when Previews are served, a timestamp
of how old the Preview is can be added from the PreviewsInfoBarDelegate.

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

to

==========
Add timestamp support to the Previews infobar

Add a PreviewsInfobar so that when Previews are served, a timestamp
of how old the Preview is can be added from the PreviewsInfoBarDelegate.

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

[megjablon, 2017-05-04 23:12:49]

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

[commit-bot: I haz the power, 2017-05-04 23:13:33]

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-05-05 00:29:22]

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

[commit-bot: I haz the power, 2017-05-05 00:29:23]

Dry run: This issue passed the CQ dry run.

[megjablon, 2017-05-05 17:35:21]

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

[megjablon, 2017-05-05 17:35:21]

PTAL dfalcantara, thanks!

[gone, 2017-05-06 01:02:27]

lgtm % time travel

https://codereview.chromium.org/2860083003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/PreviewsInfoBar.java
(right):

https://codereview.chromium.org/2860083003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/infobar/PreviewsInfoBar.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
2017 for all the years

[megjablon, 2017-05-08 20:26:46]

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

[megjablon, 2017-05-08 20:27:50]

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

[megjablon, 2017-05-08 20:27:51]

ryansturm: previews_infobar_delegate.*

[commit-bot: I haz the power, 2017-05-08 20:28:01]

Dry run: CQ is trying da patch.

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

[RyanSturm, 2017-05-08 20:48:17]

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

https://codereview.chromium.org/2860083003/diff/20001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.h:69: static
std::unique_ptr<infobars::InfoBar> CreateInfoBar(
Why is this declared here and defined in 
chrome/browser/ui/android/infobars/previews_infobar.cc?

https://codereview.chromium.org/2860083003/diff/20001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/previews_infobar.cc (right):

https://codereview.chromium.org/2860083003/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/previews_infobar.cc:25:
static_cast<PreviewsInfoBarDelegate*>(GetDelegate());
nit: It would be cleaner to add a AsPreviewsInfoBarDelegate method to
InfoBarDelegate since there is a common pattern there rather than casting here.
There's a pretty strong guarantee from the constructor that the cast is safe,
but you might want to follow suit:

PreviewsInfoBarDelegate* InfoBarDelegate::AsPreviewsInfoBarDelegate()

[megjablon, 2017-05-08 22:49:20]

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

https://codereview.chromium.org/2860083003/diff/20001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.h:69: static
std::unique_ptr<infobars::InfoBar> CreateInfoBar(
On 2017/05/08 20:48:17, Ryan Sturm wrote:
> Why is this declared here and defined in 
> chrome/browser/ui/android/infobars/previews_infobar.cc?

This is the pattern used by infobars and infobar delegates. dfalcantara@ can you
comment on why? I moved this to PreviewsInfobar now.

https://codereview.chromium.org/2860083003/diff/20001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/previews_infobar.cc (right):

https://codereview.chromium.org/2860083003/diff/20001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/previews_infobar.cc:25:
static_cast<PreviewsInfoBarDelegate*>(GetDelegate());
On 2017/05/08 20:48:17, Ryan Sturm wrote:
> nit: It would be cleaner to add a AsPreviewsInfoBarDelegate method to
> InfoBarDelegate since there is a common pattern there rather than casting
here.
> There's a pretty strong guarantee from the constructor that the cast is safe,
> but you might want to follow suit:
> 
> PreviewsInfoBarDelegate* InfoBarDelegate::AsPreviewsInfoBarDelegate()

Done.

[megjablon, 2017-05-08 22:50:21]

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

[megjablon, 2017-05-08 22:50:22]

pkasting: components/infobars/*

[Peter Kasting, 2017-05-08 23:33:35]

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

https://codereview.chromium.org/2860083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.cc:89:
std::unique_ptr<PreviewsInfoBarDelegate>(new PreviewsInfoBarDelegate(
Nit: Since PreviewsInfoBarDelegate is a ConfirmInfoBarDelegate, it seems like
you should be able to factor the creation of that inner object into a common
block above the #if, and only have the rest (or maybe even only the
CreateInfoBar() portion of the statement) ifdefed.

https://codereview.chromium.org/2860083003/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/previews_infobar.cc (right):

https://codereview.chromium.org/2860083003/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/previews_infobar.cc:24: if (!delegate)
When could there be no delegate?  It seems like this shouldn't be able to fail. 
In that case, I would remove the conditional.

In either case, it seems like this can never be called with a delegate of a
different type than PreviewsInfoBarDelegate.  If so, then don't add the virtual
downcast method; those are only needed for the equivalent of dynamic_cast.  You
can just do a static_cast here to get the right type.

[RyanSturm, 2017-05-08 23:36:53]

lgtm % what I said before about the split declaration/definition. Not blocking,
just weird.

[megjablon, 2017-05-09 00:14:33]

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

https://codereview.chromium.org/2860083003/diff/40001/chrome/browser/previews...
chrome/browser/previews/previews_infobar_delegate.cc:89:
std::unique_ptr<PreviewsInfoBarDelegate>(new PreviewsInfoBarDelegate(
On 2017/05/08 23:33:35, Peter Kasting wrote:
> Nit: Since PreviewsInfoBarDelegate is a ConfirmInfoBarDelegate, it seems like
> you should be able to factor the creation of that inner object into a common
> block above the #if, and only have the rest (or maybe even only the
> CreateInfoBar() portion of the statement) ifdefed.

Done.

https://codereview.chromium.org/2860083003/diff/40001/chrome/browser/ui/andro...
File chrome/browser/ui/android/infobars/previews_infobar.cc (right):

https://codereview.chromium.org/2860083003/diff/40001/chrome/browser/ui/andro...
chrome/browser/ui/android/infobars/previews_infobar.cc:24: if (!delegate)
On 2017/05/08 23:33:35, Peter Kasting wrote:
> When could there be no delegate?  It seems like this shouldn't be able to
fail. 
> In that case, I would remove the conditional.
> 
> In either case, it seems like this can never be called with a delegate of a
> different type than PreviewsInfoBarDelegate.  If so, then don't add the
virtual
> downcast method; those are only needed for the equivalent of dynamic_cast. 
You
> can just do a static_cast here to get the right type.

Done.

[megjablon, 2017-05-09 00:14:59]

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

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

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-05-09 01:21:20]

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

[commit-bot: I haz the power, 2017-05-09 01:21:21]

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_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[megjablon (incorrect), 2017-05-09 17:44:22]

The CQ bit was checked by megjablon@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 17:45:05]

Dry run: CQ is trying da patch.

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

[megjablon, 2017-05-09 21:14:56]

megjablon@chromium.org changed reviewers:
- megjablon@google.com

[megjablon, 2017-05-09 21:36:31]

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

[commit-bot: I haz the power, 2017-05-09 21:38:32]

Dry run: CQ is trying da patch.

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

[megjablon, 2017-05-09 23:10:14]

The CQ bit was unchecked by megjablon@chromium.org

[megjablon, 2017-05-09 23:10:21]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2017-05-09 23:10:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2860083003/#ps80001
(title: "sync and rebase")

[commit-bot: I haz the power, 2017-05-09 23:11:05]

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-05-10 01:14:49]

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

[commit-bot: I haz the power, 2017-05-10 01:14:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[megjablon, 2017-05-10 17:07:53]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2017-05-10 17:08:49]

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-05-10 19:51:51]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494436073629900,
"parent_rev": "419bda2ce75c18c90e66fae1d03fdd1e5479ce04", "commit_rev":
"5eba853218417dd3fcc66332e95a9183e477c973"}

[commit-bot: I haz the power, 2017-05-10 19:52:01]

Description was changed from

==========
Add timestamp support to the Previews infobar

Add a PreviewsInfobar so that when Previews are served, a timestamp
of how old the Preview is can be added from the PreviewsInfoBarDelegate.

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

to

==========
Add timestamp support to the Previews infobar

Add a PreviewsInfobar so that when Previews are served, a timestamp
of how old the Preview is can be added from the PreviewsInfoBarDelegate.

BUG=718643

Review-Url: https://codereview.chromium.org/2860083003
Cr-Commit-Position: refs/heads/master@{#470678}
Committed:
https://chromium.googlesource.com/chromium/src/+/5eba853218417dd3fcc66332e95a...
==========

[commit-bot: I haz the power, 2017-05-10 19:52:02]

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