[Autofill] Add UMA timing metrics for requestAutocomplete dialog.

[Autofill] Add UMA timing metrics for requestAutocomplete dialog.

Records the duration for which the requestAutocomplete dialog was open.

Also, move a handful of classes into the autofill namespace.

BUG=165570
TEST=unit_tests --gtest_filter=AutofillMetricsTest.RequestAutocompleteDialogClosed


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

[Albert Bodenhamer, 2013-01-31 16:31:06]

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/auto...
File chrome/browser/autofill/autofill_metrics.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/auto...
chrome/browser/autofill/autofill_metrics.cc:274:
UMA_HISTOGRAM_LONG_TIMES("RequestAutocomplete.UiDuration", duration);
What's the best way to split this out by invocation type?  autocheckout will run
through this flow as well, but we'll want to track that separately.

[Dan Beam, 2013-01-31 19:47:15]

some optional nits

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/auto...
File chrome/browser/autofill/autofill_metrics.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/auto...
chrome/browser/autofill/autofill_metrics.h:33: DIALOG_CANCELED       // The user
canceled out of the dialog.
super optional nit: trailing ,

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:497:
base::Time::Now() - dialog_shown_timestamp_;
opt nit: used once, inline, also

  DialogDismissalAction dismissal_action;

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:515:
dialog_duration, AutofillMetrics::DIALOG_ACCEPTED);
opt nit:

  dismissal_action = AutofillMetrics::DIALOG_ACCEPTED;

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:519:
dialog_duration, AutofillMetrics::DIALOG_CANCELED);
opt nit:

  dismissal_action = AutofillMetrics::DIALOG_CANCELED;

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:520: }
opt nit:

  metric_logger_.LogRequestAutocompleteUiDuration(
      base::Time::Now() - dialog_shown_timestamp_, dismissal_action);

[Ilya Sherman, 2013-02-01 09:55:17]

Still need to build on a Views platform to chase down the memory errors in the
new test.  Will do that tomorrow.

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/auto...
File chrome/browser/autofill/autofill_metrics.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/auto...
chrome/browser/autofill/autofill_metrics.cc:274:
UMA_HISTOGRAM_LONG_TIMES("RequestAutocomplete.UiDuration", duration);
On 2013/01/31 16:31:06, Albert Bodenhamer wrote:
> What's the best way to split this out by invocation type?  autocheckout will
run
> through this flow as well, but we'll want to track that separately.

Done.

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/auto...
File chrome/browser/autofill/autofill_metrics.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/auto...
chrome/browser/autofill/autofill_metrics.h:33: DIALOG_CANCELED       // The user
canceled out of the dialog.
On 2013/01/31 19:47:16, Dan Beam wrote:
> super optional nit: trailing ,

Left it out to be consistent with the rest of the file.

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:497:
base::Time::Now() - dialog_shown_timestamp_;
On 2013/01/31 19:47:16, Dan Beam wrote:
> opt nit: used once, inline, also
> 
>   DialogDismissalAction dismissal_action;

Done.

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:515:
dialog_duration, AutofillMetrics::DIALOG_ACCEPTED);
On 2013/01/31 19:47:16, Dan Beam wrote:
> opt nit:
> 
>   dismissal_action = AutofillMetrics::DIALOG_ACCEPTED;

Done.

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:519:
dialog_duration, AutofillMetrics::DIALOG_CANCELED);
On 2013/01/31 19:47:16, Dan Beam wrote:
> opt nit:
> 
>   dismissal_action = AutofillMetrics::DIALOG_CANCELED;

Done.

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:520: }
On 2013/01/31 19:47:16, Dan Beam wrote:
> opt nit:
> 
>   metric_logger_.LogRequestAutocompleteUiDuration(
>       base::Time::Now() - dialog_shown_timestamp_, dismissal_action);

Done.

https://chromiumcodereview.appspot.com/12091086/diff/1010/chrome/browser/auto...
File chrome/browser/autofill/autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/1010/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_delegate.h:41: };
These names are rather ickily long.  Should I drop the "autocomplete" prefix? 
Any suggestions for even better/shorter names?

[Dan Beam, 2013-02-01 17:17:28]

https://chromiumcodereview.appspot.com/12091086/diff/1010/chrome/browser/auto...
File chrome/browser/autofill/autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/1010/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_delegate.h:41: };
On 2013/02/01 09:55:17, Ilya Sherman wrote:
> These names are rather ickily long.  Should I drop the "autocomplete" prefix? 
> Any suggestions for even better/shorter names?

I think just REQUESTER_ is enough, that's what I did in my patch.

