Password bubble: Create subviews for ManagePasswordsBubbleView.

Password bubble: Create subviews for ManagePasswordsBubbleView.

Same idea as we've done for ManagePasswordItemView: separate subview
classes make it simpler to change the content and to understand where
the content is coming from.

After this CL, it will be a simpler addition to shoehorn in a blacklist
confirmation screen in the same way that we've done undo for the item
views.

BUG=261628
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267213

[Mike West, 2014-04-28 11:05:05]

Hi Vaclav, Markus!

This CL should introduce no behavioral changes (with the exception of the DCHECK
fix in manage_passwords_bubble_ui_controller that was masked by my test mock. :(
), and is blocked on https://codereview.chromium.org/246393004/.

Mind taking a look?

-mike

[Mike West, 2014-04-28 12:41:18]

https://codereview.chromium.org/255903006/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble_model.cc (right):

https://codereview.chromium.org/255903006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble_model.cc:31:
manage_passwords_bubble_ui_controller->PendingCredentials();
This was getting called regardless of the model's state, which crashed at the
|DCHECK(form_manager_)|. That was masked in tests because we've mocked out the
pendingcredentials call. :(

[vabr (Chromium), 2014-04-28 13:17:38]

LGTM with one comment.

Cheers,
Vaclav

https://codereview.chromium.org/255903006/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble.h (right):

https://codereview.chromium.org/255903006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:32:
ManagePasswordsBubbleModel* model() const { return model_.get(); }
I still mean my previous comment [1] :). Or, as Peter put it: "const methods
should not return non-const pointers" [2].

[1]
https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
[2]
https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...

[Mike West, 2014-04-28 14:02:27]

The latest patchset adds a const version of the method you pointed out, and then
consts All The Things in the model in order to make the compiler happy.

https://codereview.chromium.org/255903006/diff/1/chrome/browser/ui/passwords/...
File chrome/browser/ui/passwords/manage_passwords_bubble.h (right):

https://codereview.chromium.org/255903006/diff/1/chrome/browser/ui/passwords/...
chrome/browser/ui/passwords/manage_passwords_bubble.h:32:
ManagePasswordsBubbleModel* model() const { return model_.get(); }
On 2014/04/28 13:17:38, vabr (Chromium) wrote:
> I still mean my previous comment [1] :). Or, as Peter put it: "const methods
> should not return non-const pointers" [2].
> 
> [1]
>
https://codereview.chromium.org/232753002/diff/1/chrome/browser/ui/passwords/...
> [2]
>
https://codereview.chromium.org/246393004/diff/70001/chrome/browser/ui/views/...

Hrm. I apparently _really_ want this to be const. :/

[vabr (Chromium), 2014-04-28 14:08:31]

patchset 2 LGTM!
Thanks,
Vaclav

[markusheintz_, 2014-04-29 07:51:37]

https://codereview.chromium.org/255903006/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/255903006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:103:
parent->BuildColumnSet(layout, SINGLE_VIEW_COLUMN_SET);
It seems like this method is not used on the parent any more. So it should not
live there.

Maybe this can be a utility method in a anonymous namespace.

https://codereview.chromium.org/255903006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:104:
parent->AddTitleRow(layout);
The same as above seems to hold for this one.

[Mike West, 2014-04-29 08:11:47]

On 2014/04/29 07:51:37, markusheintz_ wrote:
>
https://codereview.chromium.org/255903006/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
(right):
> 
>
https://codereview.chromium.org/255903006/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:103:
> parent->BuildColumnSet(layout, SINGLE_VIEW_COLUMN_SET);
> It seems like this method is not used on the parent any more. So it should not
> live there.
> 
> Maybe this can be a utility method in a anonymous namespace.
> 
>
https://codereview.chromium.org/255903006/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:104:
> parent->AddTitleRow(layout);
> The same as above seems to hold for this one.

Sure. I can make that change.

[Mike West, 2014-04-29 08:17:28]

Thanks Markus, WDYT about the latest patchset?

[markusheintz_, 2014-04-29 08:30:26]

On 2014/04/29 08:17:28, Mike West wrote:
> Thanks Markus, WDYT about the latest patchset?

LGTM

[Mike West, 2014-04-29 13:01:21]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-29 13:01:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/255903006/40001

[Mike West, 2014-04-29 13:23:54]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2014-04-30 10:00:11]

Rebased onto the shiny, new https://codereview.chromium.org/254263002/. Will CQ
again once that lands.

[Mike West, 2014-04-30 12:42:47]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-30 12:42:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/255903006/60001

[commit-bot: I haz the power, 2014-04-30 12:45:45]

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

[commit-bot: I haz the power, 2014-04-30 12:45:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Mike West, 2014-04-30 12:54:19]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-30 12:54:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/255903006/60001

[commit-bot: I haz the power, 2014-04-30 15:01:21]

Change committed as 267213