Introduce PasswordsModelDelegate as an abstraction between ManagePasswordsBubbleModel and ManagePas…

Introduce PasswordsModelDelegate as an abstraction between ManagePasswordsBubbleModel and ManagePasswordsUIController.

BUG=557128
TBR=groby@chromium.org

Committed: https://crrev.com/0570f9baad88e88811ab8f63291b0ab32bcac592
Cr-Commit-Position: refs/heads/master@{#360547}

[vasilii, 2015-11-17 18:48:58]

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

[vasilii, 2015-11-17 18:48:58]

Hi Vaclav,

please review.

[vasilii, 2015-11-17 19:29:50]

vasilii@chromium.org changed reviewers:
+ groby@chromium.org

[vasilii, 2015-11-17 19:29:50]

Rachel, please review changes in 
account_chooser_view_controller_unittest.mm

[vabr (Chromium), 2015-11-18 08:59:45]

Hi Vasilii,

This looks mostly good to me, I just have a handful of comments.

Cheers,
Vaclav

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_test_base.cc:141: :
ui_controller_(PasswordsModelDelegateFromWebContents(web_contents)) {}
nit: Should the variable name be updated as well, to match the type?

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/account_chooser_view_controller_unittest.mm
(left):

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/account_chooser_view_controller_unittest.mm:159:
EXPECT_EQ(password_manager::ui::CREDENTIAL_REQUEST_STATE,
Why is it OK to remove these checks?

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:226: // Avoid
using |state()| which is overridden for some unit tests.
There is no more state(), and the code below is using the replacement,
GetState(). The comment should be updated (or deleted, if there is no more
concern).

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:415:
PasswordsModelDelegate* PasswordsModelDelegateFromWebContents(
I am not happy about the definition of this function sitting with
ManagePasswordsUIController, while the declaration being with
PasswordsModelDelegate. It creates the false impression that you don't need
ManagePasswordsUIController to get the PasswordsModelDelegate instance.
Separating the two files in the future could cause problems.

I suggest placing this function (def. and decl.) in one .h/.cc pair. They
obviously cannot be in password_model_delegate.* due to a circular dependency of
headers, but they can very well be in the UI controller file. That would mean
that that you have to #include the controller everywhere where you obtain an
instance of the delegate (as opposed to just have it passed from elsewhere), but
that's fair, because the knowledge of the particular implementation is already
tied to calling this function.

Alternatively you can move the function to any other .h/.cc pair, as long as
#includes do not form a circle.

[vasilii, 2015-11-18 12:27:45]

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_test_base.cc (right):

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_test_base.cc:141: :
ui_controller_(PasswordsModelDelegateFromWebContents(web_contents)) {}
On 2015/11/18 08:59:45, vabr (Chromium) wrote:
> nit: Should the variable name be updated as well, to match the type?

Done.

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/account_chooser_view_controller_unittest.mm
(left):

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/account_chooser_view_controller_unittest.mm:159:
EXPECT_EQ(password_manager::ui::CREDENTIAL_REQUEST_STATE,
On 2015/11/18 08:59:45, vabr (Chromium) wrote:
> Why is it OK to remove these checks?

Because it tests the mock UI controller. This class is 3 layers far from the
code which should really be tested here.

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:226: // Avoid
using |state()| which is overridden for some unit tests.
On 2015/11/18 08:59:45, vabr (Chromium) wrote:
> There is no more state(), and the code below is using the replacement,
> GetState(). The comment should be updated (or deleted, if there is no more
> concern).

Done.

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:415:
PasswordsModelDelegate* PasswordsModelDelegateFromWebContents(
On 2015/11/18 08:59:45, vabr (Chromium) wrote:
> I am not happy about the definition of this function sitting with
> ManagePasswordsUIController, while the declaration being with
> PasswordsModelDelegate. It creates the false impression that you don't need
> ManagePasswordsUIController to get the PasswordsModelDelegate instance.
> Separating the two files in the future could cause problems.
> 
> I suggest placing this function (def. and decl.) in one .h/.cc pair. They
> obviously cannot be in password_model_delegate.* due to a circular dependency
of
> headers, but they can very well be in the UI controller file. That would mean
> that that you have to #include the controller everywhere where you obtain an
> instance of the delegate (as opposed to just have it passed from elsewhere),
but
> that's fair, because the knowledge of the particular implementation is already
> tied to calling this function.
> 
> Alternatively you can move the function to any other .h/.cc pair, as long as
> #includes do not form a circle.

Done.

[vabr (Chromium), 2015-11-18 12:36:40]

LGTM, thanks!
Vaclav

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/passwords/account_chooser_view_controller_unittest.mm
(left):

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/passwords/account_chooser_view_controller_unittest.mm:159:
EXPECT_EQ(password_manager::ui::CREDENTIAL_REQUEST_STATE,
On 2015/11/18 12:27:45, vasilii wrote:
> On 2015/11/18 08:59:45, vabr (Chromium) wrote:
> > Why is it OK to remove these checks?
> 
> Because it tests the mock UI controller. This class is 3 layers far from the
> code which should really be tested here.

Acknowledged.

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/1445383003/diff/20001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:415:
PasswordsModelDelegate* PasswordsModelDelegateFromWebContents(
On 2015/11/18 12:27:45, vasilii wrote:
> On 2015/11/18 08:59:45, vabr (Chromium) wrote:
> > I am not happy about the definition of this function sitting with
> > ManagePasswordsUIController, while the declaration being with
> > PasswordsModelDelegate. It creates the false impression that you don't need
> > ManagePasswordsUIController to get the PasswordsModelDelegate instance.
> > Separating the two files in the future could cause problems.
> > 
> > I suggest placing this function (def. and decl.) in one .h/.cc pair. They
> > obviously cannot be in password_model_delegate.* due to a circular
dependency
> of
> > headers,

Just to acknowledge that I was incorrect here, and that I like your current
solution.

> > but they can very well be in the UI controller file. That would mean
> > that that you have to #include the controller everywhere where you obtain an
> > instance of the delegate (as opposed to just have it passed from elsewhere),
> but
> > that's fair, because the knowledge of the particular implementation is
already
> > tied to calling this function.
> > 
> > Alternatively you can move the function to any other .h/.cc pair, as long as
> > #includes do not form a circle.
> 
> Done.

[vasilii, 2015-11-19 09:14:12]

Description was changed from

==========
Introduce PasswordsModelDelegate as an abstraction between
ManagePasswordsBubbleModel and ManagePasswordsUIController.

BUG=557128
==========

to

==========
Introduce PasswordsModelDelegate as an abstraction between
ManagePasswordsBubbleModel and ManagePasswordsUIController.

BUG=557128
TBR=groby@chromium.org
==========

[vasilii, 2015-11-19 09:14:12]

Description was changed from

==========
Introduce PasswordsModelDelegate as an abstraction between
ManagePasswordsBubbleModel and ManagePasswordsUIController.

BUG=557128
==========

to

==========
Introduce PasswordsModelDelegate as an abstraction between
ManagePasswordsBubbleModel and ManagePasswordsUIController.

BUG=557128
TBR=groby@chromium.org
==========

[vasilii, 2015-11-19 09:15:14]

I'm landing as the Mac changes are trivial.

[vasilii, 2015-11-19 09:15:27]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2015-11-19 09:15:47]

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

[commit-bot: I haz the power, 2015-11-19 10:36:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-19 10:38:13]

Patchset 3 (id:??) landed as
https://crrev.com/0570f9baad88e88811ab8f63291b0ab32bcac592
Cr-Commit-Position: refs/heads/master@{#360547}