Add InfoBar delegate for previews

chrome/browser/previews/previews_infobar_delegate.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/previews/previews_infobar_delegate.h"
+
+#include "chrome/browser/infobars/infobar_service.h"
+#include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h"
+#include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings_factory.h"
+#include "chrome/grit/generated_resources.h"
+#include "components/infobars/core/infobar.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"

[Peter Kasting, 2016/08/24 03:18:27]

Nit: I think at least this #include can now be removed, check for other extras
too.

[megjablon, 2016/08/24 21:28:36]

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

+#include "grit/theme_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+
+// static
+void PreviewsInfoBarDelegate::Create(content::WebContents* web_contents,
+                                     PreviewsInfoBarType infobar_type) {
+  // TODO: Check that infobar was not already shown.

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

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.

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

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:28:36]

Acknowledged.

+
+  InfoBarService* infobar_service =
+      InfoBarService::FromWebContents(web_contents);
+  std::unique_ptr<infobars::InfoBar> infobar(

[Peter Kasting, 2016/08/24 03:18:27]

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)

[megjablon, 2016/08/24 21:28:36]

Done.

+      infobar_service->CreateConfirmInfoBar(
+          std::unique_ptr<ConfirmInfoBarDelegate>(
+              new PreviewsInfoBarDelegate(web_contents, infobar_type))));

[Peter Kasting, 2016/08/24 03:18:27]

Nit: Prefer MakeUnique to unique_ptr(new X).

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

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

[megjablon, 2016/08/24 21:28:36]

Acknowledged.

+
+  infobar_service->AddInfoBar(std::move(infobar));
+
+  if (infobar_type == LITE_PAGE || infobar_type == LOFI) {
+    auto* data_reduction_proxy_settings =

[Peter Kasting, 2016/08/24 03:18:27]

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().

[megjablon, 2016/08/24 21:28:36]

Done.

+        DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
+            web_contents->GetBrowserContext());
+    data_reduction_proxy_settings->IncrementLoFiUIShown();

[Peter Kasting, 2016/08/24 03:18:27]

You should only increment this if AddInfoBar() succeeded.

[megjablon, 2016/08/24 21:28:36]

Done.

+  }
+}
+
+PreviewsInfoBarDelegate::PreviewsInfoBarDelegate(
+    content::WebContents* web_contents,
+    PreviewsInfoBarType infobar_type)
+    : ConfirmInfoBarDelegate(),
+      infobar_type_(infobar_type) {}
+
+PreviewsInfoBarDelegate::~PreviewsInfoBarDelegate() {}
+
+bool PreviewsInfoBarDelegate::ShouldExpire(
+    const NavigationDetails& details) const {
+  // TODO: record uma data
+  return true;

[Peter Kasting, 2016/08/24 03:18:27]

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.

[megjablon, 2016/08/24 21:28:36]

Updated this to calling InfoBarDelegate which checks the navigation details.

+}
+
+infobars::InfoBarDelegate::InfoBarIdentifier
+PreviewsInfoBarDelegate::GetIdentifier() const {
+  return DATA_REDUCTION_PROXY_PREVIEW_INFOBAR_DELEGATE;
+}
+
+int PreviewsInfoBarDelegate::GetIconId() const {
+  return IDR_INFOBAR_PREVIEWS;
+}
+
+base::string16 PreviewsInfoBarDelegate::GetMessageText() const {
+  return l10n_util::GetStringUTF16((infobar_type_ == OFFLINE)
+                                       ? IDS_PREVIEWS_INFOBAR_FASTER_PAGE_TITLE
+                                       : IDS_PREVIEWS_INFOBAR_SAVED_DATA_TITLE);
+}
+
+int PreviewsInfoBarDelegate::GetButtons() const {
+  return BUTTON_NONE;
+}
+
+base::string16 PreviewsInfoBarDelegate::GetLinkText() const {
+  return l10n_util::GetStringUTF16(IDS_PREVIEWS_INFOBAR_LINK);
+}
+
+bool PreviewsInfoBarDelegate::LinkClicked(WindowOpenDisposition disposition) {
+  // TODO: record uma data
+  content::WebContents* web_contents =
+      InfoBarService::WebContentsFromInfoBar(infobar());
+
+  DataReductionProxyChromeSettings* data_reduction_proxy_settings =
+      DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
+          web_contents->GetBrowserContext());
+
+  if (infobar_type_ == LITE_PAGE) {
+    web_contents->GetController().ReloadDisableLoFi(true);
+    data_reduction_proxy_settings->IncrementLoFiUserRequestsForImages();
+  } else if (infobar_type_ == LOFI) {
+    web_contents->ReloadLoFiImages();
+    data_reduction_proxy_settings->IncrementLoFiUserRequestsForImages();
+  }
+
+  return true;
+}