Extract AutofillDialogController interface and common utilities.

Extract AutofillDialogController interface and common utilities.

BUG=259089
TBR=battre@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216820

[aruslan, 2013-08-07 21:06:59]

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_common.h (right):

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_common.h:33: bool
IsCreditCardType(ServerFieldType type);
Description.

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:95: // Called when
the tab hosting this dialog is activated by a user gesture.
Remove descriptions, condense the overrides.
Add "// AutofillDialogController implementation." and reorder functions to match
the order in AutofillDialogController if necessary.

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/ta...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/ta...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:150: form,
Indent.

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/ta...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.h (right):

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/ta...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.h:94:
AutofillDialogController* GetDialogdControllerForTesting() {
Typo, wouldn't compile.

[aruslan, 2013-08-07 21:21:22]

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_common.h (right):

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_common.h:33: bool
IsCreditCardType(ServerFieldType type);
On 2013/08/07 21:07:00, aruslan wrote:
> Description.

Done.

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.h (right):

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.h:95: // Called when
the tab hosting this dialog is activated by a user gesture.
On 2013/08/07 21:07:00, aruslan wrote:
> Remove descriptions, condense the overrides.
> Add "// AutofillDialogController implementation." and reorder functions to
match
> the order in AutofillDialogController if necessary.

Done.

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/ta...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/ta...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:150: form,
On 2013/08/07 21:07:00, aruslan wrote:
> Indent.

Done.

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/ta...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.h (right):

https://codereview.chromium.org/22623002/diff/1/chrome/browser/ui/autofill/ta...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.h:94:
AutofillDialogController* GetDialogdControllerForTesting() {
On 2013/08/07 21:07:00, aruslan wrote:
> Typo, wouldn't compile.

Done.

[aruslan, 2013-08-07 22:53:49]

Evan -- PTAL, thanks!

This extracts AutofillDialogController from AutofillDialogControllerImpl,
removes the impl from Android builds entirely (together with #ifdefs) and
establishes autofill::common.

There is one unaddressed comment regarding "you should also move the static
functions from autofill_dialog_types to here" -- do you mean those three
DialogSectionToUiXXXEvent functions in autofill_dialog_types.h?

https://codereview.chromium.org/22623002/diff/22001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_common.h (right):

https://codereview.chromium.org/22623002/diff/22001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_common.h:21: namespace common {
estade@: I would call this common, not utils. These are mostly functions for
operating on the types in autofill_dialog_types.h so you should also move the
static functions from autofill_dialog_types to here.

[aruslan, 2013-08-08 22:51:51]

On 2013/08/07 22:53:49, aruslan wrote:
> Evan -- PTAL, thanks!
> 
> This extracts AutofillDialogController from AutofillDialogControllerImpl,
> removes the impl from Android builds entirely (together with #ifdefs) and
> establishes autofill::common.
> 
> There is one unaddressed comment regarding "you should also move the static
> functions from autofill_dialog_types to here" -- do you mean those three
> DialogSectionToUiXXXEvent functions in autofill_dialog_types.h?
> 
>
https://codereview.chromium.org/22623002/diff/22001/chrome/browser/ui/autofil...
> File chrome/browser/ui/autofill/autofill_dialog_common.h (right):
> 
>
https://codereview.chromium.org/22623002/diff/22001/chrome/browser/ui/autofil...
> chrome/browser/ui/autofill/autofill_dialog_common.h:21: namespace common {
> estade@: I would call this common, not utils. These are mostly functions for
> operating on the types in autofill_dialog_types.h so you should also move the
> static functions from autofill_dialog_types to here.

Ping.

[Evan Stade, 2013-08-08 23:03:44]

> do you mean those three
DialogSectionToUiXXXEvent functions in autofill_dialog_types.h?

yes

https://codereview.chromium.org/22623002/diff/33002/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/22623002/diff/33002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:291:
autofill::AutofillDialogController::RegisterProfilePrefs(registry);
you can follow the precedent below and move this into a #if block

https://codereview.chromium.org/22623002/diff/33002/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller.cc (right):

https://codereview.chromium.org/22623002/diff/33002/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller.cc:27:
user_prefs::PrefRegistrySyncable* registry) {
nit: {} on same line

[aruslan, 2013-08-09 00:01:30]

Thanks, Evan; PTAL.

https://codereview.chromium.org/22623002/diff/22001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_common.h (right):

https://codereview.chromium.org/22623002/diff/22001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_common.h:21: namespace common {
On 2013/08/07 22:53:49, aruslan wrote:
> estade@: I would call this common, not utils. These are mostly functions for
> operating on the types in autofill_dialog_types.h so you should also move the
> static functions from autofill_dialog_types to here.

Done.

https://codereview.chromium.org/22623002/diff/33002/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/22623002/diff/33002/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:291:
autofill::AutofillDialogController::RegisterProfilePrefs(registry);
On 2013/08/08 23:03:44, Evan Stade wrote:
> you can follow the precedent below and move this into a #if block

Done.

https://codereview.chromium.org/22623002/diff/33002/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller.cc (right):

https://codereview.chromium.org/22623002/diff/33002/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller.cc:27:
user_prefs::PrefRegistrySyncable* registry) {
On 2013/08/08 23:03:44, Evan Stade wrote:
> nit: {} on same line

Done.

[Evan Stade, 2013-08-09 16:14:51]

lgtm

[aruslan, 2013-08-09 16:57:34]

Thanks, Evan!
TBR battre@ for the trivial chrome/browser/prefs change.

[commit-bot: I haz the power, 2013-08-09 16:57:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/22623002/52001

[commit-bot: I haz the power, 2013-08-09 17:34:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-09 17:42:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/22623002/83001

[commit-bot: I haz the power, 2013-08-09 18:38:03]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-09 21:07:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/22623002/105001

[commit-bot: I haz the power, 2013-08-09 22:05:14]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-08-09 23:49:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/22623002/122001

[commit-bot: I haz the power, 2013-08-10 07:28:53]

Change committed as 216820

[battre, 2013-08-10 11:48:43]

chrome/browser/prefs/browser_prefs.cc LGTM