Boldly refuse to show rAc dialog if cc info is not requested.

Boldly refuse to show rAc dialog if cc info is not requested.

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

[Evan Stade, 2014-04-10 00:38:21]

depends on https://codereview.chromium.org/232263002/

[Dan Beam, 2014-04-10 17:54:43]

https://codereview.chromium.org/232353003/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/232353003/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:719: }
these tests are great, but i meant checking that "unsupported" was returned as
the result/reason.  can we check that end-to-end in the browser tests?

[Evan Stade, 2014-04-10 19:50:56]

https://codereview.chromium.org/232353003/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/232353003/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:719: }
On 2014/04/10 17:54:43, Dan Beam wrote:
> these tests are great, but i meant checking that "unsupported" was returned as
> the result/reason.  can we check that end-to-end in the browser tests?

We don't need end-to-end tests to check that, we could check it in this test,
however I'm not sure I see the value in adding that degree of specificity.

[Dan Beam, 2014-04-15 02:16:41]

https://codereview.chromium.org/232353003/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/232353003/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:719: }
On 2014/04/10 19:50:57, Evan Stade wrote:
> On 2014/04/10 17:54:43, Dan Beam wrote:
> > these tests are great, but i meant checking that "unsupported" was returned
as
> > the result/reason.  can we check that end-to-end in the browser tests?
> 
> We don't need end-to-end tests to check that, we could check it in this test,
> however I'm not sure I see the value in adding that degree of specificity.

taking an extra hour (if that) to verify a part of the public HTML API actually
works is worth it, imo.  if you still disagree, i'm sure others can review or i
can write this test for you.

[Evan Stade, 2014-04-15 02:33:16]

https://codereview.chromium.org/232353003/diff/40001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc (right):

https://codereview.chromium.org/232353003/diff/40001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc:719: }
On 2014/04/15 02:16:42, Dan Beam wrote:
> On 2014/04/10 19:50:57, Evan Stade wrote:
> > On 2014/04/10 17:54:43, Dan Beam wrote:
> > > these tests are great, but i meant checking that "unsupported" was
returned
> as
> > > the result/reason.  can we check that end-to-end in the browser tests?
> > 
> > We don't need end-to-end tests to check that, we could check it in this
test,
> > however I'm not sure I see the value in adding that degree of specificity.
> 
> taking an extra hour (if that) to verify a part of the public HTML API
actually
> works is worth it, imo.  if you still disagree, i'm sure others can review or
i
> can write this test for you.

My preference for unit tests and regression-driven test development hasn't
changed over the weekend, so let me know which of the two options you gave is
preferable to you.

[Evan Stade, 2014-04-15 02:50:50]

-dbeam, +groby

[Dan Beam, 2014-04-15 02:51:03]

not lgtm i don't see why we're adding code until we think it'll be used.  you
mentioned that we're not sure "unsupported" will be added, so I'd wait until
that's decided first before wasting review/implementation cycles.

[Evan Stade, 2014-04-15 02:53:22]

Reviewing this is not a waste, although it's true that one line may later change
from [...]Unsupported[...] to [...]Disabled[...]. Luckily, I've written the test
in such a way that it succeeds either way, because all we really care about is
that the dialog fails to show.

Rachel, please review.

[groby-ooo-7-16, 2014-04-16 15:52:22]

Functionally LGTM - I'll let you and Dan sort out the unsupported issue

[Evan Stade, 2014-04-16 21:30:18]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-04-16 21:31:06]

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

[commit-bot: I haz the power, 2014-04-16 22:28:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-16 22:28:56]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Evan Stade, 2014-04-17 17:00:09]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-04-17 17:01:00]

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

[commit-bot: I haz the power, 2014-04-17 20:13:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-17 20:13:21]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[Evan Stade, 2014-04-21 21:46:32]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-04-21 21:46:38]

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

[commit-bot: I haz the power, 2014-04-22 04:50:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-22 04:50:42]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[Evan Stade, 2014-04-22 17:13:21]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-04-22 17:14:44]

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

[commit-bot: I haz the power, 2014-04-22 21:33:46]

Change committed as 265366

[Dan Beam, 2014-04-22 21:37:06]

On 2014/04/16 15:52:22, groby wrote:
> Functionally LGTM - I'll let you and Dan sort out the unsupported issue

when did this happen?

[Evan Stade, 2014-04-22 21:47:22]

On 2014/04/22 21:37:06, Dan Beam wrote:
> On 2014/04/16 15:52:22, groby wrote:
> > Functionally LGTM - I'll let you and Dan sort out the unsupported issue
> 
> when did this happen?

My understanding from our discussion offline was that you were bowing out of
this review.