Add InfoBar delegate for previews

chrome/browser/previews/previews_infobar_delegate.cc

Index: chrome/browser/previews/previews_infobar_delegate.cc
diff --git a/chrome/browser/previews/previews_infobar_delegate.cc b/chrome/browser/previews/previews_infobar_delegate.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a0ebe292ea19ae7853884fe7d89f4802a88f93cc
--- /dev/null
+++ b/chrome/browser/previews/previews_infobar_delegate.cc
@@ -0,0 +1,129 @@
+// 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"
+#include "grit/theme_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+
+class PreviewsInfoBarDelegate::PreviewInfoBarWebContentsObserver
+    : public content::WebContentsObserver {
+ public:
+  PreviewInfoBarWebContentsObserver(content::WebContents* web_contents,
+                                    PreviewsInfoBarDelegate* delegate)
+      : content::WebContentsObserver(web_contents), delegate_(delegate) {}
+
+  // content::WebContentsObserver implementation:
+  void DidStartProvisionalLoadForFrame(

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

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.

[megjablon, 2016/08/23 23:54:17]

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.

+      content::RenderFrameHost* render_frame_host,
+      const GURL& validated_url,
+      bool is_error_page,
+      bool is_iframe_srcdoc) override {
+    if (!render_frame_host->GetParent()) {

[Peter Kasting, 2016/08/23 19:48:17]

Nit: No {}

[megjablon, 2016/08/23 23:54:17]

Acknowledged.

+      delegate_->RemoveInfoBar();

[Peter Kasting, 2016/08/23 19:48:17]

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.

[megjablon, 2016/08/23 23:54:16]

Acknowledged.

+    }
+  }
+
+ private:
+  PreviewsInfoBarDelegate* delegate_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(PreviewInfoBarWebContentsObserver);

[Peter Kasting, 2016/08/23 19:48:17]

Nit: DISALLOW_COPY_AND_ASSIGN is sufficient, you've already declared a custom
constructor.

[megjablon, 2016/08/23 23:54:17]

Acknowledged.

+};
+
+// static
+void PreviewsInfoBarDelegate::Create(content::WebContents* web_contents,
+                                     PreviewsInfoBarType infobar_type) {
+  // TODO: Check that infobar was not already shown.
+
+  InfoBarService* infobar_service =
+      InfoBarService::FromWebContents(web_contents);
+  std::unique_ptr<infobars::InfoBar> infobar(
+      infobar_service->CreateConfirmInfoBar(
+          std::unique_ptr<ConfirmInfoBarDelegate>(
+              new PreviewsInfoBarDelegate(web_contents, infobar_type))));
+
+  infobar_service->AddInfoBar(std::move(infobar));
+
+  if (infobar_type == PREVIEW || infobar_type == LOFI) {

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

Nit: Why not just "if (infobar_type != OFFLINE) {"?

[megjablon, 2016/08/23 23:54:17]

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.

+    DataReductionProxyChromeSettings* data_reduction_proxy_settings =

[Peter Kasting, 2016/08/23 19:48:17]

Nit: To reduce verbosity I'd probably just start this "auto* proxy_settings =",
but up to you.

[megjablon, 2016/08/23 23:54:16]

Done.

+        DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
+            web_contents->GetBrowserContext());
+    data_reduction_proxy_settings->IncrementLoFiUIShown();
+  }
+}
+
+PreviewsInfoBarDelegate::PreviewsInfoBarDelegate(
+    content::WebContents* web_contents,
+    PreviewsInfoBarType infobar_type)
+    : ConfirmInfoBarDelegate(),
+      web_contents_observer_(
+          new PreviewInfoBarWebContentsObserver(web_contents, this)),
+      infobar_type_(infobar_type) {}
+
+PreviewsInfoBarDelegate::~PreviewsInfoBarDelegate() {}
+
+void PreviewsInfoBarDelegate::RemoveInfoBar() {
+  infobar()->owner()->RemoveInfoBar(infobar());
+}
+
+bool PreviewsInfoBarDelegate::ShouldExpire(
+    const NavigationDetails& details) const {
+  // InfoBars are dismissed on navigation commit by ShouldExpire. This InfoBar
+  // can be shown for a page before a navigation commits, so
+  // DidStartProvisionalLoadForFrame is used to dismiss it.
+  return false;
+}
+
+infobars::InfoBarDelegate::InfoBarIdentifier
+PreviewsInfoBarDelegate::GetIdentifier() const {
+  return DATA_REDUCTION_PROXY_PREVIEW_INFOBAR_DELEGATE;
+}
+
+int PreviewsInfoBarDelegate::GetIconId() const {
+  return IDR_INFOBAR_PREVIEWS;

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

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.

[megjablon, 2016/08/23 23:54:17]

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

+}
+
+base::string16 PreviewsInfoBarDelegate::GetMessageText() const {
+  if (infobar_type_ == OFFLINE)
+    return l10n_util::GetStringUTF16(IDS_PREVIEWS_INFOBAR_FASTER_PAGE_TITLE);
+  else
+    return l10n_util::GetStringUTF16(IDS_PREVIEWS_INFOBAR_SAVED_DATA_TITLE);

[Peter Kasting, 2016/08/23 19:48:17]

Nit: Simpler:

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

[megjablon, 2016/08/23 23:54:17]

Done.

+}
+
+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
+  content::WebContents* web_contents =
+      InfoBarService::WebContentsFromInfoBar(infobar());
+  web_contents_observer_.reset();

[Peter Kasting, 2016/08/23 19:48:17]

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

[megjablon, 2016/08/23 23:54:17]

Acknowledged.

+
+  DataReductionProxyChromeSettings* data_reduction_proxy_settings =

[Peter Kasting, 2016/08/23 19:48:17]

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.

[megjablon, 2016/08/23 23:54:17]

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();
  }

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

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

[megjablon, 2016/08/24 21:44:29]

Done.

+      DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
+          web_contents->GetBrowserContext());
+
+  if (infobar_type_ == PREVIEW) {
+    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;
+}