Observe visibility of password inputs, for HTTP-bad phase 1

components/password_manager/content/browser/password_visibility_service_factory.h

Index: components/password_manager/content/browser/password_visibility_service_factory.h
diff --git a/components/password_manager/content/browser/password_visibility_service_factory.h b/components/password_manager/content/browser/password_visibility_service_factory.h
new file mode 100644
index 0000000000000000000000000000000000000000..b2308db0c1187164d11913bf396d56f2c71ff232
--- /dev/null
+++ b/components/password_manager/content/browser/password_visibility_service_factory.h
@@ -0,0 +1,59 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.

[vabr (Chromium), 2016/10/11 16:52:45]

Seeing the amount of code duplicated amongst this factory and
content_password_manager_driver_factory.* and
components/autofill/content/browser/content_autofill_driver_factory.*, I wish we
had a well-tested base class providing all the "retrieve service for a RFH"
logic.

Because this is a pre-existing problem (the two mentioned factories already
duplicate the code), I won't block this CL on it. But if you want, you can file
a bug about the code duplication and insert a TODO(crbug.com/XXX) pointing to it
here. If you do, feel free to Cc me and add any other candidates for sharing
this code you might know of. Thanks!

[estark, 2016/10/11 23:45:14]

I did file a bug for this (https://crbug.com/654902), but then I realized that I
was confused when I split this out into a separate service+factory in patch set
5. Instead of having this whole separate PasswordVisibilityService, we can just
a have a BindingSet member of ContentPasswordManagerDriver to bind to requests
for the new interface. This means I don't have to duplicate all this code. So
patch set 11 is much more like patch set 4 (which you previously reviewed) where
it handles these requests on ContentPasswordManagerDriver instead of introducing
a new service implementation + factory for it.

[vabr (Chromium), 2016/10/12 09:45:22]

Thanks, the current version looks simpler and therefore better to me!

Also, thanks for the bug. And to be clear -- I did not mean to ask you to fix
the duplication, just to note it in the bugtracker.

[estark, 2016/10/12 15:53:43]

Got it, thanks for clarifying (and for the code review!). I felt bad adding on
to the tech debt without trying to help, but now that this CL is simplified, I
feel less guilty and will probably mark it as Available. :)

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_PASSWORD_MANAGER_CONTENT_BROWSER_PASSWORD_VISIBILITY_SERVICE_FACTORY_H_
+#define COMPONENTS_PASSWORD_MANAGER_CONTENT_BROWSER_PASSWORD_VISIBILITY_SERVICE_FACTORY_H_
+
+#include <map>
+#include <memory>
+
+#include "base/macros.h"
+#include "base/supports_user_data.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "third_party/WebKit/public/platform/modules/sensitive_input_visibility/sensitive_input_visibility_service.mojom.h"
+
+namespace content {
+class RenderFrameHost;
+class WebContents;
+}
+
+namespace password_manager {
+
+class PasswordVisibilityService;
+
+// Creates and owns PasswordVisibilityServices. There is one
+// factory per WebContents, and one driver per render frame.
+class PasswordVisibilityServiceFactory : public content::WebContentsObserver,
+                                         public base::SupportsUserData::Data {
+ public:
+  static void CreateForWebContents(content::WebContents* web_contents);
+  ~PasswordVisibilityServiceFactory() override;
+
+  static PasswordVisibilityServiceFactory* FromWebContents(
+      content::WebContents* web_contents);
+
+  static void BindSensitiveInputVisibilityService(
+      content::RenderFrameHost* render_frame_host,
+      blink::mojom::SensitiveInputVisibilityServiceRequest request);
+
+  PasswordVisibilityService* GetServiceForFrame(
+      content::RenderFrameHost* render_frame_host);
+
+  // content::WebContentsObserver:
+  void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
+  void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
+
+ private:
+  PasswordVisibilityServiceFactory(content::WebContents* web_contents);

[vabr (Chromium), 2016/10/11 16:52:45]

Should this constructor be "explicit"?

[estark, 2016/10/11 23:45:14]

no longer applicable

+
+  std::map<content::RenderFrameHost*,
+           std::unique_ptr<PasswordVisibilityService>>
+      frame_service_map_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordVisibilityServiceFactory);
+};
+
+}  // namespace password_manager
+
+#endif  // COMPONENTS_PASSWORD_MANAGER_CONTENT_BROWSER_PASSWORD_VISIBILITY_SERVICE_FACTORY_H_