Move ShouldFilterAutofillResult from ChromePasswordManagerClient to PasswordManager

Move ShouldFilterAutofillResult from ChromePasswordManagerClient to PasswordManager

The goal is to put as much of the platform-independent logic to the shared code, and leave the CPMC just a dumb redirector to the embedding system. This is one of many steps.

Also, this CL removes IsPasswordManagementEnabledForCurrentPage() from the client mock in the client unit test. It is dangerous to mix mocking methods with testing the real implementations, and none of the tests actually needs this method mocked.

BUG=474577, 515108
Committed: https://crrev.com/5410d0bc855495b61ca084a8760d32613f684d01
Cr-Commit-Position: refs/heads/master@{#342332}

[vabr (Chromium), 2015-07-28 14:30:15]

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

[vabr (Chromium), 2015-07-28 14:30:15]

Hi Balázs,

PTAL.

Thanks!
Vaclav

[vabr (Chromium), 2015-07-28 15:07:34]

The failures are real, I'm looking into fixing them.

Sorry about that.

Cheers,
Vaclav

[vabr (Chromium), 2015-07-28 15:29:27]

I fixed the failed tests (at least locally).

[vabr (Chromium), 2015-07-29 15:16:20]

Hi Balázs,

Just an update -- patch set 4 contains the sync layer. There is a particular
TODO concerning moving IsSyncCredential from the client to the sync layer, so I
have not done proofreading of the whole CL yet.

If you want to discuss the overall structure, please feel free to have a look
and let me know your thoughts. For a fine-details review this is still too
early, though.

Cheers,
Vaclav

[vabr (Chromium), 2015-07-29 15:50:58]

Note to myself: The iOS builder failure is
gyp: Dependency
'/b/build/slave/ios_rel_device_ninja/build/src/components/components.gyp:password_manager_sync_browser#target'
not found while trying to load target
/b/build/slave/ios_rel_device_ninja/build/src/components/components_tests.gyp:components_unittests#target
Error: Command '/usr/bin/python src/build/gyp_chromium' returned non-zero exit
status 1 in /b/build/slave/ios_rel_device_ninja/build

[vabr (Chromium), 2015-07-31 14:07:17]

Hi Balázs,

Please take a look. The CL is ready for review, except for 2 things:

(1) The failing Mac UI tests, which I'm fixing right now. This is similar to
what was in the corresponding Views tests, no impact on the rest of the CL.

(2) The TODO in sync_store_result_filter.cc. I prefer to separate addressing
that in another CL.

Cheers,
Vaclav

[vabr (Chromium), 2015-07-31 18:31:34]

> (1) The failing Mac UI tests, which I'm fixing right now. This is similar to
> what was in the corresponding Views tests, no impact on the rest of the CL.
This is fixed now.

[engedy, 2015-08-04 07:30:31]

Just one comment with substance: Would there be any alternatives to logging the
statistics in the destructor? It seems that it places quite a few requirements
on the architecture, and it is not entirely clear that the implementation of the
StoreResultFilter interface can rely on lifetime being managed this way, nor is
it clear in PasswordFormManager that it needs to manage the lifetime this way
way.

[vabr (Chromium), 2015-08-04 07:52:47]

Hi Balázs,

On 2015/08/04 07:30:31, engedy wrote:
> Just one comment with substance: Would there be any alternatives to logging
the
> statistics in the destructor?
Yes, the previous approach with an explicit method for that. The downside would
be a no verifiable contract: "Be sure to call this once, and as the last
method." That contract is implicit for a destructor.

> It seems that it places quite a few requirements
> on the architecture,
Could you please name the requirements?

> and it is not entirely clear that the implementation of the
> StoreResultFilter interface can rely on lifetime being managed this way,
I neither understand what you mean by "this way", not why should the
implementation care about its lifetime.

> nor is
> it clear in PasswordFormManager that it needs to manage the lifetime this way
> way.
I'm afraid I also don't understand what you mean here. What is it that the PFM
needs to do?

Thanks,
Vaclav

[engedy, 2015-08-04 10:11:29]

LGTM % comments.

https://codereview.chromium.org/1260263002/diff/60001/components/password_man...
File components/password_manager/README (left):

https://codereview.chromium.org/1260263002/diff/60001/components/password_man...
components/password_manager/README:2:
(https://sites.google.com/a/chromium.org/dev/developers/design-documents/layer...)
nit: We could keep the URL.

https://codereview.chromium.org/1260263002/diff/140001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(left):

https://codereview.chromium.org/1260263002/diff/140001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:51:
MOCK_CONST_METHOD0(IsPasswordManagementEnabledForCurrentPage, bool());
nit: Why this is going away? Is it just drive-by refactoring?

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/README (left):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/README:2:
(https://sites.google.com/a/chromium.org/dev/developers/design-documents/layer...)
Please keep link to layered component design.

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/content/DEPS (right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/content/DEPS:11:
"-components/password_manager/sync",
Not sure how DEPS work, but would it be possible to move this to the parent
directory?

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:12: #include
"components/password_manager/sync/browser/sync_store_result_filter.h"
Shouldn't this be core/browser/store_result_filter.h?

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:169: //
Creates a single-use filter for PasswordFormManager to process password
We should update this comment to better describe the lifetime requirements.

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/core/browser/store_result_filter.h (right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/core/browser/store_result_filter.h:8: #include
"components/autofill/core/common/password_form.h"
nit: I don't feel strongly about this, and know that forward-declarations are a
contentious topic, just wanted to make sure this is intentionally including and
not forward-declaring.

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/core/browser/stub_password_manager_client.cc
(right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/core/browser/stub_password_manager_client.cc:18:
class DisabledStoreResultFilter : public StoreResultFilter {
nit: s/Disabled/PassThrough/g

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/sync/DEPS (right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/sync/DEPS:2: # PasswordManager is a layered
component; subdirectories must explicitly
Hmm, I do not know much about how DEPS work, but shouldn't the parent
directory's DEPS do this already?

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/sync/browser/sync_store_result_filter.cc
(right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/sync/browser/sync_store_result_filter.cc:48:
form.signon_realm))
nit: Include password_form.h directly, there seems to be no reasons why it would
be necessarily pulled in by any other includes

[vabr (Chromium), 2015-08-04 12:21:24]

vabr@chromium.org changed reviewers:
+ jochen@chromium.org, rogerta@chromium.org, stuartmorgan@chromium.org

[vabr (Chromium), 2015-08-04 12:21:25]

@jochen: Please review chrome/browser/BUILD.gn

@stuartmorgan: Please review ios/chrome/ios_chrome.gyp. I won't land this until
I have the corresponding patch for downstream ready for landing as well, to make
merging painless.

@rogerta: Please review me adding google_apis/gaia to
components/password_manager/sync/browser/DEPS (sorry for the patch set looking
as if I actually removed net/base; next time I upload it I'll try to tweak
--similarity)

@engedy: Thanks for your review. Please see how I addressed your comments.

Thanks all,
Vaclav

https://codereview.chromium.org/1260263002/diff/60001/components/password_man...
File components/password_manager/README (left):

https://codereview.chromium.org/1260263002/diff/60001/components/password_man...
components/password_manager/README:2:
(https://sites.google.com/a/chromium.org/dev/developers/design-documents/layer...)
On 2015/08/04 10:11:28, engedy wrote:
> nit: We could keep the URL.

Done.

https://codereview.chromium.org/1260263002/diff/140001/chrome/browser/passwor...
File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc
(left):

https://codereview.chromium.org/1260263002/diff/140001/chrome/browser/passwor...
chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:51:
MOCK_CONST_METHOD0(IsPasswordManagementEnabledForCurrentPage, bool());
On 2015/08/04 10:11:29, engedy wrote:
> nit: Why this is going away? Is it just drive-by refactoring?

Yes. I will mention it in the CL description, this is for
http://crbug.com/474577 in particular.

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/README (left):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/README:2:
(https://sites.google.com/a/chromium.org/dev/developers/design-documents/layer...)
On 2015/08/04 10:11:29, engedy wrote:
> Please keep link to layered component design.

Done.

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/content/DEPS (right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/content/DEPS:11:
"-components/password_manager/sync",
On 2015/08/04 10:11:29, engedy wrote:
> Not sure how DEPS work, but would it be possible to move this to the parent
> directory?

Done.

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:12: #include
"components/password_manager/sync/browser/sync_store_result_filter.h"
On 2015/08/04 10:11:29, engedy wrote:
> Shouldn't this be core/browser/store_result_filter.h?

Indeed. And had I done the DEPS right, presubmit would have caught that. Fixed
now.

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:169: //
Creates a single-use filter for PasswordFormManager to process password
On 2015/08/04 10:11:29, engedy wrote:
> We should update this comment to better describe the lifetime requirements.

Done. Please check if it is any better now.

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/core/browser/store_result_filter.h (right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/core/browser/store_result_filter.h:8: #include
"components/autofill/core/common/password_form.h"
On 2015/08/04 10:11:29, engedy wrote:
> nit: I don't feel strongly about this, and know that forward-declarations are
a
> contentious topic, just wanted to make sure this is intentionally including
and
> not forward-declaring.

Thanks for checking. This was deliberate:

The public Google C++ guide linked from Chromium style [1] says: "The practical
efficiency benefits of forward declarations are unproven." While it also allows
forward declarations, it does not suggest using them, and the internal C++ guide
is against forward declarations.

I'm aware that most of Chromium code, even password manager code, uses forward
declarations. I might even have introduced forward declarations in this CL, out
of old habit. This is not ideal, but I also do not see it as a big problem so
far. If you feel strongly about consistency, let's file a bug to get rid of
forward declarations in password manager, and do a sweep to fix this.

[1]
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Decla...

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/core/browser/stub_password_manager_client.cc
(right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/core/browser/stub_password_manager_client.cc:18:
class DisabledStoreResultFilter : public StoreResultFilter {
On 2015/08/04 10:11:29, engedy wrote:
> nit: s/Disabled/PassThrough/g

Done.

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/sync/DEPS (right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/sync/DEPS:2: # PasswordManager is a layered
component; subdirectories must explicitly
On 2015/08/04 10:11:29, engedy wrote:
> Hmm, I do not know much about how DEPS work, but shouldn't the parent
> directory's DEPS do this already?

Done.

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/sync/browser/sync_store_result_filter.cc
(right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/sync/browser/sync_store_result_filter.cc:48:
form.signon_realm))
On 2015/08/04 10:11:29, engedy wrote:
> nit: Include password_form.h directly, there seems to be no reasons why it
would
> be necessarily pulled in by any other includes

The header for the interface includes it, no? Anyway, with #ifdef guards I don't
think it hurts to include it in the .cc file. Until we have a tool to sanitise
#includes, there will be redundancy and other mess anyway. :)
Done.

[engedy, 2015-08-04 13:06:35]

Still LGTM, thanks!

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/core/browser/password_manager_client.h (right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/core/browser/password_manager_client.h:169: //
Creates a single-use filter for PasswordFormManager to process password
SGTM, thanks!

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/core/browser/store_result_filter.h (right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/core/browser/store_result_filter.h:8: #include
"components/autofill/core/common/password_form.h"
As I said, I don't feel strongly about this. Just wanted to double-check if you
do. :)

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/sync/browser/sync_store_result_filter.cc
(right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/sync/browser/sync_store_result_filter.cc:48:
form.signon_realm))
On 2015/08/04 12:21:25, vabr (Chromium) wrote:
> On 2015/08/04 10:11:29, engedy wrote:
> > nit: Include password_form.h directly, there seems to be no reasons why it
> would
> > be necessarily pulled in by any other includes
> 
> The header for the interface includes it, no? Anyway, with #ifdef guards I
don't
> think it hurts to include it in the .cc file. Until we have a tool to sanitise
> #includes, there will be redundancy and other mess anyway. :)
> Done.

Yes, but technically, it would not be required to include it in the header, it
could be only forward declared, which is still very common in Chromium code,
despite the style guide now discouraging it.

For background, my line of thinking is usually as follows. When my code includes
a header X.h and depends on (the complete definition of) type T, and it so
happens that X.h includes T.h, I do not rely on this unless my code would break
(in aspects other than missing an include) if X.h ever stopped including T.h.

For example, if X is a subclass of T, or X has a public member variable of type
T by value, then I do not include T.h, because the only way to remove including
T.h in X.h implies such fundamental changes to X that it is unlikely that my
code would still be able to use X as before without being refactored itself.

[vabr (Chromium), 2015-08-04 13:37:54]

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
File components/password_manager/sync/browser/sync_store_result_filter.cc
(right):

https://codereview.chromium.org/1260263002/diff/140001/components/password_ma...
components/password_manager/sync/browser/sync_store_result_filter.cc:48:
form.signon_realm))
On 2015/08/04 13:06:35, engedy wrote:
> On 2015/08/04 12:21:25, vabr (Chromium) wrote:
> > On 2015/08/04 10:11:29, engedy wrote:
> > > nit: Include password_form.h directly, there seems to be no reasons why it
> > would
> > > be necessarily pulled in by any other includes
> > 
> > The header for the interface includes it, no? Anyway, with #ifdef guards I
> don't
> > think it hurts to include it in the .cc file. Until we have a tool to
sanitise
> > #includes, there will be redundancy and other mess anyway. :)
> > Done.
> 
> Yes, but technically, it would not be required to include it in the header, it
> could be only forward declared, which is still very common in Chromium code,
> despite the style guide now discouraging it.
> 
> For background, my line of thinking is usually as follows. When my code
includes
> a header X.h and depends on (the complete definition of) type T, and it so
> happens that X.h includes T.h, I do not rely on this unless my code would
break
> (in aspects other than missing an include) if X.h ever stopped including T.h.
> 
> For example, if X is a subclass of T, or X has a public member variable of
type
> T by value, then I do not include T.h, because the only way to remove
including
> T.h in X.h implies such fundamental changes to X that it is unlikely that my
> code would still be able to use X as before without being refactored itself.

In this case, though, if the header is not included, the compilation will break
(the compiler will not know, what offset is .username_value). So this sounds
similar to the case of X inheriting T.
But as I said, no issue with adding the #include, which I did in patch set 10.

[jochen (gone - plz use gerrit), 2015-08-04 13:50:01]

c/b/BUILD.gn lgtm

[Roger Tawa OOO till Jul 10th, 2015-08-04 15:52:18]

lgtm google_api deps

[vabr (Chromium), 2015-08-05 14:53:53]

Thanks all so far!

@stuartmorgan: Please review/stamp ios/chrome/ios_chrome.gyp. The iOS internal
part is internal CL 226537013, but that one is being reviewed by droger@.

Cheers,
Vaclav

[stuartmorgan, 2015-08-05 23:30:33]

lgtm

[vabr (Chromium), 2015-08-06 05:35:51]

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

[commit-bot: I haz the power, 2015-08-06 05:36:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260263002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260263002/180001

[commit-bot: I haz the power, 2015-08-06 07:13:05]

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

[commit-bot: I haz the power, 2015-08-06 07:13:07]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[vabr (Chromium), 2015-08-06 11:37:05]

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

[commit-bot: I haz the power, 2015-08-06 11:37:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260263002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260263002/180001

[commit-bot: I haz the power, 2015-08-06 13:18:46]

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

[commit-bot: I haz the power, 2015-08-06 13:18:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[vabr (Chromium), 2015-08-06 14:30:10]

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

[commit-bot: I haz the power, 2015-08-06 14:30:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260263002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260263002/180001

[commit-bot: I haz the power, 2015-08-06 14:35:39]

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

[commit-bot: I haz the power, 2015-08-06 14:35:40]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[vabr (Chromium), 2015-08-07 05:53:59]

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

[commit-bot: I haz the power, 2015-08-07 05:54:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260263002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260263002/200001

[commit-bot: I haz the power, 2015-08-07 07:19:18]

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

[commit-bot: I haz the power, 2015-08-07 07:19:19]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vabr (Chromium), 2015-08-07 10:19:22]

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

[commit-bot: I haz the power, 2015-08-07 10:20:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260263002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260263002/200001

[commit-bot: I haz the power, 2015-08-07 10:24:29]

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

[commit-bot: I haz the power, 2015-08-07 10:24:30]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2015-08-07 10:56:16]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2015-08-07 10:56:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
stuartmorgan@chromium.org, engedy@chromium.org, rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1260263002/#ps200001
(title: "Just rebased")

[commit-bot: I haz the power, 2015-08-07 10:56:32]

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

[commit-bot: I haz the power, 2015-08-07 11:02:11]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-08-07 11:02:51]

Patchset 11 (id:??) landed as
https://crrev.com/5410d0bc855495b61ca084a8760d32613f684d01
Cr-Commit-Position: refs/heads/master@{#342332}