[autofill] Add support for PSL domain matching for password autofill.

[autofill] Add support for PSL domain matching for password autofill.

* Adds support for using the PSL for matching the origin when looking
up passwords using the default password store.
* On Android, change the layout for label and sublabels in autofill
popups.

The feature is currently behind the flag
--enable-password-autofill-psl-domain-matching

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

[nilesh, 2013-06-04 23:02:36]

https://codereview.chromium.org/15660018/diff/7001/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java
(right):

https://codereview.chromium.org/15660018/diff/7001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java:41:
if (TextUtils.isEmpty(getItem(position).mLabel)) {
better to store a a reference to getItem(position).mLabel in a local var.

https://codereview.chromium.org/15660018/diff/7001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/15660018/diff/7001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1235: "enable-autofill-psl-domain-matching",
should be enable-password-autofill-domain-matching

Having password in the name will not confuse it with form autofill.

https://codereview.chromium.org/15660018/diff/7001/chrome/browser/password_ma...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://codereview.chromium.org/15660018/diff/7001/chrome/browser/password_ma...
chrome/browser/password_manager/password_form_manager.cc:348: // either: (1) we
are in Incognito mode, or (2) the ACTION paths don't match.
update the comment.

https://codereview.chromium.org/15660018/diff/7001/chrome/browser/password_ma...
chrome/browser/password_manager/password_form_manager.cc:541: // Exact origin
match gives an addition of 32 (1 << 5) + # of matching url
Update these comments.

https://codereview.chromium.org/15660018/diff/7001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/15660018/diff/7001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:498: const char
kEnableAutofillPSLDomainMatching[] =
kEnablePasswordAutofill..

https://codereview.chromium.org/15660018/diff/7001/components/autofill/common...
File components/autofill/common/password_form_fill_data.h (right):

https://codereview.chromium.org/15660018/diff/7001/components/autofill/common...
components/autofill/common/password_form_fill_data.h:37: // wait_for_username
tells us whether we need to wait for the user to enter
Comments for the new fields.

[nyquist, 2013-06-05 00:08:51]

Addressed all comments.

https://codereview.chromium.org/15660018/diff/7001/chrome/android/java/src/or...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java
(right):

https://codereview.chromium.org/15660018/diff/7001/chrome/android/java/src/or...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java:41:
if (TextUtils.isEmpty(getItem(position).mLabel)) {
On 2013/06/04 23:02:36, nilesh wrote:
> better to store a a reference to getItem(position).mLabel in a local var. 

Done.

https://codereview.chromium.org/15660018/diff/7001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/15660018/diff/7001/chrome/browser/about_flags...
chrome/browser/about_flags.cc:1235: "enable-autofill-psl-domain-matching",
On 2013/06/04 23:02:36, nilesh wrote:
> should be enable-password-autofill-domain-matching
> 
> Having password in the name will not confuse it with form autofill.

Done.

https://codereview.chromium.org/15660018/diff/7001/chrome/browser/password_ma...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://codereview.chromium.org/15660018/diff/7001/chrome/browser/password_ma...
chrome/browser/password_manager/password_form_manager.cc:348: // either: (1) we
are in Incognito mode, or (2) the ACTION paths don't match.
On 2013/06/04 23:02:36, nilesh wrote:
> update the comment.

Done.

https://codereview.chromium.org/15660018/diff/7001/chrome/browser/password_ma...
chrome/browser/password_manager/password_form_manager.cc:541: // Exact origin
match gives an addition of 32 (1 << 5) + # of matching url
On 2013/06/04 23:02:36, nilesh wrote:
> Update these comments.

Done.

https://codereview.chromium.org/15660018/diff/7001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/15660018/diff/7001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:498: const char
kEnableAutofillPSLDomainMatching[] =
On 2013/06/04 23:02:36, nilesh wrote:
> kEnablePasswordAutofill..

Done.

https://codereview.chromium.org/15660018/diff/7001/components/autofill/common...
File components/autofill/common/password_form_fill_data.h (right):

https://codereview.chromium.org/15660018/diff/7001/components/autofill/common...
components/autofill/common/password_form_fill_data.h:37: // wait_for_username
tells us whether we need to wait for the user to enter
On 2013/06/04 23:02:36, nilesh wrote:
> Comments for the new fields.

Done.

[nyquist, 2013-06-05 00:28:18]

nileshagrawal: overall and Android-specific
joi: For content/public/common
isherman: For components/autofill, chrome/renderer/autofill,
chrome/browser/password_manager
aurimas: chrome/android/java/

tim: FYI for chrome/browser/password_manager

[Jói, 2013-06-05 16:28:43]

//content/public/common LGTM

[Ilya Sherman, 2013-06-06 09:25:35]

+Mike for login db changes

Has the security team approved this approach?  If so, could you please point me
to that approval?  I don't feel comfortable approving this CL without
confirmation from the security team.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
File chrome/browser/password_manager/login_database.cc (right):

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:52: std::string
GetPSLDomain(std::string signon_realm_str) {
nit: Pass by const reference.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:52: std::string
GetPSLDomain(std::string signon_realm_str) {
Please spell out acronyms like PSL; or better yet, find a briefer alternative
that is still clear to someone unfamiliar with the acronym.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/password_form_manager.cc:544: // Partial match
gives an addition of 16 (1 << 5) + # matching url dirs
16 -> 32?

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.cc:158: element_bounds_,
suggestions, origins, empty, password_ids, GetWeakPtr());
Hmm.  Are the origins really just surfaced as labels in the UI?  That seems like
a relatively strange UI to me.

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.h:85: const
std::vector<base::string16>& origins,
origins or realms?

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:763: bounds);
nit: Needs curlies now.

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
File components/autofill/browser/autofill_manager.h (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:256: const
std::vector<base::string16>& origins);
origins or realms?

https://codereview.chromium.org/15660018/diff/37001/components/autofill/commo...
File components/autofill/common/password_form_fill_data.h (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/commo...
components/autofill/common/password_form_fill_data.h:35: //                     
    for the form.
I don't quite understand:  Are these supposed to be iterated over in lock-step? 
It seems better to just keep a single collection, but change the type of the map
value to be a struct that contains both the password and the realm.

https://codereview.chromium.org/15660018/diff/37001/components/autofill/rende...
File components/autofill/renderer/autofill_agent.h (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/rende...
components/autofill/renderer/autofill_agent.h:81: InputEventSource
input_event_source) OVERRIDE;
This change seems completely unrelated to PSL domain matching.  Please split it
off to a separate CL.

https://codereview.chromium.org/15660018/diff/37001/components/autofill/rende...
File components/autofill/renderer/page_click_tracker.cc (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/rende...
components/autofill/renderer/page_click_tracker.cc:116: TAP_GESTURE);
Please refactor the code so that the common parts (i.e. nearly everything)
between this method and DidHandleMouseEvent() are shared.

https://codereview.chromium.org/15660018/diff/37001/content/public/common/pas...
File content/public/common/password_form.h (right):

https://codereview.chromium.org/15660018/diff/37001/content/public/common/pas...
content/public/common/password_form.h:175: bool is_psl_origin_match;
If you were to just always set both signon_realm and original_signon_realm,
would this field then become redundant?

[aurimas (slooooooooow), 2013-06-06 16:52:26]

Your description claims:
>* On Android, adds support for description label in autofill
> popups. 

Autofill popups on Android already support the description. The thing that you
are changing is the layout for each popup entry.

https://chromiumcodereview.appspot.com/15660018/diff/37001/chrome/android/jav...
File chrome/android/java/res/layout/autofill_text.xml (right):

https://chromiumcodereview.appspot.com/15660018/diff/37001/chrome/android/jav...
chrome/android/java/res/layout/autofill_text.xml:16: <TextView
android:id="@+id/autofill_name"
While we are at it lets rename these TextViews to something reasonable like
Label and Sublabel.

[palmer, 2013-06-06 21:16:41]

Haven't reviewed everything yet, but wanted to get you something.

https://codereview.chromium.org/15660018/diff/37001/chrome/android/java/res/l...
File chrome/android/java/res/layout/autofill_text.xml (right):

https://codereview.chromium.org/15660018/diff/37001/chrome/android/java/res/l...
chrome/android/java/res/layout/autofill_text.xml:30: android:ellipsize="end"
Hmm, could long text strings cause user confusion?

https://codereview.chromium.org/15660018/diff/37001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java
(right):

https://codereview.chromium.org/15660018/diff/37001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java:43:
labelView.setVisibility(View.GONE);
Does this mean that if the |label| is empty, that the feature still works?
Shouldn't we not use the feature of the informative label is invalid (empty
might be one kind of invalid)?

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1238: kOsAll,
Shouldn't this be Android-only?

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
File chrome/browser/password_manager/login_database.cc (right):

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:52: std::string
GetPSLDomain(std::string signon_realm_str) {
> Please spell out acronyms like PSL; or better yet, find a briefer alternative
> that is still clear to someone unfamiliar with the acronym.

|GetPublicDomainSuffix|? |GetPublicSuffix|?

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:53: GURL
signon_realm(signon_realm_str);
Declare this const if the callee takes a const &.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:378: std::string sql_query =
"SELECT origin_url, action_url, "
I think you can declare these strings const? I'm big on that because it's a nice
guarantee.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:387: //
s.Assign(db_.GetCachedStatement(SQL_FROM_HERE, sql_query.c_str()));
Perhaps you still can? Have you tried it? It should be possible to cache the
statement and then bind new arguments in without invalidating the cache. I
think.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:396: std::string regexp =
"(\\w+?:\\/\\/)([\\w\\-_]+\\.)*" + domain + "\\/$";
Can we instead use an explicit list of schemata, instead of \w+ ? There are some
new and exciting schemata like googlesearch and so on that I'd like to ensure
never get involved in this. Not that I have any specific fears, just that
explicitness is more safe in a thing like this.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:408: std::string
found_psl_domain = GetPSLDomain(new_form->signon_realm);
More constiness, if possible

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/password_form_manager.cc:233: return
pending_credentials_.is_psl_origin_match;
Nit: Some people prefer to see these getter functions inlined in the .h file.
(I'm fine either way.)

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/password_form_manager.cc:393: if (form.ssl_valid
&&
Hmm, I am not sure I like either the old or the new logic. Shouldn't it be that
if any form has invalid SSL, that we don't do any autofilling?

[nyquist, 2013-06-07 22:51:09]

Addressed all comments.
palmer, isherman, aurimas: PTAL

https://codereview.chromium.org/15660018/diff/37001/chrome/android/java/res/l...
File chrome/android/java/res/layout/autofill_text.xml (right):

https://codereview.chromium.org/15660018/diff/37001/chrome/android/java/res/l...
chrome/android/java/res/layout/autofill_text.xml:16: <TextView
android:id="@+id/autofill_name"
On 2013/06/06 16:52:26, aurimas wrote:
> While we are at it lets rename these TextViews to something reasonable like
> Label and Sublabel.

Done.

https://codereview.chromium.org/15660018/diff/37001/chrome/android/java/res/l...
chrome/android/java/res/layout/autofill_text.xml:30: android:ellipsize="end"
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> Hmm, could long text strings cause user confusion?

Yes, I believe they would. Do you have any suggestions for how to remedy that?

https://codereview.chromium.org/15660018/diff/37001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java
(right):

https://codereview.chromium.org/15660018/diff/37001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java:43:
labelView.setVisibility(View.GONE);
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> Does this mean that if the |label| is empty, that the feature still works?
> Shouldn't we not use the feature of the informative label is invalid (empty
> might be one kind of invalid)?

This code is just the generic autofill adapter for Android. If something is
invalid, it should be stripped out earlier than this.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/about_flag...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/about_flag...
chrome/browser/about_flags.cc:1238: kOsAll,
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> Shouldn't this be Android-only?

Done.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
File chrome/browser/password_manager/login_database.cc (right):

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:52: std::string
GetPSLDomain(std::string signon_realm_str) {
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> > Please spell out acronyms like PSL; or better yet, find a briefer
alternative
> > that is still clear to someone unfamiliar with the acronym.
> 
> |GetPublicDomainSuffix|? |GetPublicSuffix|?

Is |GetRegistryControlledDomain| OK? Since we technically get more than just the
suffix.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:53: GURL
signon_realm(signon_realm_str);
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> Declare this const if the callee takes a const &.

Done.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:378: std::string sql_query =
"SELECT origin_url, action_url, "
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> I think you can declare these strings const? I'm big on that because it's a
nice
> guarantee.

Done.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:387: //
s.Assign(db_.GetCachedStatement(SQL_FROM_HERE, sql_query.c_str()));
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> Perhaps you still can? Have you tried it? It should be possible to cache the
> statement and then bind new arguments in without invalidating the cache. I
> think.

Will try again before I submit this. I agree that it should work, but it didn't
for some very odd reason that I don't understand.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:396: std::string regexp =
"(\\w+?:\\/\\/)([\\w\\-_]+\\.)*" + domain + "\\/$";
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> Can we instead use an explicit list of schemata, instead of \w+ ? There are
some
> new and exciting schemata like googlesearch and so on that I'd like to ensure
> never get involved in this. Not that I have any specific fears, just that
> explicitness is more safe in a thing like this.

Done. Also added ^ at start of query. Still keeping any port though, since we
are regardless ignoring invalid SSL things later.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:408: std::string
found_psl_domain = GetPSLDomain(new_form->signon_realm);
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> More constiness, if possible

Done.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/password_form_manager.cc:233: return
pending_credentials_.is_psl_origin_match;
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> Nit: Some people prefer to see these getter functions inlined in the .h file.
> (I'm fine either way.)

Done.

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/password_form_manager.cc:393: if (form.ssl_valid
&&
On 2013/06/06 21:16:42, Chromium Palmer wrote:
> Hmm, I am not sure I like either the old or the new logic. Shouldn't it be
that
> if any form has invalid SSL, that we don't do any autofilling?

I've gone back to the old way which never autofills anything where the source is
HTTPS and the destination form is HTTP. Would it be OK to keep it like that?

https://codereview.chromium.org/15660018/diff/37001/chrome/browser/password_m...
chrome/browser/password_manager/password_form_manager.cc:544: // Partial match
gives an addition of 16 (1 << 5) + # matching url dirs
On 2013/06/06 09:25:35, Ilya Sherman wrote:
> 16 -> 32?

Done.

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.cc:158: element_bounds_,
suggestions, origins, empty, password_ids, GetWeakPtr());
On 2013/06/06 09:25:35, Ilya Sherman wrote:
> Hmm.  Are the origins really just surfaced as labels in the UI?  That seems
like
> a relatively strange UI to me.

Yes, that is the approved UI.
Also changed to |realms| here.

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.h:85: const
std::vector<base::string16>& origins,
On 2013/06/06 09:25:35, Ilya Sherman wrote:
> origins or realms?

Done.

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:763: bounds);
On 2013/06/06 09:25:35, Ilya Sherman wrote:
> nit: Needs curlies now.

Done.

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
File components/autofill/browser/autofill_manager.h (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
components/autofill/browser/autofill_manager.h:256: const
std::vector<base::string16>& origins);
On 2013/06/06 09:25:35, Ilya Sherman wrote:
> origins or realms?

Done.

https://codereview.chromium.org/15660018/diff/37001/components/autofill/commo...
File components/autofill/common/password_form_fill_data.h (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/commo...
components/autofill/common/password_form_fill_data.h:35: //                     
    for the form.
On 2013/06/06 09:25:35, Ilya Sherman wrote:
> I don't quite understand:  Are these supposed to be iterated over in
lock-step? 
> It seems better to just keep a single collection, but change the type of the
map
> value to be a struct that contains both the password and the realm.

Done.

https://codereview.chromium.org/15660018/diff/37001/components/autofill/rende...
File components/autofill/renderer/autofill_agent.h (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/rende...
components/autofill/renderer/autofill_agent.h:81: InputEventSource
input_event_source) OVERRIDE;
On 2013/06/06 09:25:35, Ilya Sherman wrote:
> This change seems completely unrelated to PSL domain matching.  Please split
it
> off to a separate CL.

Done. See https://codereview.chromium.org/16049009/

https://codereview.chromium.org/15660018/diff/37001/components/autofill/rende...
File components/autofill/renderer/page_click_tracker.cc (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/rende...
components/autofill/renderer/page_click_tracker.cc:116: TAP_GESTURE);
On 2013/06/06 09:25:35, Ilya Sherman wrote:
> Please refactor the code so that the common parts (i.e. nearly everything)
> between this method and DidHandleMouseEvent() are shared.

Done in the other CL.

https://codereview.chromium.org/15660018/diff/37001/content/public/common/pas...
File content/public/common/password_form.h (right):

https://codereview.chromium.org/15660018/diff/37001/content/public/common/pas...
content/public/common/password_form.h:175: bool is_psl_origin_match;
On 2013/06/06 09:25:35, Ilya Sherman wrote:
> If you were to just always set both signon_realm and original_signon_realm,
> would this field then become redundant?

We need a flag to separate where this result came from, so either we have to set
original_signon_realm to something magic (NULL or empty?) and use that as an
identifyer instead. Probably we could then have a helper method defined for
PasswordForm, such as PasswordForm::IsPSLOriginMatch() (or a better name like
you mentioned earlier).

[aurimas (slooooooooow), 2013-06-07 23:24:29]

https://codereview.chromium.org/15660018/diff/65001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java
(right):

https://codereview.chromium.org/15660018/diff/65001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java:41:
CharSequence label = getItem(position).mLabel;
Could you also changed this to mSubLabel and the one above from mName to mLabel?

[Ilya Sherman, 2013-06-07 23:47:56]

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/15660018/diff/37001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.cc:158: element_bounds_,
suggestions, origins, empty, password_ids, GetWeakPtr());
On 2013/06/07 22:51:10, nyquist wrote:
> On 2013/06/06 09:25:35, Ilya Sherman wrote:
> > Hmm.  Are the origins really just surfaced as labels in the UI?  That seems
> like
> > a relatively strange UI to me.
> 
> Yes, that is the approved UI.
> Also changed to |realms| here.

Hrm, ok.  Approved by the Chromium UI leads?

https://codereview.chromium.org/15660018/diff/37001/content/public/common/pas...
File content/public/common/password_form.h (right):

https://codereview.chromium.org/15660018/diff/37001/content/public/common/pas...
content/public/common/password_form.h:175: bool is_psl_origin_match;
On 2013/06/07 22:51:10, nyquist wrote:
> On 2013/06/06 09:25:35, Ilya Sherman wrote:
> > If you were to just always set both signon_realm and original_signon_realm,
> > would this field then become redundant?
> 
> We need a flag to separate where this result came from, so either we have to
set
> original_signon_realm to something magic (NULL or empty?) and use that as an
> identifyer instead. Probably we could then have a helper method defined for
> PasswordForm, such as PasswordForm::IsPSLOriginMatch() (or a better name like
> you mentioned earlier).

Could IsPSLOriginMatch() just return true iff signon_realm !=
original_signon_realm?  Or am I misunderstanding what the variables mean?

https://codereview.chromium.org/15660018/diff/65001/chrome/browser/password_m...
File chrome/browser/password_manager/password_form_manager.h (right):

https://codereview.chromium.org/15660018/diff/65001/chrome/browser/password_m...
chrome/browser/password_manager/password_form_manager.h:84: bool
IsPSLOriginMatched() {
nit: Please expand "PSL" to something that's clear to folks unfamiliar with the
acronym.

https://codereview.chromium.org/15660018/diff/65001/chrome/browser/password_m...
chrome/browser/password_manager/password_form_manager.h:86: }
nit: I've pretty much only ever seen hacker_case methods inlined.

https://codereview.chromium.org/15660018/diff/65001/components/autofill/commo...
File components/autofill/common/password_form_fill_data.cc (right):

https://codereview.chromium.org/15660018/diff/65001/components/autofill/commo...
components/autofill/common/password_form_fill_data.cc:71: }
nit: No longer a need for curlies.

https://codereview.chromium.org/15660018/diff/65001/content/public/common/pas...
File content/public/common/password_form.h (right):

https://codereview.chromium.org/15660018/diff/65001/content/public/common/pas...
content/public/common/password_form.h:61: // PSL.
This comment still leaves me somewhat uncertain as to when this field is set and
how exactly it differs from signon_realm.  IMO it would be very helpful to
include some motivating examples in the comment, and also to use slightly
simpler (or less technical/jargon-y) language in the explanation.  (That said,
it might be wise to keep the very technical language as well, for the sake of
precision.)

https://codereview.chromium.org/15660018/diff/65001/content/public/common/pas...
content/public/common/password_form.h:61: // PSL.
nit: Even in comments, please expand out acronyms like "PSL".  Think about
someone reading this code a year from now, who is completely new to the password
autofill code.  Try to phrase things in such a way that the prose will be clear
to such a reader :)

[nyquist, 2013-06-11 03:36:01]

adressed all comments again

https://chromiumcodereview.appspot.com/15660018/diff/37001/components/autofil...
File components/autofill/browser/autofill_external_delegate.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/37001/components/autofil...
components/autofill/browser/autofill_external_delegate.cc:158: element_bounds_,
suggestions, origins, empty, password_ids, GetWeakPtr());
On 2013/06/07 23:47:56, Ilya Sherman wrote:
> On 2013/06/07 22:51:10, nyquist wrote:
> > On 2013/06/06 09:25:35, Ilya Sherman wrote:
> > > Hmm.  Are the origins really just surfaced as labels in the UI?  That
seems
> > like
> > > a relatively strange UI to me.
> > 
> > Yes, that is the approved UI.
> > Also changed to |realms| here.
> 
> Hrm, ok.  Approved by the Chromium UI leads?

It has been approved for now by UI, but it has not gone through formal UI review
yet.

https://chromiumcodereview.appspot.com/15660018/diff/37001/content/public/com...
File content/public/common/password_form.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/37001/content/public/com...
content/public/common/password_form.h:175: bool is_psl_origin_match;
On 2013/06/07 23:47:56, Ilya Sherman wrote:
> On 2013/06/07 22:51:10, nyquist wrote:
> > On 2013/06/06 09:25:35, Ilya Sherman wrote:
> > > If you were to just always set both signon_realm and
original_signon_realm,
> > > would this field then become redundant?
> > 
> > We need a flag to separate where this result came from, so either we have to
> set
> > original_signon_realm to something magic (NULL or empty?) and use that as an
> > identifyer instead. Probably we could then have a helper method defined for
> > PasswordForm, such as PasswordForm::IsPSLOriginMatch() (or a better name
like
> > you mentioned earlier).
> 
> Could IsPSLOriginMatch() just return true iff signon_realm !=
> original_signon_realm?  Or am I misunderstanding what the variables mean?

Removed this field, and instead checking for |original_signon_realm| to be
non-empty.

https://chromiumcodereview.appspot.com/15660018/diff/65001/chrome/android/jav...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java
(right):

https://chromiumcodereview.appspot.com/15660018/diff/65001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillListAdapter.java:41:
CharSequence label = getItem(position).mLabel;
On 2013/06/07 23:24:29, aurimas wrote:
> Could you also changed this to mSubLabel and the one above from mName to
mLabel?

Done.

https://chromiumcodereview.appspot.com/15660018/diff/65001/chrome/browser/pas...
File chrome/browser/password_manager/password_form_manager.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/65001/chrome/browser/pas...
chrome/browser/password_manager/password_form_manager.h:84: bool
IsPSLOriginMatched() {
On 2013/06/07 23:47:56, Ilya Sherman wrote:
> nit: Please expand "PSL" to something that's clear to folks unfamiliar with
the
> acronym.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/65001/chrome/browser/pas...
chrome/browser/password_manager/password_form_manager.h:86: }
On 2013/06/07 23:47:56, Ilya Sherman wrote:
> nit: I've pretty much only ever seen hacker_case methods inlined.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/65001/components/autofil...
File components/autofill/common/password_form_fill_data.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/65001/components/autofil...
components/autofill/common/password_form_fill_data.cc:71: }
On 2013/06/07 23:47:56, Ilya Sherman wrote:
> nit: No longer a need for curlies.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/65001/content/public/com...
File content/public/common/password_form.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/65001/content/public/com...
content/public/common/password_form.h:61: // PSL.
On 2013/06/07 23:47:56, Ilya Sherman wrote:
> This comment still leaves me somewhat uncertain as to when this field is set
and
> how exactly it differs from signon_realm.  IMO it would be very helpful to
> include some motivating examples in the comment, and also to use slightly
> simpler (or less technical/jargon-y) language in the explanation.  (That said,
> it might be wise to keep the very technical language as well, for the sake of
> precision.)

Updated the comment. I hope it is clearer now.

[palmer, 2013-06-11 18:55:45]

https://codereview.chromium.org/15660018/diff/89001/chrome/browser/password_m...
File chrome/browser/password_manager/login_database.cc (right):

https://codereview.chromium.org/15660018/diff/89001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:389: // We need to escape ., -
and _ in the domain. Since the domain has already
The comment doesn't match the code --- you don't escape _. I don't know which is
right.

https://codereview.chromium.org/15660018/diff/89001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:398:
"^((http|https):\\/\\/)([\\w\\-_]+\\.)*" + domain + "(:\\d+)?\\/$";
It doesn't feel right to match loosely on the port, and less so on the scheme.
Where are we on matching schemes exactly?

https://codereview.chromium.org/15660018/diff/89001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate_unittest.cc (right):

https://codereview.chromium.org/15660018/diff/89001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate_unittest.cc:270: static
const base::string16 kSignonRealm = ASCIIToUTF16("signon_realm");
Is this the file to test specific fuzzy matches that should and should not
match? I'd really like to see some tests for matches that should (functionality)
and should not (security) work.

https://codereview.chromium.org/15660018/diff/89001/content/public/common/pas...
File content/public/common/password_form.h (right):

https://codereview.chromium.org/15660018/diff/89001/content/public/common/pas...
content/public/common/password_form.h:62: // For example, if you have a password
for http://www.foo.bar (where .bar is
Document here what our scheme-matching (HTTP vs. HTTPS) policy is, and why.

My strong desire is still to never autofill from HTTPS forms into HTTP. I'd like
to have that documented and tested.

[nyquist, 2013-06-11 23:54:42]

PTAL.

Added more tests, and adressed all comments.

https://codereview.chromium.org/15660018/diff/89001/chrome/browser/password_m...
File chrome/browser/password_manager/login_database.cc (right):

https://codereview.chromium.org/15660018/diff/89001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:389: // We need to escape ., -
and _ in the domain. Since the domain has already
On 2013/06/11 18:55:45, Chromium Palmer wrote:
> The comment doesn't match the code --- you don't escape _. I don't know which
is
> right.

I don't think _ has to be escaped in regexp. Updated comment.

https://codereview.chromium.org/15660018/diff/89001/chrome/browser/password_m...
chrome/browser/password_manager/login_database.cc:398:
"^((http|https):\\/\\/)([\\w\\-_]+\\.)*" + domain + "(:\\d+)?\\/$";
On 2013/06/11 18:55:45, Chromium Palmer wrote:
> It doesn't feel right to match loosely on the port, and less so on the scheme.
> Where are we on matching schemes exactly?

Made this more strict. Now both scheme and port have to match.

https://codereview.chromium.org/15660018/diff/89001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate_unittest.cc (right):

https://codereview.chromium.org/15660018/diff/89001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate_unittest.cc:270: static
const base::string16 kSignonRealm = ASCIIToUTF16("signon_realm");
On 2013/06/11 18:55:45, Chromium Palmer wrote:
> Is this the file to test specific fuzzy matches that should and should not
> match? I'd really like to see some tests for matches that should
(functionality)
> and should not (security) work.

I have now added these tests at the lowest level, namely in
login_database_unittest.cc

https://codereview.chromium.org/15660018/diff/89001/content/public/common/pas...
File content/public/common/password_form.h (right):

https://codereview.chromium.org/15660018/diff/89001/content/public/common/pas...
content/public/common/password_form.h:62: // For example, if you have a password
for http://www.foo.bar (where .bar is
On 2013/06/11 18:55:45, Chromium Palmer wrote:
> Document here what our scheme-matching (HTTP vs. HTTPS) policy is, and why.
> 
> My strong desire is still to never autofill from HTTPS forms into HTTP. I'd
like
> to have that documented and tested.

Done.

[aurimas (slooooooooow), 2013-06-13 01:27:28]

https://chromiumcodereview.appspot.com/15660018/diff/119004/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopup.java
(right):

https://chromiumcodereview.appspot.com/15660018/diff/119004/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopup.java:161:
private float getDesiredWidth(AutofillSuggestion[] data) {
You have to change this method because now that views are not side by side as
they used to be. The desired width is smaller than before your change.

[palmer, 2013-06-15 00:03:26]

https://codereview.chromium.org/15660018/diff/140001/chrome/browser/password_...
File chrome/browser/password_manager/login_database_unittest.cc (right):

https://codereview.chromium.org/15660018/diff/140001/chrome/browser/password_...
chrome/browser/password_manager/login_database_unittest.cc:385: //
http://foo.com should match since the scheme is wrong.
Typo: "should not match"

[nyquist, 2013-06-17 23:20:10]

Addressed all comments again. PTAL.

https://chromiumcodereview.appspot.com/15660018/diff/119004/chrome/android/ja...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopup.java
(right):

https://chromiumcodereview.appspot.com/15660018/diff/119004/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopup.java:161:
private float getDesiredWidth(AutofillSuggestion[] data) {
On 2013/06/13 01:27:28, aurimas wrote:
> You have to change this method because now that views are not side by side as
> they used to be. The desired width is smaller than before your change.

Removed all of this from my CL. Depending on your CL at
https://codereview.chromium.org/16994006/ instead.

https://chromiumcodereview.appspot.com/15660018/diff/140001/chrome/browser/pa...
File chrome/browser/password_manager/login_database_unittest.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/140001/chrome/browser/pa...
chrome/browser/password_manager/login_database_unittest.cc:385: //
http://foo.com should match since the scheme is wrong.
On 2013/06/15 00:03:27, Chromium Palmer wrote:
> Typo: "should not match"

Done.

[palmer, 2013-06-18 00:36:15]

lgtm

[Ilya Sherman, 2013-06-19 09:40:53]

Thanks, this is looking much nicer :)

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
File chrome/browser/password_manager/login_database.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
chrome/browser/password_manager/login_database.h:119: bool
public_suffix_domain_matching_;
Doesn't look like this is initialized in the constructor for the class -- please
double check that it is, or add initialization code for it.

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
chrome/browser/password_manager/password_form_manager.cc:157: return
!pending_credentials_.original_signon_realm.empty();
nit: Might be nice to add a method "IsPublicSuffixMatch" to
content::PasswordForm, rather than requiring clients of the class to map that to
original_signon_realm being empty.  My request previously was mostly just to
drop the redundant variable, not necessarily the convenience method :)

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
chrome/browser/password_manager/password_form_manager.cc:199: is_new_login_ =
!pending_credentials_.original_signon_realm.empty();
nit: You might as well re-use IsPendingCredentialsPublicSuffixMatch() here.

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
File chrome/browser/password_manager/password_form_manager.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
chrome/browser/password_manager/password_form_manager.h:85: bool
IsPendingCredentialsPublicSuffixMatch();
Optional nit: IMO this name would be clearer as
"PendingCredentialsIsPublicSuffixMatch()", though I do admit that it's nice to
follow the pattern of having boolean methods start with "Is".

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
File chrome/browser/password_manager/password_manager.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
chrome/browser/password_manager/password_manager.cc:307:
!provisional_save_manager_->IsPendingCredentialsPublicSuffixMatch()) {
nit: Could this logic be pushed into the provisional_save_manager_'s API?  This
if-stmt's condition is getting a bit involved.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/browser/autofill_external_delegate_unittest.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/browser/autofill_external_delegate_unittest.cc:303: static
const base::string16 kSignonRealm = ASCIIToUTF16("signon_realm");
nit: Perhaps use a real example of a signon realm?

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/common/autofill_messages.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/autofill_messages.h:307: std::vector<base::string16>
/* suggestions realms */)
nit: Though not all of the code in this file does so, I believe the comments are
supposed to be variable names, so this should be just "realms" rather than
"suggestions realms"

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/common/password_form_fill_data.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data.cc:56:
result->preferred_realm = ASCIIToUTF16(preferred_match->signon_realm);
How do you know that the signon_realm is ASCII?

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data.cc:60: }
nit: Might be nice to decompose a helper function GetPreferredSignonRealm, which
you could use here and on lines 68-71.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/common/password_form_fill_data.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data.h:30: base::string16 realm;
nit: Why is this a string16 rather than a GURL, or at least an std::string? 
Seems like that conversion doesn't need to happen until it reaches the browser
side.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data.h:38: //                  
pairs for the form.
nit: Would be nice to move these comments to be right above the fields they
describe, rather than above the whole struct.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/common/password_form_fill_data_unittest.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
nit: 2013, and please omit the "(c)".

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:6: #include
"components/autofill/common/password_form_fill_data.h"
nit: This include should precede all the others, just as the main header include
for an implementation file.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:11: using
autofill::PasswordFormFillData;
nit: Please wrap all the test code in the autofill:: namespace, and omit this
line.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:15: class
PasswordFormFillDataTest : public testing::Test {};
nit: You can omit this line, and just replace all the TEST_F's below with TEST
instead.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:17:
TEST_F(PasswordFormFillDataTest, TestSinglePreferredMatch) {
nit: Please add a comment describing what this is testing.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:55:
EXPECT_TRUE(result.wait_for_username);
Why is wait_for_username true for an exact match?

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:60:
TEST_F(PasswordFormFillDataTest, TestPublicSuffixDomainMatching) {
nit: Please add a comment describing what this is testing.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/content/renderer/page_click_tracker.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/content/renderer/page_click_tracker.h:11: #include
"components/autofill/content/renderer/page_click_listener.h"
nit: Not needed?

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/content/renderer/password_autofill_agent.cc:7: #include
<vector>
nit: Doesn't the header file already #include this?

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/content/renderer/password_autofill_agent.cc:538:
std::vector<base::string16> suggestions_realms;
nit: IMO "realms" is actually a slightly clearer variable name.

https://chromiumcodereview.appspot.com/15660018/diff/148001/content/public/co...
File content/public/common/password_form.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/content/public/co...
content/public/common/password_form.h:66: // public suffix.
Optional nit: Would login.m.foo.bar also resolve to the same registry controlled
domain?  Might be nice to clarify here; or leave it omitted if you think that
would actually just make things more confusing / hard to follow.

https://chromiumcodereview.appspot.com/15660018/diff/148001/content/public/co...
content/public/common/password_form.h:69: // For example, if you have a password
for http://www.foo.bar (where .bar is
nit: Prefer to avoid pronouns.  In this case, perhaps replace "you have a
password" to "Chrome has a saved password", "the login database has a saved
password", or something like that.

[nyquist, 2013-06-19 22:56:06]

isherman: PTAL

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
File chrome/browser/password_manager/login_database.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
chrome/browser/password_manager/login_database.h:119: bool
public_suffix_domain_matching_;
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> Doesn't look like this is initialized in the constructor for the class --
please
> double check that it is, or add initialization code for it.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
File chrome/browser/password_manager/password_form_manager.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
chrome/browser/password_manager/password_form_manager.cc:157: return
!pending_credentials_.original_signon_realm.empty();
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Might be nice to add a method "IsPublicSuffixMatch" to
> content::PasswordForm, rather than requiring clients of the class to map that
to
> original_signon_realm being empty.  My request previously was mostly just to
> drop the redundant variable, not necessarily the convenience method :)

Done. Mapped to original_signon_realm NOT being empty :-)

