InstantExtended: Committed NTP

chrome/browser/instant/instant_ntp.h

+// Copyright 2012 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.
+
+#ifndef CHROME_BROWSER_INSTANT_INSTANT_NTP_H_
+#define CHROME_BROWSER_INSTANT_INSTANT_NTP_H_
+
+#include <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/instant/instant_client.h"
+
+class InstantController;
+class Profile;
+struct ThemeBackgroundInfo;
+
+namespace content {
+class WebContents;
+}
+
+// InstantNTP is used to communicate with a WebContents that it owns and loads
+// the "Instant URL" into. This preview is used when the user navigates to a New
+// Tab Page.
+class InstantNTP : public InstantClient::Delegate {
+ public:
+  // Returns the InstantNTP for |contents| if it's used for Instant.
+  static InstantNTP* FromWebContents(const content::WebContents* contents);
+
+  // Doesn't take ownership of |controller|.
+  InstantNTP(InstantController* controller, const std::string& instant_url);
+  virtual ~InstantNTP();
+
+  // The NTP WebContents. InstantNTP retains ownership. This will be
+  // non-NULL after InitFromContents(), and until ReleaseContents() is called.
+  content::WebContents* contents() const { return contents_.get(); }
+
+  // Creates a new WebContents and loads |instant_url_| into it. Uses
+  // |active_tab|, if non-NULL, to initialize the size of the WebContents.
+  void InitContents(Profile* profile,
+                    const content::WebContents* active_tab);
+
+  // Releases the preview WebContents passing ownership to the caller. This
+  // should be called when the preview is committed.
+  content::WebContents* ReleaseContents() WARN_UNUSED_RESULT;
+
+  // Returns the URL that we're loading.
+  const std::string& instant_url() const { return instant_url_; }
+
+  // Returns true if the preview is known to support the Instant API. This
+  // starts out false, and becomes true whenever we get any message from the
+  // page. Once true, it never becomes false (the page isn't expected to drop
+  // Instant API support suddenly).
+  bool supports_instant() const { return supports_instant_; }
+
+  // Calls through to methods of the same name on InstantClient.
+  void SetMarginSize(int start, int end);
+  void SendThemeBackgroundInfo(const ThemeBackgroundInfo& theme_info);
+  void SendThemeAreaHeight(int height);
+  void SetDisplayInstantResults(bool display_instant_results);
+
+ private:
+  class WebContentsDelegateImpl;
+
+  // Overridden from InstantClient::Delegate:
+  virtual void SetSuggestions(
+      const std::vector<InstantSuggestion>& suggestions) OVERRIDE;
+  virtual void InstantSupportDetermined(bool supports_instant) OVERRIDE;
+  virtual void ShowInstantPreview(InstantShownReason reason,
+                                  int height,
+                                  InstantSizeUnits units) OVERRIDE;
+  virtual void StartCapturingKeyStrokes() OVERRIDE;
+  virtual void StopCapturingKeyStrokes() OVERRIDE;
+  virtual void RenderViewGone() OVERRIDE;
+  virtual void AboutToNavigateMainFrame(const GURL& url) OVERRIDE;
+  virtual void NavigateToURL(const GURL& url,
+                             content::PageTransition transition) OVERRIDE;
+
+  void SetupPreviewContents();
+  void CleanupPreviewContents();
+  void ReplacePreviewContents(content::WebContents* old_contents,
+                              content::WebContents* new_contents);
+
+  InstantClient client_;

[dhollowa, 2013/01/10 18:50:29]

I notice that all three InstantOverlay, InstantNTP, and InstantTab have the same
state: client_, controller_, and supports_instant_.

This makes me think there is informal commonality that probably should be
formalized.  Either by keeping this state in InstantClient::Delegate directly,
or better yet, introduce a common concrete base-class, InstantPage say.  It
would look something like this:

class InstantPage : public InstantClient::Delegate {
  ...
 private:
  InstantClient client_;
  InstantController* controller_;
  bool supports_instant_;
}

class InstantOverlay : public InstantPage {...}
class InstantNTP : public InstantNTP { ... }
class InstantTab : public InstantTab { ... }

Thoughts?

[dhollowa, 2013/01/10 21:08:03]

Err... correction:
class InstantNTP : public InstantPage { ... }
class InstantTab : public InstantPage { ... }

(Thanks Thiago).

[samarth, 2013/01/11 19:43:05]

Channeling sky (who was channeling Ben):
"""
Ben was griping today about a review he was looking at (not this one) and
happened to say, 'god calls a kitten every time you subclass a concrete type.'
It made me think of this patch.
"""
Plus I'm hesistant to add yet another class here .. the object hierarchy here is
already a little tricky to follow.

That said, I agree that something about the repetitiveness between
Instant{NTP,Overlay,Tab} smells wrong.  Likewise for some of the code
duplication in InstantController that Jered pointed out.

Let me mull this over some more; feels like some more refactoring is in order
here.

[dhollowa, 2013/01/11 19:55:01]

God also calls a kitten every time the DRY (don't repeat yourself) principle is
violated.  A possibly more acceptable alternate to a the concrete subclass is to
define a common data struct that any delegate needs to provide to *some*
concrete common methods declared and defined in the InstantClient::Delegate
class.  So then common code lives in the InstantClient::Delegate, but data it
acts upon lives in the subclasses of the delegate.

This way no kittens need to be harmed.

+  InstantController* const controller_;
+
+  // Delegate of the preview WebContents.
+  scoped_ptr<WebContentsDelegateImpl> delegate_;
+  scoped_ptr<content::WebContents> contents_;
+
+  const std::string instant_url_;
+  bool supports_instant_;
+
+  DISALLOW_COPY_AND_ASSIGN(InstantNTP);
+};
+
+#endif  // CHROME_BROWSER_INSTANT_INSTANT_NTP_H_