[Dan Beam, 2013-02-01 17:18:08]

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/auto...
File chrome/browser/autofill/autofill_metrics.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/4001/chrome/browser/auto...
chrome/browser/autofill/autofill_metrics.h:33: DIALOG_CANCELED       // The user
canceled out of the dialog.
On 2013/02/01 09:55:17, Ilya Sherman wrote:
> On 2013/01/31 19:47:16, Dan Beam wrote:
> > super optional nit: trailing ,
> 
> Left it out to be consistent with the rest of the file.

Why does the rest of the file hate , :P?

[Ilya Sherman, 2013-02-02 01:22:13]

https://chromiumcodereview.appspot.com/12091086/diff/1010/chrome/browser/auto...
File chrome/browser/autofill/autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/1010/chrome/browser/auto...
chrome/browser/autofill/autofill_manager_delegate.h:41: };
On 2013/02/01 17:17:28, Dan Beam wrote:
> On 2013/02/01 09:55:17, Ilya Sherman wrote:
> > These names are rather ickily long.  Should I drop the "autocomplete"
prefix? 
> > Any suggestions for even better/shorter names?
> 
> I think just REQUESTER_ is enough, that's what I did in my patch.

Compromised by going with DIALOG_REQUESTER_, which seems to look ok in the
contexts where it's used.

