InstantExtended: Committed NTP

chrome/browser/instant/instant_overlay.cc

+// Copyright 2013 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/instant/instant_overlay.h"
+
+#include "base/supports_user_data.h"
+#include "chrome/browser/instant/instant_controller.h"
+#include "content/public/browser/web_contents.h"
+
+namespace {
+
+int kUserDataKey;
+
+class InstantOverlayUserData : public base::SupportsUserData::Data {
+ public:
+  explicit InstantOverlayUserData(InstantOverlay* overlay)
+      : overlay_(overlay) {}
+
+  InstantOverlay* overlay() const { return overlay_; }
+
+ private:
+  ~InstantOverlayUserData() {}
+
+  InstantOverlay* const overlay_;
+
+  DISALLOW_COPY_AND_ASSIGN(InstantOverlayUserData);
+};

[sreeram, 2013/02/01 20:36:53]

How about we get rid of this class, and just make InstantOverlay directly
implement base::SupportsUserData::Data? Then, we instead of sticking a pointer
to InstantOverlayUserData in a web contents, we can just stick the
InstantOverlay pointer directly. So, FromWebContents would look like this:

// static
InstantOverlay* InstantOverlay::FromWebContents(...) {
  return static_cast<InstantOverlay*>(web_contents->GetUserData(&kUserDataKey));
}

[samarth, 2013/02/04 20:43:32]

As we discussed, this doesn't work because SetUserData takes ownership of the
passed in the pointer.

+
+}  // namespace
+
+// static
+InstantOverlay* InstantOverlay::FromWebContents(
+    const content::WebContents* web_contents) {
+  InstantOverlayUserData* data = static_cast<InstantOverlayUserData*>(
+      web_contents->GetUserData(&kUserDataKey));
+  return data ? data->overlay() : NULL;
+}
+
+InstantOverlay::InstantOverlay(InstantController* controller,
+                               const std::string& instant_url)
+    : InstantPage(controller),
+      loader_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      instant_url_(instant_url),
+      is_stale_(false),
+      is_pointer_down_from_activate_(false) {
+}
+
+InstantOverlay::~InstantOverlay() {
+}
+
+void InstantOverlay::InitContents(Profile* profile,
+                                  const content::WebContents* active_tab) {
+  loader_.Load(GURL(instant_url_), profile, active_tab,
+                base::Bind(&InstantOverlay::HandleStalePage,
+                           base::Unretained(this)));

[sreeram, 2013/02/01 20:36:53]

You need to split this into two methods:
  loader_.InitContents(...);
  SetContents(loader_.contents());
  contents()->SetUserData(&kUserDataKey, this);
  loader_.LoadURL(...);

The reason is that as it stands, it won't play nicely with Shishir's upcoming
change to do SearchBox initialization in RenderViewCreated. RenderViewCreated()
is called on the WebContentsObserver, and it happens *during* the call to
LoadURL(). But since you SetContents() only later below, that's too late; we'll
miss the RenderViewCreated call.

[samarth, 2013/02/04 20:43:32]

Done.

+  SetContents(loader_.contents());
+  contents()->SetUserData(&kUserDataKey, new InstantOverlayUserData(this));
+}
+
+scoped_ptr<content::WebContents> InstantOverlay::ReleaseContents() {
+  contents()->RemoveUserData(&kUserDataKey);
+  SetContents(NULL);
+  return loader_.ReleaseContents();
+}
+
+void InstantOverlay::DidNavigate(
+    const history::HistoryAddPageArgs& add_page_args) {
+  last_navigation_ = add_page_args;
+}
+
+bool InstantOverlay::IsUsingLocalPreview() const {
+  return instant_url_ == InstantController::kLocalOmniboxPopupURL;
+}
+
+void InstantOverlay::Update(const string16& text,
+                            size_t selection_start,
+                            size_t selection_end,
+                            bool verbatim) {
+  last_navigation_ = history::HistoryAddPageArgs();
+  InstantPage::Update(text, selection_start, selection_end, verbatim);
+}
+
+void InstantOverlay::OnSwappedContents() {
+  contents()->RemoveUserData(&kUserDataKey);
+  SetContents(loader_.contents());
+  contents()->SetUserData(&kUserDataKey, new InstantOverlayUserData(this));
+  instant_controller()->SwappedOverlayContents();
+}
+
+void InstantOverlay::OnFocus() {
+  // The preview is getting focus. Equivalent to it being clicked.
+  bool tmp = is_pointer_down_from_activate_;

[sreeram, 2013/02/01 20:36:53]

Use base::AutoReset<> here.

[samarth, 2013/02/04 20:43:32]

Done.

+  is_pointer_down_from_activate_ = true;
+  instant_controller()->FocusedOverlayContents();
+  is_pointer_down_from_activate_ = tmp;
+}
+
+void InstantOverlay::OnMouseDown() {
+  is_pointer_down_from_activate_ = true;
+}
+
+void InstantOverlay::OnMouseUp() {
+  if (is_pointer_down_from_activate_) {
+    is_pointer_down_from_activate_ = false;
+    instant_controller()->CommitIfPossible(INSTANT_COMMIT_FOCUS_LOST);
+  }
+}
+
+content::WebContents* InstantOverlay::OpenURLFromTab(
+    content::WebContents* source,
+    const content::OpenURLParams& params) {
+  content::WebContents* overlay = contents();

[sreeram, 2013/02/01 20:36:53]

Add a comment on top of this line to say why we have to cache the overlay
WebContents* (because CommitIfPossible() will ReleaseContents(), so after that,
contents() will be NULL).

[samarth, 2013/02/04 20:43:32]

Done.

+  if (instant_controller()->CommitIfPossible(INSTANT_COMMIT_NAVIGATED)) {
+    // Allow navigation to continue if we are able to commit the overlay. At
+    // this point, the overlay's delegate should be the tab strip, which will
+    // be able to handle the navigation.

[sreeram, 2013/02/01 20:36:53]

To be safe:
DCHECK_NE(this, overlay->GetDelegate());

[samarth, 2013/02/04 20:43:32]

Done.  (Actually compared it to loader_ which is the old delegate.)

+    return overlay->GetDelegate()->OpenURLFromTab(source, params);
+  }
+  return NULL;
+}
+
+void InstantOverlay::HandleStalePage() {
+  is_stale_ = true;
+  instant_controller()->ReloadOverlayIfStale();
+}