Password generation logging

Password generation logging

BUG=None

Review-Url: https://codereview.chromium.org/2739373002
Cr-Commit-Position: refs/heads/master@{#456085}
Committed: https://chromium.googlesource.com/chromium/src/+/263e504efa7a68bac5af7a84cac2d46bce31bc82

[dvadym, 2017-03-10 12:36:11]

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

[dvadym, 2017-03-10 12:36:12]

Hi Maxim,

could you please review this CL?

Regards,
Vadym

https://codereview.chromium.org/2739373002/diff/1/components/password_manager...
File components/password_manager/core/browser/password_generation_manager.cc
(right):

https://codereview.chromium.org/2739373002/diff/1/components/password_manager...
components/password_manager/core/browser/password_generation_manager.cc:46:
const AutofillField* generation_field = nullptr;
I can't resist to make this small refactoring

[kolos1, 2017-03-10 13:05:56]

https://codereview.chromium.org/2739373002/diff/1/components/autofill/content...
File components/autofill/content/renderer/password_generation_agent.h (right):

https://codereview.chromium.org/2739373002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_generation_agent.h:70: virtual
bool ShouldAnalyzeDocument();
Why did you remove 'const'?

https://codereview.chromium.org/2739373002/diff/1/components/autofill/core/co...
File components/autofill/core/common/save_password_progress_logger.cc (right):

https://codereview.chromium.org/2739373002/diff/1/components/autofill/core/co...
components/autofill/core/common/save_password_progress_logger.cc:421: return
"Generation: fields for generation are not found ";
Remove trailing space.

https://codereview.chromium.org/2739373002/diff/1/components/password_manager...
File components/password_manager/core/browser/password_generation_manager.cc
(right):

https://codereview.chromium.org/2739373002/diff/1/components/password_manager...
components/password_manager/core/browser/password_generation_manager.cc:46:
const AutofillField* generation_field = nullptr;
On 2017/03/10 12:36:12, dvadym wrote:
> I can't resist to make this small refactoring

Acknowledged.

https://codereview.chromium.org/2739373002/diff/1/components/password_manager...
components/password_manager/core/browser/password_generation_manager.cc:92: ?
Logger::STRING_GENERATION_DISABLED_NO_SYNC
nit: I would prefer to not assume that PasswordSyncState will be never changed.
If new entry will be added, the 'else'-branch will be wrong.

[dvadym, 2017-03-10 13:35:24]

Thanks for comments! I've addressed them. PTAL

https://codereview.chromium.org/2739373002/diff/1/components/autofill/content...
File components/autofill/content/renderer/password_generation_agent.h (right):

https://codereview.chromium.org/2739373002/diff/1/components/autofill/content...
components/autofill/content/renderer/password_generation_agent.h:70: virtual
bool ShouldAnalyzeDocument();
On 2017/03/10 13:05:56, kolos1 wrote:
> Why did you remove 'const'?

LogMessage (it's called from this method) isn't be const

https://codereview.chromium.org/2739373002/diff/1/components/autofill/core/co...
File components/autofill/core/common/save_password_progress_logger.cc (right):

https://codereview.chromium.org/2739373002/diff/1/components/autofill/core/co...
components/autofill/core/common/save_password_progress_logger.cc:421: return
"Generation: fields for generation are not found ";
On 2017/03/10 13:05:56, kolos1 wrote:
> Remove trailing space.

Done.

https://codereview.chromium.org/2739373002/diff/1/components/password_manager...
File components/password_manager/core/browser/password_generation_manager.cc
(right):

https://codereview.chromium.org/2739373002/diff/1/components/password_manager...
components/password_manager/core/browser/password_generation_manager.cc:92: ?
Logger::STRING_GENERATION_DISABLED_NO_SYNC
On 2017/03/10 13:05:56, kolos1 wrote:
> nit: I would prefer to not assume that PasswordSyncState will be never
changed.
> If new entry will be added, the 'else'-branch will be wrong.

Done.

[kolos1, 2017-03-10 13:48:10]

On 2017/03/10 13:35:24, dvadym wrote:
> Thanks for comments! I've addressed them. PTAL
> 
>
https://codereview.chromium.org/2739373002/diff/1/components/autofill/content...
> File components/autofill/content/renderer/password_generation_agent.h (right):
> 
>
https://codereview.chromium.org/2739373002/diff/1/components/autofill/content...
> components/autofill/content/renderer/password_generation_agent.h:70: virtual
> bool ShouldAnalyzeDocument();
> On 2017/03/10 13:05:56, kolos1 wrote:
> > Why did you remove 'const'?
> 
> LogMessage (it's called from this method) isn't be const
> 
>
https://codereview.chromium.org/2739373002/diff/1/components/autofill/core/co...
> File components/autofill/core/common/save_password_progress_logger.cc (right):
> 
>
https://codereview.chromium.org/2739373002/diff/1/components/autofill/core/co...
> components/autofill/core/common/save_password_progress_logger.cc:421: return
> "Generation: fields for generation are not found ";
> On 2017/03/10 13:05:56, kolos1 wrote:
> > Remove trailing space.
> 
> Done.
> 
>
https://codereview.chromium.org/2739373002/diff/1/components/password_manager...
> File components/password_manager/core/browser/password_generation_manager.cc
> (right):
> 
>
https://codereview.chromium.org/2739373002/diff/1/components/password_manager...
> components/password_manager/core/browser/password_generation_manager.cc:92: ?
> Logger::STRING_GENERATION_DISABLED_NO_SYNC
> On 2017/03/10 13:05:56, kolos1 wrote:
> > nit: I would prefer to not assume that PasswordSyncState will be never
> changed.
> > If new entry will be added, the 'else'-branch will be wrong.
> 
> Done.

LGTM

[dvadym, 2017-03-10 15:33:42]

The CQ bit was checked by dvadym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 15:40:54]

Dry run: 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-03-10 16:47:13]

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

[commit-bot: I haz the power, 2017-03-10 16:47:14]

Dry run: This issue passed the CQ dry run.

[dvadym, 2017-03-10 17:05:55]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2017-03-10 17:05:56]

The patchset sent to the CQ was uploaded after l-g-t-m from kolos@chromium.org
Link to the patchset: https://codereview.chromium.org/2739373002/#ps40001
(title: "compilation fix")

[commit-bot: I haz the power, 2017-03-10 17:07:44]

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-03-10 17:13:21]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489165555628920,
"parent_rev": "33939c01a9cc0064f9a9a50060af1036c5b86ae1", "commit_rev":
"263e504efa7a68bac5af7a84cac2d46bce31bc82"}

[commit-bot: I haz the power, 2017-03-10 17:14:05]

Description was changed from

==========
Password generation logging

BUG=None
==========

to

==========
Password generation logging

BUG=None

Review-Url: https://codereview.chromium.org/2739373002
Cr-Commit-Position: refs/heads/master@{#456085}
Committed:
https://chromium.googlesource.com/chromium/src/+/263e504efa7a68bac5af7a84cac2...
==========

[commit-bot: I haz the power, 2017-03-10 17:14:06]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/263e504efa7a68bac5af7a84cac2...