Factor out RenderWidgetHostConnector

content/browser/android/render_process_connector.h

+// Copyright 2017 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 CONTENT_BROWSER_ANDROID_RENDER_PROCESS_CONNECTOR_H_
+#define CONTENT_BROWSER_ANDROID_RENDER_PROCESS_CONNECTOR_H_
+
+#include "base/memory/weak_ptr.h"
+#include "content/public/browser/web_contents_observer.h"
+
+namespace content {
+
+class RenderWidgetHostViewAndroid;
+
+// An base class used to connect an object of the content layer lifecycle
+// to |RenderWidgetHostViewAndroid|. The inherting class needs to do following:
+//
+// 1) Override |UpdateRenderProcessConnection| to set itself to the RWHVA
+//    brought up foreground, and null out its reference in the RWHVA going
+//    away so it won't  access the object any more.
+// 2) Call |UpdateRenderProcessConnection(rwhva(), nullptr)| in its destructor
+//    to clean up its reference from existing RWHVA. Note that this cannot be
+//    done in the base destructor since the method is pure virtual.
+class RenderProcessConnector : public WebContentsObserver {

[boliu, 2017/04/07 18:47:10]

this has nothing to do with RenderProcess though, since RWHVA lifetime is
totally different from the actual renderer process

RenderWidgetHostConnector?

Also I think composition over inheritance applies here for WebContentsObserver,
don't want to make all methods of Observer available to subclasses

[Jinsuk Kim, 2017/04/10 05:11:48]

- Renamed as suggested.
- Defined an inner class (Observer) to compose (as opposed to) inherit
WebContentsObserver.

+ public:
+  RenderProcessConnector(WebContents* web_contents);

[boliu, 2017/04/07 18:47:10]

explicit

[Jinsuk Kim, 2017/04/10 05:11:47]

Done.

+  ~RenderProcessConnector() override;
+
+  // WebContentsObserver implementation.
+  void RenderViewReady() override;
+  void RenderViewHostChanged(RenderViewHost* old_host,
+                             RenderViewHost* new_host) override;
+  void DidAttachInterstitialPage() override;
+  void DidDetachInterstitialPage() override;
+  void WebContentsDestroyed() override;
+
+  // Method to set itself to the |new_rwhva|, and null out the reference
+  // in |old_rwvha|. Example:
+  //
+  // if (old_rwhva)
+  //   old_rwhva->set_object(nullptr);
+  // if (new_rwhva)
+  //   new_rwhva->set_object(this);
+  virtual void UpdateRenderProcessConnection(
+      RenderWidgetHostViewAndroid* old_rwhva,
+      RenderWidgetHostViewAndroid* new_rhwva) = 0;
+
+  RenderWidgetHostViewAndroid* rwhva() const { return rwhva_.get(); }

[boliu, 2017/04/07 18:47:10]

Hmm, people will forget the null check in the future.

How about separate HasFooView and GetFooView, where Get DCHECKs it's not null?
Or just GetFooViewMaybeNull?

[boliu, 2017/04/07 18:48:40]

Actually, even better idea, this should not exist at all. It should be redundant
given UpdateRenderProcessConnection

[Jinsuk Kim, 2017/04/10 05:11:47]

Sorry but what should not exist? null checking? If so, I think null checking is
still necessary for the calls coming from ThreadedInputConnection while rwhva_
is set to null.

[Jinsuk Kim, 2017/04/10 05:11:47]

Acknowledged.

+
+ private:
+  RenderWidgetHostViewAndroid* GetRenderWidgetHostViewAndroid() const;
+  void UpdateRenderWidgetHostView(RenderWidgetHostViewAndroid* new_rwhva);
+
+  base::WeakPtr<RenderWidgetHostViewAndroid> rwhva_;

[boliu, 2017/04/07 18:47:10]

DISALLOW_

[Jinsuk Kim, 2017/04/10 05:11:47]

Done.

+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_ANDROID_RENDER_PROCESS_CONNECTOR_H_