[benquan, 2013-02-02 02:02:31]

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
File chrome/browser/autofill/autocheckout_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
chrome/browser/autofill/autocheckout_infobar_delegate.h:21: namespace autofill {
It's better to move this file and autocheckout_manager.[h|cc] to autocheckout/
dir and put them under namespace autocheckout.

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
File chrome/browser/autofill/autofill_metrics.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
chrome/browser/autofill/autofill_metrics.cc:296: break;
Nit: handle default ?

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
chrome/browser/autofill/autofill_metrics.cc:307: break;
Ditto

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
chrome/browser/autofill/autofill_metrics.cc:310: LogUMAHistogramLongTimes(prefix
+ ".UiDuration", duration);
Do we need to log both?

[Ilya Sherman, 2013-02-02 02:09:12]

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
File chrome/browser/autofill/autocheckout_infobar_delegate.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
chrome/browser/autofill/autocheckout_infobar_delegate.h:21: namespace autofill {
On 2013/02/02 02:02:31, benquan wrote:
> It's better to move this file and autocheckout_manager.[h|cc] to autocheckout/
> dir and put them under namespace autocheckout.

That's sufficiently tangential to this CL that I'd rather not do it here.  Feel
free to move it in a separate CL.

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
File chrome/browser/autofill/autofill_metrics.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
chrome/browser/autofill/autofill_metrics.cc:296: break;
On 2013/02/02 02:02:31, benquan wrote:
> Nit: handle default ?

The default case is intentionally omitted, so that it is a compile-time error to
omit a case.

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
chrome/browser/autofill/autofill_metrics.cc:307: break;
On 2013/02/02 02:02:31, benquan wrote:
> Ditto

Ditto.

https://chromiumcodereview.appspot.com/12091086/diff/10001/chrome/browser/aut...
chrome/browser/autofill/autofill_metrics.cc:310: LogUMAHistogramLongTimes(prefix
+ ".UiDuration", duration);
On 2013/02/02 02:02:31, benquan wrote:
> Do we need to log both?

Per the bug, we want both, yes.  It's often convenient to have both an aggregate
view and a breakdown view.

[Ilya Sherman, 2013-02-05 07:16:37]

+Evan for review (or deferral of review)

Now with a working test.

[Evan Stade, 2013-02-05 20:06:41]

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/aut...
chrome/browser/autofill/autofill_manager_delegate.h:36: enum DialogRequester {
Not a fan of this name. DialogType?

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/aut...
File chrome/browser/autofill/autofill_metrics.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/aut...
chrome/browser/autofill/autofill_metrics.cc:313: 
^H

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:100: #if
defined(TOOLKIT_VIEWS)
do you have to ifdef out the whole test instead of just DISABLEing it?

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:144:
dialog_shown_timestamp_(base::Time::Now()),
why here and not in Show()?

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.h:32: virtual
content::BrowserContext* GetBrowserContext() const OVERRIDE;
// Foo implementation.

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:477: 
^H

[Ilya Sherman, 2013-02-06 00:02:24]

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/aut...
File chrome/browser/autofill/autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/aut...
chrome/browser/autofill/autofill_manager_delegate.h:36: enum DialogRequester {
On 2013/02/05 20:06:41, Evan Stade wrote:
> Not a fan of this name. DialogType?

Done.

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/aut...
File chrome/browser/autofill/autofill_metrics.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/aut...
chrome/browser/autofill/autofill_metrics.cc:313: 
On 2013/02/05 20:06:41, Evan Stade wrote:
> ^H

Done.

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:100: #if
defined(TOOLKIT_VIEWS)
On 2013/02/05 20:06:41, Evan Stade wrote:
> do you have to ifdef out the whole test instead of just DISABLEing it?

Done.

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:144:
dialog_shown_timestamp_(base::Time::Now()),
On 2013/02/05 20:06:41, Evan Stade wrote:
> why here and not in Show()?

Done.

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/autofill/tab_autofill_manager_delegate.h (right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
chrome/browser/ui/autofill/tab_autofill_manager_delegate.h:32: virtual
content::BrowserContext* GetBrowserContext() const OVERRIDE;
On 2013/02/05 20:06:41, Evan Stade wrote:
> // Foo implementation.

Done.

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/23001/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:477: 
On 2013/02/05 20:06:41, Evan Stade wrote:
> ^H

Done.

[Evan Stade, 2013-02-06 00:29:32]

https://chromiumcodereview.appspot.com/12091086/diff/24022/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/24022/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:467: void
AutofillDialogViews::SubmitForTesting() {
seems like this should be:

if (Accept())
  Hide();

[Ilya Sherman, 2013-02-06 00:41:34]

https://chromiumcodereview.appspot.com/12091086/diff/24022/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/24022/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:467: void
AutofillDialogViews::SubmitForTesting() {
On 2013/02/06 00:29:32, Evan Stade wrote:
> seems like this should be:
> 
> if (Accept())
>   Hide();

Accept() fails because the inputs fail to validate (because they're empty).  Do
you have a suggestion on how to implement the test so that it fills the inputs
first?  Add more ForTesting() methods?

[Evan Stade, 2013-02-06 00:46:22]

probably the easiest thing to do is override the validation function in
TestAutofillDialogController

[Ilya Sherman, 2013-02-06 01:45:28]

https://chromiumcodereview.appspot.com/12091086/diff/24022/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12091086/diff/24022/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:467: void
AutofillDialogViews::SubmitForTesting() {
On 2013/02/06 00:41:34, Ilya Sherman wrote:
> On 2013/02/06 00:29:32, Evan Stade wrote:
> > seems like this should be:
> > 
> > if (Accept())
> >   Hide();
> 
> Accept() fails because the inputs fail to validate (because they're empty). 
Do
> you have a suggestion on how to implement the test so that it fills the inputs
> first?  Add more ForTesting() methods?

Done.  Good call on override the AutofillDialogController method.  For some
reason when I looked at this before, I thought I'd need to override a view
method.

[Evan Stade, 2013-02-06 21:11:38]

lgtm

[Ilya Sherman, 2013-02-06 21:46:33]

Scott, could you sanity check the c/b/ui/views and gypi changes?

[sky, 2013-02-06 21:48:17]

LGTM

[Albert Bodenhamer, 2013-02-06 21:56:21]

LGTM


On Wed, Feb 6, 2013 at 1:48 PM, <sky@chromium.org> wrote:

> LGTM
>
>
https://chromiumcodereview.**appspot.com/12091086/<https://chromiumcodereview...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[commit-bot: I haz the power, 2013-02-06 22:11:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12091086/26025

[commit-bot: I haz the power, 2013-02-06 22:11:36]

Failed to apply patch for chrome/browser/autofill/autocheckout_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/autofill/autocheckout_manager.cc
  Hunk #1 succeeded at 68 with fuzz 1 (offset 1 line).
  Hunk #2 FAILED at 78.
  Hunk #3 FAILED at 124.
  Hunk #4 succeeded at 211 (offset 35 lines).
  2 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/autofill/autocheckout_manager.cc.rej

Patch:       chrome/browser/autofill/autocheckout_manager.cc
Index: chrome/browser/autofill/autocheckout_manager.cc
diff --git a/chrome/browser/autofill/autocheckout_manager.cc
b/chrome/browser/autofill/autocheckout_manager.cc
index
2a42bab1cc1af21af6fa1ecf600aa2191cb7c83d..122f499e2b01538e20bf9751da8b8c56b625c1af
100644
--- a/chrome/browser/autofill/autocheckout_manager.cc
+++ b/chrome/browser/autofill/autocheckout_manager.cc
@@ -67,6 +67,8 @@ FormData BuildAutocheckoutFormData() {
 
 }  // namespace
 
+namespace autofill {
+
 AutocheckoutManager::AutocheckoutManager(AutofillManager* autofill_manager)
     : autofill_manager_(autofill_manager),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
@@ -76,9 +78,9 @@ AutocheckoutManager::~AutocheckoutManager() {
 }
 
 void AutocheckoutManager::SetProceedElementDescriptor(
-    const autofill::WebElementDescriptor& proceed) {
+    const WebElementDescriptor& proceed) {
   // make a local copy.
-  proceed_descriptor_.reset(new autofill::WebElementDescriptor(proceed));
+  proceed_descriptor_.reset(new WebElementDescriptor(proceed));
 }
 
 void AutocheckoutManager::FillForms() {
@@ -122,7 +124,8 @@ void AutocheckoutManager::ShowAutocheckoutDialog(
       base::Bind(&AutocheckoutManager::ReturnAutocheckoutData,
                  weak_ptr_factory_.GetWeakPtr());
   autofill_manager_->ShowRequestAutocompleteDialog(
-      BuildAutocheckoutFormData(), frame_url, ssl_status, callback);
+      BuildAutocheckoutFormData(), frame_url, ssl_status,
+      DIALOG_TYPE_AUTOCHECKOUT, callback);
 }
 
 void AutocheckoutManager::ReturnAutocheckoutData(const FormStructure* result) {
@@ -174,3 +177,5 @@ void AutocheckoutManager::SetValue(const AutofillField&
field,
   else
     profile_->FillFormField(field, 0, field_to_fill);
 }
+
+}  // namespace autofill

[Dan Beam, 2013-02-07 00:48:05]

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:29: :
dialog_type_(static_cast<DialogType>(-1)),
^ what is this dark magic?

[Ilya Sherman, 2013-02-07 00:58:47]

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:29: :
dialog_type_(static_cast<DialogType>(-1)),
On 2013/02/07 00:48:05, Dan Beam wrote:
> ^ what is this dark magic?

Is there a white magic version of this that you'd prefer?

[Dan Beam, 2013-02-07 19:13:10]

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:29: :
dialog_type_(static_cast<DialogType>(-1)),
On 2013/02/07 00:58:48, Ilya Sherman wrote:
> On 2013/02/07 00:48:05, Dan Beam wrote:
> > ^ what is this dark magic?
> 
> Is there a white magic version of this that you'd prefer?

does an INVALID member in the enum work?

[Ilya Sherman, 2013-02-11 05:57:47]

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:29: :
dialog_type_(static_cast<DialogType>(-1)),
On 2013/02/07 19:13:11, Dan Beam wrote:
> On 2013/02/07 00:58:48, Ilya Sherman wrote:
> > On 2013/02/07 00:48:05, Dan Beam wrote:
> > > ^ what is this dark magic?
> > 
> > Is there a white magic version of this that you'd prefer?
> 
> does an INVALID member in the enum work?

Not really, because it would only be used by the test code, and I don't like the
idea of adding enum values that are only used by test code.

[Dan Beam, 2013-02-11 16:48:23]

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:29: :
dialog_type_(static_cast<DialogType>(-1)),
On 2013/02/11 05:57:48, Ilya Sherman wrote:
> On 2013/02/07 19:13:11, Dan Beam wrote:
> > On 2013/02/07 00:58:48, Ilya Sherman wrote:
> > > On 2013/02/07 00:48:05, Dan Beam wrote:
> > > > ^ what is this dark magic?
> > > 
> > > Is there a white magic version of this that you'd prefer?
> > 
> > does an INVALID member in the enum work?
> 
> Not really, because it would only be used by the test code, and I don't like
the
> idea of adding enum values that are only used by test code.

I don't think that's a big deal, there's lots of *ForTesting() code in Chrome,
also if you really want...

  enum Blah {
#if defined(UNIT_TEST)
    INVALID = -1,
#endif
    VALID_1 = 0,
    VALID_2,
  }

anyways, it was just a drive-by nit, not blocking.

[benquan, 2013-02-14 02:46:54]

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/12091086/diff/26025/chrome/browser/ui/...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:29: :
dialog_type_(static_cast<DialogType>(-1)),
On 2013/02/11 16:48:23, Dan Beam wrote:
> On 2013/02/11 05:57:48, Ilya Sherman wrote:
> > On 2013/02/07 19:13:11, Dan Beam wrote:
> > > On 2013/02/07 00:58:48, Ilya Sherman wrote:
> > > > On 2013/02/07 00:48:05, Dan Beam wrote:
> > > > > ^ what is this dark magic?
> > > > 
> > > > Is there a white magic version of this that you'd prefer?
> > > 
> > > does an INVALID member in the enum work?
> > 
> > Not really, because it would only be used by the test code, and I don't like
> the
> > idea of adding enum values that are only used by test code.
> 
> I don't think that's a big deal, there's lots of *ForTesting() code in Chrome,
> also if you really want...
> 
>   enum Blah {
> #if defined(UNIT_TEST)
>     INVALID = -1,
> #endif
This will break switch statement w/o default option when you build unit_tests.
 
>     VALID_1 = 0,
>     VALID_2,
>   }
> 
> anyways, it was just a drive-by nit, not blocking.

[commit-bot: I haz the power, 2013-02-14 06:00:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12091086/34001

[commit-bot: I haz the power, 2013-02-14 06:26:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12091086/43002

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

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

[commit-bot: I haz the power, 2013-02-14 09:17:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/12091086/43002

[commit-bot: I haz the power, 2013-02-14 20:32:34]

Change committed as 182528