rAc: split TestableAutofillDialogView implementation into its own class

rAc: split TestableAutofillDialogView implementation into its own class

This:
- makes use of a funky downcast
- moves TestableAutofillDialogViews code out of the production binary
- makes it impossible to accidentally call testing code from production code

mac implementation by groby@chromium.org

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

[Evan Stade, 2014-01-16 03:50:25]

still need to update Mac, but WDYT?

[groby-ooo-7-16, 2014-01-16 04:15:38]

It slightly scares me :)

(If anybody ever is crazy enough to add member vars to the TestableView, life
will get interesting :)

But I don't have an off-hand alternative that isn't either lots of code or too
clever for its own good... So yeah, let's just flip the Mac side to this and be
done with it.

(We might need to move the cocoa-part into a category. If you don't know how to
do that, ping me tomorrow)

[Evan Stade, 2014-01-16 19:50:01]

turns out that downcasting approach didn't actually work, I think because
casting changes the pointer to account for vtable differences.

So here's a more standard approach, just create a wrapper class.

[Evan Stade, 2014-01-23 20:05:08]

review over to dbeam

[Dan Beam, 2014-01-24 23:37:40]

you have a lot of work to do on mac

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:852:
GetViewTester()->GetSize();
unwrap everything that's now short enough to fit in 1 line

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:889:
static_cast<TestAutofillDialogController*>(controller)->view());
i don't think this is a valid cast (I don't think this controller is a test
version)

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_view_tester.h (right):

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_view_tester.h:26: static
scoped_ptr<AutofillDialogViewTester> Wrap(AutofillDialogView* view);
nit: s/Wrap/For/ like WebContentsTester [1]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_view_tester.h:26: static
scoped_ptr<AutofillDialogViewTester> Wrap(AutofillDialogView* view);
what's the advantage to changing the ownership model (from weak ptr ->
scoped_ptr)?

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
File chrome/browser/ui/autofill/testable_autofill_dialog_view.h (left):

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/autofill/testable_autofill_dialog_view.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
deleting this interface will break mac

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
File chrome/browser/ui/views/autofill/autofill_dialog_view_tester_views.h
(right):

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/views/autofill/autofill_dialog_view_tester_views.h:7: 
nit: #include "base/basictypes.h"

[groby-ooo-7-16, 2014-01-27 20:09:30]

Not sure why the Mac should be a lot of work?

Here's the steps as I see them

* Move AutofillDialogWindowController's TestableAutofillDialogView category into
a separate .h/.mm, and only link that in for test builds.

* For the TestableView in C++, don't go through the C++ object. Instead,
retrieve the window controller (sheet_delegate_), and just happily invoke the
category methods on the controller. I.e. CancelForTesting just changes from 
    [sheet_delegate_ cancel:nil];

to
    [wrapped_view_->sheet_delegate_ cancel:nil];

Evan: If that didn't make sense, happy to do that in a patch later today

[Evan Stade, 2014-01-27 21:08:34]

> Evan: If that didn't make sense, happy to do that in a patch later today

I would be grateful for your help.

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:852:
GetViewTester()->GetSize();
On 2014/01/24 23:37:40, Dan Beam wrote:
> unwrap everything that's now short enough to fit in 1 line

Done.

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:889:
static_cast<TestAutofillDialogController*>(controller)->view());
On 2014/01/24 23:37:40, Dan Beam wrote:
> i don't think this is a valid cast (I don't think this controller is a test
> version)

yet the code compiles and runs successfully

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
File chrome/browser/ui/autofill/autofill_dialog_view_tester.h (right):

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_view_tester.h:26: static
scoped_ptr<AutofillDialogViewTester> Wrap(AutofillDialogView* view);
On 2014/01/24 23:37:40, Dan Beam wrote:
> nit: s/Wrap/For/ like WebContentsTester [1]
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

Done.

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/autofill/autofill_dialog_view_tester.h:26: static
scoped_ptr<AutofillDialogViewTester> Wrap(AutofillDialogView* view);
On 2014/01/24 23:37:40, Dan Beam wrote:
> what's the advantage to changing the ownership model (from weak ptr ->
> scoped_ptr)?

AutofillDialogView is still a weak pointer. TestableAutofillDialogView, which is
now a separate class, didn't really have its own ownership model before.

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
File chrome/browser/ui/autofill/testable_autofill_dialog_view.h (left):

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/autofill/testable_autofill_dialog_view.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/01/24 23:37:40, Dan Beam wrote:
> deleting this interface will break mac

https://chromiumcodereview.appspot.com/135933003/#msg1

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
File chrome/browser/ui/views/autofill/autofill_dialog_view_tester_views.h
(right):

https://chromiumcodereview.appspot.com/135933003/diff/290001/chrome/browser/u...
chrome/browser/ui/views/autofill/autofill_dialog_view_tester_views.h:7: 
On 2014/01/24 23:37:40, Dan Beam wrote:
> nit: #include "base/basictypes.h"

Done.

[Dan Beam, 2014-01-28 18:04:47]

lgtm

https://chromiumcodereview.appspot.com/135933003/diff/590001/chrome/browser/u...
File chrome/browser/ui/views/autofill/autofill_dialog_view_tester_views.cc
(right):