I didn't add it there earlier because I thought it was frowned upon to add
methods to structs in chromium. I am happy to do the change, though, as it is
much, much easier to read.

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
chrome/browser/password_manager/password_form_manager.cc:199: is_new_login_ =
!pending_credentials_.original_signon_realm.empty();
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: You might as well re-use IsPendingCredentialsPublicSuffixMatch() here.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
File chrome/browser/password_manager/password_form_manager.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
chrome/browser/password_manager/password_form_manager.h:85: bool
IsPendingCredentialsPublicSuffixMatch();
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> Optional nit: IMO this name would be clearer as
> "PendingCredentialsIsPublicSuffixMatch()", though I do admit that it's nice to
> follow the pattern of having boolean methods start with "Is".

Staying with Is* pattern for now to follow the rest of the file.

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
File chrome/browser/password_manager/password_manager.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/chrome/browser/pa...
chrome/browser/password_manager/password_manager.cc:307:
!provisional_save_manager_->IsPendingCredentialsPublicSuffixMatch()) {
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Could this logic be pushed into the provisional_save_manager_'s API? 
This
> if-stmt's condition is getting a bit involved.

Kept it here for now, but extracted it as the method
ShouldShowSavePasswordInfoBar() to make it much easier to read the code.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/browser/autofill_external_delegate_unittest.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/browser/autofill_external_delegate_unittest.cc:303: static
const base::string16 kSignonRealm = ASCIIToUTF16("signon_realm");
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Perhaps use a real example of a signon realm?

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/common/autofill_messages.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/autofill_messages.h:307: std::vector<base::string16>
/* suggestions realms */)
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Though not all of the code in this file does so, I believe the comments
are
> supposed to be variable names, so this should be just "realms" rather than
> "suggestions realms"

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/common/password_form_fill_data.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data.cc:56:
result->preferred_realm = ASCIIToUTF16(preferred_match->signon_realm);
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> How do you know that the signon_realm is ASCII?

