Add InfoBar delegate for previews

Add InfoBar delegate for previews

Add PreviewsInfoBarDelegate to show an infobar when a preview
page has been loaded. Also add the corresponding resources.

BUG=615566
Committed: https://crrev.com/82aba5257757fad4ab092411822d1895589b9a5b
Cr-Commit-Position: refs/heads/master@{#418413}

[megjablon, 2016-08-18 22:35:22]

Description was changed from

==========
WIP New Previews InfoBars

BUG=615566
==========

to

==========
Add InfoBar delegate for previews

Add PreviewsInfoBarDelegate to show an infobar when a preview page has been
loaded. Also add the corresponding resources.

BUG=615566
==========

[megjablon, 2016-08-18 22:35:44]

Patchset #2 (id:20001) has been deleted

[megjablon, 2016-08-18 22:35:48]

Patchset #1 (id:1) has been deleted

[megjablon, 2016-08-18 22:35:56]

Description was changed from

==========
Add InfoBar delegate for previews

Add PreviewsInfoBarDelegate to show an infobar when a preview page has been
loaded. Also add the corresponding resources.

BUG=615566
==========

to

==========
Add InfoBar delegate for previews

Add PreviewsInfoBarDelegate to show an infobar when a preview
page has been loaded. Also add the corresponding resources.

BUG=615566
==========

[megjablon, 2016-08-18 22:37:26]

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

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

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

[megjablon, 2016-08-18 23:16:19]

Patchset #2 (id:60001) has been deleted

[megjablon, 2016-08-18 23:40:45]

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

[megjablon, 2016-08-18 23:40:46]

PTAL, thanks!

[megjablon, 2016-08-18 23:42:06]

On 2016/08/18 23:40:46, megjablon wrote:
> PTAL, thanks!

This is the first of a chain of cls which will also remove the old lo-fi
snackbar and add uma and tests.

[jochen (gone - plz use gerrit), 2016-08-23 12:49:50]

hi!

Was there already a design doc posted about previews? Could you please point me
at it?

In general, please find OWNERS that are closer to the files you've changed - I
don't have enough background here to do a good review

[megjablon, 2016-08-23 17:55:27]

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

[megjablon, 2016-08-23 17:55:28]

Discussed with jochen@ offline. Adding pkasting@ to check the infobar specific
code and bengr@ as new owner of chrome/browser/previews/* and for components/*.

[Peter Kasting, 2016-08-23 19:48:18]

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/app/gene...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/app/gene...
chrome/app/generated_resources.grd:6368: +         Saved data.
Nit: Putting periods at the end of these fragments seems a bit unusual, make
sure you have a tech writer/UI sign off on your strings.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:46:
PreviewsInfoBarDelegate::Create(web_contents,
Is this code accessible on non-Android builds?

Right now the resources, enum values, implementation locations, etc. are all
done in cross-platform ways, but it seems as if this is maybe an
Android-specific thing.  If it is, we want to make that as clear as possible and
avoid compiling in this stuff for the other platforms.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:26: void
DidStartProvisionalLoadForFrame(
It seems like this whole object is defined just so the infobar can be removed
when we start a load in the top-level frame.

The comments in ShouldExpire() talk about this being necessary because the
infobar can be shown before a navigation commits.  Presumably, you're having
some sort of issue where you're adding an infobar when a load is pending, and
then the commit of that load removes the infobar again.

If that's the case, the solution should probably be that we don't show the
infobar until the load commits to begin with.  Pending loads aren't loads yet,
and the UI in the browser is still showing the URL of the previous page, etc. 
We don't want to show an infobar for the upcoming page because we don't know
what the page is, we don't know if the load will happen, and the UI to the user
is confused.

If you're not doing what I've described above, then I need to understand what
you're trying to do.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:31: if
(!render_frame_host->GetParent()) {
Nit: No {}

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:32:
delegate_->RemoveInfoBar();
I believe a member class is an implicit friend of the enclosing class, in which
case we can inline the contents of RemoveInfoBar() here and remove that from the
delegate's public API.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:39:
DISALLOW_IMPLICIT_CONSTRUCTORS(PreviewInfoBarWebContentsObserver);
Nit: DISALLOW_COPY_AND_ASSIGN is sufficient, you've already declared a custom
constructor.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:56: if (infobar_type ==
PREVIEW || infobar_type == LOFI) {
Nit: Why not just "if (infobar_type != OFFLINE) {"?

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:57:
DataReductionProxyChromeSettings* data_reduction_proxy_settings =
Nit: To reduce verbosity I'd probably just start this "auto* proxy_settings =",
but up to you.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:92: return
IDR_INFOBAR_PREVIEWS;
If at all possible, prefer to add new vector icons and override GetVectorIconId
rather than adding new bitmap icons.  We're trying to eliminate bitmap resources
from the UI.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:99: return
l10n_util::GetStringUTF16(IDS_PREVIEWS_INFOBAR_SAVED_DATA_TITLE);
Nit: Simpler:

  return l10n_util::GetStringUTF16((infobar_type_ == OFFLINE)
                                       ? IDS_PREVIEWS_INFOBAR_FASTER_PAGE_TITLE
                                       : IDS_PREVIEWS_INFOBAR_SAVED_DATA_TITLE);

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:114:
web_contents_observer_.reset();
Are you doing this because if you don't do it, the reload calls below will
delete the infobar, and then returning true will cause memory corruption?  If so
consider just returning false in that case; that's what that return value is
for.  In any case, this needs comments as to what's going on.

(This is another argument for finding a way to do this through the normal expiry
path instead of having an external listener able to delete us.)

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:116:
DataReductionProxyChromeSettings* data_reduction_proxy_settings =
Nit: Seems like it might be better to do something like:

  if (infobar_type != OFFLINE) {
    if (infobar_type_ == PREVIEW)
      web_contents->GetController().ReloadDisableLoFi(true);
    else
      web_contents->ReloadLoFiImages();

    auto* web_contents = InfoBarService::WebContentsFromInfoBar(infobar());
    auto* proxy_settings =
        DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
            web_contents->GetBrowserContext());
    proxy_settings->IncrementLoFiUserRequestsForImages();
  }

This scopes the temps as narrowly as possible, groups them together, and
eliminates duplicated code.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:10: #include
"components/infobars/core/confirm_infobar_delegate.h"
This is the only #include we actually need.

Even if you wanted to do strict IWYU, we wouldn't need infobar_delegate.h,
because we don't ever refer to its contents.  But Chromium code doesn't
generally do IWYU.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:15: }  // namespace content
Nit: I'd omit the trailing comment here; you omitted it below, and on these
short forward-decl blocks, it's just noise.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:23: // The type of the
infobar. It controls the strings and records UMA for the
Nit: An enum can't "record UMA"; do you mean "and what UMA data is recorded for
the infobar"? (2 places)

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:26: LOFI,
Nit: Consider explaining what these types mean.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:37:
~PreviewsInfoBarDelegate() override;
Nit: Per Google style guide, destructor goes above static methods

[megjablon, 2016-08-23 23:53:56]

Patchset #4 (id:120001) has been deleted

[megjablon, 2016-08-23 23:54:18]

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/app/gene...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/app/gene...
chrome/app/generated_resources.grd:6368: +         Saved data.
On 2016/08/23 19:48:17, Peter Kasting wrote:
> Nit: Putting periods at the end of these fragments seems a bit unusual, make
> sure you have a tech writer/UI sign off on your strings.

Yep! These have been signed off on by UI.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:46:
PreviewsInfoBarDelegate::Create(web_contents,
On 2016/08/23 19:48:17, Peter Kasting wrote:
> Is this code accessible on non-Android builds?
> 
> Right now the resources, enum values, implementation locations, etc. are all
> done in cross-platform ways, but it seems as if this is maybe an
> Android-specific thing.  If it is, we want to make that as clear as possible
and
> avoid compiling in this stuff for the other platforms.

Right now we can enable previews on desktop via a command line flag and
chrome://flags. We've always had the context menu items for it, but not the
infobar. bengr@ what do you think about having this on desktop? Will we need it
from a product standpoint? It's particularly useful for testing and debugging.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:26: void
DidStartProvisionalLoadForFrame(
On 2016/08/23 19:48:18, Peter Kasting wrote:
> It seems like this whole object is defined just so the infobar can be removed
> when we start a load in the top-level frame.
> 
> The comments in ShouldExpire() talk about this being necessary because the
> infobar can be shown before a navigation commits.  Presumably, you're having
> some sort of issue where you're adding an infobar when a load is pending, and
> then the commit of that load removes the infobar again.
> 
> If that's the case, the solution should probably be that we don't show the
> infobar until the load commits to begin with.  Pending loads aren't loads yet,
> and the UI in the browser is still showing the URL of the previous page, etc. 
> We don't want to show an infobar for the upcoming page because we don't know
> what the page is, we don't know if the load will happen, and the UI to the
user
> is confused.
> 
> If you're not doing what I've described above, then I need to understand what
> you're trying to do.

I've moved the preview infobar to being displayed by DidFinishNavigation in a
WebContentsObserver which is added in
https://chromiumcodereview.appspot.com/2256383002/. This allowed me to remove
this code.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:31: if
(!render_frame_host->GetParent()) {
On 2016/08/23 19:48:17, Peter Kasting wrote:
> Nit: No {}

Acknowledged.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:32:
delegate_->RemoveInfoBar();
On 2016/08/23 19:48:17, Peter Kasting wrote:
> I believe a member class is an implicit friend of the enclosing class, in
which
> case we can inline the contents of RemoveInfoBar() here and remove that from
the
> delegate's public API.

Acknowledged.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:39:
DISALLOW_IMPLICIT_CONSTRUCTORS(PreviewInfoBarWebContentsObserver);
On 2016/08/23 19:48:17, Peter Kasting wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN is sufficient, you've already declared a custom
> constructor.

Acknowledged.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:56: if (infobar_type ==
PREVIEW || infobar_type == LOFI) {
On 2016/08/23 19:48:18, Peter Kasting wrote:
> Nit: Why not just "if (infobar_type != OFFLINE) {"?

We will be adding new preview types to this class as they are implemented and
need an infobar. This makes sure that those new types have to explicitly specify
if IncrementLoFiUIShown() is called. This ensures that if a dev adding a new
type and doesn't realizing to exclude it here, then IncrementLoFiUIShown() won't
accidentally be called.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:57:
DataReductionProxyChromeSettings* data_reduction_proxy_settings =
On 2016/08/23 19:48:17, Peter Kasting wrote:
> Nit: To reduce verbosity I'd probably just start this "auto* proxy_settings
=",
> but up to you.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:92: return
IDR_INFOBAR_PREVIEWS;
On 2016/08/23 19:48:18, Peter Kasting wrote:
> If at all possible, prefer to add new vector icons and override
GetVectorIconId
> rather than adding new bitmap icons.  We're trying to eliminate bitmap
resources
> from the UI.

If we don't restrict this to just Android, I'll update this.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:99: return
l10n_util::GetStringUTF16(IDS_PREVIEWS_INFOBAR_SAVED_DATA_TITLE);
On 2016/08/23 19:48:17, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return l10n_util::GetStringUTF16((infobar_type_ == OFFLINE)
>                                        ?
IDS_PREVIEWS_INFOBAR_FASTER_PAGE_TITLE
>                                        :
IDS_PREVIEWS_INFOBAR_SAVED_DATA_TITLE);

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:114:
web_contents_observer_.reset();
On 2016/08/23 19:48:17, Peter Kasting wrote:
> Are you doing this because if you don't do it, the reload calls below will
> delete the infobar, and then returning true will cause memory corruption?  If
so
> consider just returning false in that case; that's what that return value is
> for.  In any case, this needs comments as to what's going on.
> 
> (This is another argument for finding a way to do this through the normal
expiry
> path instead of having an external listener able to delete us.)

Acknowledged.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:116:
DataReductionProxyChromeSettings* data_reduction_proxy_settings =
On 2016/08/23 19:48:17, Peter Kasting wrote:
> Nit: Seems like it might be better to do something like:
> 
>   if (infobar_type != OFFLINE) {
>     if (infobar_type_ == PREVIEW)
>       web_contents->GetController().ReloadDisableLoFi(true);
>     else
>       web_contents->ReloadLoFiImages();
> 
>     auto* web_contents = InfoBarService::WebContentsFromInfoBar(infobar());
>     auto* proxy_settings =
>         DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
>             web_contents->GetBrowserContext());
>     proxy_settings->IncrementLoFiUserRequestsForImages();
>   }
> 
> This scopes the temps as narrowly as possible, groups them together, and
> eliminates duplicated code.

As mentioned above, more types will be added, and I think this keeps clear what
actions need to happen for each infobar type. An action will be added for
offline once that part is implemented and be added here. I think that since only
the IncrementLoFiUserRequestsForImages is duplicated that this is a more
readable and adaptable. The webcontents will most likely be needed for any
reload we do for the infobar. I'd be ok with changing it the following, but
still think the current code makes it super clear what happens with each
infobar.

  content::WebContents* web_contents =
      InfoBarService::WebContentsFromInfoBar(infobar());

  if (infobar_type_ == LITE_PAGE || infobar_type_ == LOFI) {
    if (infobar_type_ == LITE_PAGE) 
      web_contents->GetController().ReloadDisableLoFi(true);
    if (infobar_type_ == LOFI)
      web_contents->ReloadLoFiImages();

    DataReductionProxyChromeSettings* data_reduction_proxy_settings =
        DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
            web_contents->GetBrowserContext());
    data_reduction_proxy_settings->IncrementLoFiUserRequestsForImages();
  }

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:10: #include
"components/infobars/core/confirm_infobar_delegate.h"
On 2016/08/23 19:48:18, Peter Kasting wrote:
> This is the only #include we actually need.
> 
> Even if you wanted to do strict IWYU, we wouldn't need infobar_delegate.h,
> because we don't ever refer to its contents.  But Chromium code doesn't
> generally do IWYU.

Kept macros because it's used by DISALLOW_COPY_AND_ASSIGN

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:15: }  // namespace content
On 2016/08/23 19:48:18, Peter Kasting wrote:
> Nit: I'd omit the trailing comment here; you omitted it below, and on these
> short forward-decl blocks, it's just noise.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:23: // The type of the
infobar. It controls the strings and records UMA for the
On 2016/08/23 19:48:18, Peter Kasting wrote:
> Nit: An enum can't "record UMA"; do you mean "and what UMA data is recorded
for
> the infobar"? (2 places)

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:26: LOFI,
On 2016/08/23 19:48:18, Peter Kasting wrote:
> Nit: Consider explaining what these types mean.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:37:
~PreviewsInfoBarDelegate() override;
On 2016/08/23 19:48:18, Peter Kasting wrote:
> Nit: Per Google style guide, destructor goes above static methods

Done.

[Peter Kasting, 2016-08-24 03:18:28]

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:46:
PreviewsInfoBarDelegate::Create(web_contents,
On 2016/08/23 23:54:16, megjablon wrote:
> On 2016/08/23 19:48:17, Peter Kasting wrote:
> > Is this code accessible on non-Android builds?
> > 
> > Right now the resources, enum values, implementation locations, etc. are all
> > done in cross-platform ways, but it seems as if this is maybe an
> > Android-specific thing.  If it is, we want to make that as clear as possible
> and
> > avoid compiling in this stuff for the other platforms.
> 
> Right now we can enable previews on desktop via a command line flag and
> chrome://flags. We've always had the context menu items for it, but not the
> infobar. bengr@ what do you think about having this on desktop? Will we need
it
> from a product standpoint? It's particularly useful for testing and debugging.

I am OK with resolving this issue as a follow-up, but do strive to resolve it --
we've had a lot of confusion and trouble over the years with
seemingly-cross-platform code that turned out to be Android-only in practice, so
either being clearly Android-only or else actually working on all platforms are
better end states.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:116:
DataReductionProxyChromeSettings* data_reduction_proxy_settings =
On 2016/08/23 23:54:17, megjablon wrote:
> On 2016/08/23 19:48:17, Peter Kasting wrote:
> > Nit: Seems like it might be better to do something like:
> > 
> >   if (infobar_type != OFFLINE) {
> >     if (infobar_type_ == PREVIEW)
> >       web_contents->GetController().ReloadDisableLoFi(true);
> >     else
> >       web_contents->ReloadLoFiImages();
> > 
> >     auto* web_contents = InfoBarService::WebContentsFromInfoBar(infobar());
> >     auto* proxy_settings =
> >         DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
> >             web_contents->GetBrowserContext());
> >     proxy_settings->IncrementLoFiUserRequestsForImages();
> >   }
> > 
> > This scopes the temps as narrowly as possible, groups them together, and
> > eliminates duplicated code.
> 
> As mentioned above, more types will be added, and I think this keeps clear
what
> actions need to happen for each infobar type. An action will be added for
> offline once that part is implemented and be added here. I think that since
only
> the IncrementLoFiUserRequestsForImages is duplicated that this is a more
> readable and adaptable. The webcontents will most likely be needed for any
> reload we do for the infobar. I'd be ok with changing it the following, but
> still think the current code makes it super clear what happens with each
> infobar.
> 
>   content::WebContents* web_contents =
>       InfoBarService::WebContentsFromInfoBar(infobar());
> 
>   if (infobar_type_ == LITE_PAGE || infobar_type_ == LOFI) {
>     if (infobar_type_ == LITE_PAGE) 
>       web_contents->GetController().ReloadDisableLoFi(true);
>     if (infobar_type_ == LOFI)
>       web_contents->ReloadLoFiImages();
> 
>     DataReductionProxyChromeSettings* data_reduction_proxy_settings =
>         DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
>             web_contents->GetBrowserContext());
>     data_reduction_proxy_settings->IncrementLoFiUserRequestsForImages();
>   }

Your counter-proposal looks fine to me.  I would still move |web_contents| into
the conditional since it's not used outside it (but it does have to be done
first there, not later on like I had it; my version wouldn't compile).

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:10: #include
"components/infobars/core/confirm_infobar_delegate.h"
On 2016/08/23 23:54:18, megjablon wrote:
> On 2016/08/23 19:48:18, Peter Kasting wrote:
> > This is the only #include we actually need.
> > 
> > Even if you wanted to do strict IWYU, we wouldn't need infobar_delegate.h,
> > because we don't ever refer to its contents.  But Chromium code doesn't
> > generally do IWYU.
> 
> Kept macros because it's used by DISALLOW_COPY_AND_ASSIGN

Sure, but confirm_infobar_delegate.h pulls that in too.

It's not a big deal, especially when it's macros.h and your list is two items
long anyway.  I just feel weird being in a place where we're not quite "minimum
includes possible" and not quite "IWYU".

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:23: // The type of the
infobar. It controls the strings and records UMA for the
On 2016/08/23 23:54:18, megjablon wrote:
> On 2016/08/23 19:48:18, Peter Kasting wrote:
> > Nit: An enum can't "record UMA"; do you mean "and what UMA data is recorded
> for
> > the infobar"? (2 places)
> 
> Done.

Consider also updating the comment on Create() that has similar text.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:14: #include
"content/public/browser/web_contents_observer.h"
Nit: I think at least this #include can now be removed, check for other extras
too.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:21: // TODO: Check that
infobar was not already shown.
By this I assume you're referring to your separate CL with the tab helper.

However, this also reminds me that there's something else you should do. 
InfoBarManager enforces that you can't add dupe infobars.  It does it by running
EqualsDelegate() to check if two infobar delegates are the same.  You need to
override that, because the default impl in ConfirmInfoBarDelegate checks the
results of GetMessageText(), but for you that function doesn't return something
unique to this infobar.  Therefore you run the risk that, if someone else has an
infobar that also returns the empty string from their GetMessageText(), the two
of you won't be able to both show infobars; the second one will just get
silently rejected.

So you should implement that function.  I dunno if you can just call
GetIdentifier() on both delegates, or if you want to consider a LOFI infobar
different from a LITE_PAGE one, or whatever.  Probably the simplest solution
works because this function is normally only called for dupe add prevention and
you are going to be writing your own code for dupe additions.  So it just has to
not break when called with your delegate and someone else's.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:25:
std::unique_ptr<infobars::InfoBar> infobar(
Nit: I'd probably inline this below as most Create() functions do.  This is only
partly to save verbosity; it's also to minimize direct naming of unique_ptrs
(see next comment)

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:28: new
PreviewsInfoBarDelegate(web_contents, infobar_type))));
Nit: Prefer MakeUnique to unique_ptr(new X).

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:33: auto*
data_reduction_proxy_settings =
Nit: If you're going to use auto here (which I'm in favor of), I'd also use it
in the identical code in LinkClicked().

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:36:
data_reduction_proxy_settings->IncrementLoFiUIShown();
You should only increment this if AddInfoBar() succeeded.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:51: return true;
I don't think you want to unconditionally return true here.  That will mean that
navigations considered to be "within the same page" (like clicks on a wikipedia
anchor tag) will expire this infobar.

You likely want to just leave this method not overridden, but I'm not sure.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:38: class
PreviewInfoBarWebContentsObserver;
I think this declaration is dead.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:43: // InfoBarDelegate
overrides:
Nit: You don't directly override InfoBarDelegate, so just list this function in
the ConfirmInfoBarDelegate list, since that's the class you override it through.

(This is already happening for GetIdentifier() and GetIconId(); you'll want to
list this below those to match the original declaration order.)

[Peter Kasting, 2016-08-24 03:43:07]

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:28: new
PreviewsInfoBarDelegate(web_contents, infobar_type))));
On 2016/08/24 03:18:27, Peter Kasting wrote:
> Nit: Prefer MakeUnique to unique_ptr(new X).

Belay that, you can't use MakeUnique() with a private constructor.  Sorry!

[Peter Kasting, 2016-08-24 03:53:04]

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:21: // TODO: Check that
infobar was not already shown.
On 2016/08/24 03:18:28, Peter Kasting wrote:
> By this I assume you're referring to your separate CL with the tab helper.
> 
> However, this also reminds me that there's something else you should do. 
> InfoBarManager enforces that you can't add dupe infobars.  It does it by
running
> EqualsDelegate() to check if two infobar delegates are the same.  You need to
> override that, because the default impl in ConfirmInfoBarDelegate checks the
> results of GetMessageText(), but for you that function doesn't return
something
> unique to this infobar.  Therefore you run the risk that, if someone else has
an
> infobar that also returns the empty string from their GetMessageText(), the
two
> of you won't be able to both show infobars; the second one will just get
> silently rejected.
> 
> So you should implement that function.  I dunno if you can just call
> GetIdentifier() on both delegates, or if you want to consider a LOFI infobar
> different from a LITE_PAGE one, or whatever.  Probably the simplest solution
> works because this function is normally only called for dupe add prevention
and
> you are going to be writing your own code for dupe additions.  So it just has
to
> not break when called with your delegate and someone else's.

Wow, I am sorry, I totally cross-wired.  This comment should have gone on
someone else's review that returned an empty string from GetMessageText().  You
can ignore it completely.

Sigh.  Two bad directives in one comment set.  My apologies :(

[megjablon, 2016-08-24 21:27:43]

Patchset #6 (id:180001) has been deleted

[megjablon, 2016-08-24 21:28:37]

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:14: #include
"content/public/browser/web_contents_observer.h"
On 2016/08/24 03:18:27, Peter Kasting wrote:
> Nit: I think at least this #include can now be removed, check for other extras
> too.

Ah oops. I've gotten too used to eclipse pointing out unused code for java :/

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:21: // TODO: Check that
infobar was not already shown.
On 2016/08/24 03:53:04, Peter Kasting wrote:
> On 2016/08/24 03:18:28, Peter Kasting wrote:
> > By this I assume you're referring to your separate CL with the tab helper.
> > 
> > However, this also reminds me that there's something else you should do. 
> > InfoBarManager enforces that you can't add dupe infobars.  It does it by
> running
> > EqualsDelegate() to check if two infobar delegates are the same.  You need
to
> > override that, because the default impl in ConfirmInfoBarDelegate checks the
> > results of GetMessageText(), but for you that function doesn't return
> something
> > unique to this infobar.  Therefore you run the risk that, if someone else
has
> an
> > infobar that also returns the empty string from their GetMessageText(), the
> two
> > of you won't be able to both show infobars; the second one will just get
> > silently rejected.
> > 
> > So you should implement that function.  I dunno if you can just call
> > GetIdentifier() on both delegates, or if you want to consider a LOFI infobar
> > different from a LITE_PAGE one, or whatever.  Probably the simplest solution
> > works because this function is normally only called for dupe add prevention
> and
> > you are going to be writing your own code for dupe additions.  So it just
has
> to
> > not break when called with your delegate and someone else's.
> 
> Wow, I am sorry, I totally cross-wired.  This comment should have gone on
> someone else's review that returned an empty string from GetMessageText(). 
You
> can ignore it completely.
> 
> Sigh.  Two bad directives in one comment set.  My apologies :(

Acknowledged.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:25:
std::unique_ptr<infobars::InfoBar> infobar(
On 2016/08/24 03:18:27, Peter Kasting wrote:
> Nit: I'd probably inline this below as most Create() functions do.  This is
only
> partly to save verbosity; it's also to minimize direct naming of unique_ptrs
> (see next comment)

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:28: new
PreviewsInfoBarDelegate(web_contents, infobar_type))));
On 2016/08/24 03:43:07, Peter Kasting wrote:
> On 2016/08/24 03:18:27, Peter Kasting wrote:
> > Nit: Prefer MakeUnique to unique_ptr(new X).
> 
> Belay that, you can't use MakeUnique() with a private constructor.  Sorry!

Acknowledged.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:33: auto*
data_reduction_proxy_settings =
On 2016/08/24 03:18:27, Peter Kasting wrote:
> Nit: If you're going to use auto here (which I'm in favor of), I'd also use it
> in the identical code in LinkClicked().

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:36:
data_reduction_proxy_settings->IncrementLoFiUIShown();
On 2016/08/24 03:18:27, Peter Kasting wrote:
> You should only increment this if AddInfoBar() succeeded.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:51: return true;
On 2016/08/24 03:18:27, Peter Kasting wrote:
> I don't think you want to unconditionally return true here.  That will mean
that
> navigations considered to be "within the same page" (like clicks on a
wikipedia
> anchor tag) will expire this infobar.
> 
> You likely want to just leave this method not overridden, but I'm not sure.

Updated this to calling InfoBarDelegate which checks the navigation details.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:38: class
PreviewInfoBarWebContentsObserver;
On 2016/08/24 03:18:28, Peter Kasting wrote:
> I think this declaration is dead.

Ah sorry, I was worrying about getting this out quickly and forgot to do the
cleanup for this.

https://chromiumcodereview.appspot.com/2250223002/diff/140001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:43: // InfoBarDelegate
overrides:
On 2016/08/24 03:18:28, Peter Kasting wrote:
> Nit: You don't directly override InfoBarDelegate, so just list this function
in
> the ConfirmInfoBarDelegate list, since that's the class you override it
through.
> 
> (This is already happening for GetIdentifier() and GetIconId(); you'll want to
> list this below those to match the original declaration order.)

Done.

[megjablon, 2016-08-24 21:31:08]

Patchset #6 (id:200001) has been deleted

[Peter Kasting, 2016-08-24 21:33:15]

It looks like you might have missed the further replies I had written to your
replies on patch set 3.  Check the first four comments listed in my last long
email.

[megjablon, 2016-08-24 21:34:55]

On 2016/08/24 21:33:15, Peter Kasting wrote:
> It looks like you might have missed the further replies I had written to your
> replies on patch set 3.  Check the first four comments listed in my last long
> email.

Ah my bad, I didn't see those. I'll take a look.

[megjablon, 2016-08-24 21:44:34]

Here they are!

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:46:
PreviewsInfoBarDelegate::Create(web_contents,
On 2016/08/24 03:18:27, Peter Kasting wrote:
> On 2016/08/23 23:54:16, megjablon wrote:
> > On 2016/08/23 19:48:17, Peter Kasting wrote:
> > > Is this code accessible on non-Android builds?
> > > 
> > > Right now the resources, enum values, implementation locations, etc. are
all
> > > done in cross-platform ways, but it seems as if this is maybe an
> > > Android-specific thing.  If it is, we want to make that as clear as
possible
> > and
> > > avoid compiling in this stuff for the other platforms.
> > 
> > Right now we can enable previews on desktop via a command line flag and
> > chrome://flags. We've always had the context menu items for it, but not the
> > infobar. bengr@ what do you think about having this on desktop? Will we need
> it
> > from a product standpoint? It's particularly useful for testing and
debugging.
> 
> I am OK with resolving this issue as a follow-up, but do strive to resolve it
--
> we've had a lot of confusion and trouble over the years with
> seemingly-cross-platform code that turned out to be Android-only in practice,
so
> either being clearly Android-only or else actually working on all platforms
are
> better end states.

The code fully works on all platforms so it doesn't fall into the
seemingly-cross-platform bucket. This UI was the only part of the feature living
in Android before and now we can display it on desktop too.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:116:
DataReductionProxyChromeSettings* data_reduction_proxy_settings =
On 2016/08/24 03:18:27, Peter Kasting wrote:
> On 2016/08/23 23:54:17, megjablon wrote:
> > On 2016/08/23 19:48:17, Peter Kasting wrote:
> > > Nit: Seems like it might be better to do something like:
> > > 
> > >   if (infobar_type != OFFLINE) {
> > >     if (infobar_type_ == PREVIEW)
> > >       web_contents->GetController().ReloadDisableLoFi(true);
> > >     else
> > >       web_contents->ReloadLoFiImages();
> > > 
> > >     auto* web_contents =
InfoBarService::WebContentsFromInfoBar(infobar());
> > >     auto* proxy_settings =
> > >         DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
> > >             web_contents->GetBrowserContext());
> > >     proxy_settings->IncrementLoFiUserRequestsForImages();
> > >   }
> > > 
> > > This scopes the temps as narrowly as possible, groups them together, and
> > > eliminates duplicated code.
> > 
> > As mentioned above, more types will be added, and I think this keeps clear
> what
> > actions need to happen for each infobar type. An action will be added for
> > offline once that part is implemented and be added here. I think that since
> only
> > the IncrementLoFiUserRequestsForImages is duplicated that this is a more
> > readable and adaptable. The webcontents will most likely be needed for any
> > reload we do for the infobar. I'd be ok with changing it the following, but
> > still think the current code makes it super clear what happens with each
> > infobar.
> > 
> >   content::WebContents* web_contents =
> >       InfoBarService::WebContentsFromInfoBar(infobar());
> > 
> >   if (infobar_type_ == LITE_PAGE || infobar_type_ == LOFI) {
> >     if (infobar_type_ == LITE_PAGE) 
> >       web_contents->GetController().ReloadDisableLoFi(true);
> >     if (infobar_type_ == LOFI)
> >       web_contents->ReloadLoFiImages();
> > 
> >     DataReductionProxyChromeSettings* data_reduction_proxy_settings =
> >         DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
> >             web_contents->GetBrowserContext());
> >     data_reduction_proxy_settings->IncrementLoFiUserRequestsForImages();
> >   }
> 
> Your counter-proposal looks fine to me.  I would still move |web_contents|
into
> the conditional since it's not used outside it (but it does have to be done
> first there, not later on like I had it; my version wouldn't compile).

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:10: #include
"components/infobars/core/confirm_infobar_delegate.h"
On 2016/08/24 03:18:27, Peter Kasting wrote:
> On 2016/08/23 23:54:18, megjablon wrote:
> > On 2016/08/23 19:48:18, Peter Kasting wrote:
> > > This is the only #include we actually need.
> > > 
> > > Even if you wanted to do strict IWYU, we wouldn't need infobar_delegate.h,
> > > because we don't ever refer to its contents.  But Chromium code doesn't
> > > generally do IWYU.
> > 
> > Kept macros because it's used by DISALLOW_COPY_AND_ASSIGN
> 
> Sure, but confirm_infobar_delegate.h pulls that in too.
> 
> It's not a big deal, especially when it's macros.h and your list is two items
> long anyway.  I just feel weird being in a place where we're not quite
"minimum
> includes possible" and not quite "IWYU".

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/100001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:23: // The type of the
infobar. It controls the strings and records UMA for the
On 2016/08/24 03:18:27, Peter Kasting wrote:
> On 2016/08/23 23:54:18, megjablon wrote:
> > On 2016/08/23 19:48:18, Peter Kasting wrote:
> > > Nit: An enum can't "record UMA"; do you mean "and what UMA data is
recorded
> > for
> > > the infobar"? (2 places)
> > 
> > Done.
> 
> Consider also updating the comment on Create() that has similar text.

Done.

[Peter Kasting, 2016-08-24 21:58:00]

LGTM

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:18: void
PreviewsInfoBarDelegate::Create(content::WebContents* web_contents,
Nit: Function definition order in .cc must match declaration order in .h.

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:15: class
DataReductionProxySettings;
This forward-decl is no longer needed.

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:32: // recorded for the
infobar.
Nit: Now that this second sentence is verbatim with the enum, do you think we
should just kill it as being "obvious"?  I don't have a strong opinion here.

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:40: //
ConfirmInfoBarDelegate overrides:
Nit: " overrides" not necessary

https://chromiumcodereview.appspot.com/2250223002/diff/220001/components/data...
File components/data_reduction_proxy/content/browser/content_lofi_ui_service.h
(right):

https://chromiumcodereview.appspot.com/2250223002/diff/220001/components/data...
components/data_reduction_proxy/content/browser/content_lofi_ui_service.h:28:
OnLoFiResponseReceivedCallback;
Nit: Prefer type aliases (with "using") to typedefs

https://chromiumcodereview.appspot.com/2250223002/diff/220001/components/data...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://chromiumcodereview.appspot.com/2250223002/diff/220001/components/data...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:230:
// that many "Load image" snackbars, reset the consecutive sessions count.
Nit: Fix reference to "snackbar"

[megjablon, 2016-08-24 22:23:43]

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:18: void
PreviewsInfoBarDelegate::Create(content::WebContents* web_contents,
On 2016/08/24 21:58:00, Peter Kasting wrote:
> Nit: Function definition order in .cc must match declaration order in .h.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:15: class
DataReductionProxySettings;
On 2016/08/24 21:58:00, Peter Kasting wrote:
> This forward-decl is no longer needed.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:32: // recorded for the
infobar.
On 2016/08/24 21:58:00, Peter Kasting wrote:
> Nit: Now that this second sentence is verbatim with the enum, do you think we
> should just kill it as being "obvious"?  I don't have a strong opinion here.

I'm good with removing it.

https://chromiumcodereview.appspot.com/2250223002/diff/220001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:40: //
ConfirmInfoBarDelegate overrides:
On 2016/08/24 21:58:00, Peter Kasting wrote:
> Nit: " overrides" not necessary

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/220001/components/data...
File components/data_reduction_proxy/content/browser/content_lofi_ui_service.h
(right):

https://chromiumcodereview.appspot.com/2250223002/diff/220001/components/data...
components/data_reduction_proxy/content/browser/content_lofi_ui_service.h:28:
OnLoFiResponseReceivedCallback;
On 2016/08/24 21:58:00, Peter Kasting wrote:
> Nit: Prefer type aliases (with "using") to typedefs

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/220001/components/data...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://chromiumcodereview.appspot.com/2250223002/diff/220001/components/data...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:230:
// that many "Load image" snackbars, reset the consecutive sessions count.
On 2016/08/24 21:58:00, Peter Kasting wrote:
> Nit: Fix reference to "snackbar"

Done.

[megjablon, 2016-08-24 23:57:38]

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

[commit-bot: I haz the power, 2016-08-24 23:58:37]

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, 2016-08-25 01:46:12]

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

[commit-bot: I haz the power, 2016-08-25 01:46:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bengr, 2016-08-25 23:26:19]

https://codereview.chromium.org/2250223002/diff/280001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2250223002/diff/280001/chrome/app/generated_r...
chrome/app/generated_resources.grd:6370: +      <message
name="IDS_PREVIEWS_INFOBAR_SAVED_DATA_TITLE" desc="The text of the infobar
notifying the user that a preview page has been shown to save them data.">
remove the word "them"

https://codereview.chromium.org/2250223002/diff/280001/chrome/app/generated_r...
chrome/app/generated_resources.grd:6373: +      <message
name="IDS_PREVIEWS_INFOBAR_FASTER_PAGE_TITLE" desc="The text of the infobar
notifying the user that a preview page has been shown so that it loaded
faster.">
so that it loaded -> to load

https://codereview.chromium.org/2250223002/diff/280001/chrome/app/generated_r...
chrome/app/generated_resources.grd:6376: +      <message
name="IDS_PREVIEWS_INFOBAR_LINK" desc="The link to load the original page for
the infobar notifying the user that a preview page has been shown.">
link -> text of the link
for -> from

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:39: // For
Android builds, notifies the TabAndroid associated with |web_contents|
Is this still accurate? You removed references in code to TabAndroid.

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:22: // TODO: Check that
infobar was not already shown.
// TODO(megjablon): Check that the infobar was not already shown.

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:30: new
PreviewsInfoBarDelegate(web_contents, infobar_type))));
You can use base::MakeUnique<PreviewsInfobarDelegate>(web_contents,
infobar_type);

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:36: if
(data_reduction_proxy_settings)
Can this ever be null?

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:62: // TODO: record uma
data
// TODO(megjablon): Record UMA data.

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:81: // TODO: record uma
data
// TODO(megjablon): Record UMA data.

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:86: if (infobar_type_ ==
LITE_PAGE)
This might be a little clearer with a switch.

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:94: if
(data_reduction_proxy_settings)
Can this ever be null?

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.h:14: class
PreviewsInfoBarDelegate : public ConfirmInfoBarDelegate {
Please add a class comment.

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.h:16: // The type of the
infobar. It controls the strings and what UMA data is
Strings shoudn't be affected.

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.h:35: //
ConfirmInfoBarDelegate:
add the word "overrides"

[Peter Kasting, 2016-08-25 23:55:18]

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:30: new
PreviewsInfoBarDelegate(web_contents, infobar_type))));
On 2016/08/25 23:26:18, bengr wrote:
> You can use base::MakeUnique<PreviewsInfobarDelegate>(web_contents,
> infobar_type);

Nope.  I already suggested that and quickly withdrew it when I remembered that
the constructor is private.

https://codereview.chromium.org/2250223002/diff/280001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:86: if (infobar_type_ ==
LITE_PAGE)
On 2016/08/25 23:26:19, bengr wrote:
> This might be a little clearer with a switch. 

I don't think so:

    switch (infobar_type) {
      case LITE_PAGE:
        web_contents->GetController().ReloadDisableLoFi(true);
        break;
      case LOFI:
        web_contents->ReloadLoFiImages();
        break;
      default:
        break;
    }

...seems strictly worse.

I would suggest sticking an "else" at the beginning of the second "if" that's
here, though.

[jochen (gone - plz use gerrit), 2016-08-26 15:09:32]

lgtm once bengr and pkasting are happy

[megjablon, 2016-08-26 22:47:30]

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/app/gene...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/app/gene...
chrome/app/generated_resources.grd:6370: +      <message
name="IDS_PREVIEWS_INFOBAR_SAVED_DATA_TITLE" desc="The text of the infobar
notifying the user that a preview page has been shown to save them data.">
On 2016/08/25 23:26:18, bengr wrote:
> remove the word "them"

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/app/gene...
chrome/app/generated_resources.grd:6373: +      <message
name="IDS_PREVIEWS_INFOBAR_FASTER_PAGE_TITLE" desc="The text of the infobar
notifying the user that a preview page has been shown so that it loaded
faster.">
On 2016/08/25 23:26:18, bengr wrote:
> so that it loaded -> to load

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/app/gene...
chrome/app/generated_resources.grd:6376: +      <message
name="IDS_PREVIEWS_INFOBAR_LINK" desc="The link to load the original page for
the infobar notifying the user that a preview page has been shown.">
On 2016/08/25 23:26:18, bengr wrote:
> link -> text of the link
> for -> from

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:39: // For
Android builds, notifies the TabAndroid associated with |web_contents|
On 2016/08/25 23:26:18, bengr wrote:
> Is this still accurate? You removed references in code to TabAndroid.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:22: // TODO: Check that
infobar was not already shown.
On 2016/08/25 23:26:18, bengr wrote:
> // TODO(megjablon): Check that the infobar was not already shown.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:36: if
(data_reduction_proxy_settings)
On 2016/08/25 23:26:18, bengr wrote:
> Can this ever be null?


Nope, we shouldn't be showing a LoFi or Lite Page snackbar if we don't have the
DRP enabled and therefore will have a DRPSettings object. Removed.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:62: // TODO: record uma
data
On 2016/08/25 23:26:18, bengr wrote:
> // TODO(megjablon): Record UMA data.
> 

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:81: // TODO: record uma
data
On 2016/08/25 23:26:18, bengr wrote:
> // TODO(megjablon): Record UMA data.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:86: if (infobar_type_ ==
LITE_PAGE)
On 2016/08/25 23:55:18, Peter Kasting wrote:
> On 2016/08/25 23:26:19, bengr wrote:
> > This might be a little clearer with a switch. 
> 
> I don't think so:
> 
>     switch (infobar_type) {
>       case LITE_PAGE:
>         web_contents->GetController().ReloadDisableLoFi(true);
>         break;
>       case LOFI:
>         web_contents->ReloadLoFiImages();
>         break;
>       default:
>         break;
>     }
> 
> ...seems strictly worse.
> 
> I would suggest sticking an "else" at the beginning of the second "if" that's
> here, though.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.cc:94: if
(data_reduction_proxy_settings)
On 2016/08/25 23:26:18, bengr wrote:
> Can this ever be null?

Nope, we shouldn't be showing a LoFi or Lite Page snackbar if we don't have the
DRP enabled and therefore will have a DRPSettings object. Removed.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:14: class
PreviewsInfoBarDelegate : public ConfirmInfoBarDelegate {
On 2016/08/25 23:26:19, bengr wrote:
> Please add a class comment.

Done.

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:16: // The type of the
infobar. It controls the strings and what UMA data is
On 2016/08/25 23:26:19, bengr wrote:
> Strings shoudn't be affected.

The offline infobar gets "Faster page loaded." instead of the "Saved data."

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:35: //
ConfirmInfoBarDelegate:
On 2016/08/25 23:26:19, bengr wrote:
> add the word "overrides"

Adding this back in. pkasting asked me to remove it.

[Peter Kasting, 2016-08-27 03:42:22]

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:35: //
ConfirmInfoBarDelegate:
On 2016/08/26 22:47:30, megjablon wrote:
> On 2016/08/25 23:26:19, bengr wrote:
> > add the word "overrides"
> 
> Adding this back in. pkasting asked me to remove it.

Heh.

There's no consistency across the codebase.  Both of these forms are in wide
use, so we can't really force you to use one over the other.

I always avoid it, because it's pretty much understood, and all the infobar code
and a lot of the views code avoids it.  Less verbiage for the win.  But those
are minor reasons in the grand scheme of things.

Do whatever you find most readable and consistent :)

[megjablon, 2016-08-30 17:53:58]

On 2016/08/27 03:42:22, Peter Kasting wrote:
>
https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
> File chrome/browser/previews/previews_infobar_delegate.h (right):
> 
>
https://chromiumcodereview.appspot.com/2250223002/diff/280001/chrome/browser/...
> chrome/browser/previews/previews_infobar_delegate.h:35: //
> ConfirmInfoBarDelegate:
> On 2016/08/26 22:47:30, megjablon wrote:
> > On 2016/08/25 23:26:19, bengr wrote:
> > > add the word "overrides"
> > 
> > Adding this back in. pkasting asked me to remove it.
> 
> Heh.
> 
> There's no consistency across the codebase.  Both of these forms are in wide
> use, so we can't really force you to use one over the other.
> 
> I always avoid it, because it's pretty much understood, and all the infobar
code
> and a lot of the views code avoids it.  Less verbiage for the win.  But those
> are minor reasons in the grand scheme of things.
> 
> Do whatever you find most readable and consistent :)

I'm going to leave it in since that's consistent with the code that my team owns
and since we'll be owners of this.

[megjablon, 2016-08-30 17:54:47]

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

[megjablon, 2016-08-30 17:54:48]

+dfalcantara: For chrome/android/* files

[gone, 2016-08-30 22:37:54]

Screenshots?  Have the resources been run through
tools/resources/optimize-png-files.sh?

[gone, 2016-08-30 22:54:06]

https://chromiumcodereview.appspot.com/2250223002/diff/300001/chrome/app/gene...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/2250223002/diff/300001/chrome/app/gene...
chrome/app/generated_resources.grd:6371: +         Saved data.
This doesn't match the string in the mock; you need to use a templated one to
put the saved percentage.  Has the spec changed?

https://docs.google.com/presentation/d/1dWIxBtAM6uX3oMCh_0mpq115KS1UAHpI4MbQO...

https://chromiumcodereview.appspot.com/2250223002/diff/300001/components/info...
File components/infobars/core/infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/300001/components/info...
components/infobars/core/infobar_delegate.h:137:
DATA_REDUCTION_PROXY_PREVIEW_INFOBAR_DELEGATE = 67,
Corresponding histograms.xml change?

[megjablon, 2016-08-30 23:48:33]

https://chromiumcodereview.appspot.com/2250223002/diff/300001/chrome/app/gene...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/2250223002/diff/300001/chrome/app/gene...
chrome/app/generated_resources.grd:6371: +         Saved data.
On 2016/08/30 22:54:06, dfalcantara wrote:
> This doesn't match the string in the mock; you need to use a templated one to
> put the saved percentage.  Has the spec changed?
> 
>
https://docs.google.com/presentation/d/1dWIxBtAM6uX3oMCh_0mpq115KS1UAHpI4MbQO...

Yes the spec has changed from the mocks. See the latest comments by bengr on the
bug.

https://chromiumcodereview.appspot.com/2250223002/diff/300001/components/info...
File components/infobars/core/infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/300001/components/info...
components/infobars/core/infobar_delegate.h:137:
DATA_REDUCTION_PROXY_PREVIEW_INFOBAR_DELEGATE = 67,
On 2016/08/30 22:54:06, dfalcantara wrote:
> Corresponding histograms.xml change?

Done.

[gone, 2016-08-30 23:51:34]

lgtm

[bengr, 2016-08-31 23:37:35]

lgtm with nits.

https://codereview.chromium.org/2250223002/diff/320001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/2250223002/diff/320001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:40: //
PreviewsInfoBarDelegate is created which handles showing Lo-Fi UI
created which -> created, which
showing -> showing
UI -> UI.

https://codereview.chromium.org/2250223002/diff/320001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://codereview.chromium.org/2250223002/diff/320001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.h:16: // be shown once per
page load. Records uma data for user interactions with the
uma -> UMA

[Peter Kasting, 2016-09-01 07:16:56]

https://codereview.chromium.org/2250223002/diff/320001/chrome/browser/net/spd...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/2250223002/diff/320001/chrome/browser/net/spd...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:40: //
PreviewsInfoBarDelegate is created which handles showing Lo-Fi UI
For this piece of your suggestion:

On 2016/08/31 23:37:34, bengr wrote:
> showing -> showing

I'm not seeing the proposed change.

[megjablon, 2016-09-07 22:02:23]

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

[commit-bot: I haz the power, 2016-09-07 22:03:03]

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, 2016-09-07 22:06:24]

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

[commit-bot: I haz the power, 2016-09-07 22:06:25]

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

[megjablon, 2016-09-07 23:21:55]

Patchset #13 (id:360001) has been deleted

[megjablon, 2016-09-07 23:23:34]

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

[commit-bot: I haz the power, 2016-09-07 23:24: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, 2016-09-07 23:30:10]

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

[commit-bot: I haz the power, 2016-09-07 23:30:11]

Dry run: Exceeded global retry quota

[megjablon, 2016-09-07 23:54:41]

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

[commit-bot: I haz the power, 2016-09-07 23:55:28]

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, 2016-09-07 23:59:14]

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

[commit-bot: I haz the power, 2016-09-07 23:59:15]

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

[megjablon, 2016-09-08 00:25:46]

https://chromiumcodereview.appspot.com/2250223002/diff/320001/chrome/browser/...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://chromiumcodereview.appspot.com/2250223002/diff/320001/chrome/browser/...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:40: //
PreviewsInfoBarDelegate is created which handles showing Lo-Fi UI
On 2016/09/01 07:16:55, Peter Kasting wrote:
> For this piece of your suggestion:
> 
> On 2016/08/31 23:37:34, bengr wrote:
> > showing -> showing
> 
> I'm not seeing the proposed change.

Changed the other two

https://chromiumcodereview.appspot.com/2250223002/diff/320001/chrome/browser/...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/2250223002/diff/320001/chrome/browser/...
chrome/browser/previews/previews_infobar_delegate.h:16: // be shown once per
page load. Records uma data for user interactions with the
On 2016/08/31 23:37:34, bengr wrote:
> uma -> UMA

Done.

[megjablon, 2016-09-13 21:43:01]

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

[commit-bot: I haz the power, 2016-09-13 21:43:38]

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, 2016-09-13 21:47:16]

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

[commit-bot: I haz the power, 2016-09-13 21:47:18]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[megjablon, 2016-09-13 21:55:32]

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

[commit-bot: I haz the power, 2016-09-13 21:56:00]

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, 2016-09-13 23:03:03]

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

[commit-bot: I haz the power, 2016-09-13 23:03:04]

Dry run: This issue passed the CQ dry run.

[megjablon, 2016-09-13 23:29:05]

The CQ bit was checked by megjablon@chromium.org

[megjablon, 2016-09-13 23:29:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, jochen@chromium.org, bengr@chromium.org,
dfalcantara@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/2250223002/#ps440001 (title: "rebase")

[commit-bot: I haz the power, 2016-09-13 23:29:33]

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-09-13 23:36:19]

Description was changed from

==========
Add InfoBar delegate for previews

Add PreviewsInfoBarDelegate to show an infobar when a preview
page has been loaded. Also add the corresponding resources.

BUG=615566
==========

to

==========
Add InfoBar delegate for previews

Add PreviewsInfoBarDelegate to show an infobar when a preview
page has been loaded. Also add the corresponding resources.

BUG=615566
==========

[commit-bot: I haz the power, 2016-09-13 23:36:26]

Committed patchset #16 (id:440001)

[commit-bot: I haz the power, 2016-09-13 23:39:13]

Description was changed from

==========
Add InfoBar delegate for previews

Add PreviewsInfoBarDelegate to show an infobar when a preview
page has been loaded. Also add the corresponding resources.

BUG=615566
==========

to

==========
Add InfoBar delegate for previews

Add PreviewsInfoBarDelegate to show an infobar when a preview
page has been loaded. Also add the corresponding resources.

BUG=615566

Committed: https://crrev.com/82aba5257757fad4ab092411822d1895589b9a5b
Cr-Commit-Position: refs/heads/master@{#418413}
==========

[commit-bot: I haz the power, 2016-09-13 23:39:14]

Patchset 16 (id:??) landed as
https://crrev.com/82aba5257757fad4ab092411822d1895589b9a5b
Cr-Commit-Position: refs/heads/master@{#418413}