RBS for investigation

vabr, can someone on the passwords team investigate?

After some debugging I figured out that http://jimblackler.net/FormTest/formTest.html was blacklisted in our shared testing syncing account, after unblacklisting saving works, at least in Chromium on iOS simulator. Could anyone check it independently?

Ad #2: sure, I was planning to have a look today, but wanted to leave the bug open until then in case I got sick and somebody else wanted to pick it up.

Ad #3: Interesting. I'm pretty sure I tried this with Chrome not being signed in, but will retry and investigate.

So I double-checked the stable version, and sadly saving does not work there, although the site is not blacklisted (and Chrome was not signed into any sync account).

Some good news and some open questions:

Good news: saving works, as long as the landing page is HTML. So, e.g., http://1.chromium-test1.appspot.com/testing/psl-matching/login works.

The autofill smoke test page has a text/plain landing page and for some reason Chrome hooks for rendered forms is not triggering there. So while Chrome saves the password provisionally, it never gets the confirmation that the login was successful ("there were 0 login forms displayed"), and hence does not save.

I'm not sure why this worked on dvadym@'s computer, will look further into this.

My Chromium version where saving on http://jimblackler.net/FormTest/formTest.html works is 50.0.2658.0, it's built from sources from  25-Feb-2016

This is working for normal sites, so removing the RBS and downgrading the priority.

I believe that it makes sense to offer saving passwords on text/plain landing pages. While there is a chance that such a page is an error message (and hence likely not a successful login), it seems neither risky to offer saving there, nor increasing user annoyance (which is caused by the error itself in that case, and hopefully does not happen regularly).

So my next step is to figure out how the password manager could tell that the landing page is text/plain and save (or figure out what's wrong with my set-up, if dvadym@'s current ToT build proves to save things).

