Pop up requestAutocomplete UI when autofill server hints chrome client that it is in a multipage au…

chrome/browser/autofill/autocheckout_manager.h

+// Copyright (c) 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.
+
+#ifndef CHROME_BROWSER_AUTOFILL_AUTOCHECKOUT_MANAGER_H_
+#define CHROME_BROWSER_AUTOFILL_AUTOCHECKOUT_MANAGER_H_
+
+#include <string>
+
+#include "base/memory/ref_counted.h"
+
+class GURL;
+class FormStructure;
+
+struct FormData;
+struct FormFieldData;

[Ilya Sherman, 2013/01/17 01:21:44]

nit: These should be interleaved with the above.

[Raman Kakilate, 2013/01/17 06:40:33]

autofill_manager.h also orders classes followed by structs. I couldn't find
anything in style guide.

+
+namespace content {
+class WebContents;
+struct SSLStatus;

[Ilya Sherman, 2013/01/17 01:21:44]

nit: alphabetize

[Raman Kakilate, 2013/01/17 06:40:33]

used class'es followed by struct's

+}
+
+class AutocheckoutManager : public base::RefCounted<AutocheckoutManager>{

[Ilya Sherman, 2013/01/17 01:21:44]

This class should almost certainly not be reference counted.

[Raman Kakilate, 2013/01/17 06:40:33]

I don't completely understand base:Bind, but from what I could get, it seemed
like it needed RefCounted. Compiler failed if it is not.

[Ilya Sherman, 2013/01/17 10:50:23]

For base::Bind, you want a WeakPtr, rather than reference counting.

[Raman Kakilate, 2013/01/17 16:52:30]

Isn't WeakPtr doing more complex operations than ref counting ? 

I remember seeing compiler complain about absence of AddRef() and Release() API
on AutocheckoutManager, which doesn't seem to be present in WeakPtr. I will give
it a try anyways.

[Raman Kakilate, 2013/01/17 17:35:30]

Done. I was wrong, Moved to WeakPtr based implementation.

+ public:
+  void ShowAutocheckoutDialog(const GURL& frame_url,
+                              const content::SSLStatus& ssl_status,
+                              content::WebContents* web_contents);

[Ilya Sherman, 2013/01/17 01:21:44]

The AutocheckoutManager should have a pointer back to the AutofillManager so
that it can query it for the WebContents.

[Raman Kakilate, 2013/01/17 06:40:33]

AutofillManager.web_contents() is a protected method. Making it public didn't
look like good option.

[Ilya Sherman, 2013/01/17 10:50:23]

IMO it would be better to make it public -- in fact, I thought it already was. 
There's really not much benefit that I can see to keeping it protected, and
instead proxying any requests that rely on it.

[Raman Kakilate, 2013/01/17 16:52:30]

ah sorry, I meant
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/public/browser/web...
defines web_contents() as protected, probably for a good reason. AutofillManager
extends WebContentsObserver.

I wanted to avoid breaking this encapsulation by exposing web_contents_ through
a public API.

+
+ private:
+  friend class base::RefCounted<AutocheckoutManager>;
+
+  // Build FormFieldData based on the supplied |autocomplete_attribute|. Will
+  // fill rest of properties with default values.
+  static FormFieldData BuildField(const std::string& autocomplete_attribute);
+
+  // Build Autocheckout specific form data to be consumed by
+  // AutofillDialogController to show the Autocheckout specific UI.
+  static FormData BuildAutocheckoutFormData();

[Ilya Sherman, 2013/01/17 01:21:44]

nit: Please tuck these into an anonymous namespace in the implementation file.

[Raman Kakilate, 2013/01/17 06:40:33]

Done.

+
+  // Callback called from AutofillDialogController on filling up the UI form.
+  void ReturnAutocheckoutData(const FormStructure* result);
+
+  ~AutocheckoutManager();
+};

[Ilya Sherman, 2013/01/17 01:21:44]

nit: DISALLOW_COPY_AND_ASSIGN

[Raman Kakilate, 2013/01/17 06:40:33]

Done.

+
+#endif  // CHROME_BROWSER_AUTOFILL_AUTOCHECKOUT_MANAGER_H_