Remove hasPasswordField from the public __gCrWeb API.

Remove hasPasswordField from the public __gCrWeb API.

This method is a helper for __gCrWeb.fillPasswordForm. The only usage
outside of JavaScript is in password_controller_unittest.mm, which tests
explicitly the scenario of detecting password fields in nested iframes.
This change beefs up test cases in PasswordControllerTest so the nested
iframe case is tested using the public fillPasswordForm API.

BUG=614092
TESTED=Ran ios_chrome_unittests

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

[danyao, 2017-04-18 23:03:32]

danyao@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-04-19 00:18:12]

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

[Eugene But (OOO till 7-30), 2017-04-19 00:18:13]

vabr@ would be a better reviewer for this CL. lgtm from my perspective

https://codereview.chromium.org/2825123003/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/password_controller_unittest.mm (right):

https://codereview.chromium.org/2825123003/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/password_controller_unittest.mm:760: // A script
that we run after autofilling forms.  It returns ids and values of
Please avoid using "we" (go/avoidwe).

[vabr (Chromium), 2017-04-19 06:51:56]

LGTM, removing hasPasswordField certainly makes sense to me, and the updated
tests seem to cover more than the old ones.

Thanks for the improvement!
Vaclav

[vabr (Chromium), 2017-04-19 09:09:20]

Actually, do the new unit tests pass? Our iframe support is broken
(https://crbug.com/554927), and a quick check with the simulator shows that
Chrome does not fill in iframes yet.

If that makes the tests broken, please disable the iframe-related parts and mark
them with a TODO(crbug.com/554927). Thanks!

Vaclav

[danyao, 2017-04-19 15:43:51]

On 2017/04/19 09:09:20, vabr (Chromium) wrote:
> Actually, do the new unit tests pass? Our iframe support is broken
> (https://crbug.com/554927), and a quick check with the simulator shows that
> Chrome does not fill in iframes yet.
> 
> If that makes the tests broken, please disable the iframe-related parts and
mark
> them with a TODO(crbug.com/554927). Thanks!
> 
> Vaclav

The new tests do pass, and I can make them fail too with the wrong expectation,
so the tests are testing something. But it seems that the tests should be
affected by crbug.com/554927, so I'm not sure why they are not failling. I'll
submit this now, but will dig a bit more on the other issue.

[danyao, 2017-04-19 15:49:03]

https://codereview.chromium.org/2825123003/diff/1/ios/chrome/browser/password...
File ios/chrome/browser/passwords/password_controller_unittest.mm (right):

https://codereview.chromium.org/2825123003/diff/1/ios/chrome/browser/password...
ios/chrome/browser/passwords/password_controller_unittest.mm:760: // A script
that we run after autofilling forms.  It returns ids and values of
On 2017/04/19 00:18:13, Eugene But wrote:
> Please avoid using "we" (go/avoidwe).

Done.

[danyao, 2017-04-19 15:49:34]

The CQ bit was checked by danyao@chromium.org

[danyao, 2017-04-19 15:49:35]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2825123003/#ps20001
(title: "Fix comment")

[vabr (Chromium), 2017-04-19 15:49:40]

On 2017/04/19 15:43:51, danyao wrote:
> On 2017/04/19 09:09:20, vabr (Chromium) wrote:
> > Actually, do the new unit tests pass? Our iframe support is broken
> > (https://crbug.com/554927), and a quick check with the simulator shows that
> > Chrome does not fill in iframes yet.
> > 
> > If that makes the tests broken, please disable the iframe-related parts and
> mark
> > them with a TODO(crbug.com/554927). Thanks!
> > 
> > Vaclav
> 
> The new tests do pass, and I can make them fail too with the wrong
expectation,
> so the tests are testing something. But it seems that the tests should be
> affected by crbug.com/554927, so I'm not sure why they are not failling. I'll
> submit this now, but will dig a bit more on the other issue.

Thanks!
By all means, landing this is a good idea.
If you get time to look into this, that's great!
(But if you don't, don't worry, it's the owners' responsibility to investigate.)

Cheers,
Vaclav

[commit-bot: I haz the power, 2017-04-19 15:49:51]

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 16:00:50]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492616974694260,
"parent_rev": "7acdbdd51897c2abc030f06ecfec9116ea5bee26", "commit_rev":
"ed96115726663cd5ed367c807310400686dc0927"}

[commit-bot: I haz the power, 2017-04-19 16:00:56]

Description was changed from

==========
Remove hasPasswordField from the public __gCrWeb API.

This method is a helper for __gCrWeb.fillPasswordForm. The only usage
outside of JavaScript is in password_controller_unittest.mm, which tests
explicitly the scenario of detecting password fields in nested iframes.
This change beefs up test cases in PasswordControllerTest so the nested
iframe case is tested using the public fillPasswordForm API.

BUG=614092
TESTED=Ran ios_chrome_unittests
==========

to

==========
Remove hasPasswordField from the public __gCrWeb API.

This method is a helper for __gCrWeb.fillPasswordForm. The only usage
outside of JavaScript is in password_controller_unittest.mm, which tests
explicitly the scenario of detecting password fields in nested iframes.
This change beefs up test cases in PasswordControllerTest so the nested
iframe case is tested using the public fillPasswordForm API.

BUG=614092
TESTED=Ran ios_chrome_unittests

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

[commit-bot: I haz the power, 2017-04-19 16:00:57]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ed96115726663cd5ed367c807310...