Create superclass for IOSChromeSavePasswordInfobarDelegate

Create superclass for IOSChromeSavePasswordInfobarDelegate

Splits out IOSChromePasswordManagerInfoBarDelegate from
IOSChromeSavePasswordInfoBarDelegate to facilitate creation of an
InfoBarDelegate for updating passwords.

BUG=622244
Committed: https://crrev.com/34dc65757e2a50e8761f39e2811a4cd342a01970
Cr-Commit-Position: refs/heads/master@{#402453}

[Jackie Quinn, 2016-06-28 09:05:35]

jyquinn@chromium.org changed reviewers:
+ vabr@chromium.org

[Jackie Quinn, 2016-06-28 09:06:34]

On 2016/06/28 09:05:36, Jackie Quinn wrote:

Quick question: why are these prefixed with IOSChrome? Would it be possible to
remove this prefix?

[vabr (Chromium), 2016-06-28 09:28:24]

On 2016/06/28 09:06:34, Jackie Quinn wrote:
> On 2016/06/28 09:05:36, Jackie Quinn wrote:
> 
> Quick question: why are these prefixed with IOSChrome? Would it be possible to
> remove this prefix?

Quick reply before I jump on the review:
At the time the classes were named, IOSChrome was the prefix used consistently
for iOS versions of keyed services. So it has never been compulsory for the
class in question here. Having said that, it is a good idea of having some
prefix, it makes it much easier to search for the code by platform and to avoid
misunderstandings (e.g., we have two PasswordGenerationAgent classes, and it's a
bit messy). So I'd be fine changing the prefix to something still associated to
iOS, but not dropping it. If we change it, it would be good to have a broader
consensus on what is a good prefix across all the iOS code, though.

Cheers,
Vaclav

[vabr (Chromium), 2016-06-28 09:41:20]

Thanks for the CL, this LGTM!
Just a couple of comments/questions below.

Cheers,
Vaclav

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h
(right):

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h:36:
std::unique_ptr<password_manager::PasswordFormManager> form_manager_;
Please move the data members to the private section and provide accessors as
necessary. The style guide forbids non-private data members outside of tests:
https://google.github.io/styleguide/cppguide.html#Access_Control

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h:42:
bool is_smart_lock_branding_enabled_;
Could it be const to document that it is not updated though the life of the
object?

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.h
(right):

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.h:33:
std::unique_ptr<password_manager::PasswordFormManager> form_manager);
Just out of curiosity, why did |form_to_save| changed to |form_manager|? The
former gives a hint about the purpose, while the latter just repeats the data
type.

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.h:45: // 
base::string16 GetLinkText() const override;
Please remove commented-out code before landing.

[Jackie Quinn, 2016-06-28 09:59:05]

On 2016/06/28 09:28:24, vabr (Chromium) wrote:
> On 2016/06/28 09:06:34, Jackie Quinn wrote:
> > On 2016/06/28 09:05:36, Jackie Quinn wrote:
> > 
> > Quick question: why are these prefixed with IOSChrome? Would it be possible
to
> > remove this prefix?
> 
> Quick reply before I jump on the review:
> At the time the classes were named, IOSChrome was the prefix used consistently
> for iOS versions of keyed services. So it has never been compulsory for the
> class in question here. Having said that, it is a good idea of having some
> prefix, it makes it much easier to search for the code by platform and to
avoid
> misunderstandings (e.g., we have two PasswordGenerationAgent classes, and it's
a
> bit messy). So I'd be fine changing the prefix to something still associated
to
> iOS, but not dropping it. If we change it, it would be good to have a broader
> consensus on what is a good prefix across all the iOS code, though.
> 
> Cheers,
> Vaclav

I figured it was something like that, and I agree that it's good to have
differentiation for the platform implementations. I've seen this done a couple
of different ways in our code base (prefix with IOS, suffix with IOS, prefix
with IOSChrome...).  It would be great to consolidate these, but I think for now
leaving as is would be good for consistency within this directory :-)

[vabr (Chromium), 2016-06-28 10:01:45]

