[Mac] CREDENTIAL: Fix chooser dialog after r380352

[Mac] CREDENTIAL: Fix chooser dialog after r380352

https://codereview.chromium.org/1781433002 enabled auto-resize for
constrained web dialogs on Mac. It updated a number of callsites for
`new ConstrainedWindowMac`, but didn't update the callsite in
`password_prompt_view_bridge.mm`.

Tests continued to pass, as the chooser _was_ being popped up, it was
simply constrained to a 0x0 frame. According to
https://codereview.chromium.org/1781433002/#msg15, it's not clear that
we can add a test that would verify that this doesn't regress. That
seems unfortunate. :(

BUG=217034
,
R=vabr@chromium.org, apacible@chromium.org, thakis@chromium.org
TBR=vasilii@chromium.org

Committed: https://crrev.com/0f924bea7baf260f8729e608e0ec1baac418f021
Cr-Commit-Position: refs/heads/master@{#381740}

[Mike West, 2016-03-17 11:05:48]

vabr@: WDYT?

apacible@, thakis@: Is there any test we could add that would prevent changes
from breaking us in the future? I didn't see any in y'all's patch, and the
comments didn't seem positive, but... :) I know less than nothing about cocoa,
and our Mac expert is out on vacation, so I'd love some help putting something
together.

[vabr (Chromium), 2016-03-17 13:07:03]

This LGTM, thanks for fixing the issue so quickly!

Vaclav

[vabr (Chromium), 2016-03-17 13:50:27]

On 2016/03/17 13:07:03, vabr (Chromium) wrote:
> This LGTM, thanks for fixing the issue so quickly!
> 
> Vaclav

And I did run the ToT, experienced the issue of not seeing an account chooser on
https://w3c.github.io/webappsec/demos/credential-management/ after saving some
credentials, and it looks fixed by applying this patch. I did not see any errors
introduced by this patch.

[apacible, 2016-03-17 17:29:16]

LGTM

Thanks for the fix! Sorry for the inconvenience. This will need to be merged
into M50.

It makes sense to check to make sure the dialog has actually been shown, not
just created. If there's a fixed content size that's expected, you could
possibly check the dialog and webcontents size in an integration/browser test.

For the print preview issue specifically, I thought about it some more and from
doing some browser vs. dialog vs. contents size computing, we can probably avoid
regressions like this again.

Thanks again!

[Mike West, 2016-03-17 17:38:38]

Description was changed from

==========
[Mac] CREDENTIAL: Fix chooser dialog after r380352

https://codereview.chromium.org/1781433002 enabled auto-resize for
constrained web dialogs on Mac. It updated a number of callsites for
`new ConstrainedWindowMac`, but didn't update the callsite in
`password_prompt_view_bridge.mm`.

Tests continued to pass, as the chooser _was_ being popped up, it was
simply constrained to a 0x0 frame. According to
https://codereview.chromium.org/1781433002/#msg15, it's not clear that
we can add a test that would verify that this doesn't regress. That
seems unfortunate. :(

BUG=217034,
R=vabr@chromium.org, apacible@chromium.org, thakis@chromium.org
==========

to

==========
[Mac] CREDENTIAL: Fix chooser dialog after r380352

https://codereview.chromium.org/1781433002 enabled auto-resize for
constrained web dialogs on Mac. It updated a number of callsites for
`new ConstrainedWindowMac`, but didn't update the callsite in
`password_prompt_view_bridge.mm`.

Tests continued to pass, as the chooser _was_ being popped up, it was
simply constrained to a 0x0 frame. According to
https://codereview.chromium.org/1781433002/#msg15, it's not clear that
we can add a test that would verify that this doesn't regress. That
seems unfortunate. :(

BUG=217034,
R=vabr@chromium.org, apacible@chromium.org, thakis@chromium.org
TBR=vasilii@chromium.org
==========

[Mike West, 2016-03-17 17:38:38]

mkwst@chromium.org changed reviewers:
+ vasilii@chromium.org

[Mike West, 2016-03-17 17:40:19]

> It makes sense to check to make sure the dialog has actually been shown, not
just created. If there's a fixed content size that's expected, you could
possibly check the dialog and webcontents size in an integration/browser test.

Do we have any tests that do something like this? As I noted, I am basically
incapable of cocoa, so, cargo-culting something together would be great. :)

Landing this minimal patch so I can kick off a merge request.

[Mike West, 2016-03-17 17:40:22]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2016-03-17 17:40:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1809043002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1809043002/1

[commit-bot: I haz the power, 2016-03-17 17:46:06]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-17 17:47:13]

Description was changed from

==========
[Mac] CREDENTIAL: Fix chooser dialog after r380352

https://codereview.chromium.org/1781433002 enabled auto-resize for
constrained web dialogs on Mac. It updated a number of callsites for
`new ConstrainedWindowMac`, but didn't update the callsite in
`password_prompt_view_bridge.mm`.

Tests continued to pass, as the chooser _was_ being popped up, it was
simply constrained to a 0x0 frame. According to
https://codereview.chromium.org/1781433002/#msg15, it's not clear that
we can add a test that would verify that this doesn't regress. That
seems unfortunate. :(

BUG=217034,
R=vabr@chromium.org, apacible@chromium.org, thakis@chromium.org
TBR=vasilii@chromium.org
==========

to

==========
[Mac] CREDENTIAL: Fix chooser dialog after r380352

https://codereview.chromium.org/1781433002 enabled auto-resize for
constrained web dialogs on Mac. It updated a number of callsites for
`new ConstrainedWindowMac`, but didn't update the callsite in
`password_prompt_view_bridge.mm`.

Tests continued to pass, as the chooser _was_ being popped up, it was
simply constrained to a 0x0 frame. According to
https://codereview.chromium.org/1781433002/#msg15, it's not clear that
we can add a test that would verify that this doesn't regress. That
seems unfortunate. :(

BUG=217034,
R=vabr@chromium.org, apacible@chromium.org, thakis@chromium.org
TBR=vasilii@chromium.org

Committed: https://crrev.com/0f924bea7baf260f8729e608e0ec1baac418f021
Cr-Commit-Position: refs/heads/master@{#381740}
==========

[commit-bot: I haz the power, 2016-03-17 17:47:14]

Patchset 1 (id:??) landed as
https://crrev.com/0f924bea7baf260f8729e608e0ec1baac418f021
Cr-Commit-Position: refs/heads/master@{#381740}

[Nico, 2016-03-18 18:46:53]

Yup, having test coverage here would be excellent. Looks like it landed here
https://codereview.chromium.org/1610653002 without test coverage.

chrome/browser/ui/cocoa/passwords/OWNERS, please ask for tests for new code (and
work with folks whose code you've reviewed to add tests for this code)

[vabr (Chromium), 2016-03-21 14:05:22]

On 2016/03/18 18:46:53, Nico wrote:
> Yup, having test coverage here would be excellent. Looks like it landed here
> https://codereview.chromium.org/1610653002 without test coverage.
> 
> chrome/browser/ui/cocoa/passwords/OWNERS, please ask for tests for new code
(and
> work with folks whose code you've reviewed to add tests for this code)

I second Mike in asking for a good example of an integration/browser test
checking the dialog and webcontents size.

Cheers,
Vaclav

[Nico, 2016-03-21 20:29:15]

thakis@chromium.org changed reviewers:
+ groby@chromium.org

[Nico, 2016-03-21 20:29:15]

groby, you wrote some web contents constrained dialog thing once, didn't you? Do
you remember if that had tests checking for sizing somewhere?

(vabr: Asking for help for this is cool, but that's something that should happen
before committing untested code... If I'm misremembering groby's past
adventures, I'd recommend asking on chromium-dev)

[groby-ooo-7-16, 2016-03-22 01:05:48]

On 2016/03/21 20:29:15, Nico wrote:
> groby, you wrote some web contents constrained dialog thing once, didn't you?
Do
> you remember if that had tests checking for sizing somewhere?
> 
> (vabr: Asking for help for this is cool, but that's something that should
happen
> before committing untested code... If I'm misremembering groby's past
> adventures, I'd recommend asking on chromium-dev)

The closest I remember is probably the autocomplete sign-in dialog, here:
https://codereview.chromium.org/97993006 (the part to make this work was cycling
the runloop and draining the AR pool)

I'm not sure that's directly applicable here, though? What specifically are you
trying to test? Shouldn't CreateAndShowWebModalDialogMac cover the resize part?
(And looking at msg15 we tried for PrintPreview. IIRC what I did, it seems
_those_ tests need runloop cycling?)