Enable the save card bubble on Mac without a flag.

Enable the save card bubble on Mac without a flag.

BUG=566271
Committed: https://crrev.com/f41a2211a775b1f607eabe0c8e934daa42c15be0
Cr-Commit-Position: refs/heads/master@{#380073}

[Justin Donnelly, 2016-03-04 20:39:57]

jdonnelly@chromium.org changed reviewers:
+ estade@chromium.org

[Justin Donnelly, 2016-03-07 21:23:00]

Friendly ping.

[Evan Stade, 2016-03-07 22:42:03]

https://codereview.chromium.org/1761073005/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (left):

https://codereview.chromium.org/1761073005/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.cc:210:
AutofillCCInfoBarDelegate::CreateForLocalSave(
do you still need this fn for something? Do you need AutofillCCInfoBarDelegate
at all?

[Justin Donnelly, 2016-03-07 23:14:41]

https://codereview.chromium.org/1761073005/diff/1/chrome/browser/ui/autofill/...
File chrome/browser/ui/autofill/chrome_autofill_client.cc (left):

https://codereview.chromium.org/1761073005/diff/1/chrome/browser/ui/autofill/...
chrome/browser/ui/autofill/chrome_autofill_client.cc:210:
AutofillCCInfoBarDelegate::CreateForLocalSave(
On 2016/03/07 22:42:03, Evan Stade wrote:
> do you still need this fn for something? Do you need AutofillCCInfoBarDelegate
> at all?

Hmm, good point. we don't need CreateForLocalSave anymore. We do still need
AutofillSaveCardInfoBarDelegate though as it's extended by
AutofillSaveCardInfoBarDelegateMobile. I imagine those should be collapsed into
one class now.

I'd prefer to not do that here, though, so we don't have a big CL potentially
blocking rollback if someone encounters a problem with the new bubble. But I can
start work on the cleanup tomorrow.

[Evan Stade, 2016-03-08 00:56:39]

On 2016/03/07 23:14:41, Justin Donnelly wrote:
>
https://codereview.chromium.org/1761073005/diff/1/chrome/browser/ui/autofill/...
> File chrome/browser/ui/autofill/chrome_autofill_client.cc (left):
> 
>
https://codereview.chromium.org/1761073005/diff/1/chrome/browser/ui/autofill/...
> chrome/browser/ui/autofill/chrome_autofill_client.cc:210:
> AutofillCCInfoBarDelegate::CreateForLocalSave(
> On 2016/03/07 22:42:03, Evan Stade wrote:
> > do you still need this fn for something? Do you need
AutofillCCInfoBarDelegate
> > at all?
> 
> Hmm, good point. we don't need CreateForLocalSave anymore. We do still need
> AutofillSaveCardInfoBarDelegate though as it's extended by
> AutofillSaveCardInfoBarDelegateMobile. I imagine those should be collapsed
into
> one class now.
> 
> I'd prefer to not do that here, though, so we don't have a big CL potentially
> blocking rollback if someone encounters a problem with the new bubble. But I
can
> start work on the cleanup tomorrow.

I'm not sure how the cleanup part would block rollback. That cleanup would be
part of the rollback. If you want to do it as a separate CL, can you write that
CL first (it will be dependent on this one) and have them both LG'd before you
land the first one?

[Justin Donnelly, 2016-03-08 19:21:30]

On 2016/03/08 00:56:39, Evan Stade wrote:
> I'm not sure how the cleanup part would block rollback. That cleanup would be
> part of the rollback.

Every additional file and line of code in a CL increases the odds that a
subsequent change will touch one of the same lines or adjacent lines and create
a conflict on rollback. The cleanup touches hundreds of lines of code across 10
files.

> If you want to do it as a separate CL, can you write that
> CL first (it will be dependent on this one) and have them both LG'd before you
> land the first one?

Cleanup CL: https://codereview.chromium.org/1780443002/

[Evan Stade, 2016-03-08 21:43:34]

On 2016/03/08 19:21:30, Justin Donnelly wrote:
> On 2016/03/08 00:56:39, Evan Stade wrote:
> > I'm not sure how the cleanup part would block rollback. That cleanup would
be
> > part of the rollback.
> 
> Every additional file and line of code in a CL increases the odds that a
> subsequent change will touch one of the same lines or adjacent lines and
create
> a conflict on rollback. The cleanup touches hundreds of lines of code across
10
> files.

Most of the lines it touches are deletions or large blocks of addition. I don't
think these have a high conflict risk.

> 
> > If you want to do it as a separate CL, can you write that
> > CL first (it will be dependent on this one) and have them both LG'd before
you
> > land the first one?
> 
> Cleanup CL: https://codereview.chromium.org/1780443002/

thanks. this CL lgtm

[Justin Donnelly, 2016-03-09 03:53:29]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2016-03-09 03:53:43]

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

[commit-bot: I haz the power, 2016-03-09 05:13:11]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-09 05:15:12]

Description was changed from

==========
Enable the save card bubble on Mac without a flag.

BUG=566271
==========

to

==========
Enable the save card bubble on Mac without a flag.

BUG=566271

Committed: https://crrev.com/f41a2211a775b1f607eabe0c8e934daa42c15be0
Cr-Commit-Position: refs/heads/master@{#380073}
==========

[commit-bot: I haz the power, 2016-03-09 05:15:13]

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