On 2016/06/28 09:59:05, Jackie Quinn wrote:
> On 2016/06/28 09:28:24, vabr (Chromium) wrote:
> > On 2016/06/28 09:06:34, Jackie Quinn wrote:
> > > On 2016/06/28 09:05:36, Jackie Quinn wrote:
> > > 
> > > Quick question: why are these prefixed with IOSChrome? Would it be
possible
> to
> > > remove this prefix?
> > 
> > Quick reply before I jump on the review:
> > At the time the classes were named, IOSChrome was the prefix used
consistently
> > for iOS versions of keyed services. So it has never been compulsory for the
> > class in question here. Having said that, it is a good idea of having some
> > prefix, it makes it much easier to search for the code by platform and to
> avoid
> > misunderstandings (e.g., we have two PasswordGenerationAgent classes, and
it's
> a
> > bit messy). So I'd be fine changing the prefix to something still associated
> to
> > iOS, but not dropping it. If we change it, it would be good to have a
broader
> > consensus on what is a good prefix across all the iOS code, though.
> > 
> > Cheers,
> > Vaclav
> 
> I figured it was something like that, and I agree that it's good to have
> differentiation for the platform implementations. I've seen this done a couple
> of different ways in our code base (prefix with IOS, suffix with IOS, prefix
> with IOSChrome...).  It would be great to consolidate these, but I think for
now
> leaving as is would be good for consistency within this directory :-)

SGTM, keeping it as it is in this CL looks the best to me for now. (Of course,
if switching to just IOS would make your life easier in this CL, go for it.)

[sdefresne, 2016-06-28 10:06:46]

On 2016/06/28 09:59:05, Jackie Quinn wrote:
> On 2016/06/28 09:28:24, vabr (Chromium) wrote:
> > On 2016/06/28 09:06:34, Jackie Quinn wrote:
> > > On 2016/06/28 09:05:36, Jackie Quinn wrote:
> > > 
> > > Quick question: why are these prefixed with IOSChrome? Would it be
possible
> to
> > > remove this prefix?
> > 
> > Quick reply before I jump on the review:
> > At the time the classes were named, IOSChrome was the prefix used
consistently
> > for iOS versions of keyed services. So it has never been compulsory for the
> > class in question here. Having said that, it is a good idea of having some
> > prefix, it makes it much easier to search for the code by platform and to
> avoid
> > misunderstandings (e.g., we have two PasswordGenerationAgent classes, and
it's
> a
> > bit messy). So I'd be fine changing the prefix to something still associated
> to
> > iOS, but not dropping it. If we change it, it would be good to have a
broader
> > consensus on what is a good prefix across all the iOS code, though.
> > 
> > Cheers,
> > Vaclav
> 
> I figured it was something like that, and I agree that it's good to have
> differentiation for the platform implementations. I've seen this done a couple
> of different ways in our code base (prefix with IOS, suffix with IOS, prefix
> with IOSChrome...).  It would be great to consolidate these, but I think for
now
> leaving as is would be good for consistency within this directory :-)

The recommended prefix for code in ios/chrome/ (especially for iOS
implementation of interface declared in components and also implemented in
chrome/) is IOSChrome. This is the prefix that was selected by the iOS team
owners. The agreement came during the upstreaming/unforking effort and was not
applied consistently (help welcome to convert the existing non-compliant names).

The reason for using that prefix is that IOS is not specific enough (there are
some other IOS specific implementation of interface declared in components
outside of ios/chrome/, for example in ios/crnet/). So the prefix IOSChrome was
picked. The reason for a name prefix vs a namespace is that namespace are (were)
discouraged for application code and just recommended for libraries (i.e. code
shared by more than one application) as there is no risk of collision in
application code.

[sdefresne, 2016-06-28 10:10:00]

drive-by

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.h
(right):

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.h:33:
std::unique_ptr<password_manager::PasswordFormManager> form_manager);
On 2016/06/28 09:41:19, vabr (Chromium) wrote:
> Just out of curiosity, why did |form_to_save| changed to |form_manager|? The
> former gives a hint about the purpose, while the latter just repeats the data
> type.

+1

Good post about naming here
http://journal.stuffwithstuff.com/2016/06/16/long-names-are-long/ discourage
using meaningless words like "manager" in a label. So |form| would be a better
name than |form_manager| if |form_to_save| is no longer correct.

[Jackie Quinn, 2016-06-28 10:39:42]

