Redesign notifier settings view.

Redesign notifier settings view.

This includes following changes:
- omit the text in title bar
- instead, 'Settings' text would be in the dialog, at the top
- window size: width: 320, height: 480
- each row has 48px height
- scroll view for settings

BUG=161094
TBR=sky@chromium.org

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

[dharcourt, 2013-03-13 23:33:56]

LGTM with some possible typos and some nits, suggestions, and questions.

Once you've verified the typos, feel free to commit without further review from
me whether or not you want to address the nits, implement the suggestions, and
answer the question :-).

Onwards!

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
File ui/message_center/views/notifier_settings_view.cc (right):

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:43: class
NotifierSettingsEntryLayoutManager : public views::LayoutManager {
Nit: Since this class is private to NotifierSettingsView, the NotifierSettings
prefix in its name is IMHO redundant and the code might be more readable without
it. The Manager suffix might also be redundant since views LayoutManager
subclasses don't have it (BoxLayout, GridLayout, ...).

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:43: class
NotifierSettingsEntryLayoutManager : public views::LayoutManager {
Suggestion: Having an EntryView that implements the same Layout() and
GetPreferredSize() methods might be simpler than using this EntryLayout  with
generic views::View. If you made EntryView's constructor take a child view as an
argument it could be used with a single line of code like
"contents_view->AddChildView(new EntryView(top_label))" instead of the current
4, and it would minimize the risk of ending up with 0 or more than 1 children to
lay out. Your call.

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:46: virtual
~NotifierSettingsEntryLayoutManager() {}
Nit: Chromium style recommends against inlining even empty constructors like the
one above, and prohibits inlining virtual functions like the destructor above
(http://dev.chromium.org/developers/coding-style#TOC-Inline-functions).

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:70: DCHECK_EQ(1,
host->child_count());
Q: Why is this a DCHECK whereas a similar check in Layout() isn't? Are there
situations where a particular layout will have 0 children in its Layout() but 1
in GetPreferredSize()?

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:77: class SeparatorView :
public views::View {
Suggestion: It's kind of a hack, but you could replace this separator class with
a null-image ImageView with a 1 pixel colored border. Search for
kButtonSeparatorColor in notification_view.cc for an example. It would remove a
few lines of code but whether it would simplify the code or make it more complex
is an open question. Your call.

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:263: if (kMinimumWindowWidth <
Typo? Shouldn't this be kMinimumWindowHeight, and shouldn't the height being
compared include 1 pixel for the separator?

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:288: scroller_->SetBounds(0,
title_height, width(), height() - title_height);
Typo? Shouldn't y be (title_height + 1) and height (height - title_height - 1)
to account for the separator?

[Jun Mukai, 2013-03-14 06:47:48]

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
File ui/message_center/views/notifier_settings_view.cc (right):

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:43: class
NotifierSettingsEntryLayoutManager : public views::LayoutManager {
On 2013/03/13 23:33:56, dharcourt wrote:
> Suggestion: Having an EntryView that implements the same Layout() and
> GetPreferredSize() methods might be simpler than using this EntryLayout  with
> generic views::View. If you made EntryView's constructor take a child view as
an
> argument it could be used with a single line of code like
> "contents_view->AddChildView(new EntryView(top_label))" instead of the current
> 4, and it would minimize the risk of ending up with 0 or more than 1 children
to
> lay out. Your call.

Changed to EntryView.  The code gets more concise. Thanks!

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:46: virtual
~NotifierSettingsEntryLayoutManager() {}
On 2013/03/13 23:33:56, dharcourt wrote:
> Nit: Chromium style recommends against inlining even empty constructors like
the
> one above, and prohibits inlining virtual functions like the destructor above
> (http://dev.chromium.org/developers/coding-style#TOC-Inline-functions).

fixed...

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:70: DCHECK_EQ(1,
host->child_count());
On 2013/03/13 23:33:56, dharcourt wrote:
> Q: Why is this a DCHECK whereas a similar check in Layout() isn't? Are there
> situations where a particular layout will have 0 children in its Layout() but
1
> in GetPreferredSize()?

Added DCHECK to both.

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:77: class SeparatorView :
public views::View {
On 2013/03/13 23:33:56, dharcourt wrote:
> Suggestion: It's kind of a hack, but you could replace this separator class
with
> a null-image ImageView with a 1 pixel colored border. Search for
> kButtonSeparatorColor in notification_view.cc for an example. It would remove
a
> few lines of code but whether it would simplify the code or make it more
complex
> is an open question. Your call.

Thinking about this more, I decided to change it to a views::Border so I don't
need to care about its layout.
Unfortunately this kind of border (slightly short from the edge) isn't supported
in views::Border, so I still keep this as a class here.

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:263: if (kMinimumWindowWidth <
On 2013/03/13 23:33:56, dharcourt wrote:
> Typo? Shouldn't this be kMinimumWindowHeight, and shouldn't the height being
> compared include 1 pixel for the separator?

Aww, right. fixed.  Thanks!

https://codereview.chromium.org/12465011/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:288: scroller_->SetBounds(0,
title_height, width(), height() - title_height);
On 2013/03/13 23:33:56, dharcourt wrote:
> Typo? Shouldn't y be (title_height + 1) and height (height - title_height - 1)
> to account for the separator?

Since separator_ is gone (it now belongs to scroller_), the current code is
correct.

[commit-bot: I haz the power, 2013-03-14 06:52:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/12465011/5001

[commit-bot: I haz the power, 2013-03-14 06:52:14]

Presubmit check for 12465011-5001 failed and returned exit status 1.

INFO:root:Found 3 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    ui/base/strings/ui_strings.grd

Presubmit checks took 2.7s to calculate.

[Jun Mukai, 2013-03-15 00:47:32]

adding sky as TBR -- for ui/base/strings/ui_strings.grd

[commit-bot: I haz the power, 2013-03-15 00:53:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/12465011/10001

[commit-bot: I haz the power, 2013-03-15 07:17:52]

Change committed as 188277