Move blocked content from TabContents to TabContentsWrapper.

chrome/browser/ui/blocked_content/blocked_content_tab_helper.h

+// Copyright (c) 2011 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_UI_BLOCKED_CONTENT_BLOCKED_CONTENT_TAB_HELPER_H_
+#define CHROME_BROWSER_UI_BLOCKED_CONTENT_BLOCKED_CONTENT_TAB_HELPER_H_
+#pragma once
+
+#include "chrome/browser/ui/find_bar/find_bar_controller.h"
+#include "chrome/browser/ui/find_bar/find_notification_details.h"
+#include "content/browser/tab_contents/tab_contents_observer.h"
+#include "content/common/notification_registrar.h"
+#include "webkit/glue/window_open_disposition.h"
+
+class BlockedContentContainer;
+class BlockedContentTabHelperDelegate;
+class TabContentsWrapper;
+
+// Per-tab class to manage blocked popups.
+class BlockedContentTabHelper : public TabContentsObserver {
+ public:
+  explicit BlockedContentTabHelper(TabContentsWrapper* tab_contents);
+  virtual ~BlockedContentTabHelper();
+
+  BlockedContentTabHelperDelegate* delegate() const { return delegate_; }
+  void set_delegate(BlockedContentTabHelperDelegate* d) { delegate_ = d; }
+
+  // Sets whether all TabContents added by way of |AddNewContents| should be
+  // blocked. Transitioning from all blocked to not all blocked results in
+  // reevaluating any blocked TabContents, which may result in unblocking some
+  // of the blocked TabContents.
+  void SetAllContentsBlocked(bool value);
+
+  bool all_contents_blocked() const { return all_contents_blocked_; }
+
+  // Adds the incoming |new_contents| to the |blocked_contents_| container.
+  void AddTabContents(TabContentsWrapper* new_contents,
+                      WindowOpenDisposition disposition,
+                      const gfx::Rect& initial_pos,
+                      bool user_gesture);
+
+  // Adds the incoming |new_contents| to the |blocked_contents_| container.
+  void AddPopup(TabContentsWrapper* new_contents,
+                const gfx::Rect& initial_pos,
+                bool user_gesture);
+
+  // Shows the blocked TabContents |tab_contents|.
+  void LaunchForContents(TabContentsWrapper* tab_contents);
+
+  // Returns the number of blocked contents.
+  size_t GetBlockedContentsCount() const;
+
+  // Returns the blocked TabContentsWrappers.  |blocked_contents| must
+  // be non-NULL.
+  void GetBlockedContents(
+      std::vector<TabContentsWrapper*>* blocked_contents) const;

[stevenjb, 2011/04/21 19:03:00]

This is confusing - it's the same function name as in
BlockedContentTabHelperDelegate, and it just calls
blocked_contents_->GetBlockedContents(). I think the code would be easier to
follow if we just had a public blocked_contents() accessor and eliminated the
above two functions.

[Avi (use Gerrit), 2011/04/21 20:06:02]

Nononono. It used to be that way. The problem was that then the blocked contents
container needed to have knowledge about the upper levels, and call back out to
its owner on some conditions, etc. It was a nightmare, complicated code
severely, and caused at least one security bug that I could find.

BlockedContentTabHelper completely wraps BlockedContentContainer. No one except
for the BCTH should be accessing the BCC.

[stevenjb, 2011/04/21 21:06:44]

Yeah, this makes more sense after doing the entire review than it did when I
wrote the comment. Maybe change the function comment here to something like
"Wrapper for blocked_contents_->GetBlockedContents()" to make it clear that the
implementation is in BlockedContentContainer?

+
+  // TabContentsObserver overrides:
+  virtual void DidNavigateMainFramePostCommit(
+      const NavigationController::LoadCommittedDetails& details,
+      const ViewHostMsg_FrameNavigate_Params& params) OVERRIDE;
+
+ private:
+  // Called when the blocked popup notification is shown or hidden.
+  void PopupNotificationVisibilityChanged(bool visible);
+
+  // Object that holds any blocked TabContents spawned from this TabContents.
+  scoped_ptr<BlockedContentContainer> blocked_contents_;
+
+  // Should we block all child TabContents this attempts to spawn.
+  bool all_contents_blocked_;
+
+  // Owning TabContentsWrapper.
+  TabContentsWrapper* tab_contents_wrapper_;
+
+  // Delegate for notifying our owner about stuff. Not owned by us.
+  BlockedContentTabHelperDelegate* delegate_;

[stevenjb, 2011/04/21 19:03:00]

This confused me for a while since delegate_ is unused by this class, and
blocked_contents_ is also a BlockedContentTabHelperDelegate. It looks like this
typically points to a Browser which is also a BlockedContentTabHelperDelegate,
so that other classes can access it? I don't know if calling this owner_ would
be any clearer, but maybe add something like (e.g. a Browser) after "owner" in
the comment?

[Avi (use Gerrit), 2011/04/21 20:06:02]

Sure. In general, the Browser is always the delegate for the TabContents, the
TCW, and all the TabHelpers.

+
+  DISALLOW_COPY_AND_ASSIGN(BlockedContentTabHelper);
+};
+
+#endif  // CHROME_BROWSER_UI_BLOCKED_CONTENT_BLOCKED_CONTENT_TAB_HELPER_H_