Extracting the remaining ManagePasswordsBubbleView::Init code.

Refactor ManagePasswordsBubbleView::Init code.

This block does way too much. This CL makes a few changes
which should hopefully improve readability:

1. ColumnSets are constructed in a separate function, and collapsed down into just two types: single-view, and double-view.

2. Border logic is pushed down into the ItemView rather than being handled here.

3. Comments.

BUG=261628
TBR=markusheintz@chromium.org

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

[Mike West, 2014-03-31 14:17:28]

This is the next stop along the road that the Header CL started.

Does it make more sense like this?

-mike

[markusheintz_, 2014-04-01 21:26:59]

On 2014/03/31 14:17:28, Mike West wrote:
> This is the next stop along the road that the Header CL started.
> 
> Does it make more sense like this?
> 
> -mike

It seems like the goals of your refactoring are:

1) Reduce the code size be removing duplicated code
2) Improve the readability of the code.

In this CL you take one big method and slice it into several small methods (that
are all only called once). So instead of reducing the code size you add even
more code. Why don't you start by extracting the common code (what you mention
in your TODOs) and reduce the actual code size?

By having individual methods it seems like you want to improve the readability
of the code. The descriptive method names are supposed to tell the reader what
happens. But this also means that in order to understand the code (in case the
method name is not descriptive enough) a reader now also needs to jump forth and
back between the individual methods. So in addition to needing to understand a
big chunk of code it is now also necessary to look it up in different places. I
think by adding comments above each code section that you put into a separate
method, you could improve the readability while keeping the code together and
save unnecessary method calls and "code lookups".

[Mike West, 2014-04-02 08:13:37]

1. I disagree with you that the current code is better than patchset 1. :)
2. I'm not going to argue with you since you're many hours away and I want to
land patches at some point ever.

I've rewritten the patch into patchset 2. WDYT?

[Mike West, 2014-04-02 08:15:29]

Vaclav, can you possibly pre-review this so Markus can just stamp it? :)

[vabr (Chromium), 2014-04-02 08:38:52]

LGTM, with comments.
And to use "comments" with different meanings: I really like the comments you
added to the code.
I also have one more question about the visual result of this change, which I'm
going to ask you directly.

Cheers,
Vaclav

https://codereview.chromium.org/218993006/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_password_item_view.h (right):

https://codereview.chromium.org/218993006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_password_item_view.h:22: enum Position
{ FIRST_ITEM, MIDDLE_ITEM, LAST_ITEM };
LAST_ITEM does not seem to be used yet.
Do you have plans to use it soon?
If not, should it be dropped (and MIDDLE_ITEM renamed to denote just the
opposite of FIRST_ITEM)?

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

https://codereview.chromium.org/218993006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:152: default:
I believe that if you remove the "default:" clause, and somebody adds a new
ColumnSetType value without updating this method, that person will get a
compile-time error. With the "default:" clause in, the error will only be a
run-time (and only debug builds). So I vote for removing the "default:" clause.
:)

[Mike West, 2014-04-02 09:40:06]

https://codereview.chromium.org/218993006/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_password_item_view.h (right):

https://codereview.chromium.org/218993006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_password_item_view.h:22: enum Position
{ FIRST_ITEM, MIDDLE_ITEM, LAST_ITEM };
On 2014/04/02 08:38:52, vabr (Chromium) wrote:
> LAST_ITEM does not seem to be used yet.
> Do you have plans to use it soon?
> If not, should it be dropped (and MIDDLE_ITEM renamed to denote just the
> opposite of FIRST_ITEM)?

Yeah, not sure. I can drop it in this patch, but I think we might need it in a
later patch. *shrug* Easy. :)

[Mike West, 2014-04-02 09:40:34]

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

https://codereview.chromium.org/218993006/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:152: default:
On 2014/04/02 08:38:52, vabr (Chromium) wrote:
> I believe that if you remove the "default:" clause, and somebody adds a new
> ColumnSetType value without updating this method, that person will get a
> compile-time error. With the "default:" clause in, the error will only be a
> run-time (and only debug builds). So I vote for removing the "default:"
clause.
> :)

You're entirely correct. Thanks for the suggestion.

[Mike West, 2014-04-03 06:42:38]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2014-04-03 06:43:04]

On 2014/04/02 09:40:34, Mike West wrote:
>
https://codereview.chromium.org/218993006/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
(right):
> 
>
https://codereview.chromium.org/218993006/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:152:
default:
> On 2014/04/02 08:38:52, vabr (Chromium) wrote:
> > I believe that if you remove the "default:" clause, and somebody adds a new
> > ColumnSetType value without updating this method, that person will get a
> > compile-time error. With the "default:" clause in, the error will only be a
> > run-time (and only debug builds). So I vote for removing the "default:"
> clause.
> > :)
> 
> You're entirely correct. Thanks for the suggestion.

TBRing you, Markus. Maybe you can take a look when you wake up this morning? :)

[commit-bot: I haz the power, 2014-04-03 06:44:41]

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

[commit-bot: I haz the power, 2014-04-03 10:12:52]

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

[commit-bot: I haz the power, 2014-04-03 10:12:53]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Mike West, 2014-04-03 10:16:28]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-04-03 10:17:27]

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

[commit-bot: I haz the power, 2014-04-03 12:10:21]

Change committed as 261363

[markusheintz_, 2014-04-04 14:56:25]

https://codereview.chromium.org/218993006/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_password_item_view.cc (right):

https://codereview.chromium.org/218993006/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_password_item_view.cc:36: 0,
nit: I wonder whether we should add a comment or just the argument name to
describe these numbers

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

https://codereview.chromium.org/218993006/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:122: void
ManagePasswordsBubbleView::BuildColumnSet(views::GridLayout* layout,
I'm still not convinced that this really makes the code easier to understand.

But let's follow up offline when I'm back.