On 2016/06/28 10:06:46, sdefresne wrote:
> On 2016/06/28 09:59:05, Jackie Quinn wrote:
> > On 2016/06/28 09:28:24, vabr (Chromium) wrote:
> > > On 2016/06/28 09:06:34, Jackie Quinn wrote:
> > > > On 2016/06/28 09:05:36, Jackie Quinn wrote:
> > > > 
> > > > Quick question: why are these prefixed with IOSChrome? Would it be
> possible
> > to
> > > > remove this prefix?
> > > 
> > > Quick reply before I jump on the review:
> > > At the time the classes were named, IOSChrome was the prefix used
> consistently
> > > for iOS versions of keyed services. So it has never been compulsory for
the
> > > class in question here. Having said that, it is a good idea of having some
> > > prefix, it makes it much easier to search for the code by platform and to
> > avoid
> > > misunderstandings (e.g., we have two PasswordGenerationAgent classes, and
> it's
> > a
> > > bit messy). So I'd be fine changing the prefix to something still
associated
> > to
> > > iOS, but not dropping it. If we change it, it would be good to have a
> broader
> > > consensus on what is a good prefix across all the iOS code, though.
> > > 
> > > Cheers,
> > > Vaclav
> > 
> > I figured it was something like that, and I agree that it's good to have
> > differentiation for the platform implementations. I've seen this done a
couple
> > of different ways in our code base (prefix with IOS, suffix with IOS, prefix
> > with IOSChrome...).  It would be great to consolidate these, but I think for
> now
> > leaving as is would be good for consistency within this directory :-)
> 
> The recommended prefix for code in ios/chrome/ (especially for iOS
> implementation of interface declared in components and also implemented in
> chrome/) is IOSChrome. This is the prefix that was selected by the iOS team
> owners. The agreement came during the upstreaming/unforking effort and was not
> applied consistently (help welcome to convert the existing non-compliant
names).
> 
> The reason for using that prefix is that IOS is not specific enough (there are
> some other IOS specific implementation of interface declared in components
> outside of ios/chrome/, for example in ios/crnet/). So the prefix IOSChrome
was
> picked. The reason for a name prefix vs a namespace is that namespace are
(were)
> discouraged for application code and just recommended for libraries (i.e. code
> shared by more than one application) as there is no risk of collision in
> application code.

Thanks for the explanation! That's really good to know. Is this documented
somewhere?

[Jackie Quinn, 2016-06-28 11:39:11]

Changes made. I'm not super confident on my accessor/mutator method styling, so
if you could take a quick look to make sure they're correct that would be great!

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h
(right):

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h:36:
std::unique_ptr<password_manager::PasswordFormManager> form_manager_;
On 2016/06/28 09:41:19, vabr (Chromium) wrote:
> Please move the data members to the private section and provide accessors as
> necessary. The style guide forbids non-private data members outside of tests:
> https://google.github.io/styleguide/cppguide.html#Access_Control

Done.

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h:42:
bool is_smart_lock_branding_enabled_;
On 2016/06/28 09:41:19, vabr (Chromium) wrote:
> Could it be const to document that it is not updated though the life of the
> object?

Done.

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.h
(right):

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.h:33:
std::unique_ptr<password_manager::PasswordFormManager> form_manager);
On 2016/06/28 10:10:00, sdefresne wrote:
> On 2016/06/28 09:41:19, vabr (Chromium) wrote:
> > Just out of curiosity, why did |form_to_save| changed to |form_manager|? The
> > former gives a hint about the purpose, while the latter just repeats the
data
> > type.
> 
> +1
> 
> Good post about naming here
> http://journal.stuffwithstuff.com/2016/06/16/long-names-are-long/ discourage
> using meaningless words like "manager" in a label. So |form| would be a better
> name than |form_manager| if |form_to_save| is no longer correct.

Thanks for the post! 
It wasn't clear to me that "form_to_save" was still relevant for an update form.
If 
it can be applied the same way I'll happily change it back :-)

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.h:45: // 
base::string16 GetLinkText() const override;
On 2016/06/28 09:41:19, vabr (Chromium) wrote:
> Please remove commented-out code before landing.

Done.

[vabr (Chromium), 2016-06-28 12:13:06]

Thanks, Jackie!
LGTM, with some minor nits.

Cheers,
Vaclav

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.h
(right):

