rAc: Get rid of dialog type in rAc, there is only one type left now.

rAc: Get rid of dialog type in rAc, there is only one type left now.

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

[Raman Kakilate, 2013-09-03 20:24:47]

Evan/Ilya - PTAL.

[Ilya Sherman, 2013-09-03 20:57:05]

Thanks!

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/android/aut...
File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc
(left):

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/android/aut...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:402: //
This might delete us.
Please preserve this comment.

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:204: void
InitializeControllerOfType() {
nit: Rename to InitializeController()

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

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/ta...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:171:
content::PageTransitionIsRedirect(details.entry->GetTransitionType());
nit: Are there any #included headers that are no longer needed with this change?

https://codereview.chromium.org/23537014/diff/1/components/autofill/content/b...
File components/autofill/content/browser/risk/fingerprint.cc (right):

https://codereview.chromium.org/23537014/diff/1/components/autofill/content/b...
components/autofill/content/browser/risk/fingerprint.cc:482: callback);
nit: re-wrap.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:322:
LogUMAHistogramEnumeration(name, state, NUM_DIALOG_DISMISSAL_STATES);
nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:329: name, user_type,
NUM_DIALOG_INITIAL_USER_STATE_METRICS);
nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:335:
LogUMAHistogramTimes(name, duration);
nit: Please update this to use the regular UMA_HISTOGRAM_TIMES macro.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:340:
LogUMAHistogramEnumeration(name, event, NUM_DIALOG_POPUP_EVENTS);
nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:346:
LogUMAHistogramEnumeration(name, metric, NUM_DIALOG_SECURITY_METRICS);
nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:370:
LogUMAHistogramEnumeration(name, event, NUM_DIALOG_UI_EVENTS);
nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:375:
LogUMAHistogramEnumeration(name, metric, NUM_WALLET_ERROR_METRICS);
nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:389: name, required_action,
NUM_WALLET_REQUIRED_ACTIONS);
nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.h:369: // This should be
called when the Autofill dialog, invoked by a dialog,
nit: Omit the phrase "invoked by a dialog" as well.

[Evan Stade, 2013-09-03 20:57:35]

LGTM, although you'll need some additional OWNERS to review it.

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:204: void
InitializeControllerOfType() {
this fn name no longer makes sense. Also, you should just call it from the
constructor instead of on the first line of each test.

[joth, 2013-09-03 21:00:43]

aw/ lgtm rubberstamp


On 3 September 2013 13:57, <estade@chromium.org> wrote:

> LGTM, although you'll need some additional OWNERS to review it.
>
>
>
> https://codereview.chromium.**org/23537014/diff/1/chrome/**
>
browser/ui/autofill/autofill_**dialog_controller_browsertest.**cc<https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc>
> File
> chrome/browser/ui/autofill/**autofill_dialog_controller_**browsertest.cc
> (right):
>
> https://codereview.chromium.**org/23537014/diff/1/chrome/**
> browser/ui/autofill/autofill_**dialog_controller_browsertest.**
>
cc#newcode204<https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc#newcode204>
> chrome/browser/ui/autofill/**autofill_dialog_controller_**
> browsertest.cc:204:
> void InitializeControllerOfType() {
> this fn name no longer makes sense. Also, you should just call it from
> the constructor instead of on the first line of each test.
>
>
https://codereview.chromium.**org/23537014/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Raman Kakilate, 2013-09-03 21:50:30]

+ ruslan for chrome/browser/ui/android/autofill/

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/android/aut...
File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc
(left):

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/android/aut...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:402: //
This might delete us.
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> Please preserve this comment.

Done.

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:204: void
InitializeControllerOfType() {
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Rename to InitializeController()

Done.

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:204: void
InitializeControllerOfType() {
On 2013/09/03 20:57:36, Evan Stade wrote:
> this fn name no longer makes sense. Also, you should just call it from the
> constructor instead of on the first line of each test.

Done.

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

https://codereview.chromium.org/23537014/diff/1/chrome/browser/ui/autofill/ta...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.cc:171:
content::PageTransitionIsRedirect(details.entry->GetTransitionType());
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Are there any #included headers that are no longer needed with this
change?

Done.

https://codereview.chromium.org/23537014/diff/1/components/autofill/content/b...
File components/autofill/content/browser/risk/fingerprint.cc (right):

https://codereview.chromium.org/23537014/diff/1/components/autofill/content/b...
components/autofill/content/browser/risk/fingerprint.cc:482: callback);
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: re-wrap.

Done.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:322:
LogUMAHistogramEnumeration(name, state, NUM_DIALOG_DISMISSAL_STATES);
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

Done.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:329: name, user_type,
NUM_DIALOG_INITIAL_USER_STATE_METRICS);
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

Done.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:335:
LogUMAHistogramTimes(name, duration);
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Please update this to use the regular UMA_HISTOGRAM_TIMES macro.

Done.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:340:
LogUMAHistogramEnumeration(name, event, NUM_DIALOG_POPUP_EVENTS);
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

Done.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:346:
LogUMAHistogramEnumeration(name, metric, NUM_DIALOG_SECURITY_METRICS);
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

Done.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:370:
LogUMAHistogramEnumeration(name, event, NUM_DIALOG_UI_EVENTS);
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

Done.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:375:
LogUMAHistogramEnumeration(name, metric, NUM_WALLET_ERROR_METRICS);
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

Done.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.cc:389: name, required_action,
NUM_WALLET_REQUIRED_ACTIONS);
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Please update this to use the regular UMA_HISTOGRAM_ENUMERATION macro.

Done.

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/23537014/diff/1/components/autofill/core/brow...
components/autofill/core/browser/autofill_metrics.h:369: // This should be
called when the Autofill dialog, invoked by a dialog,
On 2013/09/03 20:57:05, Ilya Sherman wrote:
> nit: Omit the phrase "invoked by a dialog" as well.

Done.

[aruslan, 2013-09-03 21:52:32]

android/ lgtm.

[Ilya Sherman, 2013-09-03 22:09:06]

LGTM, thanks.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/23537014/8001

[commit-bot: I haz the power, 2013-09-03 22:35:17]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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-09-04 18:36:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/23537014/40001

[commit-bot: I haz the power, 2013-09-04 21:14:55]

Change committed as 221271