https://chromiumcodereview.appspot.com/135933003/diff/590001/chrome/browser/u...
chrome/browser/ui/views/autofill/autofill_dialog_view_tester_views.cc:21: return
scoped_ptr<AutofillDialogViewTester>(new
nit: a dangling new is odd looking to me

https://chromiumcodereview.appspot.com/135933003/diff/590001/chrome/browser/u...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/135933003/diff/590001/chrome/browser/u...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2221:
DCHECK_NE(!!combobox, !!decorated);
nit: DCHECK_NE(!combobox, !decorated) does the same thing, i guess you felt !!
was less odd?

[Evan Stade, 2014-01-28 18:45:25]

https://chromiumcodereview.appspot.com/135933003/diff/590001/chrome/browser/u...
File chrome/browser/ui/views/autofill/autofill_dialog_view_tester_views.cc
(right):

https://chromiumcodereview.appspot.com/135933003/diff/590001/chrome/browser/u...
chrome/browser/ui/views/autofill/autofill_dialog_view_tester_views.cc:21: return
scoped_ptr<AutofillDialogViewTester>(new
On 2014/01/28 18:04:48, Dan Beam wrote:
> nit: a dangling new is odd looking to me

better than an extra line imo

https://chromiumcodereview.appspot.com/135933003/diff/590001/chrome/browser/u...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/135933003/diff/590001/chrome/browser/u...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2221:
DCHECK_NE(!!combobox, !!decorated);
On 2014/01/28 18:04:48, Dan Beam wrote:
> nit: DCHECK_NE(!combobox, !decorated) does the same thing, i guess you felt !!
> was less odd?

easier to read as english imho

[groby-ooo-7-16, 2014-01-28 20:54:13]

There ya go, with added OSX goodness.

https://codereview.chromium.org/135933003/diff/590001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/testable_autofill_dialog_view.h (left):

https://codereview.chromium.org/135933003/diff/590001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/testable_autofill_dialog_view.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
I might be crazy, but shouldn't this file be marked 'D'eleted instead of
'M'odified?

https://codereview.chromium.org/135933003/diff/590001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/135933003/diff/590001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2221:
DCHECK_NE(!!combobox, !!decorated);
On 2014/01/28 18:45:26, Evan Stade wrote:
> On 2014/01/28 18:04:48, Dan Beam wrote:
> > nit: DCHECK_NE(!combobox, !decorated) does the same thing, i guess you felt
!!
> > was less odd?
> 
> easier to read as english imho

If you like English, DCHECK(combobox || decorated) works well :)

[Evan Stade, 2014-01-28 22:05:55]

thanks Rachel!

https://codereview.chromium.org/135933003/diff/590001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/testable_autofill_dialog_view.h (left):

https://codereview.chromium.org/135933003/diff/590001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/testable_autofill_dialog_view.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/01/28 20:54:14, groby wrote:
> I might be crazy, but shouldn't this file be marked 'D'eleted instead of
> 'M'odified?

git cl is mysterious

https://codereview.chromium.org/135933003/diff/590001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/135933003/diff/590001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2221:
DCHECK_NE(!!combobox, !!decorated);
On 2014/01/28 20:54:14, groby wrote:
> On 2014/01/28 18:45:26, Evan Stade wrote:
> > On 2014/01/28 18:04:48, Dan Beam wrote:
> > > nit: DCHECK_NE(!combobox, !decorated) does the same thing, i guess you
felt
> !!
> > > was less odd?
> > 
> > easier to read as english imho
> 
> If you like English, DCHECK(combobox || decorated) works well :)

the intended operation is XOR

[Evan Stade, 2014-01-28 22:09:51]

cocoa lgtm, I can fix these nits, but one of them I'm not sure of.

https://codereview.chromium.org/135933003/diff/630001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm
(right):

https://codereview.chromium.org/135933003/diff/630001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm:111:
ofSuggestionForSection:section];
is this the correct indent?

https://codereview.chromium.org/135933003/diff/630001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm:126: 
^H

https://codereview.chromium.org/135933003/diff/630001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm:136: }
// namespace autofill

https://codereview.chromium.org/135933003/diff/630001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm:137: 
^H

[Dan Beam, 2014-01-28 22:10:35]

lgtm, pretty similar to the views work...

https://chromiumcodereview.appspot.com/135933003/diff/630001/chrome/browser/u...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm
(right):

https://chromiumcodereview.appspot.com/135933003/diff/630001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm:104:
forType:type];
nit:

[controller() setTextContents:base::SysUTF16ToNSString(contents)
                      forType:type];

https://chromiumcodereview.appspot.com/135933003/diff/630001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm:111:
ofSuggestionForSection:section];
[controller() setTextContents:base::SysUTF16ToNSString(text)
       ofSuggestionForSection:section];

https://chromiumcodereview.appspot.com/135933003/diff/630001/chrome/browser/u...
chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm:136: }
nit: }  // namespace autofill

[Evan Stade, 2014-01-29 02:32:29]

updated, lgty Rachel?

[groby-ooo-7-16, 2014-01-29 22:10:21]

LGTM indeed

https://codereview.chromium.org/135933003/diff/630001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm
(right):

https://codereview.chromium.org/135933003/diff/630001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/autofill/autofill_dialog_view_tester_cocoa.mm:111:
ofSuggestionForSection:section];
On 2014/01/28 22:10:35, Dan Beam wrote:
> [controller() setTextContents:base::SysUTF16ToNSString(text)
>        ofSuggestionForSection:section];

What Dan said. Victim of :s// :)

[commit-bot: I haz the power, 2014-01-29 22:29:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/135933003/650001

[commit-bot: I haz the power, 2014-01-30 01:59:44]

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

[commit-bot: I haz the power, 2014-01-30 02:25:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/135933003/650001

[commit-bot: I haz the power, 2014-01-30 07:58:13]

Change committed as 247886