Refactor Autofill for out of process iframes (OOPIF).

components/autofill/content/browser/content_autofill_driver_factory.cc

+// Copyright 2014 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 "components/autofill/content/browser/content_autofill_driver_factory.h"
+
+#include "components/autofill/content/browser/content_autofill_driver.h"
+#include "components/autofill/core/browser/autofill_client.h"
+#include "components/autofill/core/browser/autofill_manager.h"
+#include "components/autofill/core/browser/form_structure.h"
+#include "components/autofill/core/common/autofill_switches.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "ipc/ipc_message_macros.h"
+
+namespace autofill {
+
+namespace {
+
+const char kContentAutofillDriverFactoryWebContentsUserDataKey[] =
+    "web_contents_autofill_driver_factory";
+
+}  // namespace
+
+// static
+void ContentAutofillDriverFactory::CreateForWebContentsAndDelegate(
+    content::WebContents* contents,
+    AutofillClient* client,
+    const std::string& app_locale,
+    AutofillManager::AutofillDownloadManagerState enable_download_manager) {
+  if (FromWebContents(contents))
+    return;
+
+  contents->SetUserData(
+      kContentAutofillDriverFactoryWebContentsUserDataKey,
+      new ContentAutofillDriverFactory(contents, client, app_locale,
+                                       enable_download_manager));
+}
+
+// static
+ContentAutofillDriverFactory* ContentAutofillDriverFactory::FromWebContents(
+    content::WebContents* contents) {
+  return static_cast<ContentAutofillDriverFactory*>(contents->GetUserData(
+      kContentAutofillDriverFactoryWebContentsUserDataKey));
+}
+
+ContentAutofillDriverFactory::ContentAutofillDriverFactory(
+    content::WebContents* web_contents,
+    AutofillClient* client,
+    const std::string& app_locale,
+    AutofillManager::AutofillDownloadManagerState enable_download_manager)
+    : content::WebContentsObserver(web_contents),
+      client_(client),
+      app_locale_(app_locale),
+      enable_download_manager_(enable_download_manager) {
+  if (web_contents->GetMainFrame())
+    RenderFrameCreated(web_contents->GetMainFrame());

[vabr (Chromium), 2014/11/10 14:34:39]

It's usually a bad idea to call virtual methods from constructors, because it is
not clear which class' implementation this uses.
You can move the body of RenderFrameCreated in a non-virtual method, and just
make both the constructor and RenderFrameCreated call that non-virtual method.
Or you can maybe call ContentAutofillDriverFactory::RenderFrameCreated, although
I'm not sure if that has the same problem as calling just RenderFrameCreated or
not.

Whatever you do, please also do this for the other classes where you used this
pattern, notably in ContentPasswordManagerDriverFactory but also elsewhere.

[Evan Stade, 2014/11/14 23:25:49]

Done.

+}
+
+ContentAutofillDriverFactory::~ContentAutofillDriverFactory() {
+}
+
+ContentAutofillDriver* ContentAutofillDriverFactory::DriverForFrame(
+    content::RenderFrameHost* render_frame_host) {
+  return frame_driver_map_[render_frame_host];

[vabr (Chromium), 2014/11/10 14:34:39]

Note that this creates a new entry in the map, if there is none, and initializes
it with probably some garbage. You should probably check if the key is present,
and if it is not, return nullptr or assert.

[Evan Stade, 2014/11/14 23:25:49]

actually it initializes with null

[vabr (Chromium), 2014/11/17 15:08:48]

You are right, sorry fir spreading misinformation. A colleague of mine made me
aware of §8.5, sentence 10 of the C++ standard, implies that T() is the zeroed
value for any basic type T (and T() is what std::map ends up calling when
inserting values through calling operator[]).

+}
+
+bool ContentAutofillDriverFactory::OnMessageReceived(
+    const IPC::Message& message,
+    content::RenderFrameHost* render_frame_host) {
+  ContentAutofillDriver* driver = frame_driver_map_[render_frame_host];
+  DCHECK(driver);

[vabr (Chromium), 2014/11/10 14:34:39]

Again, in addition to checking driver, the retrieval from the map should
actually be checked for the existence of the key.

[Evan Stade, 2014/11/14 23:25:49]

see other comments

[vabr (Chromium), 2014/11/17 15:08:48]

Acknowledged. Using the NULL-dereference to signal issues (if the returned
driver pointer is null) sounds OK.

+  return driver->HandleMessage(message);
+}
+
+void ContentAutofillDriverFactory::RenderFrameCreated(
+    content::RenderFrameHost* render_frame_host) {
+  frame_driver_map_[render_frame_host] = new ContentAutofillDriver(
+      render_frame_host, client_, app_locale_, enable_download_manager_);
+}
+
+void ContentAutofillDriverFactory::RenderFrameDeleted(
+    content::RenderFrameHost* render_frame_host) {
+  ContentAutofillDriver* driver = frame_driver_map_[render_frame_host];
+  DCHECK(driver);

[vabr (Chromium), 2014/11/10 14:34:39]

DCHECKing driver does not do much here -- if driver is NULL, delete is a no-op.
Instead it should be checked that the key in the map exists:

auto driver_it = frame_driver_map_.find(render_frame_host);
DCHECK(driver_it != frame_driver_map_.end());
delete driver_it->second;
frame_driver_map_.erase(driver_it);

[Evan Stade, 2014/11/14 23:25:49]

see other comments

[vabr (Chromium), 2014/11/17 15:08:48]

Acknowledged.

+  delete driver;
+  frame_driver_map_.erase(render_frame_host);
+}
+
+void ContentAutofillDriverFactory::DidNavigateAnyFrame(
+    content::RenderFrameHost* render_frame_host,
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  ContentAutofillDriver* driver = frame_driver_map_[render_frame_host];

[vabr (Chromium), 2014/11/10 14:34:39]

Please check the existence of the key, to avoid inserting trash into the map.

[Evan Stade, 2014/11/14 23:25:49]

a) it's not inserting trash, it's inserting null
b) it shouldn't happen, so an early return would just paper over a bug
c) DCHECK is not appropriate (I've removed it) because the deref on the next
line implies that it shouldn't be null

[vabr (Chromium), 2014/11/17 15:08:49]

Acknowledged.

+  DCHECK(driver);
+  driver->DidNavigateFrame(details, params);
+}
+
+void ContentAutofillDriverFactory::NavigationEntryCommitted(
+    const content::LoadCommittedDetails& load_details) {
+  client_->HideAutofillPopup();
+}
+
+void ContentAutofillDriverFactory::WasHidden() {
+  client_->HideAutofillPopup();
+}
+
+}  // namespace autofill