List Password Manager extensions in a static data structure

List Password Manager extensions in a static data structure

Collected the necessary information for supported Password Manager
Extensions in a static data structure.

BUG=712194
TEST=test with 1Password, Lastpass, and Dashlane extensions

Review-Url: https://codereview.chromium.org/2823883002
Cr-Commit-Position: refs/heads/master@{#465567}
Committed: https://chromium.googlesource.com/chromium/src/+/a07a8f7a373cc13ee2163341a1b9c8c813c49a1f

[pkl (ping after 24h if needed), 2017-04-17 15:57:15]

pkl@chromium.org changed reviewers:
+ jif@chromium.org, justincohen@chromium.org

[pkl (ping after 24h if needed), 2017-04-17 15:57:16]

Justin: Please take a look at this. jif@ is out today.

[justincohen, 2017-04-17 16:04:58]

lgtm

[pkl (ping after 24h if needed), 2017-04-17 16:05:27]

The CQ bit was checked by pkl@chromium.org

[commit-bot: I haz the power, 2017-04-17 16:05:45]

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, 2017-04-17 16:12:24]

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

[commit-bot: I haz the power, 2017-04-17 16:12:25]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pkl (ping after 24h if needed), 2017-04-17 16:15:02]

pkl@chromium.org changed reviewers:
+ rohitrao@chromium.org

[pkl (ping after 24h if needed), 2017-04-17 16:15:02]

+rohitrao for owners

[rohitrao (ping after 24h), 2017-04-17 16:24:54]

lgtm

Could this be a hardcoded array in a new .mm file, instead of a plist file? 
Would that be as simple to edit, but without the downsides of having to manage
and load a separate file from disk?  Are there advantages to storing this data
as a plist?

https://codereview.chromium.org/2823883002/diff/20001/ios/chrome/app/BUILD.gn
File ios/chrome/app/BUILD.gn (right):

https://codereview.chromium.org/2823883002/diff/20001/ios/chrome/app/BUILD.gn...
ios/chrome/app/BUILD.gn:79: "resources/PasswordManagerExtensions.plist",
Could this file live next to the code that uses it (and be in that BUILD.gn file
instead)?

[pkl (ping after 24h if needed), 2017-04-17 20:07:51]

Description was changed from

==========
Replace hardcoded list of Password Manager extensions with a plist

Instead of hardcoding certain password manager extensions in code and
enum list, change them to a plist of whitelisted password managers.
The list is stored in a plist format that can be easily added to with
a simple CL.

BUG=712194
TEST=test with 1Password, Lastpass, and Dashlane extensions
==========

to

==========
List Password Manager extensions in a static data structure

Collected the necessary information for supported Password Manager
Extensions in a static data structure.

BUG=712194
TEST=test with 1Password, Lastpass, and Dashlane extensions
==========

[pkl (ping after 24h if needed), 2017-04-17 20:27:04]

Removed .plist file.
Can you take a second look before I submit?

[jif, 2017-04-18 11:12:21]

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/activity_type_util.mm (right):

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:30: // The
|exact_match| field defines whether a partial prefix match of bundle_id
Disclaimers:
1/ I'm nitpicking because this comment is likely to be read by external
contributors.
2/ As I am not a native english speaker, I'll let you decide if my suggestion is
valid or not.

I find this sentence to be slightly misleading because the explanation explains
the _opposite_ of what the boolean stores.

How about:
"The |exact_match| field defines whether an exact match of bundle_id is required
to consider activity_string a match, as opposed to a partial prefix match."

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:35: const char*
bundle_id;
can we make this a:
const std::string bundle_id;
?

This would allow us to either:
-remove std::string bundle_id(kAllPasswordManagerApps[i].bundle_id);
or:
-replace it with: std::string const& bundle_id =
kAllPasswordManagerApps[i].bundle_id;

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:46: for (size_t i
= 0; i < arraysize(kAllPasswordManagerApps); ++i) {
Not sure if this compiles, but can you try:

for (const auto& password_manager_app : kAllPasswordManagerApps) {

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:106: return
APPEX_PASSWORD_MANAGEMENT;
for consistency, can you add braces?

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:122: NSNumber*
PasswordAppExActivityVersion(NSString* activityString) {
pkl: If you believe |PasswordAppExActivityVersion| may return different versions
in the futur (besides nil and kPasswordAppExVersionNumber), you can stop reading
this comment now.

I think the usage of |PasswordAppExActivityVersion| can be replaced by
|IsPasswordManagerActivity| everywhere (the returned NSNumber is only used in
the |appExItems| dictionary).

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:131: bool
IsPasswordAppExActivity(NSString* activityString) {
Shouldn't usage of |IsPasswordAppExActivity| be replaced with
|IsPasswordManagerActivity|?

[pkl (ping after 24h if needed), 2017-04-18 19:12:37]

Thanks, JF. PTAL.

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/activity_type_util.mm (right):

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:30: // The
|exact_match| field defines whether a partial prefix match of bundle_id
On 2017/04/18 11:12:20, jif wrote:
> Disclaimers:
> 1/ I'm nitpicking because this comment is likely to be read by external
> contributors.
> 2/ As I am not a native english speaker, I'll let you decide if my suggestion
is
> valid or not.
> 
> I find this sentence to be slightly misleading because the explanation
explains
> the _opposite_ of what the boolean stores.
> 
> How about:
> "The |exact_match| field defines whether an exact match of bundle_id is
required
> to consider activity_string a match, as opposed to a partial prefix match."

You are correct. I wrote this too quickly. Thank you for catching!

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:35: const char*
bundle_id;
On 2017/04/18 11:12:20, jif wrote:
> can we make this a:
> const std::string bundle_id;
> ?
> 
> This would allow us to either:
> -remove std::string bundle_id(kAllPasswordManagerApps[i].bundle_id);
> or:
> -replace it with: std::string const& bundle_id =
> kAllPasswordManagerApps[i].bundle_id;

Can't do that. That would become a static initialization.

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:46: for (size_t i
= 0; i < arraysize(kAllPasswordManagerApps); ++i) {
On 2017/04/18 11:12:20, jif wrote:
> Not sure if this compiles, but can you try:
> 
> for (const auto& password_manager_app : kAllPasswordManagerApps) {

Sounds good to me! Done.

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:106: return
APPEX_PASSWORD_MANAGEMENT;
On 2017/04/18 11:12:20, jif wrote:
> for consistency, can you add braces?

Braces around single-line if are optional. What consistency are we striving for
here?

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:122: NSNumber*
PasswordAppExActivityVersion(NSString* activityString) {
On 2017/04/18 11:12:20, jif wrote:
> pkl: If you believe |PasswordAppExActivityVersion| may return different
versions
> in the futur (besides nil and kPasswordAppExVersionNumber), you can stop
reading
> this comment now.
> 
> I think the usage of |PasswordAppExActivityVersion| can be replaced by
> |IsPasswordManagerActivity| everywhere (the returned NSNumber is only used in
> the |appExItems| dictionary).

Good point, but this seems like an API refactoring unrelated to the main purpose
of this CL. I'd
rather do it separately.

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:131: bool
IsPasswordAppExActivity(NSString* activityString) {
On 2017/04/18 11:12:20, jif wrote:
> Shouldn't usage of |IsPasswordAppExActivity| be replaced with
> |IsPasswordManagerActivity|?

This seems like an API refactoring unrelated to the main purpose of this CL. I'd
rather do it separately.

[jif, 2017-04-19 09:10:12]

lgtm

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/activity_services/activity_type_util.mm (right):

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:35: const char*
bundle_id;
On 2017/04/18 19:12:37, pklpkl wrote:
> On 2017/04/18 11:12:20, jif wrote:
> > can we make this a:
> > const std::string bundle_id;
> > ?
> > 
> > This would allow us to either:
> > -remove std::string bundle_id(kAllPasswordManagerApps[i].bundle_id);
> > or:
> > -replace it with: std::string const& bundle_id =
> > kAllPasswordManagerApps[i].bundle_id;
> 
> Can't do that. That would become a static initialization.

Acknowledged.

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:106: return
APPEX_PASSWORD_MANAGEMENT;
On 2017/04/18 19:12:37, pklpkl wrote:
> On 2017/04/18 11:12:20, jif wrote:
> > for consistency, can you add braces?
> 
> Braces around single-line if are optional. What consistency are we striving
for
> here?

consistency with lines 116-118

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:122: NSNumber*
PasswordAppExActivityVersion(NSString* activityString) {
On 2017/04/18 19:12:36, pklpkl wrote:
> On 2017/04/18 11:12:20, jif wrote:
> > pkl: If you believe |PasswordAppExActivityVersion| may return different
> versions
> > in the futur (besides nil and kPasswordAppExVersionNumber), you can stop
> reading
> > this comment now.
> > 
> > I think the usage of |PasswordAppExActivityVersion| can be replaced by
> > |IsPasswordManagerActivity| everywhere (the returned NSNumber is only used
in
> > the |appExItems| dictionary).
> 
> Good point, but this seems like an API refactoring unrelated to the main
purpose
> of this CL. I'd
> rather do it separately.

Acknowledged.

https://codereview.chromium.org/2823883002/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/activity_services/activity_type_util.mm:131: bool
IsPasswordAppExActivity(NSString* activityString) {
On 2017/04/18 19:12:36, pklpkl wrote:
> On 2017/04/18 11:12:20, jif wrote:
> > Shouldn't usage of |IsPasswordAppExActivity| be replaced with
> > |IsPasswordManagerActivity|?
> 
> This seems like an API refactoring unrelated to the main purpose of this CL.
I'd
> rather do it separately.

Acknowledged.

[rohitrao (ping after 24h), 2017-04-19 11:34:36]

lgtm

[pkl (ping after 24h if needed), 2017-04-19 12:52:42]

The CQ bit was checked by pkl@chromium.org

[pkl (ping after 24h if needed), 2017-04-19 12:52:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
justincohen@chromium.org
Link to the patchset: https://codereview.chromium.org/2823883002/#ps120001
(title: "removed dead comment")

[commit-bot: I haz the power, 2017-04-19 12:53:03]

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, 2017-04-19 13:02:42]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492606362586920,
"parent_rev": "83d87e7a1a0cd89eaa29e8806c8f959106eda34b", "commit_rev":
"a07a8f7a373cc13ee2163341a1b9c8c813c49a1f"}

[commit-bot: I haz the power, 2017-04-19 13:02:49]

Description was changed from

==========
List Password Manager extensions in a static data structure

Collected the necessary information for supported Password Manager
Extensions in a static data structure.

BUG=712194
TEST=test with 1Password, Lastpass, and Dashlane extensions
==========

to

==========
List Password Manager extensions in a static data structure

Collected the necessary information for supported Password Manager
Extensions in a static data structure.

BUG=712194
TEST=test with 1Password, Lastpass, and Dashlane extensions

Review-Url: https://codereview.chromium.org/2823883002
Cr-Commit-Position: refs/heads/master@{#465567}
Committed:
https://chromium.googlesource.com/chromium/src/+/a07a8f7a373cc13ee2163341a1b9...
==========

[commit-bot: I haz the power, 2017-04-19 13:02:51]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/a07a8f7a373cc13ee2163341a1b9...