Changed to use UTF8->UTF16 instead.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data.cc:60: }
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Might be nice to decompose a helper function GetPreferredSignonRealm,
which
> you could use here and on lines 68-71.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/common/password_form_fill_data.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data.h:30: base::string16 realm;
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Why is this a string16 rather than a GURL, or at least an std::string? 
> Seems like that conversion doesn't need to happen until it reaches the browser
> side.

Changed to use std::string for as long as possible. Using std::string instead of
GURL since the realm of PasswordForm also is std::string for consistency.
Also changed for the preferred_realm field.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data.h:38: //                  
pairs for the form.
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Would be nice to move these comments to be right above the fields they
> describe, rather than above the whole struct.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/common/password_form_fill_data_unittest.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: 2013, and please omit the "(c)".

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:6: #include
"components/autofill/common/password_form_fill_data.h"
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: This include should precede all the others, just as the main header
include
> for an implementation file.

Oops. Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:11: using
autofill::PasswordFormFillData;
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Please wrap all the test code in the autofill:: namespace, and omit this
> line.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:15: class
PasswordFormFillDataTest : public testing::Test {};
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: You can omit this line, and just replace all the TEST_F's below with TEST
> instead.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:17:
TEST_F(PasswordFormFillDataTest, TestSinglePreferredMatch) {
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Please add a comment describing what this is testing.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:55:
EXPECT_TRUE(result.wait_for_username);
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> Why is wait_for_username true for an exact match?

Added a comment about this and also added a test of passing in false.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:60:
TEST_F(PasswordFormFillDataTest, TestPublicSuffixDomainMatching) {
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Please add a comment describing what this is testing.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/content/renderer/page_click_tracker.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/content/renderer/page_click_tracker.h:11: #include
"components/autofill/content/renderer/page_click_listener.h"
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Not needed?

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/content/renderer/password_autofill_agent.cc:7: #include
<vector>
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Doesn't the header file already #include this?

Yes. I was just trying to follow the include-what-you-use rule. Removed this
now.

https://chromiumcodereview.appspot.com/15660018/diff/148001/components/autofi...
components/autofill/content/renderer/password_autofill_agent.cc:538:
std::vector<base::string16> suggestions_realms;
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: IMO "realms" is actually a slightly clearer variable name.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/content/public/co...
File content/public/common/password_form.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/148001/content/public/co...
content/public/common/password_form.h:66: // public suffix.
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> Optional nit: Would login.m.foo.bar also resolve to the same registry
controlled
> domain?  Might be nice to clarify here; or leave it omitted if you think that
> would actually just make things more confusing / hard to follow.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/148001/content/public/co...
content/public/common/password_form.h:69: // For example, if you have a password
for http://www.foo.bar (where .bar is
On 2013/06/19 09:40:54, Ilya Sherman wrote:
> nit: Prefer to avoid pronouns.  In this case, perhaps replace "you have a
> password" to "Chrome has a saved password", "the login database has a saved
> password", or something like that.

Done.

[Ilya Sherman, 2013-06-19 23:41:13]

Thanks!  LGTM for everything other than the login_database changes, which I'm
not familiar enough with to review.  Hopefully Mike can chime in.

https://chromiumcodereview.appspot.com/15660018/diff/161001/components/autofi...
File components/autofill/common/password_form_fill_data.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/161001/components/autofi...
components/autofill/common/password_form_fill_data.cc:11: namespace {
nit: Please leave a blank line after this one.

https://chromiumcodereview.appspot.com/15660018/diff/161001/components/autofi...
components/autofill/common/password_form_fill_data.cc:12: std::string
GetPreferredSignonRealm(const content::PasswordForm* form) {
nit: Can this be passed by const-reference?

https://chromiumcodereview.appspot.com/15660018/diff/161001/components/autofi...
File components/autofill/common/password_form_fill_data_unittest.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/161001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:19: // matches,
the PasswordFormFillData is filled in correctly.
Optional nit: I've generally seen such comments go immediately above the TEST
macro, rather than within it.

[Mike Mammarella, 2013-06-20 20:20:22]

Sorry for the extreme delay! I don't really work on this actively any more, so I
don't check my mail as often as I should.

https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
File chrome/browser/password_manager/login_database.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
chrome/browser/password_manager/login_database.cc:413: sql_query + "OR
signon_realm REGEXP ? ";
I might include a comment here that notes that the regex is merely an
optimization, and that we'll double check the results below. Otherwise it looks
like we're counting on the regex to actually match exactly the right things for
correctness, which would be somewhat scary given how many \ are in the regex. :)

https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
chrome/browser/password_manager/login_database.cc:419: // We need to escape .
and - in the domain. Since the domain has already
Why do you need to escape - ? Does it have some special meaning to SQLite
regexes?

https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
chrome/browser/password_manager/login_database.cc:434: std::string regexp = "^("
+ scheme + ":\\/\\/)([\\w\\-_]+\\.)*" +
_ is not actually a valid character for DNS names, so presumably it can never
occur in a valid HTTP or HTTPS URL. Do we want to include it for other schemes
that might allow it in the realm? I'm sort of inclined to say we should leave it
off, unless you have an example of where it would be needed.

https://chromiumcodereview.appspot.com/15660018/diff/164001/content/public/co...
File content/public/common/password_form.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/164001/content/public/co...
content/public/common/password_form.h:64: // 2) The registry controlled domain
is the same. For example; foo.bar,
These examples are a little unclear because the reader has to assume that "bar"
is a public suffix, so foo.bar is owned by foo, inc. or something. Could you
either explicitly state that, or use a more standard example like example.com
(or maybe something with a longer public suffix, like .co.uk or something)?

[nyquist, 2013-06-21 21:01:27]

Should be all done now.
Please take a final look.

https://chromiumcodereview.appspot.com/15660018/diff/161001/components/autofi...
File components/autofill/common/password_form_fill_data.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/161001/components/autofi...
components/autofill/common/password_form_fill_data.cc:11: namespace {
On 2013/06/19 23:41:14, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/161001/components/autofi...
components/autofill/common/password_form_fill_data.cc:12: std::string
GetPreferredSignonRealm(const content::PasswordForm* form) {
On 2013/06/19 23:41:14, Ilya Sherman wrote:
> nit: Can this be passed by const-reference?

Done.

https://chromiumcodereview.appspot.com/15660018/diff/161001/components/autofi...
File components/autofill/common/password_form_fill_data_unittest.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/161001/components/autofi...
components/autofill/common/password_form_fill_data_unittest.cc:19: // matches,
the PasswordFormFillData is filled in correctly.
On 2013/06/19 23:41:14, Ilya Sherman wrote:
> Optional nit: I've generally seen such comments go immediately above the TEST
> macro, rather than within it.

Done.

https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
File chrome/browser/password_manager/login_database.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
chrome/browser/password_manager/login_database.cc:413: sql_query + "OR
signon_realm REGEXP ? ";
On 2013/06/20 20:20:23, Mike Mammarella wrote:
> I might include a comment here that notes that the regex is merely an
> optimization, and that we'll double check the results below. Otherwise it
looks
> like we're counting on the regex to actually match exactly the right things
for
> correctness, which would be somewhat scary given how many \ are in the regex.
:)

Done.

https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
chrome/browser/password_manager/login_database.cc:419: // We need to escape .
and - in the domain. Since the domain has already
On 2013/06/20 20:20:23, Mike Mammarella wrote:
> Why do you need to escape - ? Does it have some special meaning to SQLite
> regexes?

The - is used when using things like A-Z. However, since there will presumably
be no [ or ] in the URL we receive, we can remove the escaping of -. I also
checked with an internal Google regular expression ninja which helped me out.

Also, added tests to verify we handle - correctly.

https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
chrome/browser/password_manager/login_database.cc:434: std::string regexp = "^("
+ scheme + ":\\/\\/)([\\w\\-_]+\\.)*" +
On 2013/06/20 20:20:23, Mike Mammarella wrote:
> _ is not actually a valid character for DNS names, so presumably it can never
> occur in a valid HTTP or HTTPS URL. Do we want to include it for other schemes
> that might allow it in the realm? I'm sort of inclined to say we should leave
it
> off, unless you have an example of where it would be needed.

Removed _. Since - is now the last character in the group which makes it not
need to be escaped, I removed the escaping of it. Please tell me if you want it
back.

https://chromiumcodereview.appspot.com/15660018/diff/164001/content/public/co...
File content/public/common/password_form.h (right):

https://chromiumcodereview.appspot.com/15660018/diff/164001/content/public/co...
content/public/common/password_form.h:64: // 2) The registry controlled domain
is the same. For example; foo.bar,
On 2013/06/20 20:20:23, Mike Mammarella wrote:
> These examples are a little unclear because the reader has to assume that
"bar"
> is a public suffix, so foo.bar is owned by foo, inc. or something. Could you
> either explicitly state that, or use a more standard example like
http://example.com
> (or maybe something with a longer public suffix, like .co.uk or something)?

Uses example.com now.

[Ilya Sherman, 2013-06-21 21:16:24]

https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
File chrome/browser/password_manager/login_database.cc (right):

https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
chrome/browser/password_manager/login_database.cc:419: // We need to escape .
and - in the domain. Since the domain has already
On 2013/06/21 21:01:27, nyquist wrote:
> On 2013/06/20 20:20:23, Mike Mammarella wrote:
> > Why do you need to escape - ? Does it have some special meaning to SQLite
> > regexes?
> 
> The - is used when using things like A-Z. However, since there will presumably
> be no [ or ] in the URL we receive, we can remove the escaping of -. I also
> checked with an internal Google regular expression ninja which helped me out.
> 
> Also, added tests to verify we handle - correctly.

Hmm, this "presumably" worries me.  Isn't the URL being sent over IPC as a
string from an untrusted process?  If so, we should assume that it can be
*anything* and design accordingly.

[commit-bot: I haz the power, 2013-06-21 21:27:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/15660018/188001

[nyquist, 2013-06-21 21:28:07]

On 2013/06/21 21:16:24, Ilya Sherman wrote:
>
https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
> File chrome/browser/password_manager/login_database.cc (right):
> 
>
https://chromiumcodereview.appspot.com/15660018/diff/164001/chrome/browser/pa...
> chrome/browser/password_manager/login_database.cc:419: // We need to escape .
> and - in the domain. Since the domain has already
> On 2013/06/21 21:01:27, nyquist wrote:
> > On 2013/06/20 20:20:23, Mike Mammarella wrote:
> > > Why do you need to escape - ? Does it have some special meaning to SQLite
> > > regexes?
> > 
> > The - is used when using things like A-Z. However, since there will
presumably
> > be no [ or ] in the URL we receive, we can remove the escaping of -. I also
> > checked with an internal Google regular expression ninja which helped me
out.
> > 
> > Also, added tests to verify we handle - correctly.
> 
> Hmm, this "presumably" worries me.  Isn't the URL being sent over IPC as a
> string from an untrusted process?  If so, we should assume that it can be
> *anything* and design accordingly.

Discussed offline. Regexp is only for optimization and trust lies in the SQLite
feature for binding and in GURL.

[commit-bot: I haz the power, 2013-06-21 22:13:52]

Failed to trigger a try job on linux_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-06-21 22:14:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/15660018/209001

[commit-bot: I haz the power, 2013-06-22 00:42:49]

Retried try job too often on win_rel for step(s) browser_tests,
chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests,
content_browsertests, installer_util_unittests, mini_installer_test,
nacl_integration, sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-22 06:51:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/15660018/209001

[commit-bot: I haz the power, 2013-06-22 13:48:44]

Change committed as 208022