https://codereview.chromium.org/2108593002/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/ios_chrome_save_password_infobar_delegate.h:33:
std::unique_ptr<password_manager::PasswordFormManager> form_manager);
On 2016/06/28 11:39:11, Jackie Quinn wrote:
> On 2016/06/28 10:10:00, sdefresne wrote:
> > On 2016/06/28 09:41:19, vabr (Chromium) wrote:
> > > Just out of curiosity, why did |form_to_save| changed to |form_manager|?
The
> > > former gives a hint about the purpose, while the latter just repeats the
> data
> > > type.
> > 
> > +1
> > 
> > Good post about naming here
> > http://journal.stuffwithstuff.com/2016/06/16/long-names-are-long/ discourage
> > using meaningless words like "manager" in a label. So |form| would be a
better
> > name than |form_manager| if |form_to_save| is no longer correct.
> 
> Thanks for the post! 
> It wasn't clear to me that "form_to_save" was still relevant for an update
form.
> If 
> it can be applied the same way I'll happily change it back :-)

I can confirm that "form_to_save_" is still relevant. Whether the password is
being created or updated in the PasswordStore, it is still some kind of saving
it. If you find that confusing given the usage of Save vs. Update in the
specialised delegates' names, we can use, e.g., form_to_store_ or something
similar.

https://codereview.chromium.org/2108593002/diff/10007/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h
(right):

https://codereview.chromium.org/2108593002/diff/10007/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h:43:
void SetInfoBarResponse(
nit: set_infobar_response
(http://dev.chromium.org/developers/coding-style#TOC-Inline-functions)

https://codereview.chromium.org/2108593002/diff/10007/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h:56:
// the user about, and should update as per her decision.
nit: Let's use gender-neutral phrasing: her -> their. Gender pronouns have been
removed from the code recently, e.g., in
https://codereview.chromium.org/2096783002/.

https://codereview.chromium.org/2108593002/diff/10007/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h:56:
// the user about, and should update as per her decision.
nit: "update" -> "save or update" or just "save"; if the user agrees, some data
will be saved in the PasswordStore, resulting in either updating an existing
credential, or creating a new entry.

[Jackie Quinn, 2016-06-28 12:23:51]

https://codereview.chromium.org/2108593002/diff/10007/ios/chrome/browser/pass...
File ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h
(right):

https://codereview.chromium.org/2108593002/diff/10007/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h:43:
void SetInfoBarResponse(
On 2016/06/28 12:13:06, vabr (Chromium) wrote:
> nit: set_infobar_response
> (http://dev.chromium.org/developers/coding-style#TOC-Inline-functions)

Done.

https://codereview.chromium.org/2108593002/diff/10007/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h:56:
// the user about, and should update as per her decision.
On 2016/06/28 12:13:06, vabr (Chromium) wrote:
> nit: "update" -> "save or update" or just "save"; if the user agrees, some
data
> will be saved in the PasswordStore, resulting in either updating an existing
> credential, or creating a new entry.

Done.

https://codereview.chromium.org/2108593002/diff/10007/ios/chrome/browser/pass...
ios/chrome/browser/passwords/ios_chrome_password_manager_infobar_delegate.h:56:
// the user about, and should update as per her decision.
On 2016/06/28 12:13:06, vabr (Chromium) wrote:
> nit: Let's use gender-neutral phrasing: her -> their. Gender pronouns have
been
> removed from the code recently, e.g., in
> https://codereview.chromium.org/2096783002/.

Done.

[Jackie Quinn, 2016-06-28 12:31:54]

The CQ bit was checked by jyquinn@chromium.org

[Jackie Quinn, 2016-06-28 12:31:55]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2108593002/#ps50001
(title: "Review rd 2")

[commit-bot: I haz the power, 2016-06-28 12:32:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 13:35:11]

Committed patchset #4 (id:50001)

[commit-bot: I haz the power, 2016-06-28 13:37:18]

Description was changed from

==========
Create superclass for IOSChromeSavePasswordInfobarDelegate

Splits out IOSChromePasswordManagerInfoBarDelegate from
IOSChromeSavePasswordInfoBarDelegate to facilitate creation of an
InfoBarDelegate for updating passwords.

BUG=622244
==========

to

==========
Create superclass for IOSChromeSavePasswordInfobarDelegate

Splits out IOSChromePasswordManagerInfoBarDelegate from
IOSChromeSavePasswordInfoBarDelegate to facilitate creation of an
InfoBarDelegate for updating passwords.

BUG=622244

Committed: https://crrev.com/34dc65757e2a50e8761f39e2811a4cd342a01970
Cr-Commit-Position: refs/heads/master@{#402453}
==========

[commit-bot: I haz the power, 2016-06-28 13:37:20]

Patchset 4 (id:??) landed as
https://crrev.com/34dc65757e2a50e8761f39e2811a4cd342a01970
Cr-Commit-Position: refs/heads/master@{#402453}