dvadym@ just made me aware of the exact place where Chrome on iOS refuses to handle text/plain pages: webStateDidLoadPage:(web::WebState*)webState in password_controller.mm. I am going to change that to regard text/plain as a successful login (for reasons in #8 and to match the desktop behaviour).

Also, we clarified why dvadym@ could not reproduce the issues I saw: it appears that he was using a slightly different test page than the autofill smoke test, and that one had a HTML landing page.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0903eb6c26873d6d5e37ff0507dd695d54e25d3

commit a0903eb6c26873d6d5e37ff0507dd695d54e25d3
Author: vabr <vabr@chromium.org>
Date: Fri Mar 18 13:21:11 2016

Password manager on iOS should understand non-HTML landing pages

Currently, password manager on iOS ignores all pages which are not HTML.
However, if a password form has a, e.g., text/plain landing page, this will
prevent password manager from saving a password on that form. This is
different from the desktop & Android behaviour.

This CL changes password manager on iOS, so that it considers all types of
landing pages, but unless these are HTML, does not try to find password forms
on them.

The test for the changed code is still downstream, and hence not in this CL.

BUG= 595717 

Review URL: https://codereview.chromium.org/1806333005

Cr-Commit-Position: refs/heads/master@{#381949}

[modify] https://crrev.com/a0903eb6c26873d6d5e37ff0507dd695d54e25d3/ios/chrome/browser/passwords/password_controller.mm

The test is in progress (WIP: https://codereview.chromium.org/1808853005). In the end it looks like it will be done upstream, but needs some more work. Will continue next week.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86b472697e7f5871d75963d32266332b1a95a242

commit 86b472697e7f5871d75963d32266332b1a95a242
Author: vabr <vabr@chromium.org>
Date: Wed Mar 30 09:49:02 2016

Add a unittest for PasswordController

While PasswordController already has a unittest downstream, that one is rather
an integration test (the heaviness of which is the reason it is still
downstream, stuck on some downstream-only testing framework).

When a test was needed for https://codereview.chromium.org/1806333005/, it
turned out a simple unittest would be better. This CL adds that test and
places it upstream. (Ultimately we should convert the downstream tests to a
lighter version and add them here as well, but not in this CL.)

BUG= 595717 , 598672

Review URL: https://codereview.chromium.org/1808853005

Cr-Commit-Position: refs/heads/master@{#383941}

[modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/BUILD.gn
[modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller.h
[modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller.mm
[add] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller_unittest.mm
[modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/ios_chrome_tests.gyp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86b472697e7f5871d75963d32266332b1a95a242

commit 86b472697e7f5871d75963d32266332b1a95a242
Author: vabr <vabr@chromium.org>
Date: Wed Mar 30 09:49:02 2016

Add a unittest for PasswordController

While PasswordController already has a unittest downstream, that one is rather
an integration test (the heaviness of which is the reason it is still
downstream, stuck on some downstream-only testing framework).

When a test was needed for https://codereview.chromium.org/1806333005/, it
turned out a simple unittest would be better. This CL adds that test and
places it upstream. (Ultimately we should convert the downstream tests to a
lighter version and add them here as well, but not in this CL.)

BUG= 595717 , 598672

Review URL: https://codereview.chromium.org/1808853005

Cr-Commit-Position: refs/heads/master@{#383941}

[modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/BUILD.gn
[modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller.h
[modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller.mm
[add] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller_unittest.mm
[modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/ios_chrome_tests.gyp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aaeb70a8b015d3b88fd34731efa8ad499e760c05

commit aaeb70a8b015d3b88fd34731efa8ad499e760c05
Author: vabr <vabr@chromium.org>
Date: Thu Mar 31 08:50:06 2016

Revert of Add a unittest for PasswordController (patchset #5 id:80001 of https://codereview.chromium.org/1808853005/ )

Reason for revert:
Reporter breakage, vabr@ will investigate.

Original issue's description:
> Add a unittest for PasswordController
>
> While PasswordController already has a unittest downstream, that one is rather
> an integration test (the heaviness of which is the reason it is still
> downstream, stuck on some downstream-only testing framework).
>
> When a test was needed for https://codereview.chromium.org/1806333005/, it
> turned out a simple unittest would be better. This CL adds that test and
> places it upstream. (Ultimately we should convert the downstream tests to a
> lighter version and add them here as well, but not in this CL.)
>
> BUG= 595717 , 598672
>
> Committed: https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242
> Cr-Commit-Position: refs/heads/master@{#383941}

TBR=droger@chromium.org,dvadym@chromium.org,eugenebut@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 595717 , 598672

Review URL: https://codereview.chromium.org/1842313002

Cr-Commit-Position: refs/heads/master@{#384225}

[modify] https://crrev.com/aaeb70a8b015d3b88fd34731efa8ad499e760c05/ios/chrome/BUILD.gn
[modify] https://crrev.com/aaeb70a8b015d3b88fd34731efa8ad499e760c05/ios/chrome/browser/passwords/password_controller.h
[modify] https://crrev.com/aaeb70a8b015d3b88fd34731efa8ad499e760c05/ios/chrome/browser/passwords/password_controller.mm
[delete] https://crrev.com/04143c634f21b22da89a698be7744acdd22306ae/ios/chrome/browser/passwords/password_controller_unittest.mm
[modify] https://crrev.com/aaeb70a8b015d3b88fd34731efa8ad499e760c05/ios/chrome/ios_chrome_tests.gyp

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40a58addf1b656beaac8b7a2f50cca718a0556e0

commit 40a58addf1b656beaac8b7a2f50cca718a0556e0
Author: vabr <vabr@chromium.org>
Date: Thu Mar 31 15:54:06 2016

Reland of Add a unittest for PasswordController

Original CL: https://codereview.chromium.org/1808853005/
CL with revert: https://codereview.chromium.org/1842313002/
Patch set 1 here == what landed in the original CL
Subsequent patches constitute the fix of the breakage.
TBR-ing droger@ for the GYP and GN changes, which he already approved in the
original CL, and which are not different here.

Original description:
-------------
While PasswordController already has a unittest downstream, that one is rather
an integration test (the heaviness of which is the reason it is still
downstream, stuck on some downstream-only testing framework).

When a test was needed for https://codereview.chromium.org/1806333005/, it
turned out a simple unittest would be better. This CL adds that test and
places it upstream. (Ultimately we should convert the downstream tests to a
lighter version and add them here as well, but not in this CL.)
-------------

BUG= 595717 , 598672, 599231
TBR=droger@chromium.org

Review URL: https://codereview.chromium.org/1848433004

Cr-Commit-Position: refs/heads/master@{#384291}

[modify] https://crrev.com/40a58addf1b656beaac8b7a2f50cca718a0556e0/ios/chrome/BUILD.gn
[modify] https://crrev.com/40a58addf1b656beaac8b7a2f50cca718a0556e0/ios/chrome/browser/passwords/password_controller.h
[modify] https://crrev.com/40a58addf1b656beaac8b7a2f50cca718a0556e0/ios/chrome/browser/passwords/password_controller.mm
[add] https://crrev.com/40a58addf1b656beaac8b7a2f50cca718a0556e0/ios/chrome/browser/passwords/password_controller_unittest.mm
[modify] https://crrev.com/40a58addf1b656beaac8b7a2f50cca718a0556e0/ios/chrome/ios_chrome_tests.gyp

Verified on latest chrome beta 51.0.2704.20 on iPad Air2 with iOS 9.2.1 and iPhone 5s with iOS 9.3.1, following the steps mentioned in comment #0.  Save Password info bar is displayed.  Looks good.