[Password Generation] Sends form classifier vote to autofill server

[Password Generation] Sends form classifier vote to autofill server

BUG=582434
Committed: https://crrev.com/55363c5dcf76cee98355a9c4368817a5f7825003
Cr-Commit-Position: refs/heads/master@{#400640}

[kolos1, 2016-06-10 10:41:22]

Description was changed from

==========
[Password Generation] Sends local classifier vote to autofill server


BUG=582434
==========

to

==========
[Password Generation] Sends form classifier vote to autofill server


BUG=582434
==========

[kolos1, 2016-06-10 11:17:14]

kolos@chromium.org changed reviewers:
+ dvadym@chromium.org

[kolos1, 2016-06-10 11:17:14]

Hi Vadym,

Please review this CL on sending the form classier's votes.

Regards,
Maxim

[kolos1, 2016-06-15 14:41:41]

Patchset #8 (id:140001) has been deleted

[kolos1, 2016-06-15 14:58:01]

Patchset #8 (id:160001) has been deleted

[dvadym, 2016-06-16 10:04:38]

LGTM % nits

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:211: // uploaded
protobuf.
You shouldn't write what will be done with information of this message in
browser. It's not high level enough here, and it can be changed in future.

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:212:
IPC_MESSAGE_ROUTED2(AutofillHostMsg_SaveFormClassifierVote,
Please move this message to Renderer->Browser messages

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:212:
IPC_MESSAGE_ROUTED2(AutofillHostMsg_SaveFormClassifierVote,
Name of this message should say something about generation and/or password
forms. It's not clear that this is outcome of Password form classifier, in
password code it's ok, but here it's common for password and autofill code.

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_generation_agent.h (right):

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_generation_agent.h:111: void
RunFormClassifierAndSaveVote(blink::WebFormElement& web_form,
Can we use const blink::WebFormElement&?

https://codereview.chromium.org/2055633003/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/2055633003/diff/180001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:274: // Saves
the outcome of HTML parsing based form classifier to uploaded proto.
uploaded -> upload

[kolos1, 2016-06-16 13:28:44]

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:211: // uploaded
protobuf.
On 2016/06/16 10:04:38, dvadym wrote:
> You shouldn't write what will be done with information of this message in
> browser. It's not high level enough here, and it can be changed in future.

Done.

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:212:
IPC_MESSAGE_ROUTED2(AutofillHostMsg_SaveFormClassifierVote,
On 2016/06/16 10:04:38, dvadym wrote:
> Please move this message to Renderer->Browser messages 

Done.

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
components/autofill/content/common/autofill_messages.h:212:
IPC_MESSAGE_ROUTED2(AutofillHostMsg_SaveFormClassifierVote,
On 2016/06/16 10:04:38, dvadym wrote:
> Name of this message should say something about generation and/or password
> forms. It's not clear that this is outcome of Password form classifier, in
> password code it's ok, but here it's common for password and autofill code.

Done.

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
File components/autofill/content/renderer/password_generation_agent.h (right):

https://codereview.chromium.org/2055633003/diff/180001/components/autofill/co...
components/autofill/content/renderer/password_generation_agent.h:111: void
RunFormClassifierAndSaveVote(blink::WebFormElement& web_form,
On 2016/06/16 10:04:38, dvadym wrote:
> Can we use const blink::WebFormElement&?

Done.

https://codereview.chromium.org/2055633003/diff/180001/components/password_ma...
File components/password_manager/core/browser/password_form_manager.h (right):

https://codereview.chromium.org/2055633003/diff/180001/components/password_ma...
components/password_manager/core/browser/password_form_manager.h:274: // Saves
the outcome of HTML parsing based form classifier to uploaded proto.
On 2016/06/16 10:04:38, dvadym wrote:
> uploaded -> upload

Done.

[dvadym, 2016-06-16 13:52:04]

Thanks, LGTM

[kolos1, 2016-06-16 13:56:13]

kolos@chromium.org changed reviewers:
+ isherman@chromium.org, vabr@chromium.org

[kolos1, 2016-06-16 13:56:13]

kolos@chromium.org changed reviewers:
+ isherman@chromium.org, vabr@chromium.org

[kolos1, 2016-06-16 13:56:14]

vabr@chromium.org: Please review changes in
chrome/renderer/autofill/password_generation_agent_browsertest.cc
components/autofill/core/browser/autofill_field.h
components/autofill/core/browser/proto/server.proto

[kolos1, 2016-06-16 13:56:14]

vabr@chromium.org: Please review changes in
chrome/renderer/autofill/password_generation_agent_browsertest.cc
components/autofill/core/browser/autofill_field.h
components/autofill/core/browser/proto/server.proto

[kolos1, 2016-06-16 13:57:38]

isherman@: Please review changes in tools/metrics/histograms/histograms.xml

[vabr (Chromium), 2016-06-16 14:23:47]

chrome/renderer/autofill/password_generation_agent_browsertest.cc,
components/autofill/core/browser/autofill_field.h, and
components/autofill/core/browser/proto/server.proto LGTM. But I'm not sure: do
we need to update some server-side protobuf definitions as well?

Cheers,
Vaclav

[kolos1, 2016-06-16 14:25:27]

On 2016/06/16 14:23:47, vabr (Chromium) wrote:
> chrome/renderer/autofill/password_generation_agent_browsertest.cc,
> components/autofill/core/browser/autofill_field.h, and
> components/autofill/core/browser/proto/server.proto LGTM. But I'm not sure: do
> we need to update some server-side protobuf definitions as well?
Yes, of course, we will update server-side protobuf as well.

[kolos1, 2016-06-16 14:28:00]

kolos@chromium.org changed reviewers:
+ mkwst@chromium.org

[kolos1, 2016-06-16 14:28:00]

mkwst@chromium.org: Please review changes in
components/autofill/content/common/autofill_messages.h

[Ilya Sherman, 2016-06-16 18:09:40]

histograms.xml lgtm

[Mike West, 2016-06-17 11:36:02]

IPC LGTM.

[kolos1, 2016-06-17 12:48:23]

The CQ bit was checked by kolos@chromium.org

[commit-bot: I haz the power, 2016-06-17 12:48:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055633003/200001

[commit-bot: I haz the power, 2016-06-17 13:23:32]

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

[commit-bot: I haz the power, 2016-06-17 13:23:34]

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

[kolos1, 2016-06-17 20:51:46]

The CQ bit was checked by kolos@chromium.org

[commit-bot: I haz the power, 2016-06-17 20:52:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055633003/200001

[commit-bot: I haz the power, 2016-06-17 21:27:33]

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

[commit-bot: I haz the power, 2016-06-17 21:27:35]

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

[kolos1, 2016-06-20 08:09:55]

vabr@: Please review this small change in
PasswordManager::SaveGenerationFieldDetectedByClassifier

I replaced " DCHECK(client_->IsSavingAndFillingEnabledForCurrentPage());"  with
"if (!client_->IsSavingAndFillingEnabledForCurrentPage()) return"

Unlike other messages from PasswordGenerationAgent to browser, the message from
the classifier might be sent even if saving and filling are disabled.

Only Mac tests showed that.

[kolos1, 2016-06-20 08:13:10]

> Only Mac tests showed that.
Mac and also ChromeOS tests showed that.

[vabr (Chromium), 2016-06-20 08:32:29]

Hi Maxim.

I have a question below.

Cheers,
Vaclav

https://codereview.chromium.org/2055633003/diff/220001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2055633003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:237: if
(!client_->IsSavingAndFillingEnabledForCurrentPage())
I would like to understand more what happens here. You say this is reproduced in
tests -- does that also correspond to a behaviour which a user can observe?

How does it happen that this method is called when saving & filling is disabled?
Is it some kind of delay during which a navigation happens?

How do we know that it is safe to just give up on line 238?

(It would be good to summarise the answers in a comment above line 237.)

[kolos1, 2016-06-20 09:09:15]

https://codereview.chromium.org/2055633003/diff/220001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2055633003/diff/220001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:237: if
(!client_->IsSavingAndFillingEnabledForCurrentPage())
PasswordGenerationAgent::FindPossibleGenerationForm() might be called even if
saving&filling are disabled. Is it by design?  

In PasswordGenerationAgent::DetermineGenerationElement(), we didn't go further
if we didn't received positive vote from autofill server (i.e.
OnFormsEligibleForGenerationFound should be called, but the sender,
PasswordGenerationManager::DetectFormsEligibleForGeneration, checks if the
saving&filling are enabled).

[chromium-reviews, 2016-06-20 09:22:18]

On Mon, 20 Jun 2016 at 11:09 <kolos@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2055633003/diff/220001/components/password_ma...
> File components/password_manager/core/browser/password_manager.cc
> (right):
>
>
>
https://codereview.chromium.org/2055633003/diff/220001/components/password_ma...
> components/password_manager/core/browser/password_manager.cc:237: if
> (!client_->IsSavingAndFillingEnabledForCurrentPage())
> PasswordGenerationAgent::FindPossibleGenerationForm() might be called
> even if saving&filling are disabled. Is it by design?
>

It's certainly not the design goal, but it might be due to not pushing the
enabled/disabled state from the browser to the renderer. We might want to
fix this, but not in this CL. So your approach looks good to me.


>
>
> In PasswordGenerationAgent::DetermineGenerationElement(), we didn't go
> further if we didn't received positive vote from autofill server (i.e.
> OnFormsEligibleForGenerationFound should be called, but the sender,
> PasswordGenerationManager::DetectFormsEligibleForGeneration, checks if
> the saving&filling are enabled).
>
> https://codereview.chromium.org/2055633003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kolos1, 2016-06-20 09:36:54]

The CQ bit was checked by kolos@chromium.org

[kolos1, 2016-06-20 09:36:55]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
mkwst@chromium.org, dvadym@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2055633003/#ps220001
(title: "replaced " DCHECK(client_->IsSavingAndFillingEnabledForCurrentPage())")

[commit-bot: I haz the power, 2016-06-20 09:37:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2055633003/220001

[commit-bot: I haz the power, 2016-06-20 09:41:23]

Description was changed from

==========
[Password Generation] Sends form classifier vote to autofill server


BUG=582434
==========

to

==========
[Password Generation] Sends form classifier vote to autofill server


BUG=582434
==========

[commit-bot: I haz the power, 2016-06-20 09:41:25]

Committed patchset #10 (id:220001)

[commit-bot: I haz the power, 2016-06-20 09:43:12]

Description was changed from

==========
[Password Generation] Sends form classifier vote to autofill server


BUG=582434
==========

to

==========
[Password Generation] Sends form classifier vote to autofill server

BUG=582434

Committed: https://crrev.com/55363c5dcf76cee98355a9c4368817a5f7825003
Cr-Commit-Position: refs/heads/master@{#400640}
==========

[commit-bot: I haz the power, 2016-06-20 09:43:15]

Patchset 10 (id:??) landed as
https://crrev.com/55363c5dcf76cee98355a9c4368817a5f7825003
Cr-Commit-Position: refs/heads